Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: crash on OTA reload #7196

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix: crash on OTA reload #7196

wants to merge 4 commits into from

Conversation

tjzel
Copy link
Collaborator

@tjzel tjzel commented Mar 6, 2025

Summary

Fixes #7182
Requires #7197

Currently on iOS React Native can go into a non-fatal race condition on a double reload. Double reload can happen during an OTA update, when an app is reloaded immediately after evaluating the bundle.

The flow goes more or less like this. I haven't explored it to the deepest details.

  1. On the first reload a new JavaScript runtime is created.
  2. The new runtime evaluates the bundle, starts to populate RCTModuleRegistry.
  3. On the second reload a new JavaScript runtime is created. The old runtime remains and keeps executing. Both runtimes execute on the same thread, on the same queue.
  4. WorkletsModule.installTurboModule() is called from the first runtime. WorkletsModule is fetched from the RCTModuleRegistry.
  5. The second runtime overwrites (clears) the RCTModuleRegistry and starts to populate it. I don't know why both runtimes target the same instance of RCTModuleRegistry.
  6. WorkletsModule.installTurboModule() finishes successfully on the first runtime.
  7. ReanimatedModule.installTurboModule() is invoked in quick succession on the first runtime.
  8. ReanimatedModule tries to fetch WorkletsModule from RCTModuleRegistry. However, RCTModuleRegistry was wiped and it doesn't have the fully installed instance of WorkletsModule anymore. Therefore a new instance of WorkletsModule is created and inserted into the RCTModuleRegistry.
  9. In JavaScript we guarantee that ReanimatedModule.installTurboModule() is invoked only after WorkletsModule.installTurboModule(). We follow that heuristic in Java and therefore try operating on a non-installed instance of WorkletsModule resulting in a NullPtrException.

We can effectively detect that flow and ignore it, since that race condition is fatal only for the first runtime which is torn down soon after anyway.

See the provided screenshots which illustrate the problems.

Screenshot 2025-03-07 at 10 34 00 Screenshot 2025-03-07 at 12 25 23
Setting moduleRegistry for WorkletsModule during a healthy execution of WorkletsModule.installTurboModule() Setting moduleRegistry for WorkletsModule during race-condition execution. Note that the list of the modules is much shorter and that WorkletsModule isn't there.

Test plan

To reproduce this issue, reload twice on iOS in fabric-example. For best effect try to reload after ~0.5s after the first time, but it's easily reproducible if the App isn't slowed heavily by debugger instructions.

With these changes Reanimated doesn't crash, but the race condition continues to manifest.

@tjzel tjzel requested a review from tomekzaw March 7, 2025 11:28
@tjzel tjzel marked this pull request as ready for review March 7, 2025 11:28
@tjzel tjzel changed the base branch from main to @tjzel/worklets/fix-android-invalidation March 7, 2025 11:28
@tjzel
Copy link
Collaborator Author

tjzel commented Mar 7, 2025

cc @lukmccall

@@ -249,3 +256,38 @@ See https://docs.swmansion.com/react-native-reanimated/docs/guides/troubleshooti
this.#reanimatedModuleProxy.unregisterCSSTransition(viewTag);
}
}

class DummyReanimatedModuleProxy implements ReanimatedModuleProxy {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've asked my friend GPT about JSProxy that provides a default fallback implementation for undefined methods, and he suggested me something like that:

// Define a default method to be used when a non-existent method is called
function defaultMethod(...args) {
    console.log("Default method triggered with arguments: ", args);
}

// Create a handler object with a `get` trap
const handler = {
    get(target, prop, receiver) {
        // If the property exists, return it
        if (prop in target) {
            return target[prop];
        }
        
        // If the property does not exist, return the default method
        return function(...args) {
            defaultMethod(...args);
        };
    }
};

// Define an object with some methods
const originalObject = {
    existingMethod: function() {
        console.log("Existing method called");
    }
};

// Create a proxy for the object
const proxyObject = new Proxy(originalObject, handler);

// Test invoking existing method
proxyObject.existingMethod();  // Output: Existing method called

// Test invoking a non-existent method
proxyObject.nonExistentMethod(1, 2, 3);  // Output: Default method triggered with arguments: [ 1, 2, 3 ]

we could avoid listing all methods here, and it would work for methods that we add in a future

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While using proxy looks cool I think simpler code would be more maintainable. You also you need extra methods for functions that return arguments, like subscribeForKeyboardEvent etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, TypeScript will have our back if we forget to add some methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants