From 8204134024e81947969bd8c2ba337dc931e54bd9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=A5kon=20Knutzen?= <2263015+hakonk@users.noreply.github.com> Date: Thu, 25 Jul 2024 10:22:56 -0700 Subject: [PATCH] Use std::atomic for eliminating races in RCTCxxBridge (#45558) Summary: As explained in https://github.com/facebook/react-native/issues/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] (RNTesterUnitTests:arm64+0x42b484) https://github.com/facebook/react-native/issues/1 __53-[RCTCxxBridge executeSourceCode:withSourceURL:sync:]_block_invoke (RNTesterUnitTests:arm64+0x426050) https://github.com/facebook/react-native/issues/2 decltype(std::declval()()) std::__1::__invoke[abi:ue170006](&&, decltype(std::declval()())&&...) (RNTesterUnitTests:arm64+0x456298) https://github.com/facebook/react-native/issues/3 std::__1::__function::__func, void ()>::operator()() (RNTesterUnitTests:arm64+0x455c6c) https://github.com/facebook/react-native/issues/4 std::__1::__function::__value_func::operator()[abi:ue170006]() const (RNTesterUnitTests:arm64+0x3ce2e4) https://github.com/facebook/react-native/issues/5 std::__1::function::operator()() const (RNTesterUnitTests:arm64+0x3cdfd0) https://github.com/facebook/react-native/issues/6 facebook::react::tryAndReturnError(std::__1::function const&) (RNTesterUnitTests:arm64+0x4af18c) https://github.com/facebook/react-native/issues/7 facebook::react::RCTMessageThread::tryFunc(std::__1::function const&) (RNTesterUnitTests:arm64+0x51595c) https://github.com/facebook/react-native/issues/8 facebook::react::RCTMessageThread::runOnQueue(std::__1::function&&)::$_1::operator()() const (RNTesterUnitTests:arm64+0x529df0) https://github.com/facebook/react-native/issues/9 decltype(std::declval&&)::$_1&>()()) std::__1::__invoke[abi:ue170006]&&)::$_1&>(facebook::react::RCTMessageThread::runOnQueue(std::__1::function&&)::$_1&) (RNTesterUnitTests:arm64+0x529b54) https://github.com/facebook/react-native/issues/10 void std::__1::__invoke_void_return_wrapper::__call[abi:ue170006]&&)::$_1&>(facebook::react::RCTMessageThread::runOnQueue(std::__1::function&&)::$_1&) (RNTesterUnitTests:arm64+0x529978) https://github.com/facebook/react-native/issues/11 std::__1::__function::__alloc_func&&)::$_1, std::__1::allocator&&)::$_1>, void ()>::operator()[abi:ue170006]() (RNTesterUnitTests:arm64+0x5298dc) https://github.com/facebook/react-native/issues/12 std::__1::__function::__func&&)::$_1, std::__1::allocator&&)::$_1>, void ()>::operator()() (RNTesterUnitTests:arm64+0x524518) https://github.com/facebook/react-native/issues/13 std::__1::__function::__value_func::operator()[abi:ue170006]() const (RNTesterUnitTests:arm64+0x3ce2e4) https://github.com/facebook/react-native/issues/14 std::__1::function::operator()() const (RNTesterUnitTests:arm64+0x3cdfd0) https://github.com/facebook/react-native/issues/15 invocation function for block in facebook::react::RCTMessageThread::runAsync(std::__1::function) (RNTesterUnitTests:arm64+0x515384) https://github.com/facebook/react-native/issues/16 __CFRUNLOOP_IS_CALLING_OUT_TO_A_BLOCK__ (CoreFoundation:arm64+0x8dc0c) https://github.com/facebook/react-native/issues/17 __NSThread__start__ (Foundation:arm64+0x645c60) Previous read of size 1 at 0x00010af1dfd8 by main thread: #0 -[RCTCxxBridge isLoading] (RNTesterUnitTests:arm64+0x43236c) https://github.com/facebook/react-native/issues/1 -[RCTBridge isLoading] (RNTesterUnitTests:arm64+0x3c0170) https://github.com/facebook/react-native/issues/2 -[RCTComponentPropsTests setUp] (RNTesterUnitTests:arm64+0xe6f34) https://github.com/facebook/react-native/issues/3 __70-[XCTestCase _shouldContinueAfterPerformingSetUpSequenceWithSelector:]_block_invoke.134 (XCTestCore:arm64+0x540d8) Location is heap block of size 384 at 0x00010af1de80 allocated by main thread: #0 calloc (libclang_rt.tsan_iossim_dynamic.dylib:arm64+0x4fc30) https://github.com/facebook/react-native/issues/1 _malloc_type_calloc_outlined (libsystem_malloc.dylib:arm64+0xf488) https://github.com/facebook/react-native/issues/2 -[RCTBridge initWithDelegate:bundleURL:moduleProvider:launchOptions:] (RNTesterUnitTests:arm64+0x3bc540) https://github.com/facebook/react-native/issues/3 -[RCTBridge initWithBundleURL:moduleProvider:launchOptions:] (RNTesterUnitTests:arm64+0x3bc124) https://github.com/facebook/react-native/issues/4 -[RCTComponentPropsTests setUp] (RNTesterUnitTests:arm64+0xe6b3c) https://github.com/facebook/react-native/issues/5 __70-[XCTestCase _shouldContinueAfterPerformingSetUpSequenceWithSelector:]_block_invoke.134 (XCTestCore:arm64+0x540d8) Thread T13 (tid=11290378, running) created by main thread at: #0 pthread_create (libclang_rt.tsan_iossim_dynamic.dylib:arm64+0x2bee4) https://github.com/facebook/react-native/issues/1 -[NSThread startAndReturnError:] (Foundation:arm64+0x6458f0) https://github.com/facebook/react-native/issues/2 -[RCTBridge setUp] (RNTesterUnitTests:arm64+0x3bf748) https://github.com/facebook/react-native/issues/3 -[RCTBridge initWithDelegate:bundleURL:moduleProvider:launchOptions:] (RNTesterUnitTests:arm64+0x3bc540) https://github.com/facebook/react-native/issues/4 -[RCTBridge initWithBundleURL:moduleProvider:launchOptions:] (RNTesterUnitTests:arm64+0x3bc124) https://github.com/facebook/react-native/issues/5 -[RCTComponentPropsTests setUp] (RNTesterUnitTests:arm64+0xe6b3c) https://github.com/facebook/react-native/issues/6 __70-[XCTestCase _shouldContinueAfterPerformingSetUpSequenceWithSelector:]_block_invoke.134 (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 11c09fdc7c442dd694909bebbbc8f21c3e69edf2`. * 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: https://github.com/facebook/react-native/pull/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 --- .../React/CxxBridge/RCTCxxBridge.mm | 26 ++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/packages/react-native/React/CxxBridge/RCTCxxBridge.mm b/packages/react-native/React/CxxBridge/RCTCxxBridge.mm index 044862f821c2a2..dc19bd2eb142b5 100644 --- 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 _moduleRegistryCreated; NSMutableArray *_pendingCalls; std::atomic _pendingCount; @@ -220,12 +220,32 @@ @implementation RCTCxxBridge { RCTViewRegistry *_viewRegistry_DEPRECATED; RCTBundleManager *_bundleManager; RCTCallableJSModules *_callableJSModules; + std::atomic _loading; + std::atomic _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 {