From 5792c32e362d95a94f3fafcf02ad38f11206b23f Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Mon, 25 Jan 2021 20:26:15 -0800 Subject: [PATCH] Fix race between stopSurface and PreAllocateMountItem Summary: There is a race between PreAllocateMountItems executing and killing off stale SurfaceMountingManagers. For now I'm simplifying the "eviction" mechanism. We just keep up to 15 SurfaceMountingManagers around and start evicting them from the least-recently-referenced one. Hopefully this logic can be simplified in the future. I'm also sneaking in a small perf optimization for PreAllocateMountItem: don't queue them if the associated surface is already stopped, because chewing through a bunch of dead PreAllocateMountItems on the UI thread can be expensive. Changelog: [Internal] Reviewed By: mdvacca Differential Revision: D26059378 fbshipit-source-id: 3b2503d7d8e5f045ae741d0d4a5880d1b37758d2 --- .../react/fabric/FabricUIManager.java | 59 ++++++------ .../fabric/mounting/MountingManager.java | 92 +++++++++---------- .../mounting/SurfaceMountingManager.java | 6 -- .../DispatchIntCommandMountItem.java | 5 + .../DispatchStringCommandMountItem.java | 5 + .../mountitems/IntBufferBatchMountItem.java | 3 +- .../fabric/mounting/mountitems/MountItem.java | 4 + .../mountitems/PreAllocateViewMountItem.java | 3 +- .../mountitems/SendAccessibilityEvent.java | 5 + 9 files changed, 99 insertions(+), 83 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 51fa1b80c1e..42a5dfa7a86 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java @@ -95,7 +95,7 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { // The IS_DEVELOPMENT_ENVIRONMENT variable is used to log extra data when running fabric in a // development environment. DO NOT ENABLE THIS ON PRODUCTION OR YOU WILL BE FIRED! - public static final boolean IS_DEVELOPMENT_ENVIRONMENT = false; + public static final boolean IS_DEVELOPMENT_ENVIRONMENT = false && ReactBuildConfig.DEBUG; public static final boolean ENABLE_FABRIC_LOGS = ReactFeatureFlags.enableFabricLogs || PrinterHolder.getPrinter() @@ -483,6 +483,11 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { "Caught exception in synchronouslyUpdateViewOnUIThread", ex)); } } + + @Override + public int getSurfaceId() { + return View.NO_ID; + } }; // If the reactTag exists, we assume that it might at the end of the next @@ -905,13 +910,13 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { @Override public void receiveEvent(int reactTag, String eventName, @Nullable WritableMap params) { - receiveEvent(-1, reactTag, eventName, params); + receiveEvent(View.NO_ID, reactTag, eventName, params); } @Override public void receiveEvent( int surfaceId, int reactTag, String eventName, @Nullable WritableMap params) { - if (ReactBuildConfig.DEBUG && surfaceId == -1) { + if (ReactBuildConfig.DEBUG && surfaceId == View.NO_ID) { FLog.d(TAG, "Emitted event without surfaceId: [%d] %s", reactTag, eventName); } @@ -1004,7 +1009,7 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { public void sendAccessibilityEvent(int reactTag, int eventType) { // Can be called from native, not just JS - we need to migrate the native callsites // before removing this entirely. - addMountItem(new SendAccessibilityEvent(-1, reactTag, eventType)); + addMountItem(new SendAccessibilityEvent(View.NO_ID, reactTag, eventType)); } @DoNotStrip @@ -1045,6 +1050,11 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { mountingManager.setJSResponder( surfaceId, reactTag, initialReactTag, blockNativeResponder); } + + @Override + public int getSurfaceId() { + return surfaceId; + } }); } @@ -1060,6 +1070,11 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { public void execute(MountingManager mountingManager) { mountingManager.clearJSResponder(); } + + @Override + public int getSurfaceId() { + return View.NO_ID; + } }); } @@ -1108,29 +1123,25 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { return performanceCounters; } - /** - * Abstraction between concurrent and non-concurrent MountItem list. - * - * @param mountItem - */ private void addMountItem(MountItem mountItem) { mMountItemsConcurrent.add(mountItem); } - /** - * Abstraction between concurrent and non-concurrent PreAllocateViewMountItem list. - * - * @param mountItem - */ private void addPreAllocateMountItem(PreAllocateViewMountItem mountItem) { - mPreMountItemsConcurrent.add(mountItem); + // We do this check only for PreAllocateViewMountItem - and not DispatchMountItem or regular + // MountItem - because PreAllocateViewMountItem is not batched, and is relatively more expensive + // both to queue, to drain, and to execute. + if (!mMountingManager.surfaceIsStopped(mountItem.getSurfaceId())) { + mPreMountItemsConcurrent.add(mountItem); + } else if (IS_DEVELOPMENT_ENVIRONMENT) { + FLog.e( + TAG, + "Not queueing PreAllocateMountItem: surfaceId stopped: [%d] - %s", + mountItem.getSurfaceId(), + mountItem.toString()); + } } - /** - * Abstraction between concurrent and non-concurrent DispatchCommandMountItem list. - * - * @param mountItem - */ private void addViewCommandMountItem(DispatchCommandMountItem mountItem) { mViewCommandMountItemsConcurrent.add(mountItem); } @@ -1167,13 +1178,7 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { try { dispatchPreMountItems(frameTimeNanos); - boolean dispatchedMountItems = tryDispatchMountItems(); - - // Only if we did no work (besides preallocation) and have time left, evict stale - // SurfaceMountingManagers - if (!dispatchedMountItems && !haveExceededNonBatchedFrameTime(frameTimeNanos)) { - mMountingManager.evictStaleSurfaces(); - } + tryDispatchMountItems(); } catch (Exception ex) { FLog.e(TAG, "Exception thrown when executing UIFrameGuarded", ex); stop(); diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java index c31cf4db49a..bf3ddbeaaeb 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java @@ -33,6 +33,7 @@ import com.facebook.react.uimanager.ViewManagerRegistry; import com.facebook.yoga.YogaMeasureMode; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArrayList; /** * Class responsible for actually dispatching view updates enqueued via {@link @@ -40,12 +41,13 @@ import java.util.concurrent.ConcurrentHashMap; */ public class MountingManager { public static final String TAG = MountingManager.class.getSimpleName(); + private static final int MAX_STOPPED_SURFACE_IDS_LENGTH = 15; @NonNull private final ConcurrentHashMap mSurfaceIdToManager = new ConcurrentHashMap<>(); // any thread - private volatile int mNumStaleSurfaces = 0; + private final CopyOnWriteArrayList mStoppedSurfaceIds = new CopyOnWriteArrayList<>(); @Nullable private SurfaceMountingManager mMostRecentSurfaceMountingManager; @@ -53,52 +55,13 @@ public class MountingManager { @NonNull private final ViewManagerRegistry mViewManagerRegistry; @NonNull private final RootViewManager mRootViewManager = new RootViewManager(); + private volatile int mStoppedSurfaceCacheLastId = View.NO_ID; + private volatile boolean mStoppedSurfaceCacheLastResult = false; + public MountingManager(@NonNull ViewManagerRegistry viewManagerRegistry) { mViewManagerRegistry = viewManagerRegistry; } - /** - * Evict stale SurfaceManagers. - * - *

