Dispatch JS calls to JS thread from c++ bridge

Summary: Instead of dispatching calls to the JS thread in Java, do it in the C++ bridge. This moves us closer to the cxx bridge and will allow us to dispatch to the correct web worker in C++ instead of in Java

Reviewed By: mhorowitz

Differential Revision: D2954115

fb-gh-sync-id: 7e7d4eff2c72601b8b4416f1ccd8d2985aebd755
shipit-source-id: 7e7d4eff2c72601b8b4416f1ccd8d2985aebd755

ReactAndroid/src/androidTest/java/com/facebook/react/testing/ReactIntegrationTestCase.java

@@ -70,14 +70,17 @@ public abstract class ReactIntegrationTestCase extends AndroidTestCase {
       mReactContext = null;
       mInstance = null;
 
+      final SimpleSettableFuture<Void> semaphore = new SimpleSettableFuture<>();
       UiThreadUtil.runOnUiThread(new Runnable() {
         @Override
         public void run() {
           if (contextToDestroy != null) {
             contextToDestroy.destroy();
           }
+          semaphore.set(null);
         }
       });
+      semaphore.getOrThrow();
     }
   }
 

ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java

@@ -53,7 +53,7 @@ public class CatalystInstanceImpl implements CatalystInstance {
   private final TraceListener mTraceListener;
   private final JavaScriptModuleRegistry mJSModuleRegistry;
   private final JSBundleLoader mJSBundleLoader;
-  private volatile int mTraceID = 0;
+  private final Object mTeardownLock = new Object();
 
   // Access from native modules thread
   private final NativeModuleRegistry mJavaRegistry;
@@ -168,42 +168,16 @@ public class CatalystInstanceImpl implements CatalystInstance {
       final int methodId,
       final NativeArray arguments,
       final String tracingName) {
-    if (mDestroyed) {
+    synchronized (mTeardownLock) {
-      FLog.w(ReactConstants.TAG, "Calling JS function after bridge has been destroyed.");
+      if (mDestroyed) {
-      return;
+        FLog.w(ReactConstants.TAG, "Calling JS function after bridge has been destroyed.");
+        return;
+      }
+
+      incrementPendingJSCalls();
+
+      Assertions.assertNotNull(mBridge).callFunction(moduleId, methodId, arguments, tracingName);
     }
-
-    incrementPendingJSCalls();
-
-    final int traceID = mTraceID++;
-    Systrace.startAsyncFlow(
-        Systrace.TRACE_TAG_REACT_JAVA_BRIDGE,
-        tracingName,
-        traceID);
-
-    mReactQueueConfiguration.getJSQueueThread().runOnQueue(
-        new Runnable() {
-          @Override
-          public void run() {
-            mReactQueueConfiguration.getJSQueueThread().assertIsOnThread();
-
-            Systrace.endAsyncFlow(
-                Systrace.TRACE_TAG_REACT_JAVA_BRIDGE,
-                tracingName,
-                traceID);
-
-            if (mDestroyed) {
-              return;
-            }
-
-            Systrace.beginSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE, tracingName);
-            try {
-              Assertions.assertNotNull(mBridge).callFunction(moduleId, methodId, arguments);
-            } finally {
-              Systrace.endSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE);
-            }
-          }
-        });
   }
 
   // This is called from java code, so it won't be stripped anyway, but proguard will rename it,
