[0.81] Prevent partially initialized TurboModules during shutdown - #16369
Vladimir Morozov (vmoroz) merged 2 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e5dad4bf-4f71-43ee-aa7f-d2f11d43121e
There was a problem hiding this comment.
Pull request overview
Prevents lazy TurboModules from being returned with uninitialized runtime-dependent state during shutdown.
Changes:
- Validates and retains the JSI runtime during module registration.
- Releases the temporary runtime handle afterward.
- Adds a patch change file.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
vnext/Microsoft.ReactNative/TurboModulesProvider.cpp |
Guards and stabilizes TurboModule initialization. |
change/react-native-windows-c5366f79-8343-45d0-9916-3f13b872c4d8.json |
Records the patch release note. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Khalef Hosany (@Khalef1) , could you please check that holding the runtme handler inside of the Turbo Modules does not create the ref counting loops and thus the memory leaks? Should we rather use a weak ref instead of the strong ref? |
Vladimir Morozov (@vmoroz) as discussed offline, it appears the strong reference is only held from instantiation to initialization. Can you approve if you see no further issues? |
There was a problem hiding this comment.
🟡 Changes recommended
Regression coverage and the required macOS companion validation are missing.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
vnext/Microsoft.ReactNative/TurboModulesProvider.cpp:40
vnext/Microsoft.ReactNative.Cxx/README.md:18-31,57-63explicitly lists this file and requires edits to be validated by a companionreact-native-macosPR plus the RNTester Snapshot/Screenshot test. Neither that PR nor its validation result is linked here; please provide them before merging so this shared implementation is not broken on macOS.
TurboModuleBuilder(const IReactContext &reactContext, IInspectable runtimeHandle) noexcept
: m_reactContext(reactContext), m_runtimeHandle(std::move(runtimeHandle)) {}
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| auto runtimeHandle = winrt::get_self<winrt::Microsoft::ReactNative::implementation::ReactContext>(m_reactContext) | ||
| ->GetInner() | ||
| .JsiRuntime(); | ||
| if (!runtimeHandle) { | ||
| return nullptr; |
| m_moduleBuilder(winrt::make_self<TurboModuleBuilder>(reactContext)), | ||
| m_providedModule(reactModuleProvider(m_moduleBuilder.as<IReactModuleBuilder>())) { | ||
| m_moduleBuilder(winrt::make_self<TurboModuleBuilder>(reactContext, std::move(runtimeHandle))) { | ||
| m_providedModule = reactModuleProvider(m_moduleBuilder.as<IReactModuleBuilder>()); |
There was a problem hiding this comment.
Why was this moved into the constructor body?
| return nullptr; | ||
| } | ||
|
|
||
| auto runtimeHandle = winrt::get_self<winrt::Microsoft::ReactNative::implementation::ReactContext>(m_reactContext) |
There was a problem hiding this comment.
(nit) Keep the ABI-related comment?
Description
Type of Change
Why
Access violation in
BlobTurboModule::CreateFromPartsbecausem_resourceis null. This regressed after #16237: during instance teardown,REACT_INITis skipped when the runtime handle is null, but the lazy TurboModule can still be returned to JavaScript. The module is then callable without its initializer having populated its state. The same partial-initialization pattern can affect other lazy TurboModules.What
react-native-windowspatch change file.Screenshots
Not applicable.
Testing
yarn format:verifyyarn beachball check --branch origin/0.81-stableChangelog
Should this change be included in the release notes: yes
Prevent lazy TurboModules from being created without a runtime during shutdown.
Microsoft Reviewers: Open in CodeFlow