-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Avoid crash by handling missing views in dispatchViewManagerCommand #33795
Conversation
Base commit: 7c581f3 |
Base commit: 7c581f3 |
We do something similar for Fabric, so I'm supportive of backporting this change. In the case of Fabric, we have special exceptions that are thrown and soft-logged instead of ignoring the error entirely: https://github.com/facebook/react-native/blob/main/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountItemDispatcher.java#L222-L231 In practice these warnings haven't provided much value and it's pretty normal for there to be races between ViewCommands and Views being unmounted, so I think just ignoring them is fine. |
@JoshuaGross has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
I would even support logging a soft-exception or just logging an error to logcat (but not crashing) in debug mode, I don't think that crashing in these cases is helpful, even in debug. But I won't block merging this either way. |
Additionally, in Fabric we actually have logic to retry ViewCommands exactly once (maybe the command was executed too quickly, before the View was created? Maybe it's being executed after the View was deleted? We don't really know, so we try again in the next UI frame). It's worth considering that here too, but again, I won't block this PR as it's an improvement over the status quo for non-Fabric. |
This pull request was successfully merged by @hsource in ee1a191. When will my fix make it into a release? | Upcoming Releases |
Summary
Based on Google Play crash logs, crashes due to the assertions in
dispatchViewManagerCommand
is our top source of crashes:There are many other places in UIImplementation that instead of throwing a fatal exception, just log a message and continues execution.
Logging a message and discarding the command should be safe for commands like "blur this view". We keep the previous behavior in debug mode, but just log and discard commands in production.
Changelog
[Android] [Fixed] - Avoid crash by handling missing views in dispatchViewManagerCommand
Test Plan
We had a tough time reproducing specific crashes, but we ran with this binary in the latest release of our app and it has eliminated this class of crashes from Google Play and Bugsnag instrumentation.