Fix ReactRootView mount/unmount race condition

facebook · Nov 9, 2018 · 6524591 · 6524591
1 parent 1131463
commit 6524591
Show file tree

Hide file tree

Showing 3 changed files with 91 additions and 15 deletions.
diff --git a/ReactAndroid/src/androidTest/java/com/facebook/react/tests/core/BUCK b/ReactAndroid/src/androidTest/java/com/facebook/react/tests/core/BUCK
@@ -0,0 +1,29 @@
+# BUILD FILE SYNTAX: SKYLARK
+load(
+    "//tools/build_defs/oss:rn_defs.bzl",
+    "IS_OSS_BUILD",
+    "react_native_dep",
+    "react_native_integration_tests_target",
+    "react_native_target",
+    "rn_android_library",
+)
+
+rn_android_library(
+    name = "core",
+    srcs = glob(["*.java"]),
+    deps = ([
+        react_native_dep("third-party/java/espresso:espresso"),
+        react_native_dep("third-party/java/fest:fest"),
+        react_native_dep("third-party/java/junit:junit"),
+        react_native_dep("third-party/java/testing-support-lib:testing-support-lib"),
+        react_native_integration_tests_target("java/com/facebook/react/testing:testing"),
+        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"),
+    ]) + ([
+        react_native_dep("java/com/facebook/fbreact/testing:testing"),
+    ]) if not IS_OSS_BUILD else [],
+)
diff --git a/...tAndroid/src/androidTest/java/com/facebook/react/tests/core/ReactInstanceManagerTest.java b/...tAndroid/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();
+  }
+}
diff --git a/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java b/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java
@@ -732,8 +732,9 @@ public void attachRootView(ReactRootView rootView) {
   @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());
       }
@@ -959,22 +960,22 @@ private void setupReactContext(final ReactApplicationContext reactContext) {
     Systrace.beginSection(TRACE_TAG_REACT_JAVA_BRIDGE, "setupReactContext");
     synchronized (mReactContextLock) {
       mCurrentReactContext = Assertions.assertNotNull(reactContext);
-    }
-    CatalystInstance catalystInstance =
-      Assertions.assertNotNull(reactContext.getCatalystInstance());
-
-    catalystInstance.initialize();
-    mDevSupportManager.onNewReactContextCreated(reactContext);
-    mMemoryPressureRouter.addMemoryPressureListener(catalystInstance);
-    moveReactContextToCurrentLifecycleState();
-
-    ReactMarker.logMarker(ATTACH_MEASURED_ROOT_VIEWS_START);
-    synchronized (mAttachedRootViews) {
-      for (ReactRootView rootView : mAttachedRootViews) {
-        attachRootViewToInstance(rootView);
+      CatalystInstance catalystInstance =
+          Assertions.assertNotNull(reactContext.getCatalystInstance());
+
+      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_END);
     }
-    ReactMarker.logMarker(ATTACH_MEASURED_ROOT_VIEWS_END);
 
     ReactInstanceEventListener[] listeners =
       new ReactInstanceEventListener[mReactInstanceEventListeners.size()];