Enforced thread safety on UIImplementation methods that mutate the shadowNodeRegistry #20025

SudoPlz · 2018-07-04T10:11:18Z

Changelog

[ANDROID] [Fixes] - UIImplementation: Now enforcing atomic mutation of the shadowNodeRegistry

pull-bot · 2018-07-04T10:13:24Z

	Messages
📖	📋 Missing Summary - Can you add a Summary? To do so, add a "## Summary" section to your PR description. This is a good place to explain the motivation for making this change.
📖	📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.
📖	📋 Changelog Format - Did you include a Changelog? A changelog entry has the following format: `[CATEGORY] [TYPE] - Message`. CATEGORY may be: General iOS Android TYPE may be: Added, for new features. Changed, for changes in existing functionality. Deprecated, for soon-to-be removed features. Removed, for now removed features. Fixed, for any bug fixes. Security, in case of vulnerabilities. MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

Generated by 🚫 dangerJS against 6bfe5dd

SudoPlz · 2018-07-05T13:10:56Z

I didn't change anything on iOS, not sure what's wrong with those tests :/

xkrsz · 2018-07-10T11:28:23Z

Those two tests are broken currently, don't bother yourself.

xkrsz · 2018-07-10T11:34:05Z

I've found that your fix also resolves a couple of errors we're experiencing in production (react-native v0.47.1). Here's the list:

Attempt to invoke virtual method 'void com.facebook.react.uimanager.ReactShadowNode.addChildAt(com.facebook.react.uimanager.ReactShadowNode, int)' on a null object reference
Attempt to invoke virtual method 'void com.facebook.react.uimanager.ReactShadowNode.addChildAt(com.facebook.react.uimanager.ReactShadowNode, int)' on a null object reference
Attempt to invoke virtual method 'com.facebook.react.uimanager.ThemedReactContext com.facebook.react.uimanager.ReactShadowNode.getThemedContext()' on a null object reference
IllegalViewOperationExceptioncom.facebook.react.uimanager.UIImplementation in setChildren: Trying to add unknown view tag: 292

Big thumbs up, hope this will be merged soon.

xkrsz · 2018-07-10T11:36:31Z

Fixes #20078

patrickkempff · 2018-07-10T17:36:18Z

Can confirm that this PR fixes this exception we had with the native Android ViewPager in production (0.55.3):

Fatal Exception: com.facebook.react.uimanager.IllegalViewOperationException
Trying to add unknown view tag: 3391

SudoPlz · 2018-07-11T00:17:15Z

Good. Let's hope to hear from someone in Facebook soon.

janicduplessis · 2018-07-15T05:46:10Z

Do you know which method is accessed from a different thread? I'd rather avoid locking by making sure calls are made from a single thread (unless we really have to).

cc @mdvacca

SudoPlz · 2018-07-15T06:49:31Z

Anything that mutates the cssNode sparse array list is unsafe

react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/ShadowNodeRegistry.java

Line 20 in 5c2720b

private final SparseArray<ReactShadowNode> mTagsToCSSNodes;

LorienHW · 2018-08-02T04:14:46Z

really eager to see this merged! using react-native-navigation we see these crashes in production constantly, thanks so much for this!

facebook-github-bot · 2018-08-03T17:06:56Z

@SudoPlz I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

SudoPlz · 2018-08-03T17:10:45Z

Hmm maybe @mkonicek can take a look?

krishanmarco · 2018-08-21T06:07:32Z

Any news on this?
Is there anything that can be done to speed up the process to merge this?

Aaang · 2018-09-03T11:43:54Z

Hi everyone,

we're also seeing many crashes on production caused by the mentioned issue(s) that this PR would fix, therefore we'd love to see this PR being reviewed and merged, so we don't have to (for the first time in 2 years) use a fork instead of the official RN version or add a custom UIImplementation handler as a workaround only to fix it.
Is there anything we can do to support you?

