From 912dac24fcf9f3b324dd54bff85e5132c8779014 Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Tue, 9 Jun 2020 23:20:34 -0700 Subject: [PATCH] Ignore PreAllocateViewMountItem if it's associated with a stopped surface Summary: Because of the previous diffs there's an increased chance of race conditions between JS executing and queuing up PreAllocateViewMountItems for surfaces that are stopped. Make sure those are ignored if they're queued up and a surface has been stopped. Currently stopSurface only happens on the UI thread; PreAllocateViewMountItems can be queued from any thread, but are only executed on the UI thread. So once a batch of items starts executing, there's no race between teardown and execution: we just need to make sure we check that the surface is still running initially. Changelog: [Internal] Reviewed By: mdvacca Differential Revision: D21965839 fbshipit-source-id: 0241dc171022cc923b7e38dcd110d664096dde79 --- .../react/fabric/FabricUIManager.java | 71 ++++++++++++++----- .../mountitems/PreAllocateViewMountItem.java | 4 ++ 2 files changed, 59 insertions(+), 16 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 5ae1f6b9b2e..9c80c9442fd 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java @@ -134,13 +134,19 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { @GuardedBy("mPreMountItemsLock") @NonNull - private ArrayDeque mPreMountItems = + private ArrayDeque mPreMountItems = new ArrayDeque<>(PRE_MOUNT_ITEMS_INITIAL_SIZE_ARRAY); @ThreadConfined(UI) @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. @@ -674,9 +680,9 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { } } - private ArrayDeque getAndResetPreMountItems() { + private ArrayDeque getAndResetPreMountItems() { synchronized (mPreMountItemsLock) { - ArrayDeque result = mPreMountItems; + ArrayDeque result = mPreMountItems; if (result.isEmpty()) { return null; } @@ -685,6 +691,38 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { } } + /** + * Check if a surfaceId is active and ready for MountItems to be executed against it. It is safe + * and cheap to call this repeatedly because we expect many operations to be batched with the same + * surfaceId in a row and we memoize the parameters and results. + * + * @param surfaceId + * @param context + * @return + */ + @UiThread + @ThreadConfined(UI) + private boolean surfaceActiveForExecution(int surfaceId, String context) { + if (mLastExecutedMountItemSurfaceId != surfaceId) { + mLastExecutedMountItemSurfaceId = surfaceId; + mLastExecutedMountItemSurfaceIdActive = 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)); + } + } + + return mLastExecutedMountItemSurfaceIdActive; + } + @UiThread @ThreadConfined(UI) /** Nothing should call this directly except for `tryDispatchMountItems`. */ @@ -756,7 +794,7 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { // If there are MountItems to dispatch, we make sure all the "pre mount items" are executed // first - ArrayDeque mPreMountItemsToDispatch = getAndResetPreMountItems(); + ArrayDeque mPreMountItemsToDispatch = getAndResetPreMountItems(); if (mPreMountItemsToDispatch != null) { Systrace.beginSection( @@ -765,7 +803,11 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { + mPreMountItemsToDispatch.size()); while (!mPreMountItemsToDispatch.isEmpty()) { - mPreMountItemsToDispatch.pollFirst().execute(mMountingManager); + PreAllocateViewMountItem mountItem = mPreMountItemsToDispatch.pollFirst(); + if (surfaceActiveForExecution( + mountItem.getRootTag(), "dispatchMountItems PreAllocateViewMountItem")) { + mountItem.execute(mMountingManager); + } } Systrace.endSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE); @@ -792,14 +834,8 @@ 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; - int rootTag = batchMountItem.getRootTag(); - if (mReactContextForRootTag.get(rootTag) == null) { - ReactSoftException.logSoftException( - TAG, - new ReactNoCrashSoftException( - "dispatchMountItems: skipping batched item: surface not available [" - + rootTag - + "]")); + if (!surfaceActiveForExecution( + batchMountItem.getRootTag(), "dispatchMountItems BatchMountItem")) { continue; } } @@ -855,15 +891,18 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { break; } - MountItem preMountItemsToDispatch; + PreAllocateViewMountItem preMountItemToDispatch; synchronized (mPreMountItemsLock) { if (mPreMountItems.isEmpty()) { break; } - preMountItemsToDispatch = mPreMountItems.pollFirst(); + preMountItemToDispatch = mPreMountItems.pollFirst(); } - preMountItemsToDispatch.execute(mMountingManager); + if (surfaceActiveForExecution( + preMountItemToDispatch.getRootTag(), "dispatchPreMountItems")) { + preMountItemToDispatch.execute(mMountingManager); + } } } finally { mInDispatch = false; diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java index fad513a9ea0..7f519e9bc6f 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java @@ -47,6 +47,10 @@ public class PreAllocateViewMountItem implements MountItem { mIsLayoutable = isLayoutable; } + public int getRootTag() { + return mRootTag; + } + @Override public void execute(@NonNull MountingManager mountingManager) { if (ENABLE_FABRIC_LOGS) {