From 7b770556ac6eb60094bdf59bfd14876b6e2a1307 Mon Sep 17 00:00:00 2001 From: Pieter De Baets Date: Mon, 11 Sep 2017 04:24:29 -0700 Subject: [PATCH] Improve RCTCxxBridge invalidation Reviewed By: fromcelticpark Differential Revision: D5536063 fbshipit-source-id: b0f19bebea839c7da84890c338ad4d38293bc99c --- React/Base/RCTBatchedBridge.mm | 4 + React/Base/RCTModuleData.mm | 2 + React/CxxBridge/RCTCxxBridge.mm | 120 ++++++++++--------- React/CxxModule/DispatchMessageQueueThread.h | 1 - ReactCommon/cxxreact/Instance.h | 6 +- 5 files changed, 71 insertions(+), 62 deletions(-) diff --git a/React/Base/RCTBatchedBridge.mm b/React/Base/RCTBatchedBridge.mm index 2f5a6da2c2adb3..d56d61025ae2a2 100644 --- a/React/Base/RCTBatchedBridge.mm +++ b/React/Base/RCTBatchedBridge.mm @@ -928,6 +928,10 @@ - (void)handleBuffer:(id)buffer batchEnded:(BOOL)batchEnded { RCTAssertJSThread(); + if (!self.valid) { + return; + } + if (buffer != nil && buffer != (id)kCFNull) { _wasBatchActive = YES; [self handleBuffer:buffer]; diff --git a/React/Base/RCTModuleData.mm b/React/Base/RCTModuleData.mm index c1645549b381cf..340e9a5a7db6ee 100644 --- a/React/Base/RCTModuleData.mm +++ b/React/Base/RCTModuleData.mm @@ -373,6 +373,8 @@ - (NSArray *)config - (dispatch_queue_t)methodQueue { (void)[self instance]; + RCTAssert(_methodQueue != nullptr, @"Module %@ has no methodQueue (instance: %@, bridge.valid: %d)", + self, _instance, _bridge.valid); return _methodQueue; } diff --git a/React/CxxBridge/RCTCxxBridge.mm b/React/CxxBridge/RCTCxxBridge.mm index cd6844bdb68691..bbf6b053480476 100644 --- a/React/CxxBridge/RCTCxxBridge.mm +++ b/React/CxxBridge/RCTCxxBridge.mm @@ -149,13 +149,12 @@ void onBatchComplete() override { [bridge_ partialBatchDidFlush]; [bridge_ batchDidComplete]; } - void incrementPendingJSCalls() override {} - void decrementPendingJSCalls() override {} }; @implementation RCTCxxBridge { BOOL _wasBatchActive; + BOOL _didInvalidate; NSMutableArray *_pendingCalls; std::atomic _pendingCount; @@ -194,7 +193,7 @@ - (JSContext *)jsContext - (JSGlobalContextRef)jsContextRef { - return (JSGlobalContextRef)self->_reactInstance->getJavaScriptContext(); + return (JSGlobalContextRef)(self->_reactInstance ? self->_reactInstance->getJavaScriptContext() : nullptr); } - (instancetype)initWithParentBridge:(RCTBridge *)bridge @@ -229,7 +228,7 @@ - (instancetype)initWithParentBridge:(RCTBridge *)bridge return self; } -- (void)runJSRunLoop ++ (void)runRunLoop { @autoreleasepool { RCT_PROFILE_BEGIN_EVENT(RCTProfileTagAlways, @"-[RCTCxxBridge runJSRunLoop] setup", nil); @@ -292,8 +291,8 @@ - (void)start object:_parentBridge userInfo:@{@"bridge": self}]; // Set up the JS thread early - _jsThread = [[NSThread alloc] initWithTarget:self - selector:@selector(runJSRunLoop) + _jsThread = [[NSThread alloc] initWithTarget:[self class] + selector:@selector(runRunLoop) object:nil]; _jsThread.name = RCTJSThreadName; _jsThread.qualityOfService = NSOperationQualityOfServiceUserInteractive; @@ -522,7 +521,7 @@ - (void)_initializeBridge:(std::shared_ptr)executorFactory // This is async, but any calls into JS are blocked by the m_syncReady CV in Instance _reactInstance->initializeBridge( - std::unique_ptr(new RCTInstanceCallback(self)), + std::make_unique(self), executorFactory, _jsMessageThread, [self _buildModuleRegistry]); @@ -845,6 +844,7 @@ - (void)handleError:(NSError *)error } RCTFatal(error); + // RN will stop, but let the rest of the app keep going. return; } @@ -855,27 +855,27 @@ - (void)handleError:(NSError *)error // Hack: once the bridge is invalidated below, it won't initialize any new native // modules. Initialize the redbox module now so we can still report this error. - [self redBox]; + RCTRedBox *redBox = [self redBox]; _loading = NO; _valid = NO; dispatch_async(dispatch_get_main_queue(), ^{ if (self->_jsMessageThread) { - auto thread = self->_jsMessageThread; - self->_jsMessageThread->runOnQueue([thread] { - thread->quitSynchronous(); - }); - self->_jsMessageThread.reset(); + // Make sure initializeBridge completed + self->_jsMessageThread->runOnQueueSync([] {}); } + self->_reactInstance.reset(); + self->_jsMessageThread.reset(); + [[NSNotificationCenter defaultCenter] postNotificationName:RCTJavaScriptDidFailToLoadNotification object:self->_parentBridge userInfo:@{@"bridge": self, @"error": error}]; if ([error userInfo][RCTJSRawStackTraceKey]) { - [self.redBox showErrorMessage:[error localizedDescription] - withRawStack:[error userInfo][RCTJSRawStackTraceKey]]; + [redBox showErrorMessage:[error localizedDescription] + withRawStack:[error userInfo][RCTJSRawStackTraceKey]]; } RCTFatal(error); @@ -942,63 +942,68 @@ - (void)dispatchBlock:(dispatch_block_t)block - (void)invalidate { - if (!_valid) { + if (_didInvalidate) { return; } RCTAssertMainQueue(); - RCTAssert(_reactInstance != nil, @"Can't complete invalidation without a react instance"); + RCTLogInfo(@"Invalidating %@ (parent: %@, executor: %@)", self, _parentBridge, [self executorClass]); _loading = NO; _valid = NO; + _didInvalidate = YES; + if ([RCTBridge currentBridge] == self) { [RCTBridge setCurrentBridge:nil]; } - // Invalidate modules - dispatch_group_t group = dispatch_group_create(); - for (RCTModuleData *moduleData in _moduleDataByID) { - // Be careful when grabbing an instance here, we don't want to instantiate - // any modules just to invalidate them. - if (![moduleData hasInstance]) { - continue; - } + // Stop JS instance and message thread + [self ensureOnJavaScriptThread:^{ + [self->_displayLink invalidate]; + self->_displayLink = nil; - if ([moduleData.instance respondsToSelector:@selector(invalidate)]) { - dispatch_group_enter(group); - [self dispatchBlock:^{ - [(id)moduleData.instance invalidate]; - dispatch_group_leave(group); - } queue:moduleData.methodQueue]; + if (RCTProfileIsProfiling()) { + RCTProfileUnhookModules(self); } - [moduleData invalidate]; - } - - dispatch_group_notify(group, dispatch_get_main_queue(), ^{ - [self ensureOnJavaScriptThread:^{ - [self->_displayLink invalidate]; - self->_displayLink = nil; - self->_reactInstance.reset(); - if (self->_jsMessageThread) { - self->_jsMessageThread->quitSynchronous(); - self->_jsMessageThread.reset(); + // Invalidate modules + // We're on the JS thread (which we'll be suspending soon), so no new calls will be made to native modules after + // this completes. We must ensure all previous calls were dispatched before deallocating the instance (and module + // wrappers) or we may have invalid pointers still in flight. + dispatch_group_t moduleInvalidation = dispatch_group_create(); + for (RCTModuleData *moduleData in self->_moduleDataByID) { + // Be careful when grabbing an instance here, we don't want to instantiate + // any modules just to invalidate them. + if (![moduleData hasInstance]) { + continue; } - if (RCTProfileIsProfiling()) { - RCTProfileUnhookModules(self); + if ([moduleData.instance respondsToSelector:@selector(invalidate)]) { + dispatch_group_enter(moduleInvalidation); + [self dispatchBlock:^{ + [(id)moduleData.instance invalidate]; + dispatch_group_leave(moduleInvalidation); + } queue:moduleData.methodQueue]; } + [moduleData invalidate]; + } - self->_moduleDataByName = nil; - self->_moduleDataByID = nil; - self->_moduleClassesByID = nil; - self->_pendingCalls = nil; + if (dispatch_group_wait(moduleInvalidation, dispatch_time(DISPATCH_TIME_NOW, 10 * NSEC_PER_SEC))) { + RCTLogError(@"Timed out waiting for modules to be invalidated"); + } - [self->_jsThread cancel]; - self->_jsThread = nil; - CFRunLoopStop(CFRunLoopGetCurrent()); - }]; - }); + self->_reactInstance.reset(); + self->_jsMessageThread.reset(); + + self->_moduleDataByName = nil; + self->_moduleDataByID = nil; + self->_moduleClassesByID = nil; + self->_pendingCalls = nil; + + [self->_jsThread cancel]; + self->_jsThread = nil; + CFRunLoopStop(CFRunLoopGetCurrent()); + }]; } - (void)logMessage:(NSString *)message level:(NSString *)level @@ -1127,7 +1132,6 @@ - (void)enqueueCallback:(NSNumber *)cbID args:(NSArray *)args */ RCTProfileBeginFlowEvent(); - [self _runAfterLoad:^{ RCTProfileEndFlowEvent(); @@ -1218,25 +1222,25 @@ - (JSValue *)callFunctionOnModule:(NSString *)module if (!_reactInstance) { if (error) { *error = RCTErrorWithMessage( - @"Attempt to call sync callFunctionOnModule: on uninitialized bridge"); + @"callFunctionOnModule was called on uninitialized bridge"); } return nil; } else if (self.executorClass) { if (error) { *error = RCTErrorWithMessage( - @"sync callFunctionOnModule: can only be used with JSC executor"); + @"callFunctionOnModule can only be used with JSC executor"); } return nil; } else if (!self.valid) { if (error) { *error = RCTErrorWithMessage( - @"sync callFunctionOnModule: bridge is no longer valid"); + @"Bridge is no longer valid"); } return nil; } else if (self.loading) { if (error) { *error = RCTErrorWithMessage( - @"sync callFunctionOnModule: bridge is still loading"); + @"Bridge is still loading"); } return nil; } diff --git a/React/CxxModule/DispatchMessageQueueThread.h b/React/CxxModule/DispatchMessageQueueThread.h index f9dd9d33137e52..2877f5800bf6bc 100644 --- a/React/CxxModule/DispatchMessageQueueThread.h +++ b/React/CxxModule/DispatchMessageQueueThread.h @@ -24,7 +24,6 @@ class DispatchMessageQueueThread : public MessageQueueThread { void runOnQueue(std::function&& func) override { dispatch_queue_t queue = moduleData_.methodQueue; - RCTAssert(queue != nullptr, @"Module %@ provided invalid queue", moduleData_); dispatch_block_t block = [func=std::move(func)] { func(); }; RCTAssert(block != nullptr, @"Invalid block generated in call to %@", moduleData_); if (queue && block) { diff --git a/ReactCommon/cxxreact/Instance.h b/ReactCommon/cxxreact/Instance.h index cb8504542f9d9b..5089830b48e76d 100644 --- a/ReactCommon/cxxreact/Instance.h +++ b/ReactCommon/cxxreact/Instance.h @@ -27,9 +27,9 @@ class ModuleRegistry; struct InstanceCallback { virtual ~InstanceCallback() {} - virtual void onBatchComplete() = 0; - virtual void incrementPendingJSCalls() = 0; - virtual void decrementPendingJSCalls() = 0; + virtual void onBatchComplete() {} + virtual void incrementPendingJSCalls() {} + virtual void decrementPendingJSCalls() {} }; class RN_EXPORT Instance {