@@ -211,42 +185,16 @@ public class CatalystInstanceImpl implements CatalystInstance {
   @DoNotStrip
   @Override
   public void invokeCallback(final int callbackID, final NativeArray arguments) {
-    if (mDestroyed) {
+    synchronized (mTeardownLock) {
-      FLog.w(ReactConstants.TAG, "Invoking JS callback after bridge has been destroyed.");
+      if (mDestroyed) {
-      return;
+        FLog.w(ReactConstants.TAG, "Invoking JS callback after bridge has been destroyed.");
+        return;
+      }
+
+      incrementPendingJSCalls();
+
+      Assertions.assertNotNull(mBridge).invokeCallback(callbackID, arguments);
     }
-
-    incrementPendingJSCalls();
-
-    final int traceID = mTraceID++;
-    Systrace.startAsyncFlow(
-        Systrace.TRACE_TAG_REACT_JAVA_BRIDGE,
-        "<callback>",
-        traceID);
-
-    mReactQueueConfiguration.getJSQueueThread().runOnQueue(
-        new Runnable() {
-          @Override
-          public void run() {
-            mReactQueueConfiguration.getJSQueueThread().assertIsOnThread();
-
-            Systrace.endAsyncFlow(
-                Systrace.TRACE_TAG_REACT_JAVA_BRIDGE,
-                "<callback>",
-                traceID);
-
-            if (mDestroyed) {
-              return;
-            }
-
-            Systrace.beginSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE, "<callback>");
-            try {
-              Assertions.assertNotNull(mBridge).invokeCallback(callbackID, arguments);
-            } finally {
-              Systrace.endSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE);
-            }
-          }
-        });
   }
 
   /**
@@ -258,18 +206,20 @@ public class CatalystInstanceImpl implements CatalystInstance {
   public void destroy() {
     UiThreadUtil.assertOnUiThread();
 
-    if (mDestroyed) {
+    synchronized (mTeardownLock) {
-      return;
+      if (mDestroyed) {
+        return;
+      }
+
+      // TODO: tell all APIs to shut down
+      mDestroyed = true;
+      mJavaRegistry.notifyCatalystInstanceDestroy();
+
+      Systrace.unregisterListener(mTraceListener);
+
+      synchronouslyDisposeBridgeOnJSThread();
+      mReactQueueConfiguration.destroy();
     }
-
-    // TODO: tell all APIs to shut down
-    mDestroyed = true;
-    mJavaRegistry.notifyCatalystInstanceDestroy();
-
-    Systrace.unregisterListener(mTraceListener);
-
-    synchronouslyDisposeBridgeOnJSThread();
-    mReactQueueConfiguration.destroy();
     boolean wasIdle = (mPendingJSCalls.getAndSet(0) == 0);
     if (!wasIdle && !mBridgeIdleListeners.isEmpty()) {
       for (NotThreadSafeBridgeIdleDebugListener listener : mBridgeIdleListeners) {

ReactAndroid/src/main/java/com/facebook/react/bridge/ReactBridge.java

@@ -79,7 +79,7 @@ public class ReactBridge extends Countable {
    */
   public native void loadScriptFromAssets(AssetManager assetManager, String assetName);
   public native void loadScriptFromFile(@Nullable String fileName, @Nullable String sourceURL);
-  public native void callFunction(int moduleId, int methodId, NativeArray arguments);
+  public native void callFunction(int moduleId, int methodId, NativeArray arguments, String tracingName);
   public native void invokeCallback(int callbackID, NativeArray arguments);
   public native void setGlobalVariable(String propertyName, String jsonEncodedArgument);
   public native boolean supportsProfiling();

ReactAndroid/src/main/jni/react/Bridge.cpp

@@ -5,14 +5,18 @@
 #ifdef WITH_FBSYSTRACE
 #include <fbsystrace.h>
 using fbsystrace::FbSystraceSection;
+using fbsystrace::FbSystraceAsyncFlow;
 #endif
 
