From 286b38eb53cc7f72dece01ac5c5cb499b6722ff7 Mon Sep 17 00:00:00 2001
From: Joshua Gross <joshuagross@fb.com>
Date: Thu, 30 Jun 2022 12:19:32 -0700
Subject: [PATCH] Add debugging code to recover from, and debug errors where
 Views in the hierarchy are re-added to hierarchy

Summary:
There is a very small volume of production errors caused by a View that is already in the hierarchy being added to the hierarchy again; this results in a crash on the Android platform layer.

We detect and attempt to silently recover from this case, while logging and collecting more diagnostics. This will still crash in debug mode.

It is unclear what layer this error is coming from: it could be an issue with the C++ differ (ordering, or something more tricky); that is unlikely but there are few other hypotheses at the moment.

Changelog: [Internal]

Reviewed By: javache

Differential Revision: D37557663

fbshipit-source-id: ffe320ff646e314fa921a2c2cf8058254713505c
---
 .../mounting/SurfaceMountingManager.java      | 55 ++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java
index 5955fb64676..15088ad95ec 100644
--- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java
+++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.java
@@ -84,6 +84,9 @@ public class SurfaceMountingManager {
   @ThreadConfined(UI)
   private final Stack<Integer> mReactTagsToRemove = new Stack<>();
 
+  @ThreadConfined(UI)
+  private final Set<Integer> mErroneouslyReaddedReactTags = new HashSet<>();
+
   @ThreadConfined(UI)
   private RemoveDeleteTreeUIFrameCallback mRemoveDeleteTreeUIFrameCallback;
 
@@ -378,7 +381,29 @@ public class SurfaceMountingManager {
                   + "]: View already has a parent: ["
                   + actualParentId
                   + "] "
-                  + viewParent.getClass().getSimpleName()));
+                  + " Parent: "
+                  + viewParent.getClass().getSimpleName()
+                  + " View: "
+                  + view.getClass().getSimpleName()));
+
+      // We've hit an error case, and `addView` will crash below
+      // if we don't take evasive action (it is an error to add a View
+      // to the hierarchy if it already has a parent).
+      // We don't know /why/ this happens yet, but it does happen
+      // very infrequently in production.
+      // Thus, we do three things here:
+      // (1) We logged a SoftException above, so if there's a crash later
+      // on, we might have some hints about what caused it.
+      // (2) We remove the View from its parent.
+      // (3) In case the View was removed from the hierarchy with the
+      // RemoveDeleteTree instruction, and is now being readded - which
+      // should be impossible - we mark this as a "readded" View and
+      // thus prevent the RemoveDeleteTree worker from deleting this
+      // View in the future.
+      if (viewParent instanceof ViewGroup) {
+        ((ViewGroup) viewParent).removeView(view);
+      }
+      mErroneouslyReaddedReactTags.add(tag);
     }
 
     try {
@@ -421,6 +446,17 @@ public class SurfaceMountingManager {
       return;
     }
 
+    // This is "impossible". See comments above.
+    if (mErroneouslyReaddedReactTags.contains(tag)) {
+      ReactSoftExceptionLogger.logSoftException(
+          TAG,
+          new IllegalViewOperationException(
+              "removeViewAt tried to remove a React View that was actually reused. This indicates a bug in the Differ (specifically instruction ordering). ["
+                  + tag
+                  + "]"));
+      return;
+    }
+
     UiThreadUtil.assertOnUiThread();
     ViewState parentViewState = getNullableViewState(parentTag);
 
@@ -1426,6 +1462,17 @@ public class SurfaceMountingManager {
           int reactTag = mReactTagsToRemove.pop();
           deletedViews++;
 
+          // This is "impossible". See comments above.
+          if (mErroneouslyReaddedReactTags.contains(reactTag)) {
+            ReactSoftExceptionLogger.logSoftException(
+                TAG,
+                new IllegalViewOperationException(
+                    "RemoveDeleteTree recursively tried to remove a React View that was actually reused. This indicates a bug in the Differ. ["
+                        + reactTag
+                        + "]"));
+            continue;
+          }
+
           ViewState thisViewState = getNullableViewState(reactTag);
           if (thisViewState != null) {
             View thisView = thisViewState.mView;
@@ -1489,6 +1536,12 @@ public class SurfaceMountingManager {
         if (!mReactTagsToRemove.empty()) {
           ReactChoreographer.getInstance()
               .postFrameCallback(ReactChoreographer.CallbackType.IDLE_EVENT, this);
+        } else {
+          // If there are no more tags to process, then clear the "reused"
+          // tag set. Since the RemoveDeleteTree runner executes /after/ all
+          // other mounting instructions have been executed, all in-band Remove
+          // instructions have already had a chance to execute here.
+          mErroneouslyReaddedReactTags.clear();
         }
       }
     }