Skip to content

Commit

Permalink
Fix ReactRootView attachRootView race condition
Browse files Browse the repository at this point in the history
Summary:
Yet another issue with mAttachedRootViews. Every time attachRootView is called, the root view's id is reset to NO_ID. However, there can be a case where the react context has not initialized yet, so we delay attaching the root view to the instance manager until setupReactContext. If attachRootView was called a second time before the react context has finished initializing, we end up in a situation where we try to attach the same root view twice

I'm planning to remove mAttachedRootViews all together in a future diff, but for now let's avoid these crashes.

Reviewed By: mmmulani

Differential Revision: D12835167

fbshipit-source-id: ebef3fadc5f9f467eebb3b5644c685acc5ea10bf
  • Loading branch information
ayc1 authored and facebook-github-bot committed Nov 10, 2018
1 parent d4aef08 commit be282b5
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package com.facebook.react.tests.core;

import android.app.Activity;
import android.support.test.InstrumentationRegistry;
import android.support.test.annotation.UiThreadTest;
import android.support.test.rule.ActivityTestRule;
import android.support.test.runner.AndroidJUnit4;
Expand All @@ -14,6 +15,7 @@
import com.facebook.react.common.LifecycleState;
import com.facebook.react.shell.MainReactPackage;
import com.facebook.react.testing.ReactTestHelper;
import org.junit.After;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
Expand Down Expand Up @@ -43,6 +45,19 @@ public void setup() {
.build();
}

@After
public void tearDown() {
final ReactRootView reactRootView = mReactRootView;
final ReactInstanceManager reactInstanceManager = mReactInstanceManager;
InstrumentationRegistry.getInstrumentation().runOnMainSync(new Runnable() {
@Override
public void run() {
reactRootView.unmountReactApplication();
reactInstanceManager.destroy();
}
});
}

@Test
@UiThreadTest
public void testMountUnmount() {
Expand All @@ -56,7 +71,6 @@ public void testMountUnmount() {
public void testResume() throws InterruptedException {
mReactInstanceManager.onHostResume(mActivityRule.getActivity());
mReactRootView.startReactApplication(mReactInstanceManager, TEST_MODULE);
Thread.sleep(1000);
mReactInstanceManager.onHostResume(mActivityRule.getActivity());
}

Expand All @@ -65,7 +79,14 @@ public void testResume() throws InterruptedException {
public void testRecreateContext() throws InterruptedException {
mReactInstanceManager.onHostResume(mActivityRule.getActivity());
mReactInstanceManager.createReactContextInBackground();
Thread.sleep(1000);
mReactRootView.startReactApplication(mReactInstanceManager, TEST_MODULE);
}

@Test
@UiThreadTest
public void testMountTwice() {
mReactInstanceManager.onHostResume(mActivityRule.getActivity());
mReactRootView.startReactApplication(mReactInstanceManager, TEST_MODULE);
mReactInstanceManager.attachRootView(mReactRootView);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import javax.annotation.Nullable;

/**
Expand Down Expand Up @@ -134,8 +135,8 @@ public interface ReactInstanceEventListener {
void onReactContextInitialized(ReactContext context);
}

private final List<ReactRootView> mAttachedRootViews = Collections.synchronizedList(
new ArrayList<ReactRootView>());
private final Set<ReactRootView> mAttachedRootViews = Collections.synchronizedSet(
new HashSet<ReactRootView>());

private volatile LifecycleState mLifecycleState;

Expand Down Expand Up @@ -1037,8 +1038,7 @@ public void run() {
});
}

private void attachRootViewToInstance(
final ReactRootView rootView) {
private void attachRootViewToInstance(final ReactRootView rootView) {
Log.d(ReactConstants.TAG, "ReactInstanceManager.attachRootViewToInstance()");
Systrace.beginSection(TRACE_TAG_REACT_JAVA_BRIDGE, "attachRootViewToInstance");
UIManager uiManagerModule = UIManagerHelper.getUIManager(mCurrentReactContext, rootView.getUIManagerType());
Expand Down

0 comments on commit be282b5

Please sign in to comment.