Fix race conditions in RCTModuleData

Summary:
public
The logic inside RCTBatchedBridge contained some race conditions that would occasionally cause an error if modules were loaded in the wrong order. This improves that logic and makes it safer by adding a lock to prevent concurrency.

Reviewed By: jspahrsummers

Differential Revision: D2802930

fb-gh-sync-id: d1ad25fa578649363dcaac029cb24dc3a453ae67

React/Base/RCTBatchedBridge.m

@@ -295,7 +295,8 @@ RCT_EXTERN NSArray<Class> *RCTGetModuleClasses(void);
     RCTModuleData *moduleData;
     if (module) {
       if (module != (id)kCFNull) {
-        moduleData = [[RCTModuleData alloc] initWithModuleInstance:module];
+        moduleData = [[RCTModuleData alloc] initWithModuleInstance:module
+                                                            bridge:self];
       }
     } else {
        moduleData = [[RCTModuleData alloc] initWithModuleClass:moduleClass
@@ -320,7 +321,7 @@ RCT_EXTERN NSArray<Class> *RCTGetModuleClasses(void);
 
   for (RCTModuleData *moduleData in _moduleDataByID) {
     if (moduleData.hasInstance) {
-      [moduleData setBridgeForInstance:self];
+      [moduleData setBridgeForInstance];
     }
   }
 
@@ -330,6 +331,13 @@ RCT_EXTERN NSArray<Class> *RCTGetModuleClasses(void);
     }
   }
 
+  for (RCTModuleData *moduleData in _moduleDataByID) {
+    if (moduleData.hasInstance) {
+      [self registerModuleForFrameUpdates:moduleData.instance
+                           withModuleData:moduleData];
+    }
+  }
+
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wdeprecated-declarations"
 
@@ -347,18 +355,22 @@ RCT_EXTERN NSArray<Class> *RCTGetModuleClasses(void);
   [_javaScriptExecutor setUp];
 }
 
-- (void)registerModuleForFrameUpdates:(RCTModuleData *)moduleData
+- (void)registerModuleForFrameUpdates:(id<RCTBridgeModule>)module
+                       withModuleData:(RCTModuleData *)moduleData
 {
-  if ([moduleData.moduleClass conformsToProtocol:@protocol(RCTFrameUpdateObserver)]) {
+  if (![_frameUpdateObservers containsObject:moduleData]) {
-    [_frameUpdateObservers addObject:moduleData];
+    if ([moduleData.moduleClass conformsToProtocol:@protocol(RCTFrameUpdateObserver)]) {
-    id<RCTFrameUpdateObserver> observer = (id<RCTFrameUpdateObserver>)moduleData.instance;
+      [_frameUpdateObservers addObject:moduleData];
-    __weak typeof(self) weakSelf = self;
+      // Don't access the module instance via moduleData, as this will cause deadlock
-    __weak typeof(_javaScriptExecutor) weakJavaScriptExecutor = _javaScriptExecutor;
+      id<RCTFrameUpdateObserver> observer = (id<RCTFrameUpdateObserver>)module;
-    observer.pauseCallback = ^{
+      __weak typeof(self) weakSelf = self;
-      [weakJavaScriptExecutor executeBlockOnJavaScriptQueue:^{
+      __weak typeof(_javaScriptExecutor) weakJavaScriptExecutor = _javaScriptExecutor;
-        [weakSelf updateJSDisplayLinkState];
+      observer.pauseCallback = ^{
-      }];
+        [weakJavaScriptExecutor executeBlockOnJavaScriptQueue:^{
-    };
+          [weakSelf updateJSDisplayLinkState];
+        }];
+      };
+    }
   }
 }
 

React/Base/RCTBridge+Private.h

@@ -50,7 +50,12 @@
 
 @interface RCTBridge (RCTBatchedBridge)
 
-- (void)registerModuleForFrameUpdates:(RCTModuleData *)moduleData;
+/**
+ * Used by RCTModuleData to register the module for frame updates after it is
+ * lazily initialized.
+ */
+- (void)registerModuleForFrameUpdates:(id<RCTBridgeModule>)module
+                       withModuleData:(RCTModuleData *)moduleData;
 
 /**
  * Dispatch work to a module's queue - this is also suports the fake RCTJSThread

React/Base/RCTModuleData.h

@@ -20,14 +20,15 @@
 - (instancetype)initWithModuleClass:(Class)moduleClass
                              bridge:(RCTBridge *)bridge NS_DESIGNATED_INITIALIZER;
 
-- (instancetype)initWithModuleInstance:(id<RCTBridgeModule>)instance NS_DESIGNATED_INITIALIZER;
+- (instancetype)initWithModuleInstance:(id<RCTBridgeModule>)instance
+                                bridge:(RCTBridge *)bridge;
 
 /**
  * Sets the bridge for the module instance. This is only needed when using the
  * `initWithModuleID:instance:` constructor. Otherwise, the bridge will be set
  * automatically when the module is first accessed.
  */
-- (void)setBridgeForInstance:(RCTBridge *)bridge;
+- (void)setBridgeForInstance;
 
 @property (nonatomic, strong, readonly) Class moduleClass;
 @property (nonatomic, copy, readonly) NSString *name;

React/Base/RCTModuleData.m

@@ -19,6 +19,8 @@
 {
   NSString *_queueName;
   __weak RCTBridge *_bridge;
+  NSLock *_instanceLock;
+  BOOL _setupComplete;
 }
 
 @synthesize methods = _methods;
@@ -31,81 +33,80 @@
   if ((self = [super init])) {
     _moduleClass = moduleClass;
     _bridge = bridge;
+
+    _implementsBatchDidComplete = [_moduleClass instancesRespondToSelector:@selector(batchDidComplete)];
+    _implementsPartialBatchDidFlush = [_moduleClass instancesRespondToSelector:@selector(partialBatchDidFlush)];
+
+    _instanceLock = [NSLock new];
   }
   return self;
 }
 
 - (instancetype)initWithModuleInstance:(id<RCTBridgeModule>)instance
+                                bridge:(RCTBridge *)bridge
 {
-  if ((self = [super init])) {
+  if ((self = [self initWithModuleClass:[instance class] bridge:bridge])) {
     _instance = instance;
-    _moduleClass = [instance class];
-
-    [self cacheImplementedSelectors];
   }
   return self;
 }
 
 RCT_NOT_IMPLEMENTED(- (instancetype)init);
 
-- (void)cacheImplementedSelectors
+#pragma mark - private setup methods
+
+- (void)setBridgeForInstance
 {
-  _implementsBatchDidComplete = [_instance respondsToSelector:@selector(batchDidComplete)];
+  RCTAssert(_instance, @"setBridgeForInstance called before %@ initialized", self.name);
-  _implementsPartialBatchDidFlush = [_instance respondsToSelector:@selector(partialBatchDidFlush)];
+  if ([_instance respondsToSelector:@selector(bridge)] && !_instance.bridge) {
+    @try {
+      [(id)_instance setValue:_bridge forKey:@"bridge"];
+    }
+    @catch (NSException *exception) {
+      RCTLogError(@"%@ has no setter or ivar for its bridge, which is not "
+                  "permitted. You must either @synthesize the bridge property, "
+                  "or provide your own setter method.", self.name);
+    }
+  }
 }
 
 - (void)setUpMethodQueue
 {
-  RCTAssert(!_methodQueue, @"Method queue is already set up");
-  RCTAssert(_instance, @"Instance is not yet set up");
-
-  BOOL implementsMethodQueue = [_instance respondsToSelector:@selector(methodQueue)];
-  if (implementsMethodQueue) {
-    _methodQueue = _instance.methodQueue;
-  }
   if (!_methodQueue) {
-
+    RCTAssert(_instance, @"setUpMethodQueue called before %@ initialized", self.name);
-    // Create new queue (store queueName, as it isn't retained by dispatch_queue)
+    BOOL implementsMethodQueue = [_instance respondsToSelector:@selector(methodQueue)];
-    _queueName = [NSString stringWithFormat:@"com.facebook.React.%@Queue", self.name];
-    _methodQueue = dispatch_queue_create(_queueName.UTF8String, DISPATCH_QUEUE_SERIAL);
-
-    // assign it to the module
     if (implementsMethodQueue) {
-      @try {
+      _methodQueue = _instance.methodQueue;
-        [(id)_instance setValue:_methodQueue forKey:@"methodQueue"];
+    }
-      }
+    if (!_methodQueue) {
-      @catch (NSException *exception) {
+
-        RCTLogError(@"%@ is returning nil for it's methodQueue, which is not "
+      // Create new queue (store queueName, as it isn't retained by dispatch_queue)
-                    "permitted. You must either return a pre-initialized "
+      _queueName = [NSString stringWithFormat:@"com.facebook.React.%@Queue", self.name];
-                    "queue, or @synthesize the methodQueue to let the bridge "
+      _methodQueue = dispatch_queue_create(_queueName.UTF8String, DISPATCH_QUEUE_SERIAL);
-                    "create a queue for you.", self.name);
+
+      // assign it to the module
+      if (implementsMethodQueue) {
+        @try {
+          [(id)_instance setValue:_methodQueue forKey:@"methodQueue"];
+        }
+        @catch (NSException *exception) {
+          RCTLogError(@"%@ is returning nil for it's methodQueue, which is not "
+                      "permitted. You must either return a pre-initialized "
+                      "queue, or @synthesize the methodQueue to let the bridge "
+                      "create a queue for you.", self.name);
+        }
       }
     }
+
+    // Needs to be sent after bridge has been set for all module instances.
+    // Makes sense to put it here, since the same rules apply for methodQueue.
+    [[NSNotificationCenter defaultCenter]
+     postNotificationName:RCTDidInitializeModuleNotification
+     object:_bridge userInfo:@{@"module": _instance}];
   }
-
-  // Needs to be sent after bridge has been set for all module instances.
-  // Makes sense to put it here, since the same rules apply for methodQueue.
-  [[NSNotificationCenter defaultCenter]
-   postNotificationName:RCTDidInitializeModuleNotification
-   object:_bridge userInfo:@{@"module": _instance}];
 }
 
-- (void)setUpInstance
+#pragma mark - public getters
-{
-  RCTAssert(!_instance, @"Instance is already set up");
-
-  _instance = [_moduleClass new];
-
-  // Bridge must be set before methodQueue is set up, as methodQueue
-  // initialization requires it (View Managers get their queue by calling
-  // self.bridge.uiManager.methodQueue)
-  [self setBridgeForInstance:_bridge];
-
-  // Initialize queue
-  [self setUpMethodQueue];
-
-  [self cacheImplementedSelectors];
-}
 
 - (BOOL)hasInstance
 {
@@ -114,30 +115,23 @@ RCT_NOT_IMPLEMENTED(- (instancetype)init);
 
 - (id<RCTBridgeModule>)instance
 {
-  if (!_instance) {
+  [_instanceLock lock];
-    [self setUpInstance];
+  if (!_setupComplete) {
+    if (!_instance) {
+      _instance = [_moduleClass new];
+    }
+    // Bridge must be set before methodQueue is set up, as methodQueue
+    // initialization requires it (View Managers get their queue by calling
+    // self.bridge.uiManager.methodQueue)
+    [self setBridgeForInstance];
+    [self setUpMethodQueue];
+    [_bridge registerModuleForFrameUpdates:_instance withModuleData:self];
+    _setupComplete = YES;
   }
+  [_instanceLock unlock];
   return _instance;
 }
 
-- (void)setBridgeForInstance:(RCTBridge *)bridge
-{
-  RCTAssert(_instance, @"setBridgeForInstance called before module was initialized");
-
-  _bridge = bridge;
-  if ([_instance respondsToSelector:@selector(bridge)]) {
-    @try {
-      [(id)_instance setValue:bridge forKey:@"bridge"];
-    }
-    @catch (NSException *exception) {
-      RCTLogError(@"%@ has no setter or ivar for its bridge, which is not "
-                  "permitted. You must either @synthesize the bridge property, "
-                  "or provide your own setter method.", self.name);
-    }
-  }
-  [bridge registerModuleForFrameUpdates:self];
-}
-
 - (NSString *)name
 {
   return RCTBridgeModuleNameForClass(_moduleClass);
@@ -148,7 +142,8 @@ RCT_NOT_IMPLEMENTED(- (instancetype)init);
   if (!_methods) {
     NSMutableArray<id<RCTBridgeMethod>> *moduleMethods = [NSMutableArray new];
 
-    if ([_instance respondsToSelector:@selector(methodsToExport)]) {
+    if ([_moduleClass instancesRespondToSelector:@selector(methodsToExport)]) {
+      [self instance];
       [moduleMethods addObjectsFromArray:[_instance methodsToExport]];
     }
 
@@ -182,7 +177,7 @@ RCT_NOT_IMPLEMENTED(- (instancetype)init);
 {
   __block NSDictionary<NSString *, id> *constants;
   if (RCTClassOverridesInstanceMethod(_moduleClass, @selector(constantsToExport))) {
-    [self instance]; // Initialize instance
+    [self instance];
     RCTExecuteOnMainThread(^{
       constants = [_instance constantsToExport];
     }, YES);
@@ -220,12 +215,7 @@ RCT_NOT_IMPLEMENTED(- (instancetype)init);
 
 - (dispatch_queue_t)methodQueue
 {
-  if (!_instance) {
+  [self instance];
-    [self setUpInstance];
-  }
-  if (!_methodQueue) {
-    [self setUpMethodQueue];
-  }
   return _methodQueue;
 }