Skip to content

Remove sync call from DetachRootView to fix deadlock, add public QuirkSettings API for backward compatibility #15113

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.

Sign up for GitHub

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

Merged
anupriya13 merged 5 commits into from
Sep 9, 2025
Merged

Remove sync call from DetachRootView to fix deadlock, add public QuirkSettings API for backward compatibility #15113

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0394704
Initial plan
Copilot Sep 4, 2025
a337cee
Fix deadlock in ReactInstanceWin::DetachRootView by replacing sync ca…
Copilot Sep 4, 2025
144551a
Add change file for prerelease and run formatting checks
Copilot Sep 4, 2025
4642723
Remove sync call from DetachRootView to fix deadlock, add quirk optio…
Copilot Sep 5, 2025
84e081b
Address comment
anupriya13 Sep 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions change/1756980197-fix-deadlock.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "Fix deadlock in ReactInstanceWin::DetachRootView by replacing sync call with async",
"packageName": "react-native-windows",
"email": "copilot@github.com",
"dependentChangeType": "patch"
}
7 changes: 7 additions & 0 deletions change/remove-sync-detach-20250905060318.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "Remove sync call from DetachRootView to fix deadlock, add quirk option to re-enable if needed",
"packageName": "@rnw-dependencies",
"email": "noreply@microsoft.com",
"dependentChangeType": "patch"
}
6 changes: 6 additions & 0 deletions vnext/Microsoft.ReactNative/ReactHost/React.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,12 @@ struct ReactOptions {
//! It is not safe to expose to Custom Function. Add this flag so we can turn it off for Custom Function.
bool EnableNativePerformanceNow{true};

//! QUIRK: Enable synchronous wait in DetachRootView for JS thread completion.
//! This flag re-enables the legacy runOnQueueSync call that can cause deadlocks
//! with Hermes GC. Only enable if removal of sync behavior breaks your application.
//! Default: false (sync call is disabled to prevent deadlocks)
bool EnableSyncDetachRootView{false};

@vineethkuttan vineethkuttan Sep 8, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@copilot when will EnableSyncDetachRootView set to true?

Copilot AI Sep 8, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Your comment appears incomplete. Could you clarify what you'd like to know about when EnableSyncDetachRootView is set to true?

When this flag is enabled, it restores the legacy synchronous behavior (line 1521) that can cause deadlocks with Hermes GC, which is why it defaults to false.

@acoates-ms acoates-ms Sep 8, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

React.h is not part of the public interface, and so just adding a property here wouldn't allow apps to opt into the old behavior.
You should add a property to QuirkSettings.idl/cpp/h

@anupriya13 anupriya13 Sep 8, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@copilot please apply changes as per comment - "React.h is not part of the public interface, and so just adding a property here wouldn't allow apps to opt into the old behavior.
You should add a property to QuirkSettings.idl/cpp/h"

Copilot AI Sep 8, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Moved EnableSyncDetachRootView from React.h to QuirkSettings.idl/cpp/h as requested to make it part of the public interface. Applications can now use QuirkSettings::SetEnableSyncDetachRootView(settings, true) to enable the legacy sync behavior. Updated ReactInstanceWin.cpp to use QuirkSettings::GetEnableSyncDetachRootView() instead of m_options.


ReactDevOptions DeveloperSettings = {};

//! This controls the availability of various developer support functionality including
Expand Down
9 changes: 7 additions & 2 deletions vnext/Microsoft.ReactNative/ReactHost/ReactInstanceWin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1514,8 +1514,13 @@ void ReactInstanceWin::DetachRootView(facebook::react::IReactRootView *rootView,
CallJsFunction("AppRegistry", "unmountApplicationComponentAtRootTag", std::move(params));
}

// Give the JS thread time to finish executing
m_jsMessageThread.Load()->runOnQueueSync([]() {});
Comment thread
anupriya13 marked this conversation as resolved.
// QUIRK: Legacy sync behavior can be re-enabled via EnableSyncDetachRootView option
// The sync call was removed to prevent deadlocks with Hermes GC operations
if (m_options.EnableSyncDetachRootView) {
// Legacy behavior: wait for JS thread to finish executing (can cause deadlocks)
m_jsMessageThread.Load()->runOnQueueSync([]() {});
}
// Default behavior: no synchronization call to prevent deadlocks
}

Mso::CntPtr<IReactInstanceInternal> MakeReactInstance(
Expand Down