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

RCTScreenScale may deadlock #18096

Closed
stephan-tolksdorf opened this issue Feb 26, 2018 · 8 comments
Closed

RCTScreenScale may deadlock #18096

stephan-tolksdorf opened this issue Feb 26, 2018 · 8 comments

Comments

@stephan-tolksdorf
Copy link

@stephan-tolksdorf stephan-tolksdorf commented Feb 26, 2018

RCTScreenScale will deadlock if it called for the first time on a non-main thread and the main thread is waiting synchronously for the thread on which RCTScreenScale is called, because the dispatch_sync(dispatch_get_main_queue(), ...) in RCTUnsafeExecuteOnMainQueueOnceSync will deadlock in that scenario. I don't know whether this can actually happen in a React Native app, but if not it should probably be commented on in the code (e.g. to prevent people from copying the code without taking this issue into account).

Because of this issue, having a general helper function like RCTUnsafeExecuteOnMainQueueOnceSync is problematic. For use cases like RCTScreenScale it's usually better to do the initialization work eagerly at app startup or library load time (e.g. in an Objective-C category load method).

Apart from this general issue, RCTUnsafeExecuteOnMainQueueOnceSync also misses a compiler barrier in the second if statement, see the dispatch_once inline function implementation in more recent libdispatch versions, e.g. https://github.com/apple/swift-corelibs-libdispatch/blob/b5ec5d8dfa59426912fbb4884fab642804b99827/dispatch/once.h#L86

Also, if you copy the internals of dispatch_once, I'd use exactly the same predicate test against ~0L instead of 0, just in case that it may matter for some obscure reason.

Finally, I'd like to point out that RCTScreenScale currently doesn't handle the case where the scale of the mainScreen changes, which I think could happen on the Apple TV.

@react-native-bot
Copy link

@react-native-bot react-native-bot commented Feb 26, 2018

Thanks for posting this! It looks like your issue is missing some required information. Can you please add all the details specified in the Issue Template? This is necessary for people to be able to understand and reproduce your issue.

I am going to close this, but please feel free to open a new issue with the additional information provided. Thanks!

How to ContributeWhat to Expect from Maintainers

@sophiebits
Copy link
Contributor

@sophiebits sophiebits commented Feb 26, 2018

@shergin Care to comment? I believe that we never dispatch synchronously from the main thread to this RN thread, precisely to avoid deadlock, but I am not sure.

