Permalink
Browse files

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
  • Loading branch information...
ayc1 authored and facebook-github-bot committed Oct 31, 2018
1 parent 798517a commit 309f85ace280184b5458cc701ccb81716b43b789
@@ -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"),
]) + ([
@@ -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();
}
}
@@ -738,8 +738,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());
}
@@ -977,22 +978,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()];

0 comments on commit 309f85a

Please sign in to comment.