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

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 void execute(@NonNull MountingManager mountingManager) {
                       "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 void updateRootLayoutSpecs(
 
   @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 @@ private void dispatchCommandMountItem(DispatchCommandMountItem command) {
   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 void execute(MountingManager mountingManager) {
             mountingManager.setJSResponder(
                 surfaceId, reactTag, initialReactTag, blockNativeResponder);
           }
+
+          @Override
+          public int getSurfaceId() {
+            return surfaceId;
+          }
         });
   }
 
@@ -1060,6 +1070,11 @@ public void clearJSResponder() {
           public void execute(MountingManager mountingManager) {
             mountingManager.clearJSResponder();
           }
+
+          @Override
+          public int getSurfaceId() {
+            return View.NO_ID;
+          }
         });
   }
 
@@ -1108,29 +1123,25 @@ public Map<String, Long> getPerformanceCounters() {
     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 void doFrameGuarded(long frameTimeNanos) {
 
       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();

ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java

@@ -33,72 +33,35 @@
 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
  * FabricUIManager#scheduleMountItems(int, MountItem[])} on the UI thread.
  */
 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<Integer, SurfaceMountingManager> mSurfaceIdToManager =
       new ConcurrentHashMap<>(); // any thread
 
-  private volatile int mNumStaleSurfaces = 0;
+  private final CopyOnWriteArrayList<Integer> mStoppedSurfaceIds = new CopyOnWriteArrayList<>();
 
   @Nullable private SurfaceMountingManager mMostRecentSurfaceMountingManager;
 
   @NonNull private final JSResponderHandler mJSResponderHandler = new JSResponderHandler();
   @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.
-   *
-   * <p>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.
-   *
-   * <p>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<Integer, SurfaceMountingManager> 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 void startSurface(
 
   @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 void stopSurface(final int surfaceId) {
           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 SurfaceMountingManager getSurfaceManagerEnforced(int surfaceId, String co
     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

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 boolean isStopped() {
     return mIsStopped;
   }
 
-  public boolean shouldKeepAliveStoppedSurface() {
-    assert mIsStopped;
-    return (System.currentTimeMillis() - mLastSuccessfulQueryTime) < KEEPALIVE_MILLISECONDS;
-  }
-
   public ThemedReactContext getContext() {
     return mThemedReactContext;
   }

ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/DispatchIntCommandMountItem.java

@@ -27,6 +27,11 @@ public DispatchIntCommandMountItem(
     mCommandArgs = commandArgs;
   }
 
+  @Override
+  public int getSurfaceId() {
+    return mSurfaceId;
+  }
+
   @Override
   public void execute(@NonNull MountingManager mountingManager) {
     mountingManager.receiveCommand(mSurfaceId, mReactTag, mCommandId, mCommandArgs);

ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/DispatchStringCommandMountItem.java

@@ -27,6 +27,11 @@ public DispatchStringCommandMountItem(
     mCommandArgs = commandArgs;
   }
 
+  @Override
+  public int getSurfaceId() {
+    return mSurfaceId;
+  }
+
   @Override
   public void execute(@NonNull MountingManager mountingManager) {
     mountingManager.receiveCommand(mSurfaceId, mReactTag, mCommandId, mCommandArgs);

ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/IntBufferBatchMountItem.java

@@ -182,7 +182,8 @@ public void execute(@NonNull MountingManager mountingManager) {
     endMarkers();
   }
 
-  public int getRootTag() {
+  @Override
+  public int getSurfaceId() {
     return mSurfaceId;
   }
 

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();
 }

ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java

@@ -43,7 +43,8 @@ public PreAllocateViewMountItem(
     mIsLayoutable = isLayoutable;
   }
 
-  public int getRootTag() {
+  @Override
+  public int getSurfaceId() {
     return mSurfaceId;
   }
 

ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/SendAccessibilityEvent.java

@@ -42,6 +42,11 @@ public void execute(@NonNull MountingManager mountingManager) {
     }
   }
 
+  @Override
+  public int getSurfaceId() {
+    return mSurfaceId;
+  }
+
   @Override
   public String toString() {
     return "SendAccessibilityEvent [" + mReactTag + "] " + mEventType;