Since this PR didn't get much attention lately, maybe @sophiebits could take a look?
Any progress is very much appreciated!

alimek · 2018-09-03T11:50:39Z

same issues in my app on production :(

matthargett · 2018-09-03T16:19:02Z

@SudoPlz can you respond to my review comments? I was waiting on that before lobbying other contributors. FWIW, RN Windows has a similar fix.

matthargett

Can the locks be finer-grained? It seems like there are more things in the synchronized() block than need to be.

ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java

levynir · 2018-10-18T14:06:00Z

Hey, any update on this?

alimek · 2018-10-18T16:07:21Z

maybe its worth ask: @SudoPlz and @matthargett :-)

SudoPlz · 2018-10-18T16:12:40Z

I have no idea what the future of this PR is, the Facebook team is heavily working on Fabric right now, so I'd love their 2 cents on wether they'll be accepting this or not, or if they have another way of fixing it.

SudoPlz · 2018-10-18T16:14:49Z

By the way someone made a replication repo here https://github.com/Frank1234/RNViewPager
(looks like you can steadily replicate the crash - unless you use this PR - steadily by tapping that button a couple of times)

I haven't tested it yet though.

levynir · 2018-10-21T07:53:33Z

@matthargett , could you guys take this on please? Clearly the failed tests have nothing to do with @SudoPlz 's changes and this is causing production crashes for many users. Would be happy to help if there is anything I can do, but I suspect this is a bit out of my league.

kelset · 2018-11-30T09:38:32Z

uhm the Detox fail doesn't seem to be related to this, I think I've seen it in other PRs too - I feel it may be something broken in master.

hramos · 2018-12-11T21:13:49Z

Would you mind adding a test plan to your PR?

SudoPlz · 2018-12-11T21:40:40Z

@hramos not sure how to do that? Any examples?

kelset · 2018-12-12T09:32:26Z

@SudoPlz you can check the label https://github.com/facebook/react-native/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc+label%3A%22%E2%9C%85Test+Plan%22, but basically it's at least a description of how to repro and how to ensure that there is a way to verify that the fix actually does the fix :)

shergin

Thanks for doing that! ❤️
Could you please fix some nits?

@mdvacca Should bless this before landing.

ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java

cpojer · 2019-02-21T05:31:32Z

@SudoPlz could you rebase your PR and apply the changes requested by @shergin so we can merge this?

SudoPlz · 2019-02-21T13:07:45Z

Rebased, and removed the comments.

SudoPlz · 2019-02-21T13:59:30Z

@shergin @cpojer all set.

The0racle · 2019-03-26T21:40:41Z

Seeing crashes on my app as well related to this. Were these issues fixed somewhere else?

Thanks!

cpojer · 2019-03-27T14:54:10Z

Oh wow, I'm sorry I missed this last time. Seems like it is outdated again. Would you mind rebasing it once more? Then I'll make sure we can get it reviewed and merged.

…ode children sparse array - Fixes: facebook#17178 (comment)

SudoPlz · 2019-03-27T16:23:14Z

@cpojer I've just rebased again, that should do the trick.

facebook-github-bot

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

react-native-bot · 2019-03-27T21:45:06Z

This pull request was successfully merged by @SudoPlz in f149426.

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

LorienHW · 2019-03-27T21:47:40Z

yaaaas!

mhdtouban · 2019-07-12T21:51:45Z

@cpojer We are on RN v 0.59 and we still getting this issue, have you verified the solution ? in which version it was merged?

jstheoriginal · 2019-07-21T16:39:52Z

@mhdtouban based on the tags of the merge commit, it looks like it's only in 0.60 and later.

v0.60.4 v0.60.3 v0.60.2 v0.60.1 v0.60.0 v0.60.0-rc.3 v0.60.0-rc.2 v0.60.0-rc.1 v0.60.0-rc.0 latest 0.60.2

ikesyo · 2019-09-20T10:21:34Z

