From d183fd327b7681d8393a6ad02456cecc4df373de Mon Sep 17 00:00:00 2001
From: Joshua Gross <joshuagross@fb.com>
Date: Fri, 19 Jun 2020 11:11:08 -0700
Subject: [PATCH] Improve MountingManager diagnostics around View removal

Summary:
It is possible (most recently, if there are bugs in LayoutAnimations, but also in general) to issue a `removeViewAt` MountItem that removes the incorrect view if, for whatever reason, the View hierarchy has become "corrupt"
and Views are out of order. I added two heuristics to catch if that happens: check the tag of the View being removed if possible, and ensure that all deleted views do not have a parent. This will turn weird visual glitches into
hard crashes, which we want once the UI has become corrupt.

My only concern here is with perf; maybe we could put these behind a debug-only flag or something, but it's probably not a huge deal.

Changelog: [Internal]

Reviewed By: mdvacca

Differential Revision: D22130186

fbshipit-source-id: 0942b019c3449d68edfb9db1fe8130ea351d1d8f
---
 .../fabric/mounting/MountingManager.java      | 25 ++++++++++++++++++-
 .../RemoveDeleteMultiMountItem.java           |  3 ++-
 .../mounting/mountitems/RemoveMountItem.java  |  2 +-
 3 files changed, 27 insertions(+), 3 deletions(-)

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 e48192e8963..2f7f893b384 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
@@ -224,7 +224,7 @@ public class MountingManager {
   }
 
   @UiThread
-  public void removeViewAt(int parentTag, int index) {
+  public void removeViewAt(int tag, int parentTag, int index) {
     UiThreadUtil.assertOnUiThread();
     ViewState viewState = getNullableViewState(parentTag);
 
@@ -244,6 +244,22 @@ public class MountingManager {
 
     ViewGroupManager<ViewGroup> viewGroupManager = getViewGroupManager(viewState);
 
+    // Verify that the view we're about to remove has the same tag we expect
+    if (tag != -1) {
+      View view = viewGroupManager.getChildAt(parentView, index);
+      if (view != null && view.getId() != tag) {
+        throw new IllegalStateException(
+            "Tried to delete view ["
+                + tag
+                + "] of parent ["
+                + parentTag
+                + "] at index "
+                + index
+                + ", but got view tag "
+                + view.getId());
+      }
+    }
+
     try {
       viewGroupManager.removeViewAt(parentView, index);
     } catch (RuntimeException e) {
@@ -397,6 +413,13 @@ public class MountingManager {
     }
 
     View view = viewState.mView;
+    ViewParent parentView = view.getParent();
+
+    if (parentView != null) {
+      throw new IllegalStateException(
+          "Cannot delete view that is still attached to parent: [" + reactTag + "]");
+    }
+
     if (view != null) {
       dropView(view);
     } else {
diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/RemoveDeleteMultiMountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/RemoveDeleteMultiMountItem.java
index c11cb4e4412..daf3ed5ca75 100644
--- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/RemoveDeleteMultiMountItem.java
+++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/RemoveDeleteMultiMountItem.java
@@ -49,8 +49,9 @@ public class RemoveDeleteMultiMountItem implements MountItem {
       int flags = mMetadata[i + FLAGS_INDEX];
       if ((flags & REMOVE_FLAG) != 0) {
         int parentTag = mMetadata[i + PARENT_TAG_INDEX];
+        int tag = mMetadata[i + TAG_INDEX];
         int index = mMetadata[i + VIEW_INDEX_INDEX];
-        mountingManager.removeViewAt(parentTag, index);
+        mountingManager.removeViewAt(tag, parentTag, index);
       }
     }
 
diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/RemoveMountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/RemoveMountItem.java
index f09f91e3f98..3b6fd921100 100644
--- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/RemoveMountItem.java
+++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/RemoveMountItem.java
@@ -24,7 +24,7 @@ public class RemoveMountItem implements MountItem {
 
   @Override
   public void execute(@NonNull MountingManager mountingManager) {
-    mountingManager.removeViewAt(mParentReactTag, mIndex);
+    mountingManager.removeViewAt(-1, mParentReactTag, mIndex);
   }
 
   public int getParentReactTag() {