New StopSurface deleteView mechanism

Summary:
Simplifying the StopSurface flow. Before we would still attempt to execute MountItems, but only the "Delete" operations. This was... well, frankly, overcomplicated. Instead we can just ignore all future MountInstructions for that Surface and delete all views recursively from the root.

Changelog: [Internal]

Reviewed By: mdvacca

Differential Revision: D23338752

fbshipit-source-id: 6e7ab29ad85572782bfc6a39845a8a619f001559

ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java

@@ -253,9 +253,16 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
   @AnyThread
   @ThreadConfined(ANY)
   @Override
-  public void stopSurface(int surfaceID) {
+  public void stopSurface(final int surfaceID) {
     mReactContextForRootTag.remove(surfaceID);
     mBinding.stopSurface(surfaceID);
+    UiThreadUtil.runOnUiThread(
+        new Runnable() {
+          @Override
+          public void run() {
+            mMountingManager.deleteRootView(surfaceID);
+          }
+        });
   }
 
   @Override
@@ -876,7 +883,6 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
             BatchMountItem batchMountItem = (BatchMountItem) mountItem;
             if (!surfaceActiveForExecution(
                 batchMountItem.getRootTag(), "dispatchMountItems BatchMountItem")) {
-              batchMountItem.executeDeletes(mMountingManager);
               continue;
             }
           }

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

@@ -101,12 +101,20 @@ public class MountingManager {
     rootView.setId(reactRootTag);
   }
 
+  /** Delete rootView and all children/ */
+  @UiThread
+  public void deleteRootView(int reactRootTag) {
+    if (mTagToViewState.containsKey(reactRootTag)) {
+      dropView(mTagToViewState.get(reactRootTag).mView, true);
+    }
+  }
+
   /** Releases all references to given native View. */
   @UiThread
-  private void dropView(@NonNull View view) {
+  private void dropView(@NonNull View view, boolean deleteImmediately) {
     UiThreadUtil.assertOnUiThread();
 
-    int reactTag = view.getId();
+    final int reactTag = view.getId();
     ViewState state = getViewState(reactTag);
     ViewManager viewManager = state.mViewManager;
 
@@ -115,29 +123,75 @@ public class MountingManager {
       viewManager.onDropViewInstance(view);
     }
     if (view instanceof ViewGroup && viewManager instanceof ViewGroupManager) {
-      ViewGroup viewGroup = (ViewGroup) view;
-      ViewGroupManager<ViewGroup> viewGroupManager = getViewGroupManager(state);
-      for (int i = viewGroupManager.getChildCount(viewGroup) - 1; i >= 0; i--) {
-        View child = viewGroupManager.getChildAt(viewGroup, i);
-        if (getNullableViewState(child.getId()) != null) {
-          if (SHOW_CHANGED_VIEW_HIERARCHIES) {
-            FLog.e(
-                TAG,
-                "Automatically dropping view that is still attached to a parent being dropped. Parent: ["
-                    + reactTag
-                    + "] child: ["
-                    + child.getId()
-                    + "]");
-          }
-          dropView(child);
-        }
-        viewGroupManager.removeViewAt(viewGroup, i);
+      final ViewGroup viewGroup = (ViewGroup) view;
+      final ViewGroupManager<ViewGroup> viewGroupManager = getViewGroupManager(state);
+
+      // As documented elsewhere, sometimes when a child is removed from a parent, that change
+      // is not immediately available in the hierarchy until a future UI tick. This can cause
+      // inconsistent child counts, etc, but it can _also_ cause us to drop views that shouldn't,
+      // because they're removed from the parent but that change isn't immediately visible. So,
+      // we do two things: 1) delay this logic until the next UI thread tick, 2) ignore children
+      // who don't report the expected parent.
+      // For most cases, we _do not_ want this logic to run, anyway, since it either means that we
+      // don't have a correct set of MountingInstructions; or it means that we're tearing down an
+      // entire screen, in which case we can safely delete everything immediately, not having
+      // executed any remove instructions immediately before this.
+      if (deleteImmediately) {
+        dropChildren(reactTag, viewGroup, viewGroupManager);
+      } else {
+        UiThreadUtil.runOnUiThread(
+            new Runnable() {
+              @Override
+              public void run() {
+                dropChildren(reactTag, viewGroup, viewGroupManager);
+              }
+            });
       }
     }
 
     mTagToViewState.remove(reactTag);
   }
 
+  @UiThread
+  private void dropChildren(
+      int reactTag,
+      @NonNull ViewGroup viewGroup,
+      @NonNull ViewGroupManager<ViewGroup> viewGroupManager) {
+    for (int i = viewGroupManager.getChildCount(viewGroup) - 1; i >= 0; i--) {
+      View child = viewGroupManager.getChildAt(viewGroup, i);
+      if (getNullableViewState(child.getId()) != null) {
+        if (SHOW_CHANGED_VIEW_HIERARCHIES) {
+          FLog.e(
+              TAG,
+              "Automatically dropping view that is still attached to a parent being dropped. Parent: ["
+                  + reactTag
+                  + "] child: ["
+                  + child.getId()
+                  + "]");
+        }
+        ViewParent childParent = child.getParent();
+        if (childParent == null || !childParent.equals(viewGroup)) {
+          int childParentId =
+              (childParent == null
+                  ? -1
+                  : (childParent instanceof ViewGroup ? ((ViewGroup) childParent).getId() : -1));
+          FLog.e(
+              TAG,
+              "Recursively deleting children of ["
+                  + reactTag
+                  + "] but parent of child ["
+                  + child.getId()
+                  + "] is ["
+                  + childParentId
+                  + "]");
+        } else {
+          dropView(child, true);
+        }
+      }
+      viewGroupManager.removeViewAt(viewGroup, i);
+    }
+  }
+
   @UiThread
   public void addViewAt(final int parentTag, final int tag, final int index) {
     UiThreadUtil.assertOnUiThread();
@@ -491,7 +545,7 @@ public class MountingManager {
     View view = viewState.mView;
 
     if (view != null) {
-      dropView(view);
+      dropView(view, false);
     } else {
       mTagToViewState.remove(reactTag);
     }

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

@@ -8,7 +8,6 @@
 package com.facebook.react.fabric.mounting.mountitems;
 
 import androidx.annotation.NonNull;
-import com.facebook.common.logging.FLog;
 import com.facebook.proguard.annotations.DoNotStrip;
 import com.facebook.react.bridge.ReactMarker;
 import com.facebook.react.bridge.ReactMarkerConstants;
@@ -72,32 +71,7 @@ public class BatchMountItem implements MountItem {
     beginMarkers("mountViews");
 
     for (int mountItemIndex = 0; mountItemIndex < mSize; mountItemIndex++) {
-      MountItem mountItem = mMountItems[mountItemIndex];
-      mountItem.execute(mountingManager);
-    }
-
-    endMarkers();
-  }
-
-  /**
-   * In the case of teardown/stopSurface, we want to delete all views associated with a SurfaceID.
-   * It can be the case that a single BatchMountItem contains both the create *and* delete
-   * instruction for a view, so this needs to be failsafe.
-   *
-   * @param mountingManager
-   */
-  public void executeDeletes(@NonNull MountingManager mountingManager) {
-    beginMarkers("deleteViews");
-
-    for (int mountItemIndex = 0; mountItemIndex < mSize; mountItemIndex++) {
-      MountItem mountItem = mMountItems[mountItemIndex];
-      if (mountItem instanceof RemoveDeleteMultiMountItem) {
-        try {
-          ((RemoveDeleteMultiMountItem) mountItem).executeDeletes(mountingManager, true);
-        } catch (RuntimeException e) {
-          FLog.e(TAG, "Ignoring deletion exception", e);
-        }
-      }
+      mMountItems[mountItemIndex].execute(mountingManager);
     }
 
     endMarkers();

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

@@ -56,30 +56,11 @@ public class RemoveDeleteMultiMountItem implements MountItem {
     }
 
     // After removing all views, delete all views marked for deletion.
-    executeDeletes(mountingManager, false);
-  }
-
-  /**
-   * Execute only deletion operations. When being executed as part of shutdown/stopping surface,
-   * deletion failures can be ignored. For example: if there is a batch of MountItems being executed
-   * as part of stopSurface, a "Create" that is not executed may have a matching "Delete". The
-   * Delete will fail but we can safely ignore it in those cases.
-   *
-   * @param mountingManager
-   * @param ignoreFailures
-   */
-  public void executeDeletes(@NonNull MountingManager mountingManager, boolean ignoreFailures) {
     for (int i = 0; i < mMetadata.length; i += 4) {
       int flags = mMetadata[i + FLAGS_INDEX];
       if ((flags & DELETE_FLAG) != 0) {
         int tag = mMetadata[i + TAG_INDEX];
-        try {
-          mountingManager.deleteView(tag);
-        } catch (IllegalStateException e) {
-          if (!ignoreFailures) {
-            throw e;
-          }
-        }
+        mountingManager.deleteView(tag);
       }
     }
   }