Skip to content

Commit df7e8c6

Browse files
ayc1facebook-github-bot
authored andcommittedNov 9, 2018
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
1 parent 80f92ad commit df7e8c6

File tree

2 files changed

+41
-16
lines changed

2 files changed

+41
-16
lines changed
 

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

+21-1
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@
2222
@RunWith(AndroidJUnit4.class)
2323
public class ReactInstanceManagerTest {
2424

25+
private static final String TEST_MODULE = "ViewLayoutTestApp";
26+
2527
private ReactInstanceManager mReactInstanceManager;
2628
private ReactRootView mReactRootView;
2729

@@ -45,7 +47,25 @@ public void setup() {
4547
@UiThreadTest
4648
public void testMountUnmount() {
4749
mReactInstanceManager.onHostResume(mActivityRule.getActivity());
48-
mReactRootView.startReactApplication(mReactInstanceManager, "ViewLayoutTestApp");
50+
mReactRootView.startReactApplication(mReactInstanceManager, TEST_MODULE);
4951
mReactRootView.unmountReactApplication();
5052
}
53+
54+
@Test
55+
@UiThreadTest
56+
public void testResume() throws InterruptedException {
57+
mReactInstanceManager.onHostResume(mActivityRule.getActivity());
58+
mReactRootView.startReactApplication(mReactInstanceManager, TEST_MODULE);
59+
Thread.sleep(1000);
60+
mReactInstanceManager.onHostResume(mActivityRule.getActivity());
61+
}
62+
63+
@Test
64+
@UiThreadTest
65+
public void testRecreateContext() throws InterruptedException {
66+
mReactInstanceManager.onHostResume(mActivityRule.getActivity());
67+
mReactInstanceManager.createReactContextInBackground();
68+
Thread.sleep(1000);
69+
mReactRootView.startReactApplication(mReactInstanceManager, TEST_MODULE);
70+
}
5171
}

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

+20-15
Original file line numberDiff line numberDiff line change
@@ -741,11 +741,13 @@ public void attachRootView(ReactRootView rootView) {
741741
@ThreadConfined(UI)
742742
public void detachRootView(ReactRootView rootView) {
743743
UiThreadUtil.assertOnUiThread();
744-
if (mAttachedRootViews.contains(rootView)) {
745-
ReactContext currentContext = getCurrentReactContext();
746-
mAttachedRootViews.remove(rootView);
747-
if (currentContext != null && currentContext.hasActiveCatalystInstance()) {
748-
detachViewFromInstance(rootView, currentContext.getCatalystInstance());
744+
synchronized (mAttachedRootViews) {
745+
if (mAttachedRootViews.contains(rootView)) {
746+
ReactContext currentContext = getCurrentReactContext();
747+
mAttachedRootViews.remove(rootView);
748+
if (currentContext != null && currentContext.hasActiveCatalystInstance()) {
749+
detachViewFromInstance(rootView, currentContext.getCatalystInstance());
750+
}
749751
}
750752
}
751753
}
@@ -904,10 +906,12 @@ private void recreateReactContextInBackground(
904906
private void runCreateReactContextOnNewThread(final ReactContextInitParams initParams) {
905907
Log.d(ReactConstants.TAG, "ReactInstanceManager.runCreateReactContextOnNewThread()");
906908
UiThreadUtil.assertOnUiThread();
907-
synchronized (mReactContextLock) {
908-
if (mCurrentReactContext != null) {
909-
tearDownReactContext(mCurrentReactContext);
910-
mCurrentReactContext = null;
909+
synchronized (mAttachedRootViews) {
910+
synchronized (mReactContextLock) {
911+
if (mCurrentReactContext != null) {
912+
tearDownReactContext(mCurrentReactContext);
913+
mCurrentReactContext = null;
914+
}
911915
}
912916
}
913917

@@ -979,8 +983,11 @@ private void setupReactContext(final ReactApplicationContext reactContext) {
979983
ReactMarker.logMarker(PRE_SETUP_REACT_CONTEXT_END);
980984
ReactMarker.logMarker(SETUP_REACT_CONTEXT_START);
981985
Systrace.beginSection(TRACE_TAG_REACT_JAVA_BRIDGE, "setupReactContext");
982-
synchronized (mReactContextLock) {
983-
mCurrentReactContext = Assertions.assertNotNull(reactContext);
986+
synchronized (mAttachedRootViews) {
987+
synchronized (mReactContextLock) {
988+
mCurrentReactContext = Assertions.assertNotNull(reactContext);
989+
}
990+
984991
CatalystInstance catalystInstance =
985992
Assertions.assertNotNull(reactContext.getCatalystInstance());
986993

@@ -990,10 +997,8 @@ private void setupReactContext(final ReactApplicationContext reactContext) {
990997
moveReactContextToCurrentLifecycleState();
991998

992999
ReactMarker.logMarker(ATTACH_MEASURED_ROOT_VIEWS_START);
993-
synchronized (mAttachedRootViews) {
994-
for (ReactRootView rootView : mAttachedRootViews) {
995-
attachRootViewToInstance(rootView);
996-
}
1000+
for (ReactRootView rootView : mAttachedRootViews) {
1001+
attachRootViewToInstance(rootView);
9971002
}
9981003
ReactMarker.logMarker(ATTACH_MEASURED_ROOT_VIEWS_END);
9991004
}

0 commit comments

Comments
 (0)
Please sign in to comment.