Skip to content

Commit

Permalink
Use std::atomic for eliminating races in RCTCxxBridge (#45558)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
hakonk authored and facebook-github-bot committed Jul 25, 2024
1 parent ffa6809 commit 8204134
Showing 1 changed file with 23 additions and 3 deletions.
26 changes: 23 additions & 3 deletions packages/react-native/React/CxxBridge/RCTCxxBridge.mm
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ void onBatchComplete() override

@implementation RCTCxxBridge {
BOOL _didInvalidate;
BOOL _moduleRegistryCreated;
std::atomic<BOOL> _moduleRegistryCreated;

NSMutableArray<RCTPendingCall> *_pendingCalls;
std::atomic<NSInteger> _pendingCount;
Expand All @@ -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
{
Expand Down

0 comments on commit 8204134

Please sign in to comment.