Permalink
Browse files

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
  • Loading branch information...
PeteTheHeat authored and facebook-github-bot committed Nov 3, 2018
1 parent 2486d12 commit 1c3191992a0ce408bb1d6e38934ce4b4118bd93c
Showing with 24 additions and 12 deletions.
  1. +24 −12 React/CxxBridge/RCTCxxBridge.mm
@@ -155,6 +155,7 @@ @implementation RCTCxxBridge
{
BOOL _wasBatchActive;
BOOL _didInvalidate;
BOOL _moduleRegistryCreated;

NSMutableArray<RCTPendingCall> *_pendingCalls;
std::atomic<NSInteger> _pendingCount;
@@ -169,6 +170,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;
@@ -209,9 +211,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];
@@ -442,7 +444,7 @@ - (BOOL)moduleIsInitialized:(Class)moduleClass
return _moduleDataByName[RCTBridgeModuleNameForClass(moduleClass)].hasInstance;
}

- (std::shared_ptr<ModuleRegistry>)_buildModuleRegistry
- (std::shared_ptr<ModuleRegistry>)_buildModuleRegistryUnlocked
{
if (!self.valid) {
return {};
@@ -489,12 +491,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()) {
@@ -508,6 +505,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<RCTModuleData *> *)registerModulesForClasses:(NSArray<Class> *)moduleClasses
{
RCT_PROFILE_BEGIN_EVENT(RCTProfileTagAlways,
@@ -706,11 +716,13 @@ - (void)registerExtraLazyModules

- (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];
}
}

0 comments on commit 1c31919

Please sign in to comment.