+#include "Platform.h"
+
 namespace facebook {
 namespace react {
 
 Bridge::Bridge(JSExecutorFactory* jsExecutorFactory, Callback callback) :
     m_callback(std::move(callback)),
-    m_destroyed(std::shared_ptr<bool>(new bool(false))) {
+    m_destroyed(std::make_shared<bool>(false)),
+    m_mainJSMessageQueueThread(MessageQueues::getCurrentMessageQueueThread()) {
     m_mainExecutor = jsExecutorFactory->createJSExecutor(this);
 }
 
@@ -33,24 +37,64 @@ void Bridge::loadApplicationUnbundle(
   m_mainExecutor->loadApplicationUnbundle(std::move(unbundle), startupCode, sourceURL);
 }
 
-void Bridge::callFunction(const double moduleId, const double methodId, const folly::dynamic& arguments) {
+void Bridge::callFunction(
+    const double moduleId,
+    const double methodId,
+    const folly::dynamic& arguments,
+    const std::string& tracingName) {
   if (*m_destroyed) {
     return;
   }
+
   #ifdef WITH_FBSYSTRACE
-  FbSystraceSection s(TRACE_TAG_REACT_CXX_BRIDGE, "Bridge.callFunction");
+  int systraceCookie = m_systraceCookie++;
+  FbSystraceAsyncFlow::begin(
+      TRACE_TAG_REACT_CXX_BRIDGE,
+      tracingName.c_str(),
+      systraceCookie);
   #endif
-  m_mainExecutor->callFunction(moduleId, methodId, arguments);
+  std::shared_ptr<bool> isDestroyed = m_destroyed;
+  m_mainJSMessageQueueThread->runOnQueue([=] () {
+    if (*isDestroyed) {
+      return;
+    }
+    #ifdef WITH_FBSYSTRACE
+    FbSystraceAsyncFlow::end(
+        TRACE_TAG_REACT_CXX_BRIDGE,
+        tracingName.c_str(),
+        systraceCookie);
+    FbSystraceSection s(TRACE_TAG_REACT_CXX_BRIDGE, tracingName.c_str());
+    #endif
+    m_mainExecutor->callFunction(moduleId, methodId, arguments);
+  });
 }
 
 void Bridge::invokeCallback(const double callbackId, const folly::dynamic& arguments) {
   if (*m_destroyed) {
     return;
   }
+
   #ifdef WITH_FBSYSTRACE
-  FbSystraceSection s(TRACE_TAG_REACT_CXX_BRIDGE, "Bridge.invokeCallback");
+  int systraceCookie = m_systraceCookie++;
+  FbSystraceAsyncFlow::begin(
+      TRACE_TAG_REACT_CXX_BRIDGE,
+      "<callback>",
+      systraceCookie);
   #endif
-  m_mainExecutor->invokeCallback(callbackId, arguments);
+  std::shared_ptr<bool> isDestroyed = m_destroyed;
+  m_mainJSMessageQueueThread->runOnQueue([=] () {
+    if (*isDestroyed) {
+      return;
+    }
+    #ifdef WITH_FBSYSTRACE
+    FbSystraceAsyncFlow::end(
+        TRACE_TAG_REACT_CXX_BRIDGE,
+        "<callback>",
+        systraceCookie);
+    FbSystraceSection s(TRACE_TAG_REACT_CXX_BRIDGE, "Bridge.invokeCallback");
+    #endif
+    m_mainExecutor->invokeCallback(callbackId, arguments);
+  });
 }
 
 void Bridge::setGlobalVariable(const std::string& propName, const std::string& jsonValue) {

ReactAndroid/src/main/jni/react/Bridge.h

@@ -2,11 +2,13 @@
 
 #pragma once
 
+#include <atomic>
 #include <functional>
 #include <map>
 #include <vector>
 
 #include "Executor.h"
+#include "MessageQueueThread.h"
 #include "MethodCall.h"
 #include "JSModulesUnbundle.h"
 #include "Value.h"
@@ -24,6 +26,9 @@ class Bridge {
 public:
   typedef std::function<void(std::vector<MethodCall>, bool isEndOfBatch)> Callback;
 
+  /**
+   * This must be called on the main JS thread.
+   */
   Bridge(JSExecutorFactory* jsExecutorFactory, Callback callback);
   virtual ~Bridge();
 
@@ -31,7 +36,11 @@ public:
    * Executes a function with the module ID and method ID and any additional
    * arguments in JS.
    */
-  void callFunction(const double moduleId, const double methodId, const folly::dynamic& args);
+  void callFunction(
+    const double moduleId,
+    const double methodId,
+    const folly::dynamic& args,
+    const std::string& tracingName);
 
   /**
    * Invokes a callback with the cbID, and optional additional arguments in JS.
@@ -72,6 +81,10 @@ private:
   // will have been destroyed within ~Bridge(), thus causing a SIGSEGV.
   std::shared_ptr<bool> m_destroyed;
   std::unique_ptr<JSExecutor> m_mainExecutor;
+  std::shared_ptr<MessageQueueThread> m_mainJSMessageQueueThread;
+  #ifdef WITH_FBSYSTRACE
+  std::atomic_uint_least32_t m_systraceCookie = ATOMIC_VAR_INIT();
+  #endif
 };
 
 } }

ReactAndroid/src/main/jni/react/jni/OnLoad.cpp

@@ -727,14 +727,15 @@ static void loadScriptFromFile(JNIEnv* env, jobject obj, jstring fileName, jstri
 }
 
 static void callFunction(JNIEnv* env, jobject obj, jint moduleId, jint methodId,
-                         NativeArray::jhybridobject args) {
+                         NativeArray::jhybridobject args, jstring tracingName) {
   auto bridge = extractRefPtr<CountableBridge>(env, obj);
   auto arguments = cthis(wrap_alias(args));
   try {
     bridge->callFunction(
       (double) moduleId,
       (double) methodId,
-      std::move(arguments->array)
+      std::move(arguments->array),
+      fromJString(env, tracingName)
     );
   } catch (...) {
     translatePendingCppExceptionToJavaException();