From f8bf65b8fdd6bc66b45caf15f34a50dc20fb8a8c Mon Sep 17 00:00:00 2001
From: Joshua Gross <joshuagross@fb.com>
Date: Sat, 19 Oct 2019 02:27:35 -0700
Subject: [PATCH] Make sure FabricUIManager doesn't double-free Binding, or
 double-destroy itself

Summary:
While unlikely, in exceptional cases it's possible that `onCatalystInstanceDestroy` is called multiple times, especially since there's no restrictions as to which thread this is called on (currently it's only called on the JS thread from CatalystInstanceImpl) and no synchronization. Make sure it tears down cleanly if called multiple times, and report soft exceptions if that's detected.

Changelog: [Internal]

Reviewed By: mdvacca

Differential Revision: D18001678

fbshipit-source-id: 4a7c37b5b3c2207ba1dd9c7d85690391f799518d
---
 .../react/fabric/FabricUIManager.java         | 27 ++++++++++++++-----
 1 file changed, 21 insertions(+), 6 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 75a93dd727d..39d9c75fa0e 100644
--- a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java
+++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java
@@ -34,6 +34,7 @@ import com.facebook.react.bridge.ReactApplicationContext;
 import com.facebook.react.bridge.ReactContext;
 import com.facebook.react.bridge.ReactMarker;
 import com.facebook.react.bridge.ReactMarkerConstants;
+import com.facebook.react.bridge.ReactSoftException;
 import com.facebook.react.bridge.ReadableArray;
 import com.facebook.react.bridge.ReadableMap;
 import com.facebook.react.bridge.UIManager;
@@ -123,6 +124,12 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
   @ThreadConfined(UI)
   private volatile boolean mIsMountingEnabled = true;
 
+  /**
+   * This is used to keep track of whether or not the FabricUIManager has been destroyed. Once the
+   * Catalyst instance is being destroyed, we should cease all operation here.
+   */
+  private volatile boolean mDestroyed = false;
+
   private long mRunStartTime = 0l;
   private long mBatchedExecutionTime = 0l;
   private long mDispatchViewUpdatesTime = 0l;
@@ -220,6 +227,19 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
   public void onCatalystInstanceDestroy() {
     FLog.i(TAG, "FabricUIManager.onCatalystInstanceDestroy");
 
+    if (mDestroyed) {
+      ReactSoftException.logSoftException(
+          FabricUIManager.TAG, new IllegalStateException("Cannot double-destroy FabricUIManager"));
+      return;
+    }
+
+    mDestroyed = true;
+
+    // This is not technically thread-safe, since it's read on the UI thread and written
+    // here on the JS thread. We've marked it as volatile so that this writes to UI-thread
+    // memory immediately.
+    mIsMountingEnabled = false;
+
     mEventDispatcher.removeBatchEventDispatchedListener(mEventBeatManager);
     mEventDispatcher.unregisterEventEmitter(FABRIC);
 
@@ -229,11 +249,6 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
     mReactApplicationContext.removeLifecycleEventListener(this);
     onHostPause();
 
-    // This is not technically thread-safe, since it's read on the UI thread and written
-    // here on the JS thread. We've marked it as volatile so that this writes to UI-thread
-    // memory immediately.
-    mIsMountingEnabled = false;
-
     mBinding.unregister();
     mBinding = null;
 
@@ -700,7 +715,7 @@ public class FabricUIManager implements UIManager, LifecycleEventListener {
 
     @Override
     public void doFrameGuarded(long frameTimeNanos) {
-      if (!mIsMountingEnabled) {
+      if (!mIsMountingEnabled || mDestroyed) {
         FLog.w(
             ReactConstants.TAG,
             "Not flushing pending UI operations because of previously thrown Exception");