From a78a7166b818e8d6c06f2b831b7a3fb9017b4b59 Mon Sep 17 00:00:00 2001
From: Joshua Gross <joshuagross@fb.com>
Date: Thu, 1 Oct 2020 20:12:01 -0700
Subject: [PATCH] Throw soft-error and continue when removing view from parent
 with incorrect index

Summary:
iOS Fabric actually ignores the `index` property and just uses parent and child tags to remove the child from a parent. This brings Android slightly closer to iOS: we try to use the index, but if the index is incorrect, we either (1) throw if the child isn't contained in the parent, or (2) find the correct index, and continue.

In debug, this will still crash, so we'll get more signal about why this happens.

Changelog: [Internal]

Reviewed By: shergin

Differential Revision: D24056375

fbshipit-source-id: 07507cc32ad02505d3271fc95ecb45d080109078
---
 .../fabric/mounting/MountingManager.java      | 45 +++++++++++++------
 1 file changed, 32 insertions(+), 13 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 632f57af48f..475b92bce27 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
@@ -351,7 +351,7 @@ public class MountingManager {
   }
 
   @UiThread
-  public void removeViewAt(final int tag, final int parentTag, final int index) {
+  public void removeViewAt(final int tag, final int parentTag, int index) {
     UiThreadUtil.assertOnUiThread();
     ViewState viewState = getNullableViewState(parentTag);
 
@@ -411,18 +411,30 @@ public class MountingManager {
         return;
       }
 
+      // Here we are guaranteed that the view is still in the View hierarchy, just
+      // at a different index. In debug mode we'll crash here; in production, we'll remove
+      // the child from the parent and move on.
+      // This is an issue that is safely recoverable 95% of the time. If this allows corruption
+      // of the view hierarchy and causes bugs or a crash after this point, there will be logs
+      // indicating that this happened.
+      // This is likely *only* necessary because of Fabric's LayoutAnimations implementation.
+      // If we can fix the bug there, or remove the need for LayoutAnimation index adjustment
+      // entirely, we can just throw this exception without regression user experience.
       logViewHierarchy(parentView, true);
-      throw new IllegalStateException(
-          "Tried to remove view ["
-              + tag
-              + "] of parent ["
-              + parentTag
-              + "] at index "
-              + index
-              + ", but got view tag "
-              + actualTag
-              + " - actual index of view: "
-              + tagActualIndex);
+      ReactSoftException.logSoftException(
+          TAG,
+          new IllegalStateException(
+              "Tried to remove view ["
+                  + tag
+                  + "] of parent ["
+                  + parentTag
+                  + "] at index "
+                  + index
+                  + ", but got view tag "
+                  + actualTag
+                  + " - actual index of view: "
+                  + tagActualIndex));
+      index = tagActualIndex;
     }
 
     try {
@@ -460,13 +472,20 @@ public class MountingManager {
 
     // Display children after deleting any
     if (SHOW_CHANGED_VIEW_HIERARCHIES) {
+      final int finalIndex = index;
       UiThreadUtil.runOnUiThread(
           new Runnable() {
             @Override
             public void run() {
               FLog.e(
                   TAG,
-                  "removeViewAt: [" + tag + "] -> [" + parentTag + "] idx: " + index + " AFTER");
+                  "removeViewAt: ["
+                      + tag
+                      + "] -> ["
+                      + parentTag
+                      + "] idx: "
+                      + finalIndex
+                      + " AFTER");
               logViewHierarchy(parentView, false);
             }
           });