Fix ReactRootView mount/unmount race condition

Summary:
There are multiple reports of the NativeViewHierarchyManager trying to remove a root view with id -1. This can occur because of a race condition caused by calls to startReactApplication + unmountReactApplication.

We unfortunately overload overload the id field of the ReactRootView to store its react tag. This id is typically set when the view is attached to the react instance, and reset to NO_ID right before an attach occurs or when the react context is tearing down. If the react context has already been initialized, attaching the root view is synchronous and the id is set immediately. If the context has not been initialized, we save the root view in a mAttachedRootViews list and wait until setupReactContext (where the context is created) to finish attaching the root views.

There were two issues:
1) In setupReactContext, synchronizing on mReactContextLock is not enough to ensure that the root views in mAttachedRootViews have been initialized already
2) In detachRootView, removing the root view from mAttachedRootViews immediately will cause mAttachedRootViews to be out of sync when it is accessed by the time it is accessed in setupReactContext

To address these, the mReactContextLock will synchronize both the creation of the react context AND the initialization of the root views and detachRootView will synchronize on the mReactContextLock before mutating the mAttachedRootViews list.

Reviewed By: mmmulani

Differential Revision: D12829677

fbshipit-source-id: 3f3b0669e5be2b570c9d534503d04e5d0816196b

ReactAndroid/src/androidTest/java/com/facebook/react/tests/core/BUCK

@@ -20,6 +20,7 @@ rn_android_library(
         react_native_integration_tests_target("java/com/facebook/react/testing/rule:rule"),
         react_native_target("java/com/facebook/react:react"),
         react_native_target("java/com/facebook/react/bridge:bridge"),
+        react_native_target("java/com/facebook/react/common:common"),
         react_native_target("java/com/facebook/react/shell:shell"),
         react_native_target("java/com/facebook/react/uimanager:uimanager"),
     ]) + ([

ReactAndroid/src/androidTest/java/com/facebook/react/tests/core/ReactInstanceManagerTest.java

@@ -0,0 +1,46 @@
+package com.facebook.react.tests.core;
+
+import android.app.Activity;
+import android.support.test.annotation.UiThreadTest;
+import android.support.test.rule.ActivityTestRule;
+import android.support.test.runner.AndroidJUnit4;
+import com.facebook.react.ReactInstanceManager;
+import com.facebook.react.ReactRootView;
+import com.facebook.react.common.LifecycleState;
+import com.facebook.react.shell.MainReactPackage;
+import com.facebook.react.testing.ReactTestHelper;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+@RunWith(AndroidJUnit4.class)
+public class ReactInstanceManagerTest {
+
+  private ReactInstanceManager mReactInstanceManager;
+  private ReactRootView mReactRootView;
+
+  @Rule public ActivityTestRule<Activity> mActivityRule = new ActivityTestRule<>(Activity.class);
+
+  @Before
+  public void setup() {
+    Activity activity = mActivityRule.getActivity();
+    mReactRootView = new ReactRootView(activity);
+    mReactInstanceManager =
+      ReactTestHelper.getReactTestFactory()
+        .getReactInstanceManagerBuilder()
+        .setApplication(activity.getApplication())
+        .setBundleAssetName("AndroidTestBundle.js")
+        .setInitialLifecycleState(LifecycleState.BEFORE_CREATE)
+        .addPackage(new MainReactPackage())
+        .build();
+  }
+
+  @Test
+  @UiThreadTest
+  public void testMountUnmount() {
+    mReactInstanceManager.onHostResume(mActivityRule.getActivity());
+    mReactRootView.startReactApplication(mReactInstanceManager, "ViewLayoutTestApp");
+    mReactRootView.unmountReactApplication();
+  }
+}

ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java

@@ -738,8 +738,9 @@ public class ReactInstanceManager {
   @ThreadConfined(UI)
   public void detachRootView(ReactRootView rootView) {
     UiThreadUtil.assertOnUiThread();
-    if (mAttachedRootViews.remove(rootView)) {
+    if (mAttachedRootViews.contains(rootView)) {
       ReactContext currentContext = getCurrentReactContext();
+      mAttachedRootViews.remove(rootView);
       if (currentContext != null && currentContext.hasActiveCatalystInstance()) {
         detachViewFromInstance(rootView, currentContext.getCatalystInstance());
       }
@@ -977,22 +978,22 @@ public class ReactInstanceManager {
     Systrace.beginSection(TRACE_TAG_REACT_JAVA_BRIDGE, "setupReactContext");
     synchronized (mReactContextLock) {
       mCurrentReactContext = Assertions.assertNotNull(reactContext);
-    }
-    CatalystInstance catalystInstance =
-      Assertions.assertNotNull(reactContext.getCatalystInstance());
+      CatalystInstance catalystInstance =
+          Assertions.assertNotNull(reactContext.getCatalystInstance());
 
-    catalystInstance.initialize();
-    mDevSupportManager.onNewReactContextCreated(reactContext);
-    mMemoryPressureRouter.addMemoryPressureListener(catalystInstance);
-    moveReactContextToCurrentLifecycleState();
+      catalystInstance.initialize();
+      mDevSupportManager.onNewReactContextCreated(reactContext);
+      mMemoryPressureRouter.addMemoryPressureListener(catalystInstance);
+      moveReactContextToCurrentLifecycleState();
 
-    ReactMarker.logMarker(ATTACH_MEASURED_ROOT_VIEWS_START);
-    synchronized (mAttachedRootViews) {
-      for (ReactRootView rootView : mAttachedRootViews) {
-        attachRootViewToInstance(rootView);
+      ReactMarker.logMarker(ATTACH_MEASURED_ROOT_VIEWS_START);
+      synchronized (mAttachedRootViews) {
+        for (ReactRootView rootView : mAttachedRootViews) {
+          attachRootViewToInstance(rootView);
+        }
       }
+      ReactMarker.logMarker(ATTACH_MEASURED_ROOT_VIEWS_END);
     }
-    ReactMarker.logMarker(ATTACH_MEASURED_ROOT_VIEWS_END);
 
     ReactInstanceEventListener[] listeners =
       new ReactInstanceEventListener[mReactInstanceEventListeners.size()];