Fix a Nodes crash when double detaching a clipped view

Summary:
This fixes a crash for the case when we try to drop a view that has already been dropped.

**The Problem**

We got reports of a crash (t12912526) that occurs when the resolveViewManager method can't resolve a ViewManager for a View being dropped.

Investigating this, one thing in common between all the stack traces for this is that dropView is called from line 210 of FlatNativeViewHierarchyManager. This part of the code is specifically the part we added to remove strong references to any clipped children (from views that have subview clipping enabled).

So this is a problem specifically with Nodes and clipSubviews, which brings up some questions:

**when can this happen?**

The only situation this can possibly happen is when we drop a child (which is clipped) followed by dropping its parent in the same cycle. Consider a tree where each view only has one child, such as:  A - B - C - D. This crash would happen if D is clipped, and we removed it, followed by removing any of its parents in the same frame.

**if the removes happen in different frames, does this bug occur?**

No - the reason is that before we execute the DropView operations, we run through StateBuilder, which traverses the shadow tree and marks updates, thus removing the view going away (such that the delete in the next frame doesn't try to re-delete it).

So why doesn't this happen when we're dropping in the same frame? The reason is that manageChildren (where this all starts) asks to remove some views. We handle this by removing said Nodes and their children from the shadow tree. Consequently, when StateBuilder iterates over the shadow tree, it can't do the right thing because said nodes no longer exist.

As a more concrete example, consider A - B - C - D again, and consider that both D and B are removed. StateBuilder only sees A, and realizes that it now has 0 children (whereas before it has 1), so it removes B from its children. However, this process isn't recursive, so C never gets cleaned up.

**why doesn't this happen with Nodes without clipping containers?**

The answer to this is that NativeViewHierarchyManager's dropView method checks the existance of each child before deeply dropping that child and its subtree. So in this case, we drop D and all its children, and when we come to drop B, we try to drop C (which exists) and then its children (D, which doesn't exist because we already dropped it, so we ignore it).

**why doesn't this happen with non-Nodes?**

The reason is that non-Nodes handles removes differently - every remove is enqueued in a call to NativeViewHierarchy's manageChildren, which explicitly asks the parent to remove said child. Consequently, we never try to remove a child that is already removed.

**Fix**

The initial fix was to check whether or not the view exists, but this updated patch just does the right thing at drop time - i.e. whenever a view is dropped, we notify the parent of this fact so that it can clear the reference from clipped views.

**One last Note**

There are two reasons for switching `super.dropView` to `dropView` - first, the comment is only partially correct - calling `super.dropView` will avoid looking at clipped children (as an aside, that could cause a leak in the case of nested clipping subviews), but will look at clipped grandchildren, because of the super class's iteration across the set of children.

Reviewed By: astreet

Differential Revision: D3815485

ReactAndroid/src/main/java/com/facebook/react/flat/ClippingDrawCommandManager.java

@@ -301,6 +301,12 @@ import com.facebook.react.uimanager.ReactClippingViewGroupHelper;
     return mClippedSubviews.get(id) == null;
   }
 
