From 29245e96cb90fbf26e2b3aa1f946db57ddcdde2b Mon Sep 17 00:00:00 2001
From: Kevin Gozali <fkg@fb.com>
Date: Thu, 9 Aug 2018 12:16:19 -0700
Subject: [PATCH] iOS: prevent nativemodule access from JS if bridge is no
 longer valid

Summary: This helps prevent race condition where JS calls to NativeModules got queued and executed while the bridge is invalidating itself, causing assertion failures in test setup (for example). It won't prevent it 100% of the time, due to threading (and adding lock is expensive for each nativemodule call).

Reviewed By: yungsters

Differential Revision: D9231636

fbshipit-source-id: 298eaf52ffa4b84108184124e75b206b9ca7a41d
---
 Libraries/RCTTest/RCTTestRunner.m  | 41 +++++++++++++++++++-----------
 React/CxxModule/RCTNativeModule.mm | 15 +++++++----
 2 files changed, 36 insertions(+), 20 deletions(-)

diff --git a/Libraries/RCTTest/RCTTestRunner.m b/Libraries/RCTTest/RCTTestRunner.m
index 65e126ae8..6d572a104 100644
--- a/Libraries/RCTTest/RCTTestRunner.m
+++ b/Libraries/RCTTest/RCTTestRunner.m
@@ -115,20 +115,20 @@ expectErrorBlock:(BOOL(^)(NSString *error))expectErrorBlock
 {
   __weak RCTBridge *batchedBridge;
   NSNumber *rootTag;
+  RCTLogFunction defaultLogFunction = RCTGetLogFunction();
+  // Catch all error logs, that are equivalent to redboxes in dev mode.
+  __block NSMutableArray<NSString *> *errors = nil;
+  RCTSetLogFunction(^(RCTLogLevel level, RCTLogSource source, NSString *fileName, NSNumber *lineNumber, NSString *message) {
+    defaultLogFunction(level, source, fileName, lineNumber, message);
+    if (level >= RCTLogLevelError) {
+      if (errors == nil) {
+        errors = [NSMutableArray new];
+      }
+      [errors addObject:message];
+    }
+  });
 
   @autoreleasepool {
-    __block NSMutableArray<NSString *> *errors = nil;
-    RCTLogFunction defaultLogFunction = RCTGetLogFunction();
-    RCTSetLogFunction(^(RCTLogLevel level, RCTLogSource source, NSString *fileName, NSNumber *lineNumber, NSString *message) {
-      defaultLogFunction(level, source, fileName, lineNumber, message);
-      if (level >= RCTLogLevelError) {
-        if (errors == nil) {
-          errors = [NSMutableArray new];
-        }
-        [errors addObject:message];
-      }
-    });
-
     RCTBridge *bridge = [[RCTBridge alloc] initWithBundleURL:_scriptURL
                                               moduleProvider:_moduleProvider
                                                launchOptions:nil];
@@ -172,7 +172,16 @@ expectErrorBlock:(BOOL(^)(NSString *error))expectErrorBlock
       testModule.view = nil;
     }
 
-    RCTSetLogFunction(defaultLogFunction);
+    // From this point on catch only fatal errors.
+    RCTSetLogFunction(^(RCTLogLevel level, RCTLogSource source, NSString *fileName, NSNumber *lineNumber, NSString *message) {
+      defaultLogFunction(level, source, fileName, lineNumber, message);
+      if (level >= RCTLogLevelFatal) {
+        if (errors == nil) {
+          errors = [NSMutableArray new];
+        }
+        [errors addObject:message];
+      }
+    });
 
 #if RCT_DEV
     NSArray<UIView *> *nonLayoutSubviews = [vc.view.subviews filteredArrayUsingPredicate:[NSPredicate predicateWithBlock:^BOOL(id subview, NSDictionary *bindings) {
@@ -208,8 +217,10 @@ expectErrorBlock:(BOOL(^)(NSString *error))expectErrorBlock
     [[NSRunLoop mainRunLoop] runMode:NSDefaultRunLoopMode beforeDate:[NSDate dateWithTimeIntervalSinceNow:0.1]];
     [[NSRunLoop mainRunLoop] runMode:NSRunLoopCommonModes beforeDate:[NSDate dateWithTimeIntervalSinceNow:0.1]];
   }
-  // Note: this deallocation isn't consistently working in test setup, so disable the assertion.
-  // RCTAssert(batchedBridge == nil, @"Bridge should be deallocated after the test");
+  RCTAssert(errors == nil, @"RedBox errors during bridge invalidation: %@", errors);
+  RCTAssert(batchedBridge == nil, @"Bridge should be deallocated after the test");
+
+  RCTSetLogFunction(defaultLogFunction);
 }
 
 @end
diff --git a/React/CxxModule/RCTNativeModule.mm b/React/CxxModule/RCTNativeModule.mm
index 077b72978..8cf3b74db 100644
--- a/React/CxxModule/RCTNativeModule.mm
+++ b/React/CxxModule/RCTNativeModule.mm
@@ -71,11 +71,16 @@ void RCTNativeModule::invoke(unsigned int methodId, folly::dynamic &&params, int
     invokeInner(weakBridge, weakModuleData, methodId, std::move(params));
   };
 
-  dispatch_queue_t queue = m_moduleData.methodQueue;
-  if (queue == RCTJSThread) {
-    block();
-  } else if (queue) {
-    dispatch_async(queue, block);
+  if (m_bridge.valid) {
+    dispatch_queue_t queue = m_moduleData.methodQueue;
+    if (queue == RCTJSThread) {
+      block();
+    } else if (queue) {
+      dispatch_async(queue, block);
+    }
+  } else {
+    RCTLogError(@"Attempted to invoke `%u` (method ID) on `%@` (NativeModule name) with an invalid bridge.",
+                methodId, m_moduleData.name);
   }
 }