This was actually backported and released in v0.59.5: https://github.com/react-native-community/releases/blob/master/CHANGELOG.md#v0595

Enforced thread safety on UIImplementation methods that mutate the shadowNodeRegistry (f5a318 by @SudoPlz)

Summary: Base sync before adding Flight files. This sync includes the following changes: - **[454c2211c](facebook/react@454c2211c )**: Refactor SchedulerHostConfigs ([#20025](facebook/react#20025)) //<Ricky>// - **[56e9feead](facebook/react@56e9feead )**: Remove Blocks ([#20138](facebook/react#20138)) //<Sebastian Markbåge>// - **[3fbd47b86](facebook/react@3fbd47b86 )**: Serialize pending server components by reference (lazy component) ([#20137](facebook/react#20137)) //<Sebastian Markbåge>// - **[930ce7c15](facebook/react@930ce7c15 )**: Allow values to be encoded by "reference" to a value rather than the value itself ([#20136](facebook/react#20136)) //<Sebastian Markbåge>// - **[39eb6d176](facebook/react@39eb6d176 )**: Rename ([#20134](facebook/react#20134)) //<Sebastian Markbåge>// - **[ffd842335](facebook/react@ffd842335 )**: [Flight] Add support for Module References in transport protocol ([#20121](facebook/react#20121)) //<Sebastian Markbåge>// - **[343d7a4a7](facebook/react@343d7a4a7 )**: Fast Refresh: Don't block DevTools commit hook ([#20129](facebook/react#20129)) //<Brian Vaughn>// - **[779a472b0](facebook/react@779a472b0 )**: Prevent inlining into recursive commit functions ([#20105](facebook/react#20105)) //<Andrew Clark>// - **[25b18d31c](facebook/react@25b18d31c )**: Traverse commit phase effects iteratively ([#20094](facebook/react#20094)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions 4e5d7fa...454c221 Reviewed By: rickhanlonii Differential Revision: D24698701 fbshipit-source-id: dfaf692b1051150355dece1657764a484b7ae603

ngioldasis · 2021-11-26T12:24:42Z

Hi,
I'm experiencing a "similar" error with the stack-trace below.

My environment is:

react-native: 0.66.3
react: 17.0.2
react-navigation: 6.0.6

The error occurs occasionally on android device and emulator, while navigating from one screen to another in a stack navigator.

Unfortunately, I cannot reproduce the error.

Any idea?

unknown:ReactNative: Exception in native call com.facebook.react.uimanager.IllegalViewOperationException: Trying to update non-existent view with tag 3203 at com.facebook.react.uimanager.UIImplementation.updateView(UIImplementation.java:287) at com.facebook.react.uimanager.UIManagerModule.updateView(UIManagerModule.java:519) at java.lang.reflect.Method.invoke(Native Method) at com.facebook.react.bridge.JavaMethodWrapper.invoke(JavaMethodWrapper.java:372) at com.facebook.react.bridge.JavaModuleWrapper.invoke(JavaModuleWrapper.java:188) at com.facebook.react.bridge.queue.NativeRunnable.run(Native Method) at android.os.Handler.handleCallback(Handler.java:900) at android.os.Handler.dispatchMessage(Handler.java:103) at com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage(MessageQueueThreadHandler.java:27) at android.os.Looper.loop(Looper.java:219) at com.facebook.react.bridge.queue.MessageQueueThreadImpl$4.run(MessageQueueThreadImpl.java:226) at java.lang.Thread.run(Thread.java:929)

pke · 2022-02-14T17:06:21Z

We are also getting this error still in production only:

com.facebook.react.uimanager.IllegalViewOperationException: Trying to remove a view index above child count 2 view tag: 3175
detail: View tag:3175 View Type:class com.facebook.react.views.view.ReactViewGroup
children(2): [
6957,7837,
],
indicesToRemove(1): [
2,
],
tagsToDelete(1): [
13989,
]
at com.facebook.react.uimanager.NativeViewHierarchyManager.manageChildren(NativeViewHierarchyManager.java
  at com.facebook.react.uimanager.UIViewOperationQueue$ManageChildrenOperation.execute (UIViewOperationQueue.java:217)
  at com.facebook.react.uimanager.UIViewOperationQueue$1.run (UIViewOperationQueue.java:915)
  at com.facebook.react.uimanager.UIViewOperationQueue.flushPendingBatches (UIViewOperationQueue.java:1026)
  at com.facebook.react.uimanager.UIViewOperationQueue.access$2600 (UIViewOperationQueue.java:47)
  at com.facebook.react.uimanager.UIViewOperationQueue$DispatchUIFrameCallback.doFrameGuarded (UIViewOperationQueue.java:1086)
  at com.facebook.react.uimanager.GuardedFrameCallback.doFrame (GuardedFrameCallback.java:29)
  at com.facebook.react.modules.core.ReactChoreographer$ReactChoreographerDispatcher.doFrame (ReactChoreographer.java:175)
  at com.facebook.react.modules.core.ChoreographerCompat$FrameCallback$1.doFrame (ChoreographerCompat.java:85)
  at android.view.Choreographer$CallbackRecord.run (Choreographer.java:972)
  at android.view.Choreographer.doCallbacks (Choreographer.java:797)
  at android.view.Choreographer.doFrame (Choreographer.java:728)
  at android.view.Choreographer$FrameDisplayEventReceiver.run (Choreographer.java:959)
  at android.os.Handler.handleCallback (Handler.java:938)
  at android.os.Handler.dispatchMessage (Handler.java:99)
  at android.os.Looper.loop (Looper.java:230)
  at android.app.ActivityThread.main (ActivityThread.java:7700)
  at java.lang.reflect.Method.invoke (Native Method)
  at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run (RuntimeInit.java:612)
  at com.android.internal.os.ZygoteInit.main (ZygoteInit.java:997)

Not reproducible sadly.

mannoeu · 2024-10-02T12:57:08Z

same here

react-native 0.73.4

com.facebook.react.uimanager.IllegalViewOperationException: Trying to resolve view with tag 4049 which doesn't exist
        at com.facebook.react.uimanager.NativeViewHierarchyManager.resolveView(NativeViewHierarchyManager.java:102)
        at com.facebook.react.uimanager.UIManagerModule.resolveView(UIManagerModule.java:870)
        at com.mrousavy.camera.react.CameraViewModule$findCameraView$$inlined$runOnUiThreadAndWait$1.run(runOnUiThread.kt:40)
        at android.os.Handler.handleCallback(Handler.java:914)
        at android.os.Handler.dispatchMessage(Handler.java:100)
        at android.os.Looper.loop(Looper.java:241)
        at android.app.ActivityThread.main(ActivityThread.java:7582)
        at java.lang.reflect.Method.invoke(Method.java:-2)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:492)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:941)

ngioldasis · 2024-10-02T13:01:05Z

I do have the same issue on 0.72.7

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 Jul 4, 2018

react-native-bot added Missing Test Plan This PR appears to be missing a test plan. ✅Release Notes labels Jul 4, 2018

xkrsz mentioned this pull request Jul 10, 2018

Fix Attempt to invoke virtual method 'void com.facebook.react.uimanager.ReactShadowNode.addChildAt(com.facebook.react.uimanager.ReactShadowNode, int)' on a null object reference #20079

Closed

This was referenced Jul 11, 2018

Android - App crashes with ReactRootView inside viewpager #17123

Closed

Add null check in UIIMplementation#setChildren #19651

Closed

patrickkempff added 🐛Bug Fix Platform: Android Android applications. labels Jul 13, 2018

matthargett reviewed Sep 3, 2018

View reviewed changes

ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java Show resolved Hide resolved

ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java Show resolved Hide resolved

shergin suggested changes Dec 13, 2018

View reviewed changes

ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java Show resolved Hide resolved

ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java Outdated Show resolved Hide resolved

SudoPlz force-pushed the fix/17178 branch from f2658b6 to c0e1be5 Compare February 21, 2019 13:07

Now using a thread lock on UIImplementation methods that mutate the n…

e32cf59

…ode children sparse array - Fixes: facebook#17178 (comment)

SudoPlz force-pushed the fix/17178 branch from c0e1be5 to e32cf59 Compare March 27, 2019 16:21

Removed comment

6bfe5dd

facebook-github-bot approved these changes Mar 27, 2019

View reviewed changes

facebook-github-bot closed this in f149426 Mar 27, 2019

react-native-bot added the Merged This PR has been merged. label Mar 27, 2019

react-native-bot added the Bug label May 3, 2019

Enforced thread safety on UIImplementation methods that mutate the shadowNodeRegistry #20025

Enforced thread safety on UIImplementation methods that mutate the shadowNodeRegistry #20025

Conversation

SudoPlz commented Jul 4, 2018 • edited by hramos Loading

Changelog

pull-bot commented Jul 4, 2018 • edited Loading

SudoPlz commented Jul 5, 2018

xkrsz commented Jul 10, 2018

xkrsz commented Jul 10, 2018 • edited Loading

xkrsz commented Jul 10, 2018

patrickkempff commented Jul 10, 2018 • edited Loading

SudoPlz commented Jul 11, 2018

janicduplessis commented Jul 15, 2018

SudoPlz commented Jul 15, 2018 • edited Loading

LorienHW commented Aug 2, 2018

facebook-github-bot commented Aug 3, 2018

SudoPlz commented Aug 3, 2018

krishanmarco commented Aug 21, 2018

Aaang commented Sep 3, 2018 • edited Loading

alimek commented Sep 3, 2018

matthargett commented Sep 3, 2018

matthargett left a comment

Choose a reason for hiding this comment

levynir commented Oct 18, 2018

alimek commented Oct 18, 2018 • edited Loading

SudoPlz commented Oct 18, 2018 • edited Loading

SudoPlz commented Oct 18, 2018

levynir commented Oct 21, 2018

kelset commented Nov 30, 2018

hramos commented Dec 11, 2018

SudoPlz commented Dec 11, 2018 • edited Loading

kelset commented Dec 12, 2018

shergin left a comment

Choose a reason for hiding this comment

cpojer commented Feb 21, 2019

SudoPlz commented Feb 21, 2019

SudoPlz commented Feb 21, 2019

The0racle commented Mar 26, 2019

cpojer commented Mar 27, 2019

SudoPlz commented Mar 27, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

react-native-bot commented Mar 27, 2019

LorienHW commented Mar 27, 2019

mhdtouban commented Jul 12, 2019 • edited Loading

jstheoriginal commented Jul 21, 2019

ikesyo commented Sep 20, 2019 • edited Loading

ngioldasis commented Nov 26, 2021

pke commented Feb 14, 2022

mannoeu commented Oct 2, 2024 • edited Loading

ngioldasis commented Oct 2, 2024

SudoPlz commented Jul 4, 2018 •

edited by hramos

Loading

pull-bot commented Jul 4, 2018 •

edited

Loading

xkrsz commented Jul 10, 2018 •

edited

Loading

patrickkempff commented Jul 10, 2018 •

edited

Loading

SudoPlz commented Jul 15, 2018 •

edited

Loading

Aaang commented Sep 3, 2018 •

edited

Loading

alimek commented Oct 18, 2018 •

edited

Loading

SudoPlz commented Oct 18, 2018 •

edited

Loading

SudoPlz commented Dec 11, 2018 •

edited

Loading

mhdtouban commented Jul 12, 2019 •

edited

Loading

ikesyo commented Sep 20, 2019 •

edited

Loading

mannoeu commented Oct 2, 2024 •

edited

Loading