From a1ba0918ab2f03504066790922cbec841799de09 Mon Sep 17 00:00:00 2001 From: Andy Street Date: Thu, 5 May 2016 05:51:45 -0700 Subject: [PATCH] Protect against JS module call race condition while initializing react instance Summary: Fixes a race condition where JS module functions could be called in between ##initializeWithInstance(catalystInstance);## and ##CatalystInstance#runJSBundle##, before the BatchedBridge in JS was set up. We now guarantee that all JS module methods that are called after `ReactContext#hasActiveCatalystInstance()` returns true will have the batched bridge created and ready to use. Reviewed By: lexs Differential Revision: D3258651 fb-gh-sync-id: 66e66533cc86d185e7c865376d6a5cdc6520d2d4 fbshipit-source-id: 66e66533cc86d185e7c865376d6a5cdc6520d2d4 --- .../react/testing/ReactTestHelper.java | 20 +++++++-- .../react/ReactInstanceManagerImpl.java | 45 ++++++++++++++----- .../react/bridge/CatalystInstanceImpl.java | 39 +++++++--------- .../facebook/react/bridge/ReactContext.java | 8 +++- 4 files changed, 72 insertions(+), 40 deletions(-) diff --git a/ReactAndroid/src/androidTest/java/com/facebook/react/testing/ReactTestHelper.java b/ReactAndroid/src/androidTest/java/com/facebook/react/testing/ReactTestHelper.java index 20afcb234..a36743b7a 100644 --- a/ReactAndroid/src/androidTest/java/com/facebook/react/testing/ReactTestHelper.java +++ b/ReactAndroid/src/androidTest/java/com/facebook/react/testing/ReactTestHelper.java @@ -10,6 +10,8 @@ package com.facebook.react.testing; import javax.annotation.Nullable; +import java.util.concurrent.Callable; + import android.app.Instrumentation; import android.content.Context; import android.support.test.InstrumentationRegistry; @@ -124,9 +126,21 @@ public class ReactTestHelper { @Override public CatalystInstance build() { - CatalystInstance instance = builder.build(); - testCase.initializeWithInstance(instance); - instance.runJSBundle(); + final CatalystInstance instance = builder.build(); + try { + instance.getReactQueueConfiguration().getJSQueueThread().callOnQueue( + new Callable() { + @Override + public Void call() throws Exception { + testCase.initializeWithInstance(instance); + instance.runJSBundle(); + return null; + } + }).get(); + + } catch (Exception e) { + throw new RuntimeException(e); + } testCase.waitForBridgeAndUIIdle(); return instance; } diff --git a/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManagerImpl.java b/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManagerImpl.java index 47e966c20..3eb7b72c4 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManagerImpl.java +++ b/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManagerImpl.java @@ -18,6 +18,8 @@ import java.util.Collection; import java.util.Collections; import java.util.HashSet; import java.util.List; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutionException; import android.app.Activity; import android.app.Application; @@ -799,7 +801,7 @@ import static com.facebook.react.bridge.ReactMarkerConstants.RUN_JS_BUNDLE_START NativeModuleRegistry.Builder nativeRegistryBuilder = new NativeModuleRegistry.Builder(); JavaScriptModulesConfig.Builder jsModulesBuilder = new JavaScriptModulesConfig.Builder(); - ReactApplicationContext reactContext = new ReactApplicationContext(mApplicationContext); + final ReactApplicationContext reactContext = new ReactApplicationContext(mApplicationContext); if (mUseDeveloperSupport) { reactContext.setNativeModuleCallExceptionHandler(mDevSupportManager); } @@ -862,7 +864,7 @@ import static com.facebook.react.bridge.ReactMarkerConstants.RUN_JS_BUNDLE_START ReactMarker.logMarker(CREATE_CATALYST_INSTANCE_START); Systrace.beginSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE, "createCatalystInstance"); - CatalystInstance catalystInstance; + final CatalystInstance catalystInstance; try { catalystInstance = catalystInstanceBuilder.build(); } finally { @@ -874,16 +876,37 @@ import static com.facebook.react.bridge.ReactMarkerConstants.RUN_JS_BUNDLE_START catalystInstance.addBridgeIdleDebugListener(mBridgeIdleDebugListener); } - reactContext.initializeWithInstance(catalystInstance); - - ReactMarker.logMarker(RUN_JS_BUNDLE_START); - // RUN_JS_BUNDLE_END is in JSCExecutor.cpp - Systrace.beginSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE, "runJSBundle"); try { - catalystInstance.runJSBundle(); - } finally { - // This will actually finish when `JSCExecutor#loadApplicationScript()` finishes - Systrace.endSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE); + catalystInstance.getReactQueueConfiguration().getJSQueueThread().callOnQueue( + new Callable() { + @Override + public Void call() { + // We want to ensure that any code that checks ReactContext#hasActiveCatalystInstance + // can be sure that it's safe to call a JS module function. As JS module function calls + // execute on the JS thread, and this Runnable runs on the JS thread, at this point we + // know that no JS module function calls will be executed until after this Runnable completes. + // + // This means it is now safe to say the instance is initialized. + // + // The reason we call this here instead of after this Runnable completes is so that we can + // reduce the amount of time until the React instance is able to start accepting JS calls, + // and so that any native module calls that result from runJSBundle can access JS modules. + reactContext.initializeWithInstance(catalystInstance); + + ReactMarker.logMarker(RUN_JS_BUNDLE_START); + // RUN_JS_BUNDLE_END is in JSCExecutor.cpp + Systrace.beginSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE, "runJSBundle"); + try { + catalystInstance.runJSBundle(); + } finally { + // This will actually finish when `JSCExecutor#loadApplicationScript()` finishes + Systrace.endSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE); + } + return null; + } + }).get(); + } catch (InterruptedException | ExecutionException e) { + throw new RuntimeException(e); } return reactContext; diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java index 784f8a6cd..d858c743d 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java @@ -140,33 +140,24 @@ public class CatalystInstanceImpl implements CatalystInstance { @Override public void runJSBundle() { + mReactQueueConfiguration.getJSQueueThread().assertIsOnThread(); + Assertions.assertCondition(!mJSBundleHasLoaded, "JS bundle was already loaded!"); + + incrementPendingJSCalls(); + + Systrace.beginSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE, "loadJSScript"); try { - mJSBundleHasLoaded = mReactQueueConfiguration.getJSQueueThread().callOnQueue( - new Callable() { - @Override - public Boolean call() throws Exception { - Assertions.assertCondition(!mJSBundleHasLoaded, "JS bundle was already loaded!"); + mJSBundleLoader.loadScript(mBridge); - incrementPendingJSCalls(); - - Systrace.beginSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE, "loadJSScript"); - try { - mJSBundleLoader.loadScript(mBridge); - - // This is registered after JS starts since it makes a JS call - Systrace.registerListener(mTraceListener); - } catch (JSExecutionException e) { - mNativeModuleCallExceptionHandler.handleException(e); - } finally { - Systrace.endSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE); - } - - return true; - } - }).get(); - } catch (Exception t) { - throw new RuntimeException(t); + // This is registered after JS starts since it makes a JS call + Systrace.registerListener(mTraceListener); + } catch (JSExecutionException e) { + mNativeModuleCallExceptionHandler.handleException(e); + } finally { + Systrace.endSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE); } + + mJSBundleHasLoaded = true; } @Override diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactContext.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactContext.java index 60f59f52d..f8c0f6322 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactContext.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/ReactContext.java @@ -31,6 +31,10 @@ import com.facebook.react.bridge.queue.ReactQueueConfiguration; */ public class ReactContext extends ContextWrapper { + private static final String EARLY_JS_ACCESS_EXCEPTION_MESSAGE = + "Tried to access a JS module before the React instance was fully set up. Calls to " + + "ReactContext#getJSModule should be protected by ReactContext#hasActiveCatalystInstance()."; + private final CopyOnWriteArraySet mLifecycleEventListeners = new CopyOnWriteArraySet<>(); private final CopyOnWriteArraySet mActivityEventListeners = @@ -92,14 +96,14 @@ public class ReactContext extends ContextWrapper { */ public T getJSModule(Class jsInterface) { if (mCatalystInstance == null) { - throw new RuntimeException("Trying to invoke JS before CatalystInstance has been set!"); + throw new RuntimeException(EARLY_JS_ACCESS_EXCEPTION_MESSAGE); } return mCatalystInstance.getJSModule(jsInterface); } public T getJSModule(ExecutorToken executorToken, Class jsInterface) { if (mCatalystInstance == null) { - throw new RuntimeException("Trying to invoke JS before CatalystInstance has been set!"); + throw new RuntimeException(EARLY_JS_ACCESS_EXCEPTION_MESSAGE); } return mCatalystInstance.getJSModule(executorToken, jsInterface); }