+  @Override
+  void onClippedViewDropped(View view) {
+    unclip(view.getId());
+    mFlatViewGroup.removeDetachedView(view);
+  }
+
   @Override
   public void mountViews(ViewResolver viewResolver, int[] viewsToAdd, int[] viewsToDetach) {
     for (int viewToAdd : viewsToAdd) {

ReactAndroid/src/main/java/com/facebook/react/flat/DrawCommandManager.java

@@ -11,8 +11,6 @@ package com.facebook.react.flat;
 
 import javax.annotation.Nullable;
 
-import java.util.Collection;
-
 import android.graphics.Canvas;
 import android.graphics.Rect;
 import android.util.SparseArray;
@@ -125,6 +123,13 @@ import android.view.ViewParent;
    */
   abstract @Nullable NodeRegion virtualNodeRegionWithinBounds(float touchX, float touchY);
 
+  /**
+   * Event that is fired when a clipped view is dropped.
+   *
+   * @param view the view that is dropped
+   */
+  abstract void onClippedViewDropped(View view);
+
   /**
    * Throw a runtime exception if a view we are trying to attach is already parented.
    *

ReactAndroid/src/main/java/com/facebook/react/flat/FlatNativeViewHierarchyManager.java

@@ -186,14 +186,32 @@ import com.facebook.react.uimanager.ViewManagerRegistry;
     resolveView(reactTag).setPadding(paddingLeft, paddingTop, paddingRight, paddingBottom);
   }
 
-  /* package */ void dropViews(int[] viewsToDrop) {
-    for (int viewToDrop : viewsToDrop) {
+  /* package */ void dropViews(SparseIntArray viewsToDrop) {
+    for (int i = 0, count = viewsToDrop.size(); i < count; i++) {
+      int viewToDrop = viewsToDrop.keyAt(i);
+      View view = null;
       if (viewToDrop > 0) {
-        dropView(resolveView(viewToDrop));
+        view = resolveView(viewToDrop);
+        dropView(view);
       } else {
         // Root views are noted with a negative tag from StateBuilder.
         removeRootView(-viewToDrop);
       }
+
+      int parentTag = viewsToDrop.valueAt(i);
+      // this only happens for clipped, non-root views - clipped because there is no parent, and
+      // not a root view (because we explicitly pass -1 for root views).
+      if (parentTag > 0 && view != null && view.getParent() == null) {
+        // this can only happen if the parent exists (if the parent were removed first, it'd also
+        // remove the child, so trying to explicitly remove the child afterwards would crash at
+        // the resolveView call above) - we also explicitly check for a null parent, implying that
+        // we are either clipped (or that we already removed the child from its parent, in which
+        // case this will essentially be a no-op).
+        View parent = resolveView(parentTag);
+        if (parent instanceof FlatViewGroup) {
+          ((FlatViewGroup) parent).onViewDropped(view);
+        }
+      }
     }
   }
 
@@ -211,11 +229,8 @@ import com.facebook.react.uimanager.ViewManagerRegistry;
         SparseArray<View> detachedViews = flatViewGroup.getDetachedViews();
         for (int i = 0, size = detachedViews.size(); i < size; i++) {
           View detachedChild = detachedViews.valueAt(i);
-          // we can do super here because removeClippedSubviews is currently not recursive. if/when
-          // we become recursive one day, this should call vanilla dropView to be recursive as well.
-          super.dropView(detachedChild);
-          // trigger onDetachedFromWindow - this is currently needed due to using attach/detach
-          // instead of add/remove. if we move to add/remove in the future, we can remove this.
+          dropView(detachedChild);
+          // trigger onDetachedFromWindow and clean up this detached/clipped view
           flatViewGroup.removeDetachedView(detachedChild);
         }
       }

ReactAndroid/src/main/java/com/facebook/react/flat/FlatUIImplementation.java

@@ -342,7 +342,9 @@ public class FlatUIImplementation extends UIImplementation {
         --moveFromIndex;
         moveFromChildIndex = (moveFromIndex == -1) ? -1 : mMoveProxy.getMoveFrom(moveFromIndex);
       } else if (removeFromChildIndex > moveFromChildIndex) {
-        removeChild(removeChildAt(parentNode, removeFromChildIndex, prevIndex));
+        removeChild(
+            removeChildAt(parentNode, removeFromChildIndex, prevIndex),
+            parentNode.getReactTag());
         prevIndex = removeFromChildIndex;
 
         --removeFromIndex;
@@ -359,19 +361,19 @@ public class FlatUIImplementation extends UIImplementation {
    * Unregisters given element and all of its children from ShadowNodeRegistry,
    * and drops all Views used by it and its children.
    */
-  private void removeChild(ReactShadowNode child) {
+  private void removeChild(ReactShadowNode child, int parentReactTag) {
     if (child instanceof FlatShadowNode) {
       FlatShadowNode node = (FlatShadowNode) child;
       if (node.mountsToView() && node.isBackingViewCreated()) {
         // this will recursively drop all subviews
-        mStateBuilder.dropView(node);
+        mStateBuilder.dropView(node, parentReactTag);
         removeShadowNode(node);
         return;
       }
     }
 
     for (int i = 0, childCount = child.getChildCount(); i != childCount; ++i) {
-      removeChild(child.getChildAt(i));
+      removeChild(child.getChildAt(i), child.getReactTag());
     }
 
     removeShadowNode(child);

ReactAndroid/src/main/java/com/facebook/react/flat/FlatUIViewOperationQueue.java

@@ -205,9 +205,9 @@ import com.facebook.react.uimanager.UIViewOperationQueue;
 
   private final class DropViews implements UIOperation {
 
-    private final int[] mViewsToDrop;
+    private final SparseIntArray mViewsToDrop;
 
-    private DropViews(int[] viewsToDrop) {
+    private DropViews(SparseIntArray viewsToDrop) {
       mViewsToDrop = viewsToDrop;
     }
 
@@ -478,7 +478,7 @@ import com.facebook.react.uimanager.UIViewOperationQueue;
         new SetPadding(reactTag, paddingLeft, paddingTop, paddingRight, paddingBottom));
   }
 
-  public void enqueueDropViews(int[] viewsToDrop) {
+  public void enqueueDropViews(SparseIntArray viewsToDrop) {
     enqueueUIOperation(new DropViews(viewsToDrop));
   }
 

ReactAndroid/src/main/java/com/facebook/react/flat/FlatViewGroup.java

@@ -763,6 +763,22 @@ import com.facebook.react.uimanager.ReactClippingViewGroup;
     invalidate();
   }
 
+  /**
+   * Handle a subview being dropped
+   * In most cases, we are informed about a subview being dropped via mountViews, but in some
+   * cases (such as when both the child and parent get explicit removes in the same frame),
+   * we may not find out, so this is called when the child is dropped so the parent can clean up
+   * strong references to the child.
+   *
+   * @param view the view being dropped
+   */
+  void onViewDropped(View view) {
+    if (mDrawCommandManager != null) {
+      // for now, we only care about clearing clipped subview references
+      mDrawCommandManager.onClippedViewDropped(view);
+    }
+  }
+
   /**
    * Return the NodeRegion which matches a reactTag, or EMPTY if none match.
    *

ReactAndroid/src/main/java/com/facebook/react/flat/StateBuilder.java

@@ -13,7 +13,6 @@ import javax.annotation.Nullable;
 
 import java.util.ArrayList;
 
-import android.util.SparseArray;
 import android.util.SparseIntArray;
 
 import com.facebook.csslayout.Spacing;
@@ -52,7 +51,7 @@ import com.facebook.react.uimanager.events.EventDispatcher;
 
   private final ArrayList<FlatShadowNode> mViewsToDetachAllChildrenFrom = new ArrayList<>();
   private final ArrayList<FlatShadowNode> mViewsToDetach = new ArrayList<>();
-  private final ArrayList<Integer> mViewsToDrop = new ArrayList<>();
+  private final SparseIntArray mViewsToDrop = new SparseIntArray();
   private final ArrayList<OnLayoutEvent> mOnLayoutEvents = new ArrayList<>();
   private final ArrayList<UIViewOperationQueue.UIOperation> mUpdateViewBoundsOperations =
       new ArrayList<>();
@@ -132,13 +131,8 @@ import com.facebook.react.uimanager.events.EventDispatcher;
     }
     mOnLayoutEvents.clear();
 
-    if (!mViewsToDrop.isEmpty()) {
-      int[] viewsToDrop = new int[mViewsToDrop.size()];
-      int i = 0;
-      for (int x : mViewsToDrop) {
-        viewsToDrop[i++] = x;
-      }
-      mOperationsQueue.enqueueDropViews(viewsToDrop);
+    if (mViewsToDrop.size() > 0) {
+      mOperationsQueue.enqueueDropViews(mViewsToDrop);
       mViewsToDrop.clear();
     }
 
@@ -147,7 +141,7 @@ import com.facebook.react.uimanager.events.EventDispatcher;
 
   /* package */ void removeRootView(int rootViewTag) {
     // Note root view tags with a negative value.
-    mViewsToDrop.add(-rootViewTag);
+    mViewsToDrop.put(-rootViewTag, -1);
   }
 
   /**
@@ -234,8 +228,8 @@ import com.facebook.react.uimanager.events.EventDispatcher;
    *
    * @param node The node to drop the backing view for.
    */
-  /* package */ void dropView(FlatShadowNode node) {
-    mViewsToDrop.add(node.getReactTag());
+  /* package */ void dropView(FlatShadowNode node, int parentReactTag) {
+    mViewsToDrop.put(node.getReactTag(), parentReactTag);
   }
 
   /**