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

refactor: explicit invalidations for native and cpp #6850

Merged
merged 6 commits into from
Jan 10, 2025

Conversation

tjzel
Copy link
Collaborator

@tjzel tjzel commented Dec 23, 2024

Summary

This pull requests refactors memory management of Worklets and Reanimated.

Basically, since Reanimated can obtain WorkletsModuleProxy and the Worklet Runtimes as shared pointers, it has to release them explicitly during the invalidation stage of Native Modules. Releasing them later on (e.g. via deconstructors) might lead into issues and crashes.

Ideally we'd instead use some different solution here than shared pointers, but it can wait as it's not mandatory at the moment and could be a significant refactor.

Fixes:

  • iOS crash on reload
  • Android crash on SingleInstanceChecker during third reload

Test plan

  • 0.76 iOS/Android Fabric works
  • 0.76 iOS/Android Paper works
  • 0.75 iOS/Android Fabric works
  • 0.75 iOS/Android Paper works
  • 0.74 iOS/Android Fabric works
  • 0.74 iOS/Android Paper works

@tjzel tjzel marked this pull request as ready for review December 24, 2024 13:24
@tjzel tjzel added the Check compatibility Trigger a time-consuming compatibility check action label Dec 24, 2024
Copy link
Member

@tomekzaw tomekzaw left a comment

Choose a reason for hiding this comment

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

The changes look good to me, I left some remarks in the comments. Two more general questions:

  1. Why do we need a separate invalidate method? Why can't we just clear the pointers in C++ destructors like we used to do?
  2. Once invalidate method is called, shall we add some assertions to prevent us from calling any methods of an invalidated object?
  3. Why can't we just use weak pointers to store WorkletsModule in ReanimatedModule? This way there's no strong link between those two.

Copy link
Member

@piaskowyk piaskowyk left a comment

Choose a reason for hiding this comment

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

Could you elaborate more on why performing cleanup in the C++ destructor is considered too late? This isn't entirely clear to me, but it seems to be important.

@tjzel
Copy link
Collaborator Author

tjzel commented Jan 9, 2025

@piaskowyk Native Modules are invalidated and destroyed in stages.

This is more or less how React Native gets cleaned up now (e.g. on reload):

  1. invalidate() is invoked on all its Native Modules (probably in reverse order of their initialization, but I'm not exactly sure there).
  2. React Native instance specific objects, like Shadow Nodes are deleted.
  3. Native Modules are released (and deleted).

Our Worklet Runtime can hold instances to various Host Objects, like Shadow Nodes. Therefore, if we destroy our runtime on step 3. it might crash on garbage collection because the C++ side of these host objects is already destroyed. This probably could be mitigated with a different kind of bindings, but it's definitely outside of the scope of this PR.

@tjzel tjzel added this pull request to the merge queue Jan 10, 2025
Merged via the queue into main with commit 1bc31af Jan 10, 2025
40 checks passed
@tjzel tjzel deleted the @tjzel/modules-invalidation branch January 10, 2025 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Check compatibility Trigger a time-consuming compatibility check action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants