Added locking around RN bridge cxx module registry to avoid crash

Summary:
D12904277 was my sad, optimistic attempt to fix this crash.

As @[100006577537606:Slobodan] mentioned on T35879909, the real issue is that moduleRegistry is being created (using _moduleDataByID dict) concurrently while we try to append to this dict.

I added locks around these usage of _moduleDataByID.

Reviewed By: fkgozali

Differential Revision: D12911108

fbshipit-source-id: 2435b7a477c27585898f351c4a0d4c1bd4056756

Author: Peter Argany

React/CxxBridge/RCTCxxBridge.mm

@@ -158,6 +158,7 @@ @implementation RCTCxxBridge
 {
   BOOL _wasBatchActive;
   BOOL _didInvalidate;
+  BOOL _moduleRegistryCreated;
 
   NSMutableArray<RCTPendingCall> *_pendingCalls;
   std::atomic<NSInteger> _pendingCount;
@@ -172,6 +173,7 @@ @implementation RCTCxxBridge
   // JS thread management
   NSThread *_jsThread;
   std::shared_ptr<RCTMessageThread> _jsMessageThread;
+  std::mutex _moduleRegistryLock;
 
   // This is uniquely owned, but weak_ptr is used.
   std::shared_ptr<Instance> _reactInstance;
@@ -219,9 +221,9 @@ - (instancetype)initWithParentBridge:(RCTBridge *)bridge
      */
     _valid = YES;
     _loading = YES;
+    _moduleRegistryCreated = NO;
     _pendingCalls = [NSMutableArray new];
     _displayLink = [RCTDisplayLink new];
-
     _moduleDataByName = [NSMutableDictionary new];
     _moduleClassesByID = [NSMutableArray new];
     _moduleDataByID = [NSMutableArray new];
@@ -456,7 +458,7 @@ - (BOOL)moduleIsInitialized:(Class)moduleClass
   return _moduleDataByName[RCTBridgeModuleNameForClass(moduleClass)].hasInstance;
 }
 
-- (std::shared_ptr<ModuleRegistry>)_buildModuleRegistry
+- (std::shared_ptr<ModuleRegistry>)_buildModuleRegistryUnlocked
 {
   if (!self.valid) {
     return {};
@@ -503,12 +505,7 @@ - (void)_initializeBridge:(std::shared_ptr<JSExecutorFactory>)executorFactory
     executorFactory = std::make_shared<GetDescAdapter>(self, executorFactory);
 #endif
 
-    // This is async, but any calls into JS are blocked by the m_syncReady CV in Instance
-    _reactInstance->initializeBridge(
-      std::make_unique<RCTInstanceCallback>(self),
-      executorFactory,
-      _jsMessageThread,
-      [self _buildModuleRegistry]);
+    [self _initializeBridgeLocked:executorFactory];
 
 #if RCT_PROFILE
     if (RCTProfileIsProfiling()) {
@@ -522,6 +519,19 @@ - (void)_initializeBridge:(std::shared_ptr<JSExecutorFactory>)executorFactory
   RCT_PROFILE_END_EVENT(RCTProfileTagAlways, @"");
 }
 
+- (void)_initializeBridgeLocked:(std::shared_ptr<JSExecutorFactory>)executorFactory
+{
+  std::lock_guard<std::mutex> guard(_moduleRegistryLock);
+
+  // This is async, but any calls into JS are blocked by the m_syncReady CV in Instance
+  _reactInstance->initializeBridge(
+                                   std::make_unique<RCTInstanceCallback>(self),
+                                   executorFactory,
+                                   _jsMessageThread,
+                                   [self _buildModuleRegistryUnlocked]);
+  _moduleRegistryCreated = YES;
+}
+
 - (NSArray *)configForModuleName:(NSString *)moduleName
 {
   return _moduleDataByName[moduleName].config;
@@ -682,11 +692,13 @@ - (void)registerExtraModules
 
 - (void)registerAdditionalModuleClasses:(NSArray<Class> *)modules
 {
-  @synchronized (self) {
+  std::lock_guard<std::mutex> guard(_moduleRegistryLock);
+  if (_moduleRegistryCreated) {
     NSArray<RCTModuleData *> *newModules = [self _initializeModules:modules withDispatchGroup:NULL lazilyDiscovered:YES];
-    if (_reactInstance) {
-      _reactInstance->getModuleRegistry().registerModules(createNativeModules(newModules, self, _reactInstance));
-    }
+    assert(_reactInstance); // at this point you must have reactInstance as you already called reactInstance->initialzeBridge
+    _reactInstance->getModuleRegistry().registerModules(createNativeModules(newModules, self, _reactInstance));
+  } else {
+    [self registerModulesForClasses:modules];
   }
 }