Use std::atomic for eliminating races in RCTCxxBridge (#45558)

hakonk · facebook-github-bot · commit 8204134024e8 · 2024-07-25T10:22:56.000-07:00
Summary: As explained in #45280, `TSan` picks up data races related to concurrent read/write to fields in `RCTCxxBridge`. See this report for reference: ``` WARNING: ThreadSanitizer: data race (pid=19983) Write of size 1 at 0x00010af1dfd8 by thread T13: #0 -[RCTCxxBridge _flushPendingCalls] <null> (RNTesterUnitTests:arm64+0x42b484) #1 __53-[RCTCxxBridge executeSourceCode:withSourceURL:sync:]_block_invoke <null> (RNTesterUnitTests:arm64+0x426050) #2 decltype(std::declval<void () block_pointer __strong&>()()) std::__1::__invoke[abi:ue170006]<void () block_pointer __strong&>(&&, decltype(std::declval<void () block_pointer __strong&>()())&&...) <null> (RNTesterUnitTests:arm64+0x456298) #3 std::__1::__function::__func<void () block_pointer __strong, std::__1::allocator<std::__1::allocator>, void ()>::operator()() <null> (RNTesterUnitTests:arm64+0x455c6c) #4 std::__1::__function::__value_func<void ()>::operator()[abi:ue170006]() const <null> (RNTesterUnitTests:arm64+0x3ce2e4) #5 std::__1::function<void ()>::operator()() const <null> (RNTesterUnitTests:arm64+0x3cdfd0) #6 facebook::react::tryAndReturnError(std::__1::function<void ()> const&) <null> (RNTesterUnitTests:arm64+0x4af18c) #7 facebook::react::RCTMessageThread::tryFunc(std::__1::function<void ()> const&) <null> (RNTesterUnitTests:arm64+0x51595c) #8 facebook::react::RCTMessageThread::runOnQueue(std::__1::function<void ()>&&)::$_1::operator()() const <null> (RNTesterUnitTests:arm64+0x529df0) #9 decltype(std::declval<facebook::react::RCTMessageThread::runOnQueue(std::__1::function<void ()>&&)::$_1&>()()) std::__1::__invoke[abi:ue170006]<facebook::react::RCTMessageThread::runOnQueue(std::__1::function<void ()>&&)::$_1&>(facebook::react::RCTMessageThread::runOnQueue(std::__1::function<void ()>&&)::$_1&) <null> (RNTesterUnitTests:arm64+0x529b54) #10 void std::__1::__invoke_void_return_wrapper<void, true>::__call[abi:ue170006]<facebook::react::RCTMessageThread::runOnQueue(std::__1::function<void ()>&&)::$_1&>(facebook::react::RCTMessageThread::runOnQueue(std::__1::function<void ()>&&)::$_1&) <null> (RNTesterUnitTests:arm64+0x529978) #11 std::__1::__function::__alloc_func<facebook::react::RCTMessageThread::runOnQueue(std::__1::function<void ()>&&)::$_1, std::__1::allocator<facebook::react::RCTMessageThread::runOnQueue(std::__1::function<void ()>&&)::$_1>, void ()>::operator()[abi:ue170006]() <null> (RNTesterUnitTests:arm64+0x5298dc) #12 std::__1::__function::__func<facebook::react::RCTMessageThread::runOnQueue(std::__1::function<void ()>&&)::$_1, std::__1::allocator<facebook::react::RCTMessageThread::runOnQueue(std::__1::function<void ()>&&)::$_1>, void ()>::operator()() <null> (RNTesterUnitTests:arm64+0x524518) #13 std::__1::__function::__value_func<void ()>::operator()[abi:ue170006]() const <null> (RNTesterUnitTests:arm64+0x3ce2e4) #14 std::__1::function<void ()>::operator()() const <null> (RNTesterUnitTests:arm64+0x3cdfd0) #15 invocation function for block in facebook::react::RCTMessageThread::runAsync(std::__1::function<void ()>) <null> (RNTesterUnitTests:arm64+0x515384) #16 __CFRUNLOOP_IS_CALLING_OUT_TO_A_BLOCK__ <null> (CoreFoundation:arm64+0x8dc0c) #17 __NSThread__start__ <null> (Foundation:arm64+0x645c60) Previous read of size 1 at 0x00010af1dfd8 by main thread: #0 -[RCTCxxBridge isLoading] <null> (RNTesterUnitTests:arm64+0x43236c) #1 -[RCTBridge isLoading] <null> (RNTesterUnitTests:arm64+0x3c0170) #2 -[RCTComponentPropsTests setUp] <null> (RNTesterUnitTests:arm64+0xe6f34) #3 __70-[XCTestCase _shouldContinueAfterPerformingSetUpSequenceWithSelector:]_block_invoke.134 <null> (XCTestCore:arm64+0x540d8) Location is heap block of size 384 at 0x00010af1de80 allocated by main thread: #0 calloc <null> (libclang_rt.tsan_iossim_dynamic.dylib:arm64+0x4fc30) #1 _malloc_type_calloc_outlined <null> (libsystem_malloc.dylib:arm64+0xf488) #2 -[RCTBridge initWithDelegate:bundleURL:moduleProvider:launchOptions:] <null> (RNTesterUnitTests:arm64+0x3bc540) #3 -[RCTBridge initWithBundleURL:moduleProvider:launchOptions:] <null> (RNTesterUnitTests:arm64+0x3bc124) #4 -[RCTComponentPropsTests setUp] <null> (RNTesterUnitTests:arm64+0xe6b3c) #5 __70-[XCTestCase _shouldContinueAfterPerformingSetUpSequenceWithSelector:]_block_invoke.134 <null> (XCTestCore:arm64+0x540d8) Thread T13 (tid=11290378, running) created by main thread at: #0 pthread_create <null> (libclang_rt.tsan_iossim_dynamic.dylib:arm64+0x2bee4) #1 -[NSThread startAndReturnError:] <null> (Foundation:arm64+0x6458f0) #2 -[RCTBridge setUp] <null> (RNTesterUnitTests:arm64+0x3bf748) #3 -[RCTBridge initWithDelegate:bundleURL:moduleProvider:launchOptions:] <null> (RNTesterUnitTests:arm64+0x3bc540) #4 -[RCTBridge initWithBundleURL:moduleProvider:launchOptions:] <null> (RNTesterUnitTests:arm64+0x3bc124) #5 -[RCTComponentPropsTests setUp] <null> (RNTesterUnitTests:arm64+0xe6b3c) #6 __70-[XCTestCase _shouldContinueAfterPerformingSetUpSequenceWithSelector:]_block_invoke.134 <null> (XCTestCore:arm64+0x540d8) ``` In order to fix the data races, `std::atomic` instead of primitive boolean types. In order to reproduce my findings and verify fix: * Clone this branch * Run setup code as described in README * Execute `git revert -n 11c09fd`. * Enable TSan for both RNTester and its test scheme. * Enable Runtime issue breakpoint for TSan * Run unit tests * Observe the TSan breakpoint is hit (possibly other places in the codebase as well) when accessing `_loading`, `_moduleRegistryCreated`, and `_valid`.. Continue execution if other breakpoints are hit before this breakpoint. * Execute git revert --abort * Run the tests again and observe the TSan breakpoint does not hit said code again. NB! While this will fix data races, it will not fix potential race conditions. I have not encountered bugs related to race conditions in `RCTCxxBridge`, but given the nature of how it is made use of concurrently, it is, in my opinion, plausible. ## Changelog: [iOS][Fixed] Use std::atomic for eliminating races in RCTCxxBridge. Pull Request resolved: #45558 Test Plan: I believe there are existing Unit tests in place for verifying this fix. Reviewed By: cipolleschi Differential Revision: D60233758 Pulled By: dmytrorykun fbshipit-source-id: 8aa124a0521ad43a5e17b42e0ce6d22ae6b4e667
diff --git a/packages/react-native/React/CxxBridge/RCTCxxBridge.mm b/packages/react-native/React/CxxBridge/RCTCxxBridge.mm
@@ -193,7 +193,7 @@ void onBatchComplete() override
 
 @implementation RCTCxxBridge {
   BOOL _didInvalidate;
-  BOOL _moduleRegistryCreated;
+  std::atomic<BOOL> _moduleRegistryCreated;
 
   NSMutableArray<RCTPendingCall> *_pendingCalls;
   std::atomic<NSInteger> _pendingCount;
@@ -220,12 +220,32 @@ @implementation RCTCxxBridge {
   RCTViewRegistry *_viewRegistry_DEPRECATED;
   RCTBundleManager *_bundleManager;
   RCTCallableJSModules *_callableJSModules;
+  std::atomic<BOOL> _loading;
+  std::atomic<BOOL> _valid;
 }
 
 @synthesize bridgeDescription = _bridgeDescription;
-@synthesize loading = _loading;
 @synthesize performanceLogger = _performanceLogger;
-@synthesize valid = _valid;
+
+- (BOOL)isLoading
+{
+  return _loading;
+}
+
+- (void)setLoading:(BOOL)newValue
+{
+  _loading = newValue;
+}
+
+- (BOOL)isValid
+{
+  return _valid;
+}
+
+- (void)setValid:(BOOL)newValue
+{
+  _valid = newValue;
+}
 
 - (RCTModuleRegistry *)moduleRegistry
 {
