From 9547a98a685cb9121c31cf19cd17f4ed0feeac2e Mon Sep 17 00:00:00 2001
From: Nick Lockwood <nicklockwood@fb.com>
Date: Wed, 27 Apr 2016 09:08:51 -0700
Subject: [PATCH] Fixed deadlock during app startup

Summary:
Now that we support initializing the bridge off the main thread, some of the assumptions in the bridge setup process are no longer safe.

In particular we were assuming that the JS executor and injected modules could always be synchronously initialized within bridge init, but that is only safe if those modules don't need to be set up on the main thread.

The setup for those modules was sync-dispatching to the main thread if bridge init happened on a background thread, and this lead to a deadlock under certain circumstances.

Reviewed By: javache

Differential Revision: D3224162

fb-gh-sync-id: 7319b70f541a46ef932cfe4f776e7e192f3ce1e8
fbshipit-source-id: 7319b70f541a46ef932cfe4f776e7e192f3ce1e8
---
 React/Base/RCTBatchedBridge.m | 56 ++++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 24 deletions(-)

diff --git a/React/Base/RCTBatchedBridge.m b/React/Base/RCTBatchedBridge.m
index 335944aae9f61a..1c399ceb1300a7 100644
--- a/React/Base/RCTBatchedBridge.m
+++ b/React/Base/RCTBatchedBridge.m
@@ -299,6 +299,27 @@ - (void)initModulesWithDispatchGroup:(dispatch_group_t)dispatchGroup
     moduleDataByName[moduleName] = moduleData;
     [moduleClassesByID addObject:moduleClass];
     [moduleDataByID addObject:moduleData];
+
+    // Set executor instance
+    if (moduleClass == self.executorClass) {
+      _javaScriptExecutor = (id<RCTJavaScriptExecutor>)module;
+    }
+  }
+
+  // The executor is a bridge module, but we want it to be instantiated before
+  // any other module has access to the bridge, in case they need the JS thread.
+  // TODO: once we have more fine-grained control of init (D3175632) we can
+  // probably just replace this with [self moduleForClass:self.executorClass]
+  if (!_javaScriptExecutor) {
+    id<RCTJavaScriptExecutor> executorModule = [self.executorClass new];
+    RCTModuleData *moduleData = [[RCTModuleData alloc] initWithModuleInstance:executorModule
+                                                                       bridge:self];
+    moduleDataByName[moduleData.name] = moduleData;
+    [moduleClassesByID addObject:self.executorClass];
+    [moduleDataByID addObject:moduleData];
+
+    // NOTE: _javaScriptExecutor is a weak reference
+    _javaScriptExecutor = executorModule;
   }
 
   // Set up moduleData for automatically-exported modules
@@ -335,17 +356,16 @@ - (void)initModulesWithDispatchGroup:(dispatch_group_t)dispatchGroup
   _moduleDataByName = [moduleDataByName copy];
   _moduleClassesByID = [moduleClassesByID copy];
 
-  // The executor is a bridge module, wait for it to be created and set it up
-  // before any other module has access to the bridge.
-  _javaScriptExecutor = [self moduleForClass:self.executorClass];
-
-  // Dispatch module init onto main thead for those modules that require it
+  // Synchronously set up the pre-initialized modules
   for (RCTModuleData *moduleData in _moduleDataByID) {
-    if (moduleData.hasInstance) {
-      // Modules that were pre-initialized need to be set up before bridge init
-      // has finished, otherwise the caller may try to access the module
-      // directly rather than via `[bridge moduleForClass:]`, which won't
-      // trigger the lazy initialization process.
+    if (moduleData.hasInstance &&
+        (!moduleData.requiresMainThreadSetup || [NSThread isMainThread])) {
+      // Modules that were pre-initialized should ideally be set up before
+      // bridge init has finished, otherwise the caller may try to access the
+      // module directly rather than via `[bridge moduleForClass:]`, which won't
+      // trigger the lazy initialization process. If the module cannot safely be
+      // set up on the current thread, it will instead be async dispatched
+      // to the main thread to be set up in the loop below.
       (void)[moduleData instance];
     }
   }
@@ -359,11 +379,11 @@ - (void)initModulesWithDispatchGroup:(dispatch_group_t)dispatchGroup
   NSUInteger modulesOnMainThreadCount = 0;
   for (RCTModuleData *moduleData in _moduleDataByID) {
     __weak RCTBatchedBridge *weakSelf = self;
-    if (moduleData.requiresMainThreadSetup) {
+    if (moduleData.requiresMainThreadSetup || moduleData.hasConstantsToExport) {
       // Modules that need to be set up on the main thread cannot be initialized
       // lazily when required without doing a dispatch_sync to the main thread,
       // which can result in deadlock. To avoid this, we initialize all of these
-      // modules on the main thread in parallel with loading the JS code, so that
+      // modules on the main thread in parallel with loading the JS code, so
       // they will already be available before they are ever required.
       dispatch_group_async(dispatchGroup, dispatch_get_main_queue(), ^{
         if (weakSelf.valid) {
@@ -374,18 +394,6 @@ - (void)initModulesWithDispatchGroup:(dispatch_group_t)dispatchGroup
         }
       });
       modulesOnMainThreadCount++;
-    } else if (moduleData.hasConstantsToExport) {
-      // Constants must be exported on the main thread, but module setup can
-      // be done on any queue
-      (void)[moduleData instance];
-      dispatch_group_async(dispatchGroup, dispatch_get_main_queue(), ^{
-        if (weakSelf.valid) {
-          RCTPerformanceLoggerAppendStart(RCTPLNativeModuleMainThread);
-          [moduleData gatherConstants];
-          RCTPerformanceLoggerAppendEnd(RCTPLNativeModuleMainThread);
-        }
-      });
-      modulesOnMainThreadCount++;
     }
   }