Skip to content

Commit

Permalink
Fix reload hang after debugger break and continue (#34342)
Browse files Browse the repository at this point in the history
Summary:
When using Hermes and direct debugging, creating a new React instance within the same process hangs when the previous instance was broken into and continued with a debugger (see test Test Plan section below for repro steps). This problem is tracked by issue [9662](microsoft/react-native-windows#9662) in the react-native-windows repo.

In coarse strokes, the following code actions lead to the problem:
1. During the creation of the first React instance, the [lambda in ConnectionDemux::addPage](https://github.com/facebook/react-native/blob/main/ReactCommon/hermes/inspector/chrome/ConnectionDemux.cpp#L121) creates a LocalConnection object that adds a page title into the [inspectedContexts_](https://github.com/facebook/react-native/blob/main/ReactCommon/hermes/inspector/chrome/ConnectionDemux.h#L52) set (a singleton within the process).
2. React instance teardown does not clean up the inspectedContexts_ set.
3. Upon creating the second React instance, ConnectionDemux::enableDebugging sets [waitForDebugger](https://github.com/facebook/react-native/blob/main/ReactCommon/hermes/inspector/chrome/ConnectionDemux.cpp#L89) to true because it still finds the same title in the inspectedContexts_ set.
4. waitForDebugger being true results in the creation of a [PausedWaitEnable FSM state](https://github.com/facebook/react-native/blob/main/ReactCommon/hermes/inspector/InspectorState.h#L256) that hangs the Hermes VM [here](https://github.com/facebook/react-native/blob/main/ReactCommon/hermes/inspector/InspectorState.cpp#L118).

The change proposed here causes the inspectedContexts_ set to get cleaned up on instance teardown, thus resulting in the creation of the proper FSM starting state for the second React instance in the process.

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[General] [Fixed] - Fix reload hang after debugger break and continue

Pull Request resolved: #34342

Test Plan:
### Repro Steps

The following repro steps involve the [Playground](https://github.com/microsoft/react-native-windows/tree/main/packages/playground) app from the react-native-windows repo. This is only illustrative; the same problem should occur with any RN host.

1. Start Metro bundler in [packages/playground](https://github.com/microsoft/react-native-windows/tree/main/packages/playground) folder.
4. Start Playground app. Adjust settings to use Hermes and direct debugging.
6. Select a package from the package dropdown and press the "Load" button.
7. Start Chrome and navigate to chrome://inspect. Wait a few seconds if necessary, then select "Hermes React Native" from the list of debug targets.
8. In the Chrome Inspector window, break script execution via the "Break" button.
9. Resume script execution via the "Play" button.
10. In the Playground app, select the same or another package, then press the "Load" button.

Result: Playground app hangs while displaying "Loading bundle" banner. Note that script execution cannot be resumed via the Chrome Inspector as step 6. above has already switched the debugger frontend into a "running" state.

With the change proposed here, step 7. does not hang (package reloads and executes). I've added an automated test for this scenario in the react-native-windows scenario (https://github.com/aeulitz/react-native-windows/blob/DebugHang2/packages/debug-test/DebuggingFeatures.test.ts#L245))

Reviewed By: jpporto

Differential Revision: D38423447

Pulled By: javache

fbshipit-source-id: 3a4699dfce229bd820bf1202868599bdcb18d832
  • Loading branch information
aeulitz authored and facebook-github-bot committed Aug 8, 2022
1 parent 3f8071d commit 60e7eb4
Showing 1 changed file with 2 additions and 0 deletions.
2 changes: 2 additions & 0 deletions ReactCommon/hermes/inspector/chrome/ConnectionDemux.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,8 @@ void ConnectionDemux::removePage(int pageId) {
globalInspector_.removePage(pageId);

auto conn = conns_.at(pageId);
std::string title = conn->getTitle();
inspectedContexts_->erase(title);
conn->disconnect();
conns_.erase(pageId);
}
Expand Down

0 comments on commit 60e7eb4

Please sign in to comment.