The reasoning here is that we want SurfaceManagers to stay around for a little while after - * the Surface is stopped, to gracefully handle race conditions with (1) native libraries like - * NativeAnimatedModule, (2) events emitted to nodes on the surface, (3) queued imperative calls - * like dispatchCommand or sendAccessibilityEvent. - * - *

Without keeping the SurfaceManager around, those race conditions would result in us not - * being able to resolve a tag at all, meaning some operation is happening with a totally invalid, - * unknown tag. However, we want to fail gracefully since it's common for operations to be queued - * up and races to happen with StopSurface. This way, we can distinguish between those race - * conditions and other totally invalid operations on non-existing nodes. - */ - @UiThread - public void evictStaleSurfaces() { - UiThreadUtil.assertOnUiThread(); - - if (mNumStaleSurfaces == 0) { - return; - } - - mNumStaleSurfaces = 0; - - for (Map.Entry entry : mSurfaceIdToManager.entrySet()) { - SurfaceMountingManager surfaceMountingManager = entry.getValue(); - int surfacedId = entry.getKey(); - if (surfaceMountingManager.isStopped()) { - if (surfaceMountingManager.shouldKeepAliveStoppedSurface()) { - mNumStaleSurfaces++; - } else { - FLog.e(TAG, "Evicting stale SurfaceMountingManager: [%d]", surfacedId); - mSurfaceIdToManager.remove(surfacedId); - - if (surfaceMountingManager == mMostRecentSurfaceMountingManager) { - mMostRecentSurfaceMountingManager = null; - } - } - } - } - } - /** * This mutates the rootView, which is an Android View, so this should only be called on the UI * thread. @@ -136,10 +99,20 @@ public class MountingManager { @AnyThread public void stopSurface(final int surfaceId) { + mStoppedSurfaceCacheLastId = View.NO_ID; + SurfaceMountingManager surfaceMountingManager = mSurfaceIdToManager.get(surfaceId); if (surfaceMountingManager != null) { + // Maximum number of stopped surfaces to keep track of + while (mStoppedSurfaceIds.size() >= MAX_STOPPED_SURFACE_IDS_LENGTH) { + Integer staleStoppedId = mStoppedSurfaceIds.get(0); + mSurfaceIdToManager.remove(staleStoppedId.intValue()); + mStoppedSurfaceIds.remove(staleStoppedId); + FLog.d(TAG, "Removing stale SurfaceMountingManager: [%d]", staleStoppedId.intValue()); + } + mStoppedSurfaceIds.add(surfaceId); + surfaceMountingManager.stopSurface(); - mNumStaleSurfaces++; if (surfaceMountingManager == mMostRecentSurfaceMountingManager) { mMostRecentSurfaceMountingManager = null; @@ -150,10 +123,6 @@ public class MountingManager { new IllegalViewOperationException( "Cannot call StopSurface on non-existent surface: [" + surfaceId + "]")); } - - // We do not evict surfaces right away; the SurfaceMountingManager will stay in memory for a bit - // longer. See SurfaceMountingManager.stopSurface and - // evictStaleSurfaces for more details. } @Nullable @@ -176,6 +145,33 @@ public class MountingManager { return surfaceMountingManager; } + public boolean surfaceIsStopped(int surfaceId) { + if (surfaceId == View.NO_ID) { + return false; + } + if (surfaceId == mStoppedSurfaceCacheLastId) { + return mStoppedSurfaceCacheLastResult; + } + + boolean res = surfaceIsStoppedImpl(surfaceId); + mStoppedSurfaceCacheLastResult = res; + mStoppedSurfaceCacheLastId = surfaceId; + return res; + } + + private boolean surfaceIsStoppedImpl(int surfaceId) { + if (mStoppedSurfaceIds.contains(surfaceId)) { + return true; + } + + SurfaceMountingManager surfaceMountingManager = mSurfaceIdToManager.get(surfaceId); + if (surfaceMountingManager != null && surfaceMountingManager.isStopped()) { + return true; + } + + return false; + } + /** * Get SurfaceMountingManager associated with a ReactTag. Unfortunately, this requires lookups * over N maps, where N is the number of active or recently-stopped Surfaces. Each lookup will diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java index 3cbc20118e0..5c3686e7787 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java @@ -45,7 +45,6 @@ public class SurfaceMountingManager { public static final String TAG = SurfaceMountingManager.class.getSimpleName(); private static final boolean SHOW_CHANGED_VIEW_HIERARCHIES = ReactBuildConfig.DEBUG && false; - private static final long KEEPALIVE_MILLISECONDS = 1000; private volatile boolean mIsStopped = false; @@ -84,11 +83,6 @@ public class SurfaceMountingManager { return mIsStopped; } - public boolean shouldKeepAliveStoppedSurface() { - assert mIsStopped; - return (System.currentTimeMillis() - mLastSuccessfulQueryTime) < KEEPALIVE_MILLISECONDS; - } - public ThemedReactContext getContext() { return mThemedReactContext; } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/DispatchIntCommandMountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/DispatchIntCommandMountItem.java index 176abe1f270..6f23f9b8779 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/DispatchIntCommandMountItem.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/DispatchIntCommandMountItem.java @@ -27,6 +27,11 @@ public class DispatchIntCommandMountItem extends DispatchCommandMountItem { mCommandArgs = commandArgs; } + @Override + public int getSurfaceId() { + return mSurfaceId; + } + @Override public void execute(@NonNull MountingManager mountingManager) { mountingManager.receiveCommand(mSurfaceId, mReactTag, mCommandId, mCommandArgs); diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/DispatchStringCommandMountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/DispatchStringCommandMountItem.java index 361f92ad619..1d76091b6ae 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/DispatchStringCommandMountItem.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/DispatchStringCommandMountItem.java @@ -27,6 +27,11 @@ public class DispatchStringCommandMountItem extends DispatchCommandMountItem { mCommandArgs = commandArgs; } + @Override + public int getSurfaceId() { + return mSurfaceId; + } + @Override public void execute(@NonNull MountingManager mountingManager) { mountingManager.receiveCommand(mSurfaceId, mReactTag, mCommandId, mCommandArgs); diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java index 02106de603d..311a08143ea 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java @@ -182,7 +182,8 @@ public class IntBufferBatchMountItem implements MountItem { endMarkers(); } - public int getRootTag() { + @Override + public int getSurfaceId() { return mSurfaceId; } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/MountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/MountItem.java index dc053625c2a..5ec3aeea571 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/MountItem.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/MountItem.java @@ -7,6 +7,7 @@ package com.facebook.react.fabric.mounting.mountitems; +import androidx.annotation.AnyThread; import androidx.annotation.NonNull; import androidx.annotation.UiThread; import com.facebook.react.fabric.mounting.MountingManager; @@ -16,4 +17,7 @@ public interface MountItem { /** Execute this {@link MountItem} into the operation queue received by parameter. */ @UiThread void execute(@NonNull MountingManager mountingManager); + + @AnyThread + int getSurfaceId(); } 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 a39d5ff31c0..7764bd58a3c 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 @@ -43,7 +43,8 @@ public class PreAllocateViewMountItem implements MountItem { mIsLayoutable = isLayoutable; } - public int getRootTag() { + @Override + public int getSurfaceId() { return mSurfaceId; } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/SendAccessibilityEvent.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/SendAccessibilityEvent.java index 9506857f195..a52ed29042d 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/SendAccessibilityEvent.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/SendAccessibilityEvent.java @@ -42,6 +42,11 @@ public class SendAccessibilityEvent implements MountItem { } } + @Override + public int getSurfaceId() { + return mSurfaceId; + } + @Override public String toString() { return "SendAccessibilityEvent [" + mReactTag + "] " + mEventType;