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

Use LongLivedObjects for TurboModule callbacks #10436

Merged
4 commits merged into from Aug 25, 2022
Merged

Use LongLivedObjects for TurboModule callbacks #10436

4 commits merged into from Aug 25, 2022

Conversation

vmoroz
Copy link
Member

@vmoroz vmoroz commented Aug 25, 2022

Description

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

Why

@acoates-ms has reported an issue with RNW Turbo Module implementation where we see a crash after a RN instance is unloaded. The crash is caused by a JSI Function that captured in a lambda used for asynchronous callbacks or in a Promise.
We destroy JS engine instance when unloading the RN instance, and the JSI Function cannot be safely released anymore.

What

RN Turbo Modules that wrap up CxxModules use LongLivedObject and LongLivedObjectCollection as a mechanism to address similar issues. The callbacks are wrapped up into CallbackWrapper class inherited from the LongLivedObject. The LongLivedObjectCollection is cleared when JS engine instance is shut down and it releases all associated std::shared_ptr<LongLivedObject> instances, which in turn release all associated JSI values. The deferred callback and Promise lambdas capture std::weak_ptr<LongLivedObject>, and do nothing if the weak pointer cannot be "locked" anymore.

The PR uses the same LongLivedObjectCollection for RNW Turbo Modules, but it introduces new LongLivedJsiRuntime and LongLivedJsiValue classes inherited from LongLivedObject. They have smaller size and do not use global LongLivedObjectCollection instance.

The solution consists of the following parts:

  • JSDispatcherWriter keeps std::weak_ptr<LongLivedJsiRuntime> instead of JSI Runtime and does nothing while handling results if the JSI Runtime is not available anymore.
  • TurboModulesProvider is wrapping up JSI Function instances into std::weak_ptr<LongLivedJsiFunction>. (The LongLivedJsiFunction is an alias for LongLivedJsiValue<jsi::Function>.) It helps to release all JSI Function instances while the JS engine instance is shutting down.
  • ReactNativeHost creates a new instance of LongLivedObjectCollection and passes it to the TurboModulesProvider.
  • The InstanceImpl in OInstance.h passes the LongLivedObjectCollection instance from TurboModulesProvider to the OJSIExecutorFactory. The OJSIExecutorFactory associates the LongLivedObjectCollection instance with a JS engine instance by passing it to the TurboModuleBinding::install call.

Testing

We added new unit tests in TurboModuleTests.cpp to check behavior when callbacks and Promises are released after RN instance unload. They are all passing. These tests cause a crash without changes to the TurboModulesProvider.

Also, we added a new test utility class TestNotificationService that helps to coordinate setting and waiting for synchronization events. Unlike the TestEventService that expects synchronization events to appear in a specified order, the new TestNotificationService keeps a set of events and lets the Wait function to continue if the set contains an event that it waits for.

Microsoft Reviewers: Open in CodeFlow

@vmoroz vmoroz requested a review from acoates-ms August 25, 2022 01:56
@vmoroz vmoroz requested a review from a team as a code owner August 25, 2022 01:56
@vmoroz vmoroz added the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label Aug 25, 2022
@ghost
Copy link

ghost commented Aug 25, 2022

Hello @vmoroz!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 84e50b4 into microsoft:main Aug 25, 2022
@vmoroz vmoroz deleted the PR/FixLongLivedJsiValues branch August 25, 2022 20:30
@jonthysell
Copy link
Contributor

@vmoroz Is this important enough to get backported into 0.70?

@vmoroz
Copy link
Member Author

vmoroz commented Aug 25, 2022

@vmoroz Is this important enough to get backported into 0.70?

Yes, it is important. Without this fix we have a risk of crashing every time when a RN instance is unloaded.
This is when we use JSI-based TurboModules.

ghost pushed a commit that referenced this pull request Aug 26, 2022
* Use LongLivedObjects for TurboModule callbacks (#10436)

* Fix build break

* yarn format
ghost pushed a commit that referenced this pull request Aug 26, 2022
* Use LongLivedObjects for TurboModule callbacks (#10436)

* Fix build break
ghost pushed a commit that referenced this pull request Aug 30, 2022
* Use LongLivedObjects for TurboModule callbacks (#10436)

* Use "prerelease" for the change type

Co-authored-by: Andrew Coates <30809111+acoates-ms@users.noreply.github.com>
Co-authored-by: dannyvv <dannyvv@microsoft.com>
rozele added a commit to rozele/react-native-windows that referenced this pull request Mar 20, 2023
We're seeing crashes when the React instance is destroyed if there is an active ImageLoader::getSizeWithHeaders request in flight (related to JSI trying to create an object on a destroyed runtime).

This is similar to an issue that was resolved and picked into React Native Windows v0.68 (microsoft#10436), but even with that fix, the crash still occurs. I suspect it may be related to using the ReactPromise copy constructor, but even if not, it's not so bad to avoid copying the ReactPromise.
rozele added a commit to rozele/react-native-windows that referenced this pull request Mar 20, 2023
We're seeing crashes when the React instance is destroyed if there is an active ImageLoader::getSizeWithHeaders request in flight (related to JSI trying to create an object on a destroyed runtime).

This is similar to an issue that was resolved and picked into React Native Windows v0.68 (microsoft#10436), but even with that fix, the crash still occurs. I suspect it may be related to using the ReactPromise copy constructor, but even if not, it's not so bad to avoid copying the ReactPromise.
rozele added a commit to rozele/react-native-windows that referenced this pull request Mar 20, 2023
We're seeing crashes when the React instance is destroyed if there is an active ImageLoader::getSizeWithHeaders request in flight (related to JSI trying to create an object on a destroyed runtime).

This is similar to an issue that was resolved and picked into React Native Windows v0.68 (microsoft#10436), but even with that fix, the crash still occurs. I suspect it may be related to using the ReactPromise copy constructor, but even if not, it's not so bad to avoid copying the ReactPromise.
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants