From bb7f4dc8182ef5d2345533720c01cc34f1fe2cfd Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Mon, 16 Mar 2020 19:38:43 -0700 Subject: [PATCH] Early ViewCommand dispatch: retry exactly once, log soft errors in one place Summary: As a followup to T63997094, I think it is better to attempt to execute early once, and then to execute the ViewCommand after the current batch of mount instructions if that fails. If both fail, then log a soft exception. This is to support the use-case here, when ScrollView.scrollToEnd is called during the render pass, before any Views are mounted on the screen: https://our.intern.facebook.com/intern/diffusion/FBS/browse/master/xplat/js/RKJSModules/Apps/Profile/ProfileEdit/ui/featured_highlights_migration/ProfileEditFeaturedHighlightsMigrationProfilePreview.js?commit=75f66a9077abb81b7797079b567df759dd023a79&lines=221-229 Changelog: [Internal] Fix for dispatching ViewCommands early Reviewed By: mdvacca Differential Revision: D20478681 fbshipit-source-id: 052b3ecf4913a0096f590637faf0f68a072e2427 --- .../uimanager/NativeViewHierarchyManager.java | 23 ++-- .../react/uimanager/UIViewOperationQueue.java | 111 ++++++++++++++++-- 2 files changed, 114 insertions(+), 20 deletions(-) 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 806073ad0dc..6a8f27025e9 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java @@ -24,7 +24,6 @@ import com.facebook.react.R; import com.facebook.react.bridge.Callback; import com.facebook.react.bridge.JSApplicationIllegalArgumentException; import com.facebook.react.bridge.ReactContext; -import com.facebook.react.bridge.ReactSoftException; import com.facebook.react.bridge.ReadableArray; import com.facebook.react.bridge.ReadableMap; import com.facebook.react.bridge.SoftAssertions; @@ -766,13 +765,12 @@ public class NativeViewHierarchyManager { UiThreadUtil.assertOnUiThread(); View view = mTagsToViews.get(reactTag); if (view == null) { - ReactSoftException.logSoftException( - TAG, - new IllegalViewOperationException( - "Trying to send command to a non-existing view " + "with tag " + reactTag)); - return; + throw new IllegalViewOperationException( + "Trying to send command to a non-existing view with tag [" + + reactTag + + "] and command " + + commandId); } - ViewManager viewManager = resolveViewManager(reactTag); viewManager.receiveCommand(view, commandId, args); } @@ -782,13 +780,12 @@ public class NativeViewHierarchyManager { UiThreadUtil.assertOnUiThread(); View view = mTagsToViews.get(reactTag); if (view == null) { - ReactSoftException.logSoftException( - TAG, - new IllegalViewOperationException( - "Trying to send command to a non-existing view " + "with tag " + reactTag)); - return; + throw new IllegalViewOperationException( + "Trying to send command to a non-existing view with tag [" + + reactTag + + "] and command " + + commandId); } - ViewManager viewManager = resolveViewManager(reactTag); viewManager.receiveCommand(view, commandId, args); } 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 ed5b7955fc3..378e81cdcc1 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIViewOperationQueue.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIViewOperationQueue.java @@ -11,11 +11,13 @@ import android.os.SystemClock; import android.view.View; import androidx.annotation.GuardedBy; import androidx.annotation.Nullable; +import androidx.annotation.UiThread; import com.facebook.common.logging.FLog; 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.ReactSoftException; import com.facebook.react.bridge.ReadableArray; import com.facebook.react.bridge.ReadableMap; import com.facebook.react.bridge.SoftAssertions; @@ -44,6 +46,7 @@ import java.util.Map; public class UIViewOperationQueue { public static final int DEFAULT_MIN_TIME_LEFT_IN_FRAME_FOR_NONBATCHED_OPERATION_MS = 8; + private static final String TAG = UIViewOperationQueue.class.getSimpleName(); private final int[] mMeasureBuffer = new int[4]; @@ -260,12 +263,35 @@ public class UIViewOperationQueue { } } + /** + * This is a common interface for View Command operations. Once we delete the deprecated {@link + * DispatchCommandOperation}, we can delete this interface too. It provides a set of common + * operations to simplify generic operations on all types of ViewCommands. + */ + private interface DispatchCommandViewOperation { + + /** + * Like the execute function, but throws real exceptions instead of logging soft errors and + * returning silently. + */ + void executeWithExceptions(); + + /** Increment retry counter. */ + void incrementRetries(); + + /** Get retry counter. */ + int getRetries(); + } + @Deprecated - private final class DispatchCommandOperation extends ViewOperation { + private final class DispatchCommandOperation extends ViewOperation + implements DispatchCommandViewOperation { private final int mCommand; private final @Nullable ReadableArray mArgs; + private int numRetries = 0; + public DispatchCommandOperation(int tag, int command, @Nullable ReadableArray args) { super(tag); mCommand = command; @@ -274,14 +300,38 @@ public class UIViewOperationQueue { @Override public void execute() { + try { + mNativeViewHierarchyManager.dispatchCommand(mTag, mCommand, mArgs); + } catch (Throwable e) { + ReactSoftException.logSoftException( + TAG, new RuntimeException("Error dispatching View Command", e)); + } + } + + @Override + public void executeWithExceptions() { mNativeViewHierarchyManager.dispatchCommand(mTag, mCommand, mArgs); } + + @Override + @UiThread + public void incrementRetries() { + numRetries++; + } + + @Override + @UiThread + public int getRetries() { + return numRetries; + } } - private final class DispatchStringCommandOperation extends ViewOperation { + private final class DispatchStringCommandOperation extends ViewOperation + implements DispatchCommandViewOperation { private final String mCommand; private final @Nullable ReadableArray mArgs; + private int numRetries = 0; public DispatchStringCommandOperation(int tag, String command, @Nullable ReadableArray args) { super(tag); @@ -291,8 +341,30 @@ public class UIViewOperationQueue { @Override public void execute() { + try { + mNativeViewHierarchyManager.dispatchCommand(mTag, mCommand, mArgs); + } catch (Throwable e) { + ReactSoftException.logSoftException( + TAG, new RuntimeException("Error dispatching View Command", e)); + } + } + + @Override + @UiThread + public void executeWithExceptions() { mNativeViewHierarchyManager.dispatchCommand(mTag, mCommand, mArgs); } + + @Override + @UiThread + public void incrementRetries() { + numRetries++; + } + + @Override + public int getRetries() { + return numRetries; + } } private final class ShowPopupMenuOperation extends ViewOperation { @@ -520,7 +592,7 @@ public class UIViewOperationQueue { private final ReactApplicationContext mReactApplicationContext; private final boolean mAllowViewCommandsQueue; - private ArrayList mViewCommandOperations = new ArrayList<>(); + private ArrayList mViewCommandOperations = new ArrayList<>(); // Only called from the UIManager queue? private ArrayList mOperations = new ArrayList<>(); @@ -759,7 +831,7 @@ public class UIViewOperationQueue { // Store the current operation queues to dispatch and create new empty ones to continue // receiving new operations - final ArrayList viewCommandOperations; + final ArrayList viewCommandOperations; if (!mViewCommandOperations.isEmpty()) { viewCommandOperations = mViewCommandOperations; mViewCommandOperations = new ArrayList<>(); @@ -799,10 +871,35 @@ public class UIViewOperationQueue { try { long runStartTime = SystemClock.uptimeMillis(); - // All ViewCommands should be executed first as a perf optimization + // All ViewCommands should be executed first as a perf optimization. + // This entire block is only executed if there's a separate viewCommand queue, + // which is currently gated by a ReactFeatureFlag. if (viewCommandOperations != null) { - for (UIOperation viewCommandOp : viewCommandOperations) { - viewCommandOp.execute(); + for (DispatchCommandViewOperation op : viewCommandOperations) { + try { + op.executeWithExceptions(); + } catch (Throwable 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. + if (op.getRetries() == 0) { + op.incrementRetries(); + mViewCommandOperations.add(op); + } else { + ReactSoftException.logSoftException(TAG, e); + } + } } }