It’s true that screen size can change and I don’t think our APIs are really set up to accommodate that at the moment. :(

@mmmulani
Copy link
Contributor

@mmmulani mmmulani commented Feb 26, 2018

weird, the bot closed this when it shouldn't have.

we could call the API inside +initilaize to ensure that it's on the main thread.

And yea, the caching isn't ideal either.

@stephan-tolksdorf stephan-tolksdorf changed the title RCTScreenSize may lead to deadlock RCTScreenScale may deadlock Feb 26, 2018
@hramos
Copy link
Contributor

@hramos hramos commented Feb 26, 2018

It does look like the bot worked as expected here: the issue as of this writing still does not conform to the issue template outlined in https://raw.githubusercontent.com/facebook/react-native/master/.github/ISSUE_TEMPLATE.md.

@shergin
Copy link
Contributor

@shergin shergin commented Feb 27, 2018

@stephan-tolksdorf Thank you for reporting that... and for bunch of advices!

RCTScreenScale will deadlock if it called for the first time on a non-main thread and the main thread is waiting synchronously...

Yes, I am aware of that. Theoretically, this might happen only if RCTSurface was used for instantiating a RootView. This (Surface) is very new and experimental API, so this should not affect real production cases. I plan to fix it soon anyways.
The golden rule of RN is: Never ever block UIManager queue waiting for the main queue. And there is two places where we still does not follow this rule, one of them is RCTScreenScale. 😞

Because of this issue, having a general helper function like RCTUnsafeExecuteOnMainQueueOnceSync is problematic.

Well, it's pretty low-level system helper, it's not supposed to be used outside the RN Core. It even has "Unsafe" in the name which kinda implied that... if you are using this, you know what you are going.

Finally, I'd like to point out that RCTScreenScale currently doesn't handle the case where the scale of the mainScreen changes, which I think could happen on the Apple TV.

Yeah, it is also known issue. It will work perfectly with AppleTV, but it will probably not work for attached/connected external displays.

@stephan-tolksdorf
Copy link
Author

@stephan-tolksdorf stephan-tolksdorf commented Feb 27, 2018

Thanks for replying!

RCTUtils.h seems to be part of the public API and in an embedding scenario someone might be tempted to call RCTScreenScale or RCTScreenSize before using other parts of React Native. Even if you don't want to support this use case, it might still be worth documenting the current thread-safety limitation of these functions.

Since React Native has a huge audience and many developers are looking for inspiration in its sources, it might be worth explaining the sharp edges of tools like RCTUnsafeExecuteOnMainQueueOnceSync in short code comments, even if these are obvious to the core developers.

(And if you want to continue to use RCTUnsafeExecuteOnMainQueueOnceSync internally, it would probably be prudent to add the missing compiler barrier I mentioned above.)

@hramos
Copy link
Contributor

@hramos hramos commented Jan 31, 2019

What's the status of this issue, can it be closed?

@hramos hramos removed the Bug Report label Feb 6, 2019
@hramos hramos removed the Core Team label Mar 8, 2019
@cpojer cpojer mentioned this issue Mar 19, 2019
59 of 161 tasks complete
@Titozzz
Copy link
Contributor

@Titozzz Titozzz commented Mar 19, 2019

@shergin Do you think we can close this issue?

@cpojer cpojer closed this May 9, 2019
facebook-github-bot added a commit that referenced this issue Mar 16, 2020
Summary:
This sync includes the following changes:
- **[b5c6dd2de](facebook/react@b5c6dd2 )**: Don't use Spread in DevTools Injection (#18277) //<Sebastian Markbåge>//
- **[a463fef31](facebook/react@a463fef )**: Revert "[React Native] Add getInspectorDataForViewAtPoint (#18233)" //<Sebastian Markbage>//
- **[dc7eedae3](facebook/react@dc7eeda )**: Encode server rendered host components as array tuples (#18273) //<Sebastian Markbåge>//
- **[bf351089a](facebook/react@bf35108 )**: [React Native] Add getInspectorDataForViewAtPoint (#18233) //<Ricky>//
- **[99d737186](facebook/react@99d7371 )**: [Flight] Split Streaming from Relay Implemenation (#18260) //<Sebastian Markbåge>//
- **[160505b0c](facebook/react@160505b )**: ReactDOM.useEvent: Add more scaffolding for useEvent hook (#18271) //<Dominic Gannaway>//
- **[526c12f49](facebook/react@526c12f )**: Enable enableProfilerCommitHooks flag for FB (#18230) //<Brian Vaughn>//
- **[29534252a](facebook/react@2953425 )**: ReactDOM.useEvent add flag and entry point (#18267) //<Dominic Gannaway>//
- **[704c8b011](facebook/react@704c8b0 )**: Fix Flow type for AnyNativeEvent (#18266) //<Dominic Gannaway>//
- **[bdc5cc463](facebook/react@bdc5cc4 )**: Add Relay Flight Build (#18242) //<Sebastian Markbåge>//
- **[7a1691cdf](facebook/react@7a1691c )**: Refactor Host Config Infra (getting rid of .inline*.js) (#18240) //<Sebastian Markbåge>//
- **[238b57f0f](facebook/react@238b57f )**: [Blocks] Make it possible to have lazy initialized and lazy loaded Blocks (#18220) //<Sebastian Markbåge>//
- **[235a6c4af](facebook/react@235a6c4 )**: Bugfix: Dropped effects in Legacy Mode Suspense (#18238) //<Andrew Clark>//
- **[562cf013d](facebook/react@562cf01 )**: Add a flag to disable module pattern components (#18133) //<Dan Abramov>//
- **[115cd12d9](facebook/react@115cd12 )**: Add test run that uses www feature flags (#18234) //<Andrew Clark>//
- **[4027f2a3b](facebook/react@4027f2a )**: Break up require/import statements in strings (#18222) //<Christoph Nakazawa>//
- **[024a76431](facebook/react@024a764 )**: Implemented Profiler onCommit() and onPostCommit() hooks (#17910) //<Brian Vaughn>//
- **[d35f8a581](facebook/react@d35f8a5 )**: feat: honor displayName of context types (#18224) //<Brian Vaughn>//
- **[3ee812e6b](facebook/react@3ee812e )**: Revert "feat: honor displayName of context types (#18035)" (#18223) //<Dominic Gannaway>//
- **[6a0efddd8](facebook/react@6a0efdd )**: Modern Event System: export internal FB flag for testing (#18221) //<Dominic Gannaway>//
- **[fa03206ee](facebook/react@fa03206 )**: Remove _ctor field from Lazy components (#18217) //<Sebastian Markbåge>//
- **[2fe0fbb05](facebook/react@2fe0fbb )**: Use accumulateTwoPhaseDispatchesSingle directly (#18203) //<Dominic Gannaway>//
- **[503fd82b4](facebook/react@503fd82 )**: Modern Event System: Add support for internal FB Primer (#18210) //<Dominic Gannaway>//
- **[45c172d94](facebook/react@45c172d )**: feat: honor displayName of context types (#18035) //<Brian Vaughn>//
- **[ec652f4da](facebook/react@ec652f4 )**: Bugfix: Expired partial tree infinite loops (#17949) //<Andrew Clark>//
- **[d2158d6cc](facebook/react@d2158d6 )**: Fix flow types (#18204) //<Brian Vaughn>//
- **[7e83af17c](facebook/react@7e83af1 )**: Put React.jsx and React.jsxDEV behind experimental build (#18023) //<Luna Ruan>//
- **[8cb2fb21e](facebook/react@8cb2fb2 )**: Refine isFiberSuspenseAndTimedOut (#18184) //<Dominic Gannaway>//
- **[62861bbcc](facebook/react@62861bb )**: More event system cleanup and scaffolding (#18179) //<Dominic Gannaway>//
- **[8ccfce460](facebook/react@8ccfce4 )**: Only use Rollup's CommonJS plugin for "react-art" (#18186) //<Sebastian Markbåge>//
- **[c26506a7d](facebook/react@c26506a )**: Update react-shallow-renderer from 16.12.0 to 16.13.0 (#18185) //<Minh Nguyen>//
- **[26aa1987c](facebook/react@26aa198 )**: [Native] Enable and remove targetAsInstance feature flag. (#18182) //<Eli White>//
- **[4469700bb](facebook/react@4469700 )**: Change ReactVersion from CJS to ES module (#18181) //<Sebastian Markbåge>//
- **[58eedbb02](facebook/react@58eedbb )**: Check in a forked version of object-assign only for UMD builds (#18180) //<Sebastian Markbåge>//
- **[053347e6b](facebook/react@053347e )**: react-test-renderer: improve findByType() error message (#17439) //<Henry Q. Dineen>//
- **[4ee592e95](facebook/react@4ee592e )**: Add an early invariant to debug a mystery crash (#18159) //<Dan Abramov>//
- **[7ea4e4111](facebook/react@7ea4e41 )**: Fix typo in warning text (#18103) //<Sophie Alpert>//
- **[79a25125b](facebook/react@79a2512 )**: feat: add recommended config eslint rule (#14762) //<Simen Bekkhus>//
- **[ae60caacf](facebook/react@ae60caa )**: [Fabric] Fix targetAsInstance dispatchEvent "cannot read property of null" (#18156) //<Joshua Gross>//
- **[d72700ff5](facebook/react@d72700f )**: Remove runtime dependency on prop-types (#18127) //<Dan Abramov>//
- **[549e41883](facebook/react@549e418 )**: Move remaining things to named exports (#18165) //<Sebastian Markbåge>//
- **[739f20bed](facebook/react@739f20b )**: Remove Node shallow builds (#18157) //<Sebastian Markbåge>//
- **[3e809bf5d](facebook/react@3e809bf )**: Convert React Native builds to named exports (#18136) //<Sebastian Markbåge>//
- **[869dbda72](facebook/react@869dbda )**: Don't build shallow renderer for FB (#18153) //<Dan Abramov>//
- **[293878e07](facebook/react@293878e )**: Replace ReactShallowRenderer with a dependency (#18144) //<Minh Nguyen>//
- **[b4e314891](facebook/react@b4e3148 )**: Remove unused flag (#18132) //<Dan Abramov>//
- **[849e8328b](facebook/react@849e832 )**: Remove unnecessary warnings (#18135) //<Dan Abramov>//
- **[f9c0a4544](facebook/react@f9c0a45 )**: Convert the rest of react-dom and react-test-renderer to Named Exports (#18145) //<Sebastian Markbåge>//
- **[c1c5499cc](facebook/react@c1c5499 )**: update version numbers for 16.13 (#18143) //<Sunil Pai>//
- **[e1c7e651f](facebook/react@e1c7e65 )**: Update ReactDebugHooks to handle composite hooks (#18130) //<Brian Vaughn>//
- **[d28bd2994](facebook/react@d28bd29 )**: remove OSS testing builds (#18138) //<Sunil Pai>//
- **[8e13e770e](facebook/react@8e13e77 )**: Remove /testing entry point from 'react' package (#18137) //<Sebastian Markbåge>//
- **[60016c448](facebook/react@60016c4 )**: Export React as Named Exports instead of CommonJS (#18106) //<Sebastian Markbåge>//
- **[8d7535e54](facebook/react@8d7535e )**: Add nolint to FB bundle headers (#18126) //<Dominic Gannaway>//
- **[bf13d3e3c](facebook/react@bf13d3e )**: [eslint-plugin-react-hooks] Fix cyclic caching for loops containing a… (#16853) //<Moji Izadmehr>//
- **[501a78881](facebook/react@501a788 )**: runAllPassiveEffectDestroysBeforeCreates's feature flag description typo fixed (#18115) //<adasq>//
- **[09348798a](facebook/react@0934879 )**: Codemod to import * as React from "react"; (#18102) //<Sebastian Markbåge>//
- **[78e816032](facebook/react@78e8160 )**: Don't warn about unmounted updates if pending passive unmount (#18096) //<Brian Vaughn>//
- **[2c4221ce8](facebook/react@2c4221c )**: Change string refs in function component message (#18031) //<Sebastian Markbåge>//
- **[65bbda7f1](facebook/react@65bbda7 )**: Rename Chunks API to Blocks (#18086) //<Sebastian Markbåge>//
- **[8b596e00a](facebook/react@8b596e0 )**: Remove unused arguments in the reconciler (#18092) //<Dan Abramov>//
- **[5de5b6150](facebook/react@5de5b61 )**: Bugfix: `memo` drops lower pri updates on bail out (#18091) //<Andrew Clark>//
- **[abfbae02a](facebook/react@abfbae0 )**: Update Rollup version to 1.19.4 and fix breaking changes (#15037) //<Kunuk Nykjær>//
- **[b789060dc](facebook/react@b789060 )**: Feature Flag for React.jsx` "spreading a key to jsx" warning (#18074) //<Sunil Pai>//
- **[3f85d53ca](facebook/react@3f85d53 )**: Further pre-requisite changes to plugin event system (#18083) //<Dominic Gannaway>//
- **[ea6ed3dbb](facebook/react@ea6ed3d )**: Warn for update on different component in render (#17099) //<Andrew Clark>//
- **[085d02133](facebook/react@085d021 )**: [Native] Migrate focus/blur to call TextInputState with the host component (#18068) //<Eli White>//
- **[1000f6135](facebook/react@1000f61 )**: Add container to event listener signature (#18075) //<Dominic Gannaway>//
- **[a12dd52a4](facebook/react@a12dd52 )**: Don't build some packages for WWW (#18078) //<Dan Abramov>//
- **[2512c309e](facebook/react@2512c30 )**: Remove Flare bundles from build (#18077) //<Dominic Gannaway>//
- **[4912ba31e](facebook/react@4912ba3 )**: Add modern event system flag + rename legacy plugin module (#18073) //<Dominic Gannaway>//
- **[4d9f85006](facebook/react@4d9f850 )**: Re-throw errors thrown by the renderer at the root in the complete phase (#18029) //<Andrew Clark>//
- **[14afeb103](facebook/react@14afeb1 )**: Added missing feature flag //<Brian Vaughn>//
- **[691096c95](facebook/react@691096c )**: Split recent passive effects changes into 2 flags (#18030) //<Brian Vaughn>//
- **[56d8a73af](facebook/react@56d8a73 )**: [www] Disable Scheduler `timeout` w/ dynamic flag (#18069) //<Andrew Clark>//
- **[d533229fb](facebook/react@d533229 )**: Fix Prettier //<Dan Abramov>//
- **[56a8c3532](facebook/react@56a8c35 )**: eslint-plugin-react-hooks@2.4.0 //<Dan Abramov>//
- **[93a229bab](facebook/react@93a229b )**: Update eslint rule exhaustive deps to use new suggestions feature (#17385) //<Will Douglas>//
- **[9def56ec0](facebook/react@9def56e )**: Refactor DOM plugin system to single module (#18025) //<Dominic Gannaway>//
- **[2d6be757d](facebook/react@2d6be75 )**: [Native] Delete NativeComponent and NativeMethodsMixin (#18036) //<Eli White>//
- **[d4f2b0379](facebook/react@d4f2b03 )**: Add Auto Import to Babel Plugin  (#16626) //<Luna Ruan>//
- **[8777b44e9](facebook/react@8777b44 )**: Add Modern WWW build (#18028) //<Dan Abramov>//
- **[a607ea4c4](facebook/react@a607ea4 )**: Remove getIsHydrating (#18019) //<Dan Abramov>//
- **[f7278034d](facebook/react@f727803 )**: Flush all passive destroy fns before calling create fns (#17947) //<Brian Vaughn>//
- **[529e58ab0](facebook/react@529e58a )**: Remove legacy www config from Rollup build (#18016) //<Dominic Gannaway>//
- **[42918f40a](facebook/react@42918f4 )**: Change build from babylon to babel (#18015) //<Dominic Gannaway>//
- **[df5faddcc](facebook/react@df5fadd )**: Refactor commitPlacement to recursively insert nodes (#17996) //<Dominic Gannaway>//
- **[517de74b0](facebook/react@517de74 )**: Tweak comment wording (#18007) //<Dan Abramov>//
- **[b63cb6f6c](facebook/react@b63cb6f )**: Update ReactFiberExpirationTime.js (#17825) //<haseeb>//
- **[89c6042df](facebook/react@89c6042 )**: fix: typo in test (#18005) //<Jesse Katsumata>//
- **[4f71f25a3](facebook/react@4f71f25 )**: Re-enable shorthand CSS property collision warning (#18002) //<Sophie Alpert>//
- **[c55c34e46](facebook/react@c55c34e )**: Move React Map child check to behind flags or __DEV__ (#17995) //<Dominic Gannaway>//
- **[3f814e758](facebook/react@3f814e7 )**: Fix Flow type for React Native (#17992) //<Dan Abramov>//
- **[256d78d11](facebook/react@256d78d )**: Add feature flag for removing children Map support (#17990) //<Dominic Gannaway>//
- **[9dba218d9](facebook/react@9dba218 )**: [Mock Scheduler] Mimic browser's advanceTime (#17967) //<Andrew Clark>//
- **[d6e08fe0a](facebook/react@d6e08fe )**: Remove Suspense priority warning (#17971) //<Dan Abramov>//
- **[812277dab](facebook/react@812277d )**: Fix onMouseEnter is fired on disabled buttons (#17675) //<Alfredo Granja>//
- **[3e9251d60](facebook/react@3e9251d )**: make testing builds for React/ReactDOM (#17915) //<Sunil Pai>//
- **[ace9e8134](facebook/react@ace9e81 )**: Simplify Continuous Hydration Targets (#17952) //<Sebastian Markbåge>//
- **[7df32c4c8](facebook/react@7df32c4 )**: Flush `useEffect` clean up functions in the passive effects phase (#17925) //<Brian Vaughn>//
- **[434770c3b](facebook/react@434770c )**: Add beforeRemoveInstance method to ReactNoop (#17959) //<Dominic Gannaway>//
- **[6ae2c33a7](facebook/react@6ae2c33 )**: StrictMode should call sCU twice in DEV (#17942) //<Brian Vaughn>//
- **[9dbe1c54d](facebook/react@9dbe1c5 )**: Revert "Bugfix: Expiring a partially completed tree (#17926)" (#17941) //<Andrew Clark>//
- **[b2382a715](facebook/react@b2382a7 )**: Add ReactDOM.unstable_renderSubtreeIntoContainer warning flag (#17936) //<Dominic Gannaway>//
- **[01974a867](facebook/react@01974a8 )**: Bugfix: Expiring a partially completed tree (#17926) //<Andrew Clark>//

Changelog:
[General][Changed] - React Native sync for revisions 241c446...b5c6dd2

Reviewed By: gaearon

Differential Revision: D20347361

fbshipit-source-id: e9e6282474ab6471585e8e7fb6ea8518aa48390d
@facebook facebook locked as resolved and limited conversation to collaborators May 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants