Improve RCTCxxBridge invalidation

Reviewed By: fromcelticpark

Differential Revision: D5536063

fbshipit-source-id: b0f19bebea839c7da84890c338ad4d38293bc99c

Author: javache

React/Base/RCTBatchedBridge.mm

@@ -921,6 +921,10 @@ - (void)handleBuffer:(id)buffer batchEnded:(BOOL)batchEnded
 {
   RCTAssertJSThread();
 
+  if (!self.valid) {
+    return;
+  }
+
   if (buffer != nil && buffer != (id)kCFNull) {
     _wasBatchActive = YES;
     [self handleBuffer:buffer];

React/Base/RCTModuleData.mm

@@ -357,6 +357,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;
 }
 

React/CxxBridge/RCTCxxBridge.mm

@@ -125,13 +125,12 @@ void onBatchComplete() override {
     [bridge_ partialBatchDidFlush];
     [bridge_ batchDidComplete];
   }
-  void incrementPendingJSCalls() override {}
-  void decrementPendingJSCalls() override {}
 };
 
 @implementation RCTCxxBridge
 {
   BOOL _wasBatchActive;
+  BOOL _didInvalidate;
 
   NSMutableArray<dispatch_block_t> *_pendingCalls;
   std::atomic<NSInteger> _pendingCount;
@@ -169,7 +168,7 @@ - (JSContext *)jsContext
 
 - (JSGlobalContextRef)jsContextRef
 {
-  return (JSGlobalContextRef)self->_reactInstance->getJavaScriptContext();
+  return (JSGlobalContextRef)(self->_reactInstance ? self->_reactInstance->getJavaScriptContext() : nullptr);
 }
 
 - (instancetype)initWithParentBridge:(RCTBridge *)bridge
@@ -200,7 +199,7 @@ - (instancetype)initWithParentBridge:(RCTBridge *)bridge
   return self;
 }
 
-- (void)runJSRunLoop
++ (void)runRunLoop
 {
   @autoreleasepool {
     RCT_PROFILE_BEGIN_EVENT(RCTProfileTagAlways, @"-[RCTCxxBridge runJSRunLoop] setup", nil);
@@ -263,8 +262,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;
@@ -469,7 +468,7 @@ - (void)_initializeBridge:(std::shared_ptr<JSExecutorFactory>)executorFactory
   if (_reactInstance) {
     // This is async, but any calls into JS are blocked by the m_syncReady CV in Instance
     _reactInstance->initializeBridge(
-      std::unique_ptr<RCTInstanceCallback>(new RCTInstanceCallback(self)),
+      std::make_unique<RCTInstanceCallback>(self),
       executorFactory,
       _jsMessageThread,
       [self _buildModuleRegistry]);
@@ -769,6 +768,7 @@ - (void)handleError:(NSError *)error
     }
 
     RCTFatal(error);
+
     // RN will stop, but let the rest of the app keep going.
     return;
   }
@@ -779,27 +779,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);
@@ -866,63 +866,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<RCTInvalidating>)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<RCTInvalidating>)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
@@ -1051,7 +1056,6 @@ - (void)enqueueCallback:(NSNumber *)cbID args:(NSArray *)args
    */
 
   RCTProfileBeginFlowEvent();
-
   [self _runAfterLoad:^{
     RCTProfileEndFlowEvent();
 
@@ -1142,25 +1146,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;
   }

React/CxxModule/DispatchMessageQueueThread.h

@@ -24,7 +24,6 @@ class DispatchMessageQueueThread : public MessageQueueThread {
 
   void runOnQueue(std::function<void()>&& 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) {

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 {