Fix deadlock in bridge

Summary:There was a deadlock in the bridge if a native module tried to dispatch an event through EventDispatcher (that thread would hold the mTeardownLock and want the mEventStaging lock) at the same time the EventDispatcher callback was triggered and tried to dispatch a call through JS (that thread would hold the mEventStaging lock and want the mTeardownLock).

Now there are two locks (lol). In the scenario above, the native module would hold the mJSToJavaTeardownLock and want the mEventStaging lock, while the EventDispatcher callback would hold the mEventStaging lock and want the mJavaToJSTeardownLock.

Reviewed By: lexs

Differential Revision: D3011526

fb-gh-sync-id: c3ebd5c14a6370d73caebf6c99fcba18a86c6ac1
shipit-source-id: c3ebd5c14a6370d73caebf6c99fcba18a86c6ac1

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

@@ -53,9 +53,15 @@ public class CatalystInstanceImpl implements CatalystInstance {
   private final TraceListener mTraceListener;
   private final JavaScriptModuleRegistry mJSModuleRegistry;
   private final JSBundleLoader mJSBundleLoader;
-  private final Object mTeardownLock = new Object();
   private @Nullable ExecutorToken mMainExecutorToken;
 
+  // These locks prevent additional calls from going JS<->Java after the bridge has been torn down.
+  // There are separate ones for each direction because a JS to Java call can trigger a Java to JS
+  // call: this would cause a deadlock with a traditional mutex (maybe we should be using a reader-
+  // writer lock but then we'd have to worry about starving the destroy call).
+  private final Object mJSToJavaCallsTeardownLock = new Object();
+  private final Object mJavaToJSCallsTeardownLock = new Object();
+
   // Access from native modules thread
   private final NativeModuleRegistry mJavaRegistry;
   private final NativeModuleCallExceptionHandler mNativeModuleCallExceptionHandler;
@@ -172,7 +178,7 @@ public Boolean call() throws Exception {
       int methodId,
       NativeArray arguments,
       String tracingName) {
-    synchronized (mTeardownLock) {
+    synchronized (mJavaToJSCallsTeardownLock) {
       if (mDestroyed) {
         FLog.w(ReactConstants.TAG, "Calling JS function after bridge has been destroyed.");
         return;
@@ -189,7 +195,7 @@ public Boolean call() throws Exception {
   @DoNotStrip
   @Override
   public void invokeCallback(ExecutorToken executorToken, int callbackID, NativeArray arguments) {
-    synchronized (mTeardownLock) {
+    synchronized (mJavaToJSCallsTeardownLock) {
       if (mDestroyed) {
         FLog.w(ReactConstants.TAG, "Invoking JS callback after bridge has been destroyed.");
         return;
@@ -210,18 +216,22 @@ public void invokeCallback(ExecutorToken executorToken, int callbackID, NativeAr
   public void destroy() {
     UiThreadUtil.assertOnUiThread();
 
-    synchronized (mTeardownLock) {
-      if (mDestroyed) {
-        return;
-      }
+    // This ordering is important. A JS to Java call that triggers a Java to JS call will also
+    // acquire these locks in the same order
+    synchronized (mJSToJavaCallsTeardownLock) {
+      synchronized (mJavaToJSCallsTeardownLock) {
+        if (mDestroyed) {
+          return;
+        }
 
-      // TODO: tell all APIs to shut down
-      mDestroyed = true;
-      mJavaRegistry.notifyCatalystInstanceDestroy();
+        // TODO: tell all APIs to shut down
+        mDestroyed = true;
+        mJavaRegistry.notifyCatalystInstanceDestroy();
 
-      Systrace.unregisterListener(mTraceListener);
+        Systrace.unregisterListener(mTraceListener);
 
-      synchronouslyDisposeBridgeOnJSThread();
+        synchronouslyDisposeBridgeOnJSThread();
+      }
     }
 
     mReactQueueConfiguration.destroy();
@@ -401,7 +411,7 @@ private class NativeModulesReactCallback implements ReactCallback {
     public void call(ExecutorToken executorToken, int moduleId, int methodId, ReadableNativeArray parameters) {
       mReactQueueConfiguration.getNativeModulesQueueThread().assertIsOnThread();
 
-      synchronized (mTeardownLock) {
+      synchronized (mJSToJavaCallsTeardownLock) {
         // Suppress any callbacks if destroyed - will only lead to sadness.
         if (mDestroyed) {
           return;
@@ -419,7 +429,7 @@ public void onBatchComplete() {
       // native modules could be in a bad state so we don't want to call anything on them. We
       // still want to trigger the debug listener since it allows instrumentation tests to end and
       // check their assertions without waiting for a timeout.
-      synchronized (mTeardownLock) {
+      synchronized (mJSToJavaCallsTeardownLock) {
         if (!mDestroyed) {
           Systrace.beginSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE, "onBatchComplete");
           try {
@@ -440,7 +450,7 @@ public void onExecutorUnregistered(ExecutorToken executorToken) {
       // Since onCatalystInstanceDestroy happens on the UI thread, we don't want to also execute
       // this callback on the native modules thread at the same time. Longer term, onCatalystInstanceDestroy
       // should probably be executed on the native modules thread as well instead.
-      synchronized (mTeardownLock) {
+      synchronized (mJSToJavaCallsTeardownLock) {
         if (!mDestroyed) {
           Systrace.beginSection(Systrace.TRACE_TAG_REACT_JAVA_BRIDGE, "onExecutorUnregistered");
           try {