wix / react-native-navigation

A complete native navigation solution for React Native
https://wix.github.io/react-native-navigation/
MIT License
13.04k stars 2.67k forks source link

IllegalViewOperationException: Trying to add unknown view tag #2892

Closed brunolemos closed 6 years ago

brunolemos commented 6 years ago

Issue Description

1% of the users are facing this crash. Do you have any idea what could it be?

com.facebook.react.uimanager.IllegalViewOperationException
com.reactnativenavigation.controllers.NavigationActivity
Trying to add unknown view tag: 1234

I noticed most of these users had this weird behavior of ActivityLifecycle being triggered multiple times at the same time (onDestroy(), onStop(), onResume(), onStart(), onCreate(), etc). What could cause this?

image

Stack points to https://github.com/facebook/react-native/blob/353c070be9e9a5528d2098db4df3f0dc02d758a9/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java#L479 but I didn't have this before migrating to react-native-navigation afaik.

04-29 07:07:05.094 25419-25452/com.reactnativenavigation.playground E/unknown:ReactNative: Exception in native call
    com.facebook.react.uimanager.IllegalViewOperationException: Trying to add unknown view tag: 70
        at com.facebook.react.uimanager.UIImplementation.setChildren(UIImplementation.java:480)
        at com.facebook.react.uimanager.UIManagerModule.setChildren(UIManagerModule.java:448)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.facebook.react.bridge.JavaMethodWrapper.invoke(JavaMethodWrapper.java:374)
        at com.facebook.react.bridge.JavaModuleWrapper.invoke(JavaModuleWrapper.java:162)
        at com.facebook.react.bridge.queue.NativeRunnable.run(Native Method)
        at android.os.Handler.handleCallback(Handler.java:789)
        at android.os.Handler.dispatchMessage(Handler.java:98)
        at com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage(MessageQueueThreadHandler.java:31)
        at android.os.Looper.loop(Looper.java:164)
        at com.facebook.react.bridge.queue.MessageQueueThreadImpl$3.run(MessageQueueThreadImpl.java:194)
        at java.lang.Thread.run(Thread.java:764)

Steps to Reproduce / Code Snippets / Screenshots

Could not reproduce, but maybe you could give some insights.


Environment

fodjan-philip commented 6 years ago

I am also experiencing this issue in production, but couldn't reproduce it either. Research pointed to some different solutions (see https://github.com/facebook/react-native/issues/17178):

One comment also suggested to apply a patch to react-native-navigation, however it seems that the specified line is not present in the current version of react-native-navigation anymore (see https://github.com/facebook/react-native/issues/14944#issuecomment-365586351).

brunolemos commented 6 years ago
brunolemos commented 6 years ago

Is anyone else also using this code? Maybe it's related to this issue? https://github.com/wix/react-native-navigation/pull/2043#comment-339329766

@Override
public boolean clearHostOnActivityDestroy() {
    return false;
}
Navigation.isAppLaunched().then((appLaunched) => {
  if (appLaunched) {
    app();
  }

  new NativeEventsReceiver().appLaunched(() => {
    app();
  });
}
fodjan-philip commented 6 years ago

@brunolemos I am indeed using this code. I did observe the "Trying to add unknown view tag" error in versions before using this code, but it seems like it is occuring more frequently recently.

guyca commented 6 years ago

This appears to be a tough one to crack. We're seeing this crash as well in the Wix app which uses v1. It reproduces in v2 e2e from time to time when running the test suite locally, will need to investigate further. If anyone has more insights or any relevant info, please post here as I pretty much hit a wall investigating this.

SudoPlz commented 6 years ago

If you ever figure the cause of this please ping me, that's something I've been struggling with for a while now.

Looks like a child view that is not created yet by UIManager.createView is trying to get set by UIManager.setChildren, and since it doesn't exist in the ShadowNodeRegistry the app crashes.

I think that's a timing issue, where the view is not created yet (or it's destroyed for some reason) but setChildren runs prematurely, but that is just my guess.

jshearer commented 6 years ago

@SudoPlz I just encountered this problem, and after a few minutes of digging, I realized that I was using raw text, instead of containing them inside of a Text element:

    render(){
        return (
            <Container>
                <Grid>
                    <Row>
                        <Col>
                            Cell 1
                        </Col>
                        <Col>
                            Cell 2
                        </Col>
                    </Row>
                </Grid>
            </Container>
        )
    }

vs

    render(){
        return (
            <Container>
                <Grid>
                    <Row>
                        <Col>
                            <Text>Cell 1</Text>
                        </Col>
                        <Col>
                            <Text>Cell 2</Text>
                        </Col>
                    </Row>
                </Grid>
            </Container>
        )
    }
brunolemos commented 6 years ago

I really don't believe I have that on my code, I use {Boolean(x) && everywhere on jsx. Maybe some third party package wasn't careful with this. But how do I find the specific component causing this?

In my case, based on my screen change logs, I'd guess it's related to react-native-gifted-chat or react-native-vector-icons.

brunolemos commented 6 years ago

I have one line like this: {/* ... */}, don't know if this could cause it, as I can't reproduce the issue.

brunolemos commented 6 years ago

After taking a closer look on bugsnag, it seems it happens when the device stays in background for some time and then crashes when trying to go to foreground.

In my case, when in background, the chat screen keeps updating with new messages, so that may be related. Anyone have a similar situation?

SudoPlz commented 6 years ago

@jshearer I wonder if we could build a script to detect those cases.. I can't think of a smart way to do that.

Though I think raw text outside a Text element might be ONE thing that causes it, I'm sure there are other causes too.

guyca commented 6 years ago

@brunolemos We've observed this error running Detox e2e tests on v2. It seems like a synchronisation issue... but we really hit a wall trying to debug this.

fcaride commented 6 years ago

i solved it by changing this: {contact.Phone && <ContactInfo />} into this: {!!contact.Phone && <ContactInfo />}

rawrmaan commented 6 years ago

@fcaride That's brilliant. You just made me realize that '' == false, so in your example, if contact.Phone was an empty string, it would try to render the empty string outside of a <Text> component. Just went through my code and fixed a bunch of cases where that might happen.

fcaride commented 6 years ago

Yes you are right. Glad it helps! 😄

SudoPlz commented 6 years ago

@guyca I managed to spot exactly what's causing that problem in react-native.

So what happens behind the scenes is, react-native is trying to manipulate the shadowNode list at the same time some other thread is manipulating it.

Specifically UIImplementation manipulates that list with the following methods

1) createView 2) setChildren 3) manageChildren 4) removeRootShadowNode

so if any of those get invoked at the same time, there's a really high chance that the app will crash.

In our app we fixed that issue by providing a new UIImplementation that looks like this:

https://gist.github.com/SudoPlz/23ea2dee9772cb39e1e742034cdf8b98

as you can see we don't allow those UIImplementation methods to run unsynchronised anymore.

We could constantly reproduce the issue on our end, but now that's totally fixed. We pass the new UIImplementation provided through this method here which I WISH react-native-navigation exposed but it doesn't (even-though react-native does expose this).

I had to expose it manually here to make this all work.

I hope this post helps others, because it really took me A CRAZY amount of time to figure this out.

brunolemos commented 6 years ago

@SudoPlz Can you make a PR?

SudoPlz commented 6 years ago

@brunolemos I can definitely do that, but it won't be accepted. No pr's are accepted on v1 since the team is focusing on v2, and I don't think many people are using v2 at the moment. (I know we aren't)

The getUIImplementation exposing part of react-native-navigation lives in this branch: https://github.com/SudoPlz/react-native-navigation/tree/feature/expose_uiimplementationprovider

brunolemos commented 6 years ago

@sudoplz they may accept this one as they faced this issue themselves

No pr's are accepted on v1 I don't think many people are using v2

That's a bad combination

SudoPlz commented 6 years ago

I'll wait for @guyca to comment on this, since I'm 99% sure they won't be accepting such a PR at the moment. (I already have 2 other pr's waiting that won't be accepted either)

guyca commented 6 years ago

Hey @SudoPlz Amazing work figuring this out. I wonder why the default UiImplementation isn't thread safe. I thought a class which handles UI would do all of its logic on the UI thread.

the reason we're not merging PR's to v1 is because it's not tests. I don't mind merging a pr that doesn't add new features and only changes visibility modifiers and adds overloads. So feel free to PR the needed changes. 👍

SudoPlz commented 6 years ago

I can definitely create a PR that exposes the needed method, (in fact I'll probably do that today) but the underlying issue won't be resolved. People will still have to provide their own UIImplementationProvider, and yes I was also amazed to see that those methods are not thread safe, but they are not..

liubomyrp commented 6 years ago

Hey everyone!

We use your library in our app and this issue is really affecting a lot of our users. We hope you'll solve that problem ASAP.

Thanks in advance!

Dantalion commented 6 years ago

Affecting a lot of users on our end. Especially Android 8.0 devices keep crashing.

guyca commented 6 years ago

I've added the solution proposed by @SudoPlz to v2 and we're testing it in the playground app. I can definitely say these crashes are not happening anymore and it seems to have no negative side effects. If you've migrated to v2, you can use by overriding getUiImplementationProvider:

    @Override
    protected ReactNativeHost createReactNativeHost() {
        return new NavigationReactNativeHost(this) {
            @Override
            protected UIImplementationProvider getUIImplementationProvider() {
                return new SyncUiImplementation.Provider();
            }
        };
    }

Thanks to @SudoPlz, v1 also supports passing your own UiImplementationProvider by overriding getUiImplementation provider in MainApplication

SudoPlz commented 6 years ago

I'm super happy that helped.. Let's hope facebook permanently resolves that issue too. (https://github.com/facebook/react-native/issues/17178#issuecomment-394662148)

Dantalion commented 6 years ago

Thank you very much @SudoPlz ! @guyca How likely will the overridable getUIImplementationProvider() and the SyncUiImplementation.Provider() also be available in a new release of the main branch for v1?

I feel very uneasy towards switching to v2 or applying these changes myself in the middle of a time critical major release!

Thank you!

sJJdGG commented 6 years ago

In order to override getUiImplementationProvider as already mentioned, there needs to be a bunch of new imports in MainApplication.java:

import com.reactnativenavigation.react.NavigationReactNativeHost;
import com.reactnativenavigation.react.SyncUiImplementation;
import com.facebook.react.uimanager.UIImplementationProvider;
import com.facebook.react.ReactNativeHost;
creative-git commented 6 years ago

@fcaride THANKS!!!

guyca commented 6 years ago

Closing as the solution provided by @SudoPlz resolves the issue in v2 https://github.com/wix/react-native-navigation/issues/2892#issuecomment-395360046

beausmith commented 6 years ago

It seems Android doesn't like converting non-Boolean values into booleans. So I'm now ensuring that all values which I'm using as booleans are in fact, booleans.

I have changed all occurrences similar to this where foo may not be true or false:

{foo && <MyComponent />}

…into one of these which converts foo into a boolean if it is not:

{!!foo && <MyComponent />}
{Boolean(foo) && <MyComponent />}
39otrebla commented 6 years ago

@guyca NavigationApplication's getUiImplementation can be overridden in version 1.1.483 while is still protected in 1.1.486. Is there any reason?

dev6james commented 6 years ago

@guyca The removal of UIImplementationProvider in 0.57 means this solution is no longer valid correct? Unless this issue is no longer present in RN0.57? We're both trying to deal with this problem and also upgrade an app to RN0.57 and it's unclear to me how to do both.

guyca commented 6 years ago

@dev6james We're planning on migrating to RN 0.57 during the month of Octobre, hope to have an answer soon.

freiserg commented 6 years ago

May be it was fixed in this commit https://github.com/facebook/react-native/commit/dc836780cbe71c5152a4861bd6a2dba1549bc53f#diff-e40bcf61507821200dcf1f573eb717e3

SudoPlz commented 6 years ago

I doubt it.

It's been a long time since I last worked on this, but I remember it wasn't just addRootNode that was problematic. It was any method that mutates the Sparse Array.

That means removeRootNode, addNode and others too. I'm afraid my fix will never get merged, because they're re-writing react-native, moving lot's of code to C++ with fabric, and thus they probably won't invest any time reviewing and maintaining old code that will be deprecated at some point.

makirby commented 6 years ago

@SudoPlz how do you re-create that crash? I have upgraded to 0.57.2 with a patched version of RNN 2.0.2555 and not seen any reports of this crash?

SudoPlz commented 6 years ago

I'm afraid I don't have a way of re-creating that crash in a simple react-native project. In our project, we can re-create that by synchronously/manually creating RCTRootViews and using them in a RecyclerList view.

guyca commented 6 years ago

You can also run RNN's e2e tests, after one or two runs you'll encounter the crash

SudoPlz commented 6 years ago

Oh what @guyca suggests is sooooo much better, I didn't know there were tests for that as well. Awesome!

SudoPlz commented 6 years ago

@makirby also I think there's this replication project here https://github.com/Frank1234/RNViewPager

(I didn't create it, but looks like you can steadily replicate the crash by tapping the button a couple of times)

SudoPlz commented 6 years ago

Oh by the way, the guys in react-native are adding back the UIImplementation overwrites so v2 can work with 0.57 and above (at least until the fabric re-architecture goes live).

https://github.com/facebook/react-native/commit/506f92083806e91a90d9a213bcdd05ab3b2ba888#r30882540

makirby commented 6 years ago

@SudoPlz Thanks for that, good news to hear. I am worried as to why I haven't been able to re-create with our codebase. Anyway I guess i will just hold tight until it gets re-implemented in React Native.

SudoPlz commented 6 years ago

@guyca Looks like UIImplementation was re-added in this commit https://github.com/facebook/react-native/commit/b002df945b267c6e36aa1041397ffd1c73167037 but it's only available in the master branch for the time being.

guyca commented 6 years ago

Yup, we're trying to have FB publish a patch with this fix but everybody is too busy at React Conf 😅

krislm commented 5 years ago

Just in case someone else lands here and none of the above works I will add what worked for me. I had the same error and started removing lines until it didn't happen anymore. Lo and behold - the last line I deleted was <View> <MyComponent /> </View> Did you notice the spaces surrounding MyComponent? It was a Homer moment for me

ericketts commented 5 years ago

the re-addition of UIImplementationProvider was only a halfway measure, resulting in the default UIImplementationProvider getting used in most cases where it matters, even if you override and specify your own provider. Specifically this line (and the effectively identical ones above, in the same file) did not get added back in when UIImplementationProvider was "re-added" (you can verify this is the case here; notice how the overloaded this calls use new UIImplementationProvider(), such that any custom specified provider is not used)

shaktals commented 5 years ago

I also faced this issue, and figured it was caused by having a conditional statement resulting in '#000' OR null, which was used to define backgroundColor on the style of a View. Changing the false result from null to '#fff' solved the issue.

dev6james commented 5 years ago

I believe @SudoPlz had a fix merged to React Native Core for this in a recent patch version? Is that correct?

SudoPlz commented 5 years ago

Yep, that was merged but I believe it will go live with RN .60 That being said my fix works but it's more of a workaround than an actual fix.