Skip to content

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

Merged
anupriya13 merged 5 commits intomainfrom
copilot/fix-15109-2
Sep 9, 2025
Merged

Remove sync call from DetachRootView to fix deadlock, add public QuirkSettings API for backward compatibility#15113
anupriya13 merged 5 commits intomainfrom
copilot/fix-15109-2

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Sep 4, 2025

Description

Type of Change

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

Why

This PR addresses a deadlock issue in ReactInstanceWin::DetachRootView that occurs when the UI thread calls runOnQueueSync during Hermes garbage collection operations. The deadlock happens because:

  1. UI thread calls runOnQueueSync() and blocks waiting for the JS thread to respond
  2. JS thread is busy with Hermes GC operations like markRoots which hold internal locks
  3. While GC locks are held, the JS thread cannot process its message queue, including the sync request from UI thread
  4. This results in an indefinite deadlock

Resolves #15109

What

  • Removed synchronous call from DetachRootView by default to eliminate the deadlock
  • Added public QuirkSettings API SetUseRunOnQueueSync (defaults to false) for backward compatibility
  • Updated QuirkSettings.idl/cpp/h to expose the quirk setting as part of the public interface
  • Added comprehensive documentation explaining the deadlock risk and when to use the quirk option
  • Fixed build errors by adding proper project entries and includes

The change eliminates the deadlock while providing a safety net through QuirkSettings::SetUseRunOnQueueSync(settings, true) if the removal breaks existing applications.

Screenshots

N/A - This is a runtime deadlock fix with no UI changes.

Testing

  • Verified build passes with all changes
  • The original deadlock was reproduced in production environments during Hermes GC operations
  • Testing with the quirk setting disabled (default) should prevent the deadlock
  • Testing with the quirk setting enabled should restore legacy behavior for compatibility

Changelog

Should this change be included in the release notes: yes

Add a brief summary of the change to use in the release notes for the next release.
Fixed deadlock in ReactInstanceWin::DetachRootView during Hermes garbage collection by removing synchronous call. Added QuirkSettings.SetUseRunOnQueueSync option for backward compatibility if needed.

Microsoft Reviewers: Open in CodeFlow

…ll with async

Co-authored-by: anupriya13 <54227869+anupriya13@users.noreply.github.com>
Copilot AI changed the title [WIP] ADO Bug RNW 41401609 | Deadlock Fix deadlock in ReactInstanceWin::DetachRootView by replacing sync call with async Sep 4, 2025
Copilot AI requested a review from anupriya13 September 4, 2025 08:43
Comment thread vnext/Microsoft.ReactNative/ReactHost/ReactInstanceWin.cpp
@anupriya13
Copy link
Copy Markdown
Contributor

/azp run

@anupriya13
Copy link
Copy Markdown
Contributor

@copilot run "yarn change" prerelease and "yarn lint:fix" and "yarn format"

@azure-pipelines
Copy link
Copy Markdown
Contributor

Azure Pipelines successfully started running 2 pipeline(s).

Co-authored-by: anupriya13 <54227869+anupriya13@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Sep 4, 2025

@copilot run "yarn change" prerelease and "yarn lint:fix" and "yarn format"

I've completed the requested tasks:

  1. yarn change --type prerelease: Created a manual change file since Node version incompatibility prevented running beachball directly
  2. yarn format: Ran clang-format on the changed C++ file (no formatting changes were needed)
  3. yarn lint:fix: Only C++ files were modified, which are formatted rather than linted

The change file has been created for versioning and all code formatting is correct. Committed in 144551a.

…n to re-enable if needed

Co-authored-by: anupriya13 <54227869+anupriya13@users.noreply.github.com>
Copilot AI changed the title Fix deadlock in ReactInstanceWin::DetachRootView by replacing sync call with async Remove sync call from DetachRootView to fix deadlock, add quirk option for backward compatibility Sep 5, 2025
Comment thread vnext/Microsoft.ReactNative/QuirkSettings.cpp Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a deadlock issue in DetachRootView by removing a synchronous call that could block during Hermes garbage collection operations. The solution maintains backward compatibility by adding a public quirk setting to re-enable the legacy behavior if needed.

Key changes:

  • Replaced synchronous runOnQueueSync call with conditional execution based on quirk setting
  • Added comprehensive QuirkSettings API for the new EnableSyncDetachRootView option
  • Updated project files to include the new QuirkSettings implementation

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
ReactInstanceWin.cpp Modified DetachRootView to conditionally execute sync call based on quirk setting
QuirkSettings.idl Added IDL definition for EnableSyncDetachRootView quirk option
QuirkSettings.h Added header declarations for the new quirk setting methods
QuirkSettings.cpp Implemented quirk setting getter/setter methods and property definitions
Microsoft.ReactNative.vcxproj Added QuirkSettings source files to project compilation
change/*.json Added change log entries for the deadlock fix and API changes

Comment thread vnext/Microsoft.ReactNative/QuirkSettings.cpp Outdated
Comment thread vnext/Microsoft.ReactNative/QuirkSettings.cpp Outdated
Comment thread vnext/Microsoft.ReactNative/QuirkSettings.cpp
Comment thread vnext/Microsoft.ReactNative/QuirkSettings.h
@anupriya13
Copy link
Copy Markdown
Contributor

/azp run

@anupriya13
Copy link
Copy Markdown
Contributor

@copilot update PR description as per react-native-windows PR template

@azure-pipelines
Copy link
Copy Markdown
Contributor

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Sep 9, 2025

@copilot update PR description as per react-native-windows PR template

Updated the PR description to follow the react-native-windows PR template format with proper sections for Description, Type of Change, Why, What, Screenshots, Testing, and Changelog.

@anupriya13
Copy link
Copy Markdown
Contributor

@copilot update PR description as per react-native-windows PR template

Updated the PR description to follow the react-native-windows PR template format with proper sections for Description, Type of Change, Why, What, Screenshots, Testing, and Changelog.

Help improve Copilot by leaving feedback using the 👍 or 👎 buttons

@copilot the pr description should follow the latest code changes

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Sep 9, 2025

@copilot update PR description as per react-native-windows PR template

Updated the PR description to follow the react-native-windows PR template format with proper sections for Description,...

Updated the PR description to reflect the latest code changes with QuirkSettings.SetUseRunOnQueueSync instead of the old EnableSyncDetachRootView approach.

@anupriya13 anupriya13 merged commit eecbc18 into main Sep 9, 2025
77 checks passed
@anupriya13 anupriya13 deleted the copilot/fix-15109-2 branch September 9, 2025 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ADO Bug RNW 41401609 | Deadlock

6 participants