Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[iOS][Fixed] Use std::atomic for eliminating races in RCTCxxBridge #45558

Closed
wants to merge 1 commit into from

Conversation

hakonk
Copy link
Contributor

@hakonk hakonk commented Jul 20, 2024

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 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.

Test Plan:

I believe there are existing Unit tests in place for verifying this fix.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Jul 20, 2024
@hakonk hakonk changed the title [iOS][Fixed] Use std::atomic for reliminating races in RCTCxxBridge [iOS][Fixed] Use std::atomic for eliminating races in RCTCxxBridge Jul 20, 2024
@facebook-github-bot
Copy link
Contributor

@dmytrorykun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@dmytrorykun merged this pull request in 8204134.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Jul 25, 2024
@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @hakonk in 8204134

When will my fix make it into a release? | How to file a pick request?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants