From df7e8c64ff8f5ff739fba2ba5ed6b0610567235e Mon Sep 17 00:00:00 2001
From: "Andrew Chen (Eng)" <ayc@fb.com>
Date: Fri, 9 Nov 2018 13:41:17 -0800
Subject: [PATCH] Fix ReactInstanceManager deadlock

Summary:
D12829677 introduced a deadlock where onHostResume holds the lock on `moveToResumedLifecycleState()` then waits for the `mReactContext` lock, but at the same time setupReactContext holds the `mReactContext` lock and waits for `moveToResumedLifecycleState()` https://our.intern.facebook.com/intern/everpaste/?handle=GAgXFgLQH1ZlQikBAMQzo2LZ6h9TbiAxAAAz. The purpose of the previous diff was to make sure that detachRootoView and
setupReactContext did not interfere with each other, and implemented that by blocking on mReactContext. Since this overloads the usage of mReactContext, let's use a different lock `mAttachedRootViews` to implement this behavior instead

Reviewed By: mdvacca

Differential Revision: D12989555

fbshipit-source-id: c12e5fd9c5fa4c2037167e9400dc0c1578e38959
---
 .../tests/core/ReactInstanceManagerTest.java  | 22 +++++++++++-
 .../facebook/react/ReactInstanceManager.java  | 35 +++++++++++--------
 2 files changed, 41 insertions(+), 16 deletions(-)

diff --git a/ReactAndroid/src/androidTest/java/com/facebook/react/tests/core/ReactInstanceManagerTest.java b/ReactAndroid/src/androidTest/java/com/facebook/react/tests/core/ReactInstanceManagerTest.java
index 919c9933d..e7cdff245 100644
--- a/ReactAndroid/src/androidTest/java/com/facebook/react/tests/core/ReactInstanceManagerTest.java
+++ b/ReactAndroid/src/androidTest/java/com/facebook/react/tests/core/ReactInstanceManagerTest.java
@@ -22,6 +22,8 @@ import org.junit.runner.RunWith;
 @RunWith(AndroidJUnit4.class)
 public class ReactInstanceManagerTest {
 
+  private static final String TEST_MODULE = "ViewLayoutTestApp";
+
   private ReactInstanceManager mReactInstanceManager;
   private ReactRootView mReactRootView;
 
@@ -45,7 +47,25 @@ public class ReactInstanceManagerTest {
   @UiThreadTest
   public void testMountUnmount() {
     mReactInstanceManager.onHostResume(mActivityRule.getActivity());
-    mReactRootView.startReactApplication(mReactInstanceManager, "ViewLayoutTestApp");
+    mReactRootView.startReactApplication(mReactInstanceManager, TEST_MODULE);
     mReactRootView.unmountReactApplication();
   }
+
+  @Test
+  @UiThreadTest
+  public void testResume() throws InterruptedException {
+    mReactInstanceManager.onHostResume(mActivityRule.getActivity());
+    mReactRootView.startReactApplication(mReactInstanceManager, TEST_MODULE);
+    Thread.sleep(1000);
+    mReactInstanceManager.onHostResume(mActivityRule.getActivity());
+  }
+
+  @Test
+  @UiThreadTest
+  public void testRecreateContext() throws InterruptedException {
+    mReactInstanceManager.onHostResume(mActivityRule.getActivity());
+    mReactInstanceManager.createReactContextInBackground();
+    Thread.sleep(1000);
+    mReactRootView.startReactApplication(mReactInstanceManager, TEST_MODULE);
+  }
 }
diff --git a/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java b/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java
index 0f3edbf54..c5fe183df 100644
--- a/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java
+++ b/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java
@@ -741,11 +741,13 @@ public class ReactInstanceManager {
   @ThreadConfined(UI)
   public void detachRootView(ReactRootView rootView) {
     UiThreadUtil.assertOnUiThread();
-    if (mAttachedRootViews.contains(rootView)) {
-      ReactContext currentContext = getCurrentReactContext();
-      mAttachedRootViews.remove(rootView);
-      if (currentContext != null && currentContext.hasActiveCatalystInstance()) {
-        detachViewFromInstance(rootView, currentContext.getCatalystInstance());
+    synchronized (mAttachedRootViews) {
+      if (mAttachedRootViews.contains(rootView)) {
+        ReactContext currentContext = getCurrentReactContext();
+        mAttachedRootViews.remove(rootView);
+        if (currentContext != null && currentContext.hasActiveCatalystInstance()) {
+          detachViewFromInstance(rootView, currentContext.getCatalystInstance());
+        }
       }
     }
   }
@@ -904,10 +906,12 @@ public class ReactInstanceManager {
   private void runCreateReactContextOnNewThread(final ReactContextInitParams initParams) {
     Log.d(ReactConstants.TAG, "ReactInstanceManager.runCreateReactContextOnNewThread()");
     UiThreadUtil.assertOnUiThread();
-    synchronized (mReactContextLock) {
-      if (mCurrentReactContext != null) {
-        tearDownReactContext(mCurrentReactContext);
-        mCurrentReactContext = null;
+    synchronized (mAttachedRootViews) {
+      synchronized (mReactContextLock) {
+        if (mCurrentReactContext != null) {
+          tearDownReactContext(mCurrentReactContext);
+          mCurrentReactContext = null;
+        }
       }
     }
 
@@ -979,8 +983,11 @@ public class ReactInstanceManager {
     ReactMarker.logMarker(PRE_SETUP_REACT_CONTEXT_END);
     ReactMarker.logMarker(SETUP_REACT_CONTEXT_START);
     Systrace.beginSection(TRACE_TAG_REACT_JAVA_BRIDGE, "setupReactContext");
-    synchronized (mReactContextLock) {
-      mCurrentReactContext = Assertions.assertNotNull(reactContext);
+    synchronized (mAttachedRootViews) {
+      synchronized (mReactContextLock) {
+        mCurrentReactContext = Assertions.assertNotNull(reactContext);
+      }
+
       CatalystInstance catalystInstance =
           Assertions.assertNotNull(reactContext.getCatalystInstance());
 
@@ -990,10 +997,8 @@ public class ReactInstanceManager {
       moveReactContextToCurrentLifecycleState();
 
       ReactMarker.logMarker(ATTACH_MEASURED_ROOT_VIEWS_START);
-      synchronized (mAttachedRootViews) {
-        for (ReactRootView rootView : mAttachedRootViews) {
-          attachRootViewToInstance(rootView);
-        }
+      for (ReactRootView rootView : mAttachedRootViews) {
+        attachRootViewToInstance(rootView);
       }
       ReactMarker.logMarker(ATTACH_MEASURED_ROOT_VIEWS_END);
     }