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

Correctly bypass sync calls in UIManager during remote debugging #25162

Closed
wants to merge 1 commit into from
Closed

Correctly bypass sync calls in UIManager during remote debugging #25162

wants to merge 1 commit into from

Conversation

statm
Copy link
Contributor

@statm statm commented Jun 6, 2019

Summary

Remote debugging stopped working (since 0.58, according to #23254). See #23254 (comment) for repro steps.

The root cause is incorrect checks for Chrome debugging environment in UIManager.js.

  • In one place where sync function lazilyLoadView should be avoided, we effectively use if (__DEV__ && !global.nativeCallSyncHook) to check remote debugging, which misses ship flavor (i.e. __DEV__ is false).
  • In another place where we want to pre-populate view managers' constants to avoid calling sync function getConstantsForViewManager, if (__DEV__) is used, also missing ship flavor.

This PR fixes both checks, only using the absense of global.nativeCallSyncHook to determine remote debugging environments.

Changelog

[JavaScript] [Fixed] - Correctly bypass sync calls in UIManager during remote debugging

Test Plan

(Copying repro steps from #23254 (comment))

On a freshly created project by react-native-cli in android:

  1. Shake phone -> Dev Settings -> Uncheck JS Dev Mode and Use JS Deltas. Then back.
  2. Shake phone -> Turn on Debug JS Remotely. Then back
  3. Reload

Current behavior: Invariant Violation: requireNativeComponent: "RCTView" was not found in the UIManager.

Fixed (expected) behavior: No errors, app executes correctly.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 6, 2019
Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

Thank you for this PR. I think this should be fine.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@cpojer
Copy link
Contributor

cpojer commented Jun 6, 2019

Something is wrong with our Facebook infra, retrying…

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @statm in 417e191.

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 Jun 6, 2019
@statm statm deleted the synchook-check branch June 6, 2019 17:01
M-i-k-e-l pushed a commit to M-i-k-e-l/react-native that referenced this pull request Mar 10, 2020
…ebook#25162)

Summary:
Remote debugging stopped working (since 0.58, according to facebook#23254). See facebook#23254 (comment) for repro steps.

The root cause is incorrect checks for Chrome debugging environment in `UIManager.js`.
- In one place where sync function `lazilyLoadView` should be avoided, we effectively use `if (__DEV__ && !global.nativeCallSyncHook)` to check remote debugging, which misses ship flavor (i.e. `__DEV__` is false).
- In another place where we want to pre-populate view managers' constants to avoid calling sync function `getConstantsForViewManager`, `if (__DEV__)` is used, also missing ship flavor.

This PR fixes both checks, only using the absense of `global.nativeCallSyncHook` to determine remote debugging environments.

## Changelog

[JavaScript] [Fixed] - Correctly bypass sync calls in UIManager during remote debugging
Pull Request resolved: facebook#25162

Differential Revision: D15692492

Pulled By: cpojer

fbshipit-source-id: 173b688f140916b767fcdbbaaf68a5c303adbcd1
facebook-github-bot pushed a commit that referenced this pull request Sep 12, 2022
Summary:
This sync includes the following changes:
- **[c28f313e6](facebook/react@c28f313e6 )**: experimental_use(promise) for SSR ([#25214](facebook/react#25214)) //<Andrew Clark>//
- **[d6f9628a8](facebook/react@d6f9628a8 )**: Remove some RSC subset entry points that were removed in the main entry point ([#25209](facebook/react#25209)) //<Sebastian Markbåge>//
- **[a473d08fc](facebook/react@a473d08fc )**: Update to Flow from 0.97 to 0.122 ([#25204](facebook/react#25204)) //<Jan Kassens>//
- **[7028ce745](facebook/react@7028ce745 )**: experimental_use(promise) for Server Components ([#25207](facebook/react#25207)) //<Andrew Clark>//
- **[bfb65681e](facebook/react@bfb65681e )**: experimental_use(context)([#25202](facebook/react#25202)) //<mofeiZ>//
- **[f0efa1164](facebook/react@f0efa1164 )**: [flow] remove custom suppress comment config ([#25170](facebook/react#25170)) //<Jan Kassens>//
- **[2e7f422fe](facebook/react@2e7f422fe )**: Refactor: its type is Container ([#25153](facebook/react#25153)) //<bubucuo>//
- **[2c2d9a1df](facebook/react@2c2d9a1df )**: [eslint-plugin-react-hooks] only allow capitalized component names ([#25162](facebook/react#25162)) //<Jan Kassens>//
- **[36c908a6c](facebook/react@36c908a6c )**: Don't use the Flight terminology in public error messages ([#25166](facebook/react#25166)) //<Sebastian Markbåge>//
- **[8d1b057ec](facebook/react@8d1b057ec )**: [Flight] Minor error handling fixes ([#25151](facebook/react#25151)) //<Sebastian Markbåge>//
- **[9ff738f53](facebook/react@9ff738f53 )**: [devtools][easy] Fix flow type ([#25147](facebook/react#25147)) //<Tianyu Yao>//
- **[0de3ddf56](facebook/react@0de3ddf56 )**: Remove Symbol Polyfill (again) ([#25144](facebook/react#25144)) //<Ricky>//
- **[b36f72235](facebook/react@b36f72235 )**: Remove ReactFiberFlags MountLayoutDev and MountPassiveDev ([#25091](facebook/react#25091)) //<Samuel Susla>//
- **[b6978bc38](facebook/react@b6978bc38 )**: experimental_use(promise) ([#25084](facebook/react#25084)) //<Andrew Clark>//
- **[11ed7010c](facebook/react@11ed7010c )**: [Transition Tracing] onMarkerIncomplete - Tracing Marker/Suspense Boundary Deletions ([#24885](facebook/react#24885)) //<Luna Ruan>//
- **[b79894259](facebook/react@b79894259 )**: [Flight] Add support for Webpack Async Modules ([#25138](facebook/react#25138)) //<Sebastian Markbåge>//
- **[c8b778b7f](facebook/react@c8b778b7f )**: Fix typo: supportsMicrotask -> supportsMicrotasks ([#25142](facebook/react#25142)) //<kwzr>//
- **[d0f396651](facebook/react@d0f396651 )**: Allow functions to be used as module references ([#25137](facebook/react#25137)) //<Sebastian Markbåge>//
- **[38c5d8a03](facebook/react@38c5d8a03 )**: Test the node-register hooks in unit tests ([#25132](facebook/react#25132)) //<Sebastian Markbåge>//
- **[3f70e68ce](facebook/react@3f70e68ce )**: Return closestInstance in `getInspectorDataForViewAtPoint` ([#25118](facebook/react#25118)) //<Tianyu Yao>//
- **[3d443cad7](facebook/react@3d443cad7 )**: Update fixtures/flight to webpack 5 ([#25115](facebook/react#25115)) //<Tim Neutkens>//
- **[5d1ce6513](facebook/react@5d1ce6513 )**: Align StrictMode behaviour with production ([#25049](facebook/react#25049)) //<Samuel Susla>//
- **[9e67e7a31](facebook/react@9e67e7a31 )**: Scaffolding for useMemoCache hook ([#25123](facebook/react#25123)) //<Joseph Savona>//
- **[19e9a4c68](facebook/react@19e9a4c68 )**: Add missing createServerContext for experimental shared subset ([#25114](facebook/react#25114)) //<Jiachi Liu>//
- **[6ef466c68](facebook/react@6ef466c68 )**: make preamble and postamble types explicit and fix typo ([#25102](facebook/react#25102)) //<Josh Story>//
- **[796d31809](facebook/react@796d31809 )**: Implement basic stylesheet Resources for react-dom ([#25060](facebook/react#25060)) //<Josh Story>//
- **[32baab38f](facebook/react@32baab38f )**: [Transition Tracing] Add Tag Field to Marker Instance ([#25085](facebook/react#25085)) //<Luna Ruan>//
- **[8ef3a7c08](facebook/react@8ef3a7c08 )**: Resume immediately pinged fiber without unwinding ([#25074](facebook/react#25074)) //<Andrew Clark>//
- **[7bcc68772](facebook/react@7bcc68772 )**: Remove argument committedLanes from reappearLayoutEffects and recursivelyTraverseReappearLayoutEffects ([#25080](facebook/react#25080)) //<Samuel Susla>//
- **[ca990e9a7](facebook/react@ca990e9a7 )**: Add API to force Scheduler to yield for macrotask ([#25044](facebook/react#25044)) //<Andrew Clark>//
- **[b4204ede6](facebook/react@b4204ede6 )**: Clean up unused Deletion flag ([#24992](facebook/react#24992)) //<Andrew Clark>//
- **[e193be87e](facebook/react@e193be87e )**: [Transition Tracing] Add Offscreen Test ([#25035](facebook/react#25035)) //<Luna Ruan>//
- **[9fcaf88d5](facebook/react@9fcaf88d5 )**: Remove rootContainerInstance from unnecessary places ([#25024](facebook/react#25024)) //<Sebastian Markbåge>//
- **[80f3d8819](facebook/react@80f3d8819 )**: Mount/unmount passive effects when Offscreen visibility changes ([#24977](facebook/react#24977)) //<Andrew Clark>//

Changelog:
[General][Changed] - React Native sync for revisions 4ea064e...c28f313

Reviewed By: rickhanlonii

Differential Revision: D39384898

fbshipit-source-id: 20b080a53851d6dd9d522c8468dd02aab9ba76db
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
This sync includes the following changes:
- **[c28f313e6](facebook/react@c28f313e6 )**: experimental_use(promise) for SSR ([facebook#25214](facebook/react#25214)) //<Andrew Clark>//
- **[d6f9628a8](facebook/react@d6f9628a8 )**: Remove some RSC subset entry points that were removed in the main entry point ([facebook#25209](facebook/react#25209)) //<Sebastian Markbåge>//
- **[a473d08fc](facebook/react@a473d08fc )**: Update to Flow from 0.97 to 0.122 ([facebook#25204](facebook/react#25204)) //<Jan Kassens>//
- **[7028ce745](facebook/react@7028ce745 )**: experimental_use(promise) for Server Components ([facebook#25207](facebook/react#25207)) //<Andrew Clark>//
- **[bfb65681e](facebook/react@bfb65681e )**: experimental_use(context)([facebook#25202](facebook/react#25202)) //<mofeiZ>//
- **[f0efa1164](facebook/react@f0efa1164 )**: [flow] remove custom suppress comment config ([facebook#25170](facebook/react#25170)) //<Jan Kassens>//
- **[2e7f422fe](facebook/react@2e7f422fe )**: Refactor: its type is Container ([facebook#25153](facebook/react#25153)) //<bubucuo>//
- **[2c2d9a1df](facebook/react@2c2d9a1df )**: [eslint-plugin-react-hooks] only allow capitalized component names ([facebook#25162](facebook/react#25162)) //<Jan Kassens>//
- **[36c908a6c](facebook/react@36c908a6c )**: Don't use the Flight terminology in public error messages ([facebook#25166](facebook/react#25166)) //<Sebastian Markbåge>//
- **[8d1b057ec](facebook/react@8d1b057ec )**: [Flight] Minor error handling fixes ([facebook#25151](facebook/react#25151)) //<Sebastian Markbåge>//
- **[9ff738f53](facebook/react@9ff738f53 )**: [devtools][easy] Fix flow type ([facebook#25147](facebook/react#25147)) //<Tianyu Yao>//
- **[0de3ddf56](facebook/react@0de3ddf56 )**: Remove Symbol Polyfill (again) ([facebook#25144](facebook/react#25144)) //<Ricky>//
- **[b36f72235](facebook/react@b36f72235 )**: Remove ReactFiberFlags MountLayoutDev and MountPassiveDev ([facebook#25091](facebook/react#25091)) //<Samuel Susla>//
- **[b6978bc38](facebook/react@b6978bc38 )**: experimental_use(promise) ([facebook#25084](facebook/react#25084)) //<Andrew Clark>//
- **[11ed7010c](facebook/react@11ed7010c )**: [Transition Tracing] onMarkerIncomplete - Tracing Marker/Suspense Boundary Deletions ([facebook#24885](facebook/react#24885)) //<Luna Ruan>//
- **[b79894259](facebook/react@b79894259 )**: [Flight] Add support for Webpack Async Modules ([facebook#25138](facebook/react#25138)) //<Sebastian Markbåge>//
- **[c8b778b7f](facebook/react@c8b778b7f )**: Fix typo: supportsMicrotask -> supportsMicrotasks ([facebook#25142](facebook/react#25142)) //<kwzr>//
- **[d0f396651](facebook/react@d0f396651 )**: Allow functions to be used as module references ([facebook#25137](facebook/react#25137)) //<Sebastian Markbåge>//
- **[38c5d8a03](facebook/react@38c5d8a03 )**: Test the node-register hooks in unit tests ([facebook#25132](facebook/react#25132)) //<Sebastian Markbåge>//
- **[3f70e68ce](facebook/react@3f70e68ce )**: Return closestInstance in `getInspectorDataForViewAtPoint` ([facebook#25118](facebook/react#25118)) //<Tianyu Yao>//
- **[3d443cad7](facebook/react@3d443cad7 )**: Update fixtures/flight to webpack 5 ([facebook#25115](facebook/react#25115)) //<Tim Neutkens>//
- **[5d1ce6513](facebook/react@5d1ce6513 )**: Align StrictMode behaviour with production ([facebook#25049](facebook/react#25049)) //<Samuel Susla>//
- **[9e67e7a31](facebook/react@9e67e7a31 )**: Scaffolding for useMemoCache hook ([facebook#25123](facebook/react#25123)) //<Joseph Savona>//
- **[19e9a4c68](facebook/react@19e9a4c68 )**: Add missing createServerContext for experimental shared subset ([facebook#25114](facebook/react#25114)) //<Jiachi Liu>//
- **[6ef466c68](facebook/react@6ef466c68 )**: make preamble and postamble types explicit and fix typo ([facebook#25102](facebook/react#25102)) //<Josh Story>//
- **[796d31809](facebook/react@796d31809 )**: Implement basic stylesheet Resources for react-dom ([facebook#25060](facebook/react#25060)) //<Josh Story>//
- **[32baab38f](facebook/react@32baab38f )**: [Transition Tracing] Add Tag Field to Marker Instance ([facebook#25085](facebook/react#25085)) //<Luna Ruan>//
- **[8ef3a7c08](facebook/react@8ef3a7c08 )**: Resume immediately pinged fiber without unwinding ([facebook#25074](facebook/react#25074)) //<Andrew Clark>//
- **[7bcc68772](facebook/react@7bcc68772 )**: Remove argument committedLanes from reappearLayoutEffects and recursivelyTraverseReappearLayoutEffects ([facebook#25080](facebook/react#25080)) //<Samuel Susla>//
- **[ca990e9a7](facebook/react@ca990e9a7 )**: Add API to force Scheduler to yield for macrotask ([facebook#25044](facebook/react#25044)) //<Andrew Clark>//
- **[b4204ede6](facebook/react@b4204ede6 )**: Clean up unused Deletion flag ([facebook#24992](facebook/react#24992)) //<Andrew Clark>//
- **[e193be87e](facebook/react@e193be87e )**: [Transition Tracing] Add Offscreen Test ([facebook#25035](facebook/react#25035)) //<Luna Ruan>//
- **[9fcaf88d5](facebook/react@9fcaf88d5 )**: Remove rootContainerInstance from unnecessary places ([facebook#25024](facebook/react#25024)) //<Sebastian Markbåge>//
- **[80f3d8819](facebook/react@80f3d8819 )**: Mount/unmount passive effects when Offscreen visibility changes ([facebook#24977](facebook/react#24977)) //<Andrew Clark>//

Changelog:
[General][Changed] - React Native sync for revisions 4ea064e...c28f313

Reviewed By: rickhanlonii

Differential Revision: D39384898

fbshipit-source-id: 20b080a53851d6dd9d522c8468dd02aab9ba76db
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants