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

[V3, Android] Catch rare RuntimeException in setRoot when waitForRender = true #5428

Merged
merged 4 commits into from
Sep 5, 2019

Conversation

rawrmaan
Copy link
Contributor

I've seen the following crash in production. It's rare, but it appears there's some kind of race condition in setRoot's addOnAppearedListener that results in getView throwing. Here's the stack trace:

java.lang.RuntimeException: Tried to create view after it has already been destroyed
    at com.reactnativenavigation.viewcontrollers.ViewController.getView(ViewController.java:165)
    at com.reactnativenavigation.viewcontrollers.ParentController.getView(ParentController.java:60)
    at com.reactnativenavigation.viewcontrollers.navigator.RootPresenter.lambda$setRoot$0(RootPresenter.java:41)
    at com.reactnativenavigation.viewcontrollers.navigator.-$$Lambda$RootPresenter$T5cjGG24h93OrmgUBwpekHEXTtY.run(lambda)
    at com.reactnativenavigation.viewcontrollers.-$$Lambda$P7xe27l85RASi5iJcSC7JIxgcA8.on(lambda)
    at com.reactnativenavigation.utils.CollectionUtils.forEach(CollectionUtils.java:89)
    at com.reactnativenavigation.utils.CollectionUtils.forEach(CollectionUtils.java:83)
    at com.reactnativenavigation.viewcontrollers.ViewController.lambda$onViewAppeared$1(ViewController.java:216)
    at com.reactnativenavigation.viewcontrollers.-$$Lambda$ViewController$TwzQHRxayGsTFM7gYujgloHVhEE.run(lambda)
    at android.os.Handler.handleCallback(Handler.java:751)
    at android.os.Handler.dispatchMessage(Handler.java:95)
    at android.os.Looper.loop(Looper.java:154)
    at android.app.ActivityThread.main(ActivityThread.java:6823)
    at java.lang.reflect.Method.invoke(Method.java)
    at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:1557)
    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1445)

Instead of swallowing the exception here, I've decided to pass it to JS so it can be caught by wrapping await setRoot() in a try/catch. Interested to see if others agree with this approach, or whether it should just be swallowed.

Importantly, I've observed this with RN 0.59.8 and RNN 2.22.3 as that's what I'm in production with, but I'm opening this PR against V3 so it has a chance of helping someone. Maybe someone with more knowledge of the Android view lifecycles in this library could comment on whether this is still necessary.

Thanks for the awesome library!

@guyca
Copy link
Collaborator

guyca commented Sep 2, 2019

Hey @rawrmaan
Thanks for the pr! Instead of using a try-catch block, lets explicitly check if the root is detroyd before accessing it.

if (!root.isDestroyed()) {
  root.getView().setAlpha(1);
  animateSetRootAndReportSuccess(root, listener, options);	
}

Copy link
Collaborator

@guyca guyca left a comment

Choose a reason for hiding this comment

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

Great catch! Please see my comment on how we should fix this 👍 Thanks.

@rawrmaan
Copy link
Contributor Author

rawrmaan commented Sep 4, 2019

Great, updated! I'm assuming your intention is to ignore the listener instead of notifying it, so I didn't add an else block.

@guyca guyca merged commit b048581 into wix:master Sep 5, 2019
vshkl pushed a commit to vshkl/react-native-navigation that referenced this pull request Feb 5, 2020
…#5428)

When setRoot is called with `waitForRender=true`, check if the root view is destroyed before displaying it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants