From 3503d722a18dc6c0781232e164c82b8097786082 Mon Sep 17 00:00:00 2001
From: Joshua Gross <joshuagross@fb.com>
Date: Thu, 9 Jul 2020 20:48:25 -0700
Subject: [PATCH] Fix race between teardown and PreAllocateView/CreateView
 MountItem creation

Summary:
Commits can happen during navigation/teardown which creates mount items. If we throw an exception during
teardown because we expect the Context to still be around, we crash too often. Instead, I will rely on
logic in FabricUIManager to ignore queued MountItems if we try to execute them after the surface has been torn down;
and we move the IllegalStateException to actual execution of the mount item in case there's an edge-case we're missing.

Changelog: [Internal]

Reviewed By: mdvacca

Differential Revision: D22470102

fbshipit-source-id: ad98c03994969a3c3f300d6551e90b6516ed2d8b
---
 .../facebook/react/fabric/FabricUIManager.java   | 16 +++++++++++-----
 .../mounting/mountitems/CreateMountItem.java     |  9 ++++++++-
 .../mountitems/PreAllocateViewMountItem.java     |  9 ++++++++-
 3 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java
index 3f3eb338d13..57409a31601 100644
--- a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java
+++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java
@@ -314,7 +314,11 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
       @Nullable ReadableMap props,
       @Nullable Object stateWrapper,
       boolean isLayoutable) {
-    ThemedReactContext context = mReactContextForRootTag.get(rootTag);
+
+    // This could be null if teardown/navigation away from a surface on the main thread happens
+    // while a commit is being processed in a different thread. By contract we expect this to be
+    // possible at teardown, but this race should *never* happen at startup.
+    @Nullable ThemedReactContext context = mReactContextForRootTag.get(rootTag);
 
     String component = getFabricComponentName(componentName);
     synchronized (mPreMountItemsLock) {
@@ -342,10 +346,12 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
       int reactTag,
       boolean isLayoutable) {
     String component = getFabricComponentName(componentName);
-    ThemedReactContext reactContext = mReactContextForRootTag.get(reactRootTag);
-    if (reactContext == null) {
-      throw new IllegalArgumentException("Unable to find ReactContext for root: " + reactRootTag);
-    }
+
+    // This could be null if teardown/navigation away from a surface on the main thread happens
+    // while a commit is being processed in a different thread. By contract we expect this to be
+    // possible at teardown, but this race should *never* happen at startup.
+    @Nullable ThemedReactContext reactContext = mReactContextForRootTag.get(reactRootTag);
+
     return new CreateMountItem(
         reactContext,
         reactRootTag,
diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/CreateMountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/CreateMountItem.java
index 6320eb1d04d..d96bab3326e 100644
--- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/CreateMountItem.java
+++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/CreateMountItem.java
@@ -25,7 +25,7 @@ public class CreateMountItem implements MountItem {
   private final boolean mIsLayoutable;
 
   public CreateMountItem(
-      @NonNull ThemedReactContext context,
+      @Nullable ThemedReactContext context,
       int rootTag,
       int reactTag,
       @NonNull String component,
@@ -43,6 +43,13 @@ public class CreateMountItem implements MountItem {
 
   @Override
   public void execute(@NonNull MountingManager mountingManager) {
+    if (mContext == null) {
+      throw new IllegalStateException(
+          "Cannot execute PreAllocateViewMountItem without Context for ReactTag: "
+              + mReactTag
+              + " and rootTag: "
+              + mRootTag);
+    }
     mountingManager.createView(
         mContext, mComponent, mReactTag, mProps, mStateWrapper, mIsLayoutable);
   }
diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java
index 7f519e9bc6f..0e3c7140eaf 100644
--- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java
+++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java
@@ -31,7 +31,7 @@ public class PreAllocateViewMountItem implements MountItem {
   private final boolean mIsLayoutable;
 
   public PreAllocateViewMountItem(
-      @NonNull ThemedReactContext context,
+      @Nullable ThemedReactContext context,
       int rootTag,
       int reactTag,
       @NonNull String component,
@@ -56,6 +56,13 @@ public class PreAllocateViewMountItem implements MountItem {
     if (ENABLE_FABRIC_LOGS) {
       FLog.d(TAG, "Executing pre-allocation of: " + toString());
     }
+    if (mContext == null) {
+      throw new IllegalStateException(
+          "Cannot execute PreAllocateViewMountItem without Context for ReactTag: "
+              + mReactTag
+              + " and rootTag: "
+              + mRootTag);
+    }
     mountingManager.preallocateView(
         mContext, mComponent, mReactTag, mProps, mStateWrapper, mIsLayoutable);
   }