From ee1a191cb1c10085722d57fc276734f83e86a4f3 Mon Sep 17 00:00:00 2001 From: Harry Yu Date: Tue, 10 May 2022 12:38:35 -0700 Subject: [PATCH] Avoid crash by handling missing views in dispatchViewManagerCommand (#33795) Summary: Based on Google Play crash logs, crashes due to the assertions in `dispatchViewManagerCommand` is our top source of crashes: ``` com.facebook.react.uimanager.IllegalViewOperationException: Unable to execute operation dispatchViewManagerCommand: blur on view with tag: 17549, since the view does not exists at com.facebook.react.uimanager.UIImplementation.assertViewExists(UIImplementation.java:830) at com.facebook.react.uimanager.UIImplementation.dispatchViewManagerCommand(UIImplementation.java:713) at com.facebook.react.uimanager.UIManagerModule.dispatchCommand(UIManagerModule.java:739) at com.facebook.react.uimanager.UIManagerModule.dispatchViewManagerCommand(UIManagerModule.java:726) at com.facebook.react.uimanager.ReanimatedUIManager.dispatchViewManagerCommand(ReanimatedUIManager.java:233) at java.lang.reflect.Method.invoke(Method.java:-2) at com.facebook.react.bridge.JavaMethodWrapper.invoke(JavaMethodWrapper.java:372) at com.facebook.react.bridge.JavaModuleWrapper.invoke(JavaModuleWrapper.java:151) at com.facebook.react.bridge.queue.NativeRunnable.run(NativeRunnable.java:-2) ``` 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 Pull Request resolved: https://github.com/facebook/react-native/pull/33795 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. Reviewed By: cortinico Differential Revision: D36285603 Pulled By: JoshuaGross fbshipit-source-id: db0be74926c4cb83f07686398579236da8e586bf --- .../react/uimanager/UIImplementation.java | 54 ++++++++++++++----- 1 file changed, 42 insertions(+), 12 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java index 1549f364e2e..9e3ee72e62f 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java @@ -21,6 +21,7 @@ import com.facebook.react.bridge.ReadableMap; import com.facebook.react.bridge.UiThreadUtil; import com.facebook.react.bridge.WritableArray; import com.facebook.react.common.ReactConstants; +import com.facebook.react.common.build.ReactBuildConfig; import com.facebook.react.modules.i18nmanager.I18nUtil; import com.facebook.react.uimanager.debug.NotThreadSafeViewHierarchyUpdateDebugListener; import com.facebook.react.uimanager.events.EventDispatcher; @@ -742,13 +743,23 @@ public class UIImplementation { @Deprecated public void dispatchViewManagerCommand( int reactTag, int commandId, @Nullable ReadableArray commandArgs) { - assertViewExists(reactTag, "dispatchViewManagerCommand: " + commandId); + boolean viewExists = + checkOrAssertViewExists(reactTag, "dispatchViewManagerCommand: " + commandId); + if (!viewExists) { + return; + } + mOperationsQueue.enqueueDispatchCommand(reactTag, commandId, commandArgs); } public void dispatchViewManagerCommand( int reactTag, String commandId, @Nullable ReadableArray commandArgs) { - assertViewExists(reactTag, "dispatchViewManagerCommand: " + commandId); + boolean viewExists = + checkOrAssertViewExists(reactTag, "dispatchViewManagerCommand: " + commandId); + if (!viewExists) { + return; + } + mOperationsQueue.enqueueDispatchCommand(reactTag, commandId, commandArgs); } @@ -763,7 +774,11 @@ public class UIImplementation { * no arguments if the menu is dismissed */ public void showPopupMenu(int reactTag, ReadableArray items, Callback error, Callback success) { - assertViewExists(reactTag, "showPopupMenu"); + boolean viewExists = checkOrAssertViewExists(reactTag, "showPopupMenu"); + if (!viewExists) { + return; + } + mOperationsQueue.enqueueShowPopupMenu(reactTag, items, error, success); } @@ -867,15 +882,30 @@ public class UIImplementation { outputBuffer[3] = node.getScreenHeight(); } - private void assertViewExists(int reactTag, String operationNameForExceptionMessage) { - if (mShadowNodeRegistry.getNode(reactTag) == null) { - throw new IllegalViewOperationException( - "Unable to execute operation " - + operationNameForExceptionMessage - + " on view with " - + "tag: " - + reactTag - + ", since the view does not exists"); + /** + * Returns whether a view identified by the tag exists. In debug mode, this will throw whenever + * the view doesn't exist. In production, it'll log a warning. Callers should use this and just + * return if the view doesn't exist to avoid crashing. + */ + private boolean checkOrAssertViewExists(int reactTag, String operationNameForExceptionMessage) { + boolean viewExists = mShadowNodeRegistry.getNode(reactTag) != null; + if (viewExists) { + return true; + } + + String message = + "Unable to execute operation " + + operationNameForExceptionMessage + + " on view with " + + "tag: " + + reactTag + + ", since the view does not exist"; + + if (ReactBuildConfig.DEBUG) { + throw new IllegalViewOperationException(message); + } else { + FLog.w(ReactConstants.TAG, message); + return false; } }