From 3cdef265ae75b526c4cdc44dba6c17b1c076da0d Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Mon, 7 Dec 2020 17:08:07 -0800 Subject: [PATCH] Remove premature optimization in FabricUIManager; could be causing stopSurface inconsistencies Summary: There is an optimization in FabricUIManager.surfaceActiveForExecution that ensures it returns the same value (true or false) for a given SurfaceId for a single frame (the value is cached until the next frame). It seems like this can be causing a few different crashes, in a couple different ways: 1) If StopSurface is called off the UI thread, in the middle of a batch of operations (probably less likely to cause problems) 2) If StopSurface is called on the UI thread, in between different operations; the latter operations will still execute because the `true` value of `surfaceActiveForExecution` was cached. I don't think that this optimization was providing much for us, and could be causing crashes. Remove it for now and we'll analyze impact on crashes and perf. Changelog: [Internal] Reviewed By: mdvacca Differential Revision: D25379970 fbshipit-source-id: 2c15c971bd0c828e1d38a34662d93293271041b2 --- .../react/fabric/FabricUIManager.java | 46 +++++++------------ 1 file changed, 17 insertions(+), 29 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java index c22a4b1eb77..acf4057d12f 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java @@ -168,12 +168,6 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { @NonNull private final DispatchUIFrameCallback mDispatchUIFrameCallback; - @ThreadConfined(UI) - private int mLastExecutedMountItemSurfaceId = -1; - - @ThreadConfined(UI) - private boolean mLastExecutedMountItemSurfaceIdActive = false; - /** * This is used to keep track of whether or not the FabricUIManager has been destroyed. Once the * Catalyst instance is being destroyed, we should cease all operation here. @@ -755,7 +749,6 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { didDispatchItems = dispatchMountItems(); } catch (Throwable e) { mReDispatchCounter = 0; - mLastExecutedMountItemSurfaceId = -1; throw e; } finally { // Clean up after running dispatchMountItems - even if an exception was thrown @@ -782,7 +775,6 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { tryDispatchMountItems(); } mReDispatchCounter = 0; - mLastExecutedMountItemSurfaceId = -1; } @Nullable @@ -860,25 +852,22 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { */ @UiThread @ThreadConfined(UI) - private boolean surfaceActiveForExecution(int surfaceId, String context) { - if (mLastExecutedMountItemSurfaceId != surfaceId) { - mLastExecutedMountItemSurfaceId = surfaceId; - mLastExecutedMountItemSurfaceIdActive = mReactContextForRootTag.get(surfaceId) != null; + private boolean isSurfaceActiveForExecution(int surfaceId, String context) { + boolean surfaceActive = mReactContextForRootTag.get(surfaceId) != null; - // If there are many MountItems with the same SurfaceId, we only - // log a warning for the first one that is skipped. - if (!mLastExecutedMountItemSurfaceIdActive) { - ReactSoftException.logSoftException( - TAG, - new ReactNoCrashSoftException( - "dispatchMountItems: skipping " - + context - + ", because surface not available: " - + surfaceId)); - } + // If there are many MountItems with the same SurfaceId, we only + // log a warning for the first one that is skipped. + if (!surfaceActive) { + ReactSoftException.logSoftException( + TAG, + new ReactNoCrashSoftException( + "dispatchMountItems: skipping " + + context + + ", because surface not available: " + + surfaceId)); } - return mLastExecutedMountItemSurfaceIdActive; + return surfaceActive; } private static void printMountItem(MountItem mountItem, String prefix) { @@ -970,7 +959,7 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { + preMountItemsToDispatch.size()); for (PreAllocateViewMountItem preMountItem : preMountItemsToDispatch) { - if (surfaceActiveForExecution( + if (isSurfaceActiveForExecution( preMountItem.getRootTag(), "dispatchMountItems PreAllocateViewMountItem")) { preMountItem.execute(mMountingManager); } @@ -996,7 +985,7 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { // TODO T68118357: clean up this logic and simplify this method overall if (mountItem instanceof BatchMountItem) { BatchMountItem batchMountItem = (BatchMountItem) mountItem; - if (!surfaceActiveForExecution( + if (!isSurfaceActiveForExecution( batchMountItem.getRootTag(), "dispatchMountItems BatchMountItem")) { continue; } @@ -1006,7 +995,7 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { // TODO T68118357: clean up this logic and simplify this method overall if (mountItem instanceof IntBufferBatchMountItem) { IntBufferBatchMountItem batchMountItem = (IntBufferBatchMountItem) mountItem; - if (!surfaceActiveForExecution( + if (!isSurfaceActiveForExecution( batchMountItem.getRootTag(), "dispatchMountItems IntBufferBatchMountItem")) { continue; } @@ -1065,14 +1054,13 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { break; } - if (surfaceActiveForExecution( + if (isSurfaceActiveForExecution( preMountItemToDispatch.getRootTag(), "dispatchPreMountItems")) { preMountItemToDispatch.execute(mMountingManager); } } } finally { mInDispatch = false; - mLastExecutedMountItemSurfaceId = -1; } Systrace.endSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE);