From fc98956b65edda40de5cb12f07ba53bf20198813 Mon Sep 17 00:00:00 2001 From: Nick Lockwood Date: Tue, 5 Jan 2016 09:04:08 -0800 Subject: [PATCH] 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 | 38 ++++++--- React/Base/RCTBridge+Private.h | 7 +- React/Base/RCTModuleData.h | 5 +- React/Base/RCTModuleData.m | 146 +++++++++++++++------------------ 4 files changed, 102 insertions(+), 94 deletions(-) diff --git a/React/Base/RCTBatchedBridge.m b/React/Base/RCTBatchedBridge.m index 65db38432..7ae745a39 100644 --- a/React/Base/RCTBatchedBridge.m +++ b/React/Base/RCTBatchedBridge.m @@ -295,7 +295,8 @@ RCT_EXTERN NSArray *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 *RCTGetModuleClasses(void); for (RCTModuleData *moduleData in _moduleDataByID) { if (moduleData.hasInstance) { - [moduleData setBridgeForInstance:self]; + [moduleData setBridgeForInstance]; } } @@ -330,6 +331,13 @@ RCT_EXTERN NSArray *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 *RCTGetModuleClasses(void); [_javaScriptExecutor setUp]; } -- (void)registerModuleForFrameUpdates:(RCTModuleData *)moduleData +- (void)registerModuleForFrameUpdates:(id)module + withModuleData:(RCTModuleData *)moduleData { - if ([moduleData.moduleClass conformsToProtocol:@protocol(RCTFrameUpdateObserver)]) { - [_frameUpdateObservers addObject:moduleData]; - id observer = (id)moduleData.instance; - __weak typeof(self) weakSelf = self; - __weak typeof(_javaScriptExecutor) weakJavaScriptExecutor = _javaScriptExecutor; - observer.pauseCallback = ^{ - [weakJavaScriptExecutor executeBlockOnJavaScriptQueue:^{ - [weakSelf updateJSDisplayLinkState]; - }]; - }; + if (![_frameUpdateObservers containsObject:moduleData]) { + if ([moduleData.moduleClass conformsToProtocol:@protocol(RCTFrameUpdateObserver)]) { + [_frameUpdateObservers addObject:moduleData]; + // Don't access the module instance via moduleData, as this will cause deadlock + id observer = (id)module; + __weak typeof(self) weakSelf = self; + __weak typeof(_javaScriptExecutor) weakJavaScriptExecutor = _javaScriptExecutor; + observer.pauseCallback = ^{ + [weakJavaScriptExecutor executeBlockOnJavaScriptQueue:^{ + [weakSelf updateJSDisplayLinkState]; + }]; + }; + } } } diff --git a/React/Base/RCTBridge+Private.h b/React/Base/RCTBridge+Private.h index 91675efa5..ddeddeaa4 100644 --- a/React/Base/RCTBridge+Private.h +++ b/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)module + withModuleData:(RCTModuleData *)moduleData; /** * Dispatch work to a module's queue - this is also suports the fake RCTJSThread diff --git a/React/Base/RCTModuleData.h b/React/Base/RCTModuleData.h index 79752ba29..bf55892fd 100644 --- a/React/Base/RCTModuleData.h +++ b/React/Base/RCTModuleData.h @@ -20,14 +20,15 @@ - (instancetype)initWithModuleClass:(Class)moduleClass bridge:(RCTBridge *)bridge NS_DESIGNATED_INITIALIZER; -- (instancetype)initWithModuleInstance:(id)instance NS_DESIGNATED_INITIALIZER; +- (instancetype)initWithModuleInstance:(id)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; diff --git a/React/Base/RCTModuleData.m b/React/Base/RCTModuleData.m index 71f5dd2f9..2e290ce6c 100644 --- a/React/Base/RCTModuleData.m +++ b/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)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)]; - _implementsPartialBatchDidFlush = [_instance respondsToSelector:@selector(partialBatchDidFlush)]; + RCTAssert(_instance, @"setBridgeForInstance called before %@ initialized", self.name); + 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) { - - // Create new queue (store queueName, as it isn't retained by dispatch_queue) - _queueName = [NSString stringWithFormat:@"com.facebook.React.%@Queue", self.name]; - _methodQueue = dispatch_queue_create(_queueName.UTF8String, DISPATCH_QUEUE_SERIAL); - - // assign it to the module + RCTAssert(_instance, @"setUpMethodQueue called before %@ initialized", self.name); + BOOL implementsMethodQueue = [_instance respondsToSelector:@selector(methodQueue)]; 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); + _methodQueue = _instance.methodQueue; + } + if (!_methodQueue) { + + // Create new queue (store queueName, as it isn't retained by dispatch_queue) + _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 { + [(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 -{ - 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]; -} +#pragma mark - public getters - (BOOL)hasInstance { @@ -114,30 +115,23 @@ RCT_NOT_IMPLEMENTED(- (instancetype)init); - (id)instance { - if (!_instance) { - [self setUpInstance]; + [_instanceLock lock]; + 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> *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 *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 setUpInstance]; - } - if (!_methodQueue) { - [self setUpMethodQueue]; - } + [self instance]; return _methodQueue; }