From 0fe548aa2a636c1ec14de51fcef7a99f233b83c8 Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Thu, 19 Mar 2020 22:55:46 -0700 Subject: [PATCH] Only retry ViewCommand mount items if exception is marked as "Retryable" Summary: Instead of just blindly retrying all ViewCommands if they fail - which could be dangerous, since it's arbitrary imperative commands we'd be executing twice, potentially with bad app state - we only retry if the ViewCommand throws a "RetryableMountingLayerException". Changelog: [Internal] Optimization to ViewCommands Reviewed By: mdvacca Differential Revision: D20529985 fbshipit-source-id: 0217b43f4bf92442bcc7ca48c8ae2b9a9e543dc9 --- .../bridge/ReactNoCrashSoftException.java | 12 ++++--- .../RetryableMountingLayerException.java | 26 ++++++++++++++++ .../uimanager/NativeViewHierarchyManager.java | 5 +-- .../react/uimanager/UIViewOperationQueue.java | 31 ++++++++++--------- .../views/scroll/ReactScrollViewManager.java | 9 +++++- 5 files changed, 61 insertions(+), 22 deletions(-) create mode 100644 ReactAndroid/src/main/java/com/facebook/react/bridge/RetryableMountingLayerException.java diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactNoCrashSoftException.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactNoCrashSoftException.java index 4aeaa82f613..b3234996e19 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactNoCrashSoftException.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactNoCrashSoftException.java @@ -13,11 +13,15 @@ package com.facebook.react.bridge; * and not crash, no matter what. */ public class ReactNoCrashSoftException extends RuntimeException { - public ReactNoCrashSoftException(String detailMessage) { - super(detailMessage); + public ReactNoCrashSoftException(String m) { + super(m); } - public ReactNoCrashSoftException(String detailMessage, Throwable ex) { - super(detailMessage, ex); + public ReactNoCrashSoftException(Throwable e) { + super(e); + } + + public ReactNoCrashSoftException(String m, Throwable e) { + super(m, e); } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/RetryableMountingLayerException.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/RetryableMountingLayerException.java new file mode 100644 index 00000000000..a9c8fb325f7 --- /dev/null +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/RetryableMountingLayerException.java @@ -0,0 +1,26 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +package com.facebook.react.bridge; + +/** + * ViewCommands can throw this Exception. If this is caught during the execution of a ViewCommand + * mounting instruction, it indicates that the mount item can be safely retried. + */ +public class RetryableMountingLayerException extends RuntimeException { + public RetryableMountingLayerException(String msg, Throwable e) { + super(msg, e); + } + + public RetryableMountingLayerException(Throwable e) { + super(e); + } + + public RetryableMountingLayerException(String msg) { + super(msg); + } +} diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java index f89215ece64..c3d19ecbe79 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java @@ -26,6 +26,7 @@ import com.facebook.react.bridge.JSApplicationIllegalArgumentException; import com.facebook.react.bridge.ReactContext; import com.facebook.react.bridge.ReadableArray; import com.facebook.react.bridge.ReadableMap; +import com.facebook.react.bridge.RetryableMountingLayerException; import com.facebook.react.bridge.SoftAssertions; import com.facebook.react.bridge.UiThreadUtil; import com.facebook.react.config.ReactFeatureFlags; @@ -762,7 +763,7 @@ public class NativeViewHierarchyManager { UiThreadUtil.assertOnUiThread(); View view = mTagsToViews.get(reactTag); if (view == null) { - throw new IllegalViewOperationException( + throw new RetryableMountingLayerException( "Trying to send command to a non-existing view with tag [" + reactTag + "] and command " @@ -777,7 +778,7 @@ public class NativeViewHierarchyManager { UiThreadUtil.assertOnUiThread(); View view = mTagsToViews.get(reactTag); if (view == null) { - throw new IllegalViewOperationException( + throw new RetryableMountingLayerException( "Trying to send command to a non-existing view with tag [" + reactTag + "] and command " diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIViewOperationQueue.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIViewOperationQueue.java index 378e81cdcc1..9c2eed060a7 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIViewOperationQueue.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIViewOperationQueue.java @@ -17,9 +17,11 @@ import com.facebook.react.bridge.Callback; import com.facebook.react.bridge.GuardedRunnable; import com.facebook.react.bridge.ReactApplicationContext; import com.facebook.react.bridge.ReactContext; +import com.facebook.react.bridge.ReactNoCrashSoftException; import com.facebook.react.bridge.ReactSoftException; import com.facebook.react.bridge.ReadableArray; import com.facebook.react.bridge.ReadableMap; +import com.facebook.react.bridge.RetryableMountingLayerException; import com.facebook.react.bridge.SoftAssertions; import com.facebook.react.bridge.UiThreadUtil; import com.facebook.react.common.ReactConstants; @@ -878,27 +880,26 @@ public class UIViewOperationQueue { for (DispatchCommandViewOperation op : viewCommandOperations) { try { op.executeWithExceptions(); - } catch (Throwable e) { + } catch (RetryableMountingLayerException e) { // Catch errors in DispatchCommands. We allow all commands to be retried - // exactly - // once, after the current batch of other mountitems. If the second attempt - // fails, - // then we log a soft error. This will still crash only in debug. - // We do this because it is a ~relatively common pattern to dispatch a command - // during render, for example, to scroll to the bottom of a ScrollView in - // render. - // This dispatches the command before that View is even mounted. By retrying - // once, - // we can still dispatch the vast majority of commands faster, avoid errors, - // and - // still operate correctly for most commands even when they're executed too - // soon. + // exactly once, after the current batch of other mountitems. If the second + // attempt fails, then we log a soft error. This will still crash only in + // debug. We do this because it is a ~relatively common pattern to dispatch a + // command during render, for example, to scroll to the bottom of a ScrollView + // in render. This dispatches the command before that View is even mounted. By + // retrying once, we can still dispatch the vast majority of commands faster, + // avoid errors, and still operate correctly for most commands even when + // they're executed too soon. if (op.getRetries() == 0) { op.incrementRetries(); mViewCommandOperations.add(op); } else { - ReactSoftException.logSoftException(TAG, e); + // Retryable exceptions should be logged, but never crash in debug. + ReactSoftException.logSoftException(TAG, new ReactNoCrashSoftException(e)); } + } catch (Throwable e) { + // Non-retryable exceptions should be logged in prod, and crash in Debug. + ReactSoftException.logSoftException(TAG, e); } } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollViewManager.java b/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollViewManager.java index d528caa2ea6..51dc13166b0 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollViewManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollViewManager.java @@ -9,9 +9,11 @@ package com.facebook.react.views.scroll; import android.graphics.Color; import android.util.DisplayMetrics; +import android.view.View; import androidx.annotation.Nullable; import androidx.core.view.ViewCompat; import com.facebook.react.bridge.ReadableArray; +import com.facebook.react.bridge.RetryableMountingLayerException; import com.facebook.react.common.MapBuilder; import com.facebook.react.module.annotations.ReactModule; import com.facebook.react.uimanager.DisplayMetricsHolder; @@ -272,8 +274,13 @@ public class ReactScrollViewManager extends ViewGroupManager @Override public void scrollToEnd( ReactScrollView scrollView, ReactScrollViewCommandHelper.ScrollToEndCommandData data) { + View child = scrollView.getChildAt(0); + if (child == null) { + throw new RetryableMountingLayerException("scrollToEnd called on ScrollView without child"); + } + // ScrollView always has one child - the scrollable area - int bottom = scrollView.getChildAt(0).getHeight() + scrollView.getPaddingBottom(); + int bottom = child.getHeight() + scrollView.getPaddingBottom(); if (data.mAnimated) { scrollView.reactSmoothScrollTo(scrollView.getScrollX(), bottom); } else {