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

Fix reload hang after debugger break and continue #34342

Closed
wants to merge 1 commit into from

Conversation

aeulitz
Copy link
Contributor

@aeulitz aeulitz commented Aug 3, 2022

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 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 creates a LocalConnection object that adds a page title into the inspectedContexts_ 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 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 that hangs the Hermes VM here.

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

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

Test Plan

Repro Steps

The following repro steps involve the 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 folder.
  2. Start Playground app. Adjust settings to use Hermes and direct debugging.
  3. Select a package from the package dropdown and press the "Load" button.
  4. Start Chrome and navigate to chrome://inspect. Wait a few seconds if necessary, then select "Hermes React Native" from the list of debug targets.
  5. In the Chrome Inspector window, break script execution via the "Break" button.
  6. Resume script execution via the "Play" button.
  7. 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))

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Microsoft Partner: Microsoft Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Aug 3, 2022
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,834,578 +268
android hermes armeabi-v7a 7,225,189 +378
android hermes x86 8,146,099 +452
android hermes x86_64 8,125,621 +276
android jsc arm64-v8a 9,712,584 -35
android jsc armeabi-v7a 8,465,639 -37
android jsc x86 9,661,679 -18
android jsc x86_64 10,261,065 -16

Base commit: 2e64900
Branch: main

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: c687dd3
Branch: main

@facebook-github-bot
Copy link
Contributor

@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@chrisglein
Copy link

FYI tracking issue on RNW repo is microsoft/react-native-windows#10370.
When this gets merged that issue can be unblocked to remove the fork and closed out.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @aeulitz in 60e7eb4.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Aug 8, 2022
roryabraham pushed a commit to Expensify/react-native that referenced this pull request Aug 17, 2022
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: facebook#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
roryabraham pushed a commit to Expensify/react-native that referenced this pull request Aug 17, 2022
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: facebook#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
@aeulitz aeulitz deleted the DebugHang branch September 9, 2022 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. p: Microsoft Partner: Microsoft Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants