-
Notifications
You must be signed in to change notification settings - Fork 24.2k
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] Data race in ReactCxxBridge.loading
#45280
Labels
Resolution: Fixed
A PR that fixes this issue has been merged.
Comments
ReactCxxBridge.loading
ReactCxxBridge.loading
This was referenced Jul 20, 2024
facebook-github-bot
pushed a commit
that referenced
this issue
Jul 25, 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 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
facebook-github-bot
pushed a commit
that referenced
this issue
Aug 12, 2024
…#45557) Summary: When using `TSan` while running the Unit tests of `RNTester`, there are a few data races picked up. One is described [here](#45280), while this PR deals with a race related to concurrent read/write of `ReactMarker::logTaggedMarkerImpl`. Here is the `TSan` output: ``` WARNING: ThreadSanitizer: data race (pid=5236) Read of size 8 at 0x00011a602690 by thread T34: #0 std::__1::__function::__value_func<void (facebook::react::ReactMarker::ReactMarkerId, char const*)>::operator bool[abi:ue170006]() const <null> (RNTesterUnitTests:arm64+0x18cd49c) #1 std::__1::function<void (facebook::react::ReactMarker::ReactMarkerId, char const*)>::operator bool[abi:ue170006]() const <null> (RNTesterUnitTests:arm64+0x18cd2bc) #2 facebook::react::JSIExecutor::initializeRuntime() <null> (RNTesterUnitTests:arm64+0x1c96818) #3 facebook::react::NativeToJsBridge::initializeRuntime()::$_0::operator()(facebook::react::JSExecutor*) <null> (RNTesterUnitTests:arm64+0x1a7a074) #4 decltype(std::declval<facebook::react::NativeToJsBridge::initializeRuntime()::$_0&>()(std::declval<facebook::react::JSExecutor*>())) std::__1::__invoke[abi:ue170006]<facebook::react::NativeToJsBridge::initializeRuntime()::$_0&, facebook::react::JSExecutor*>(facebook::react::NativeToJsBridge::initializeRuntime()::$_0&, facebook::react::JSExecutor*&&) <null> (RNTesterUnitTests:arm64+0x1a79fbc) #5 void std::__1::__invoke_void_return_wrapper<void, true>::__call[abi:ue170006]<facebook::react::NativeToJsBridge::initializeRuntime()::$_0&, facebook::react::JSExecutor*>(facebook::react::NativeToJsBridge::initializeRuntime()::$_0&, facebook::react::JSExecutor*&&) <null> (RNTesterUnitTests:arm64+0x1a79e5c) #6 std::__1::__function::__alloc_func<facebook::react::NativeToJsBridge::initializeRuntime()::$_0, std::__1::allocator<facebook::react::NativeToJsBridge::initializeRuntime()::$_0>, void (facebook::react::JSExecutor*)>::operator()[abi:ue170006](facebook::react::JSExecutor*&&) <null> (RNTesterUnitTests:arm64+0x1a79d84) #7 std::__1::__function::__func<facebook::react::NativeToJsBridge::initializeRuntime()::$_0, std::__1::allocator<facebook::react::NativeToJsBridge::initializeRuntime()::$_0>, void (facebook::react::JSExecutor*)>::operator()(facebook::react::JSExecutor*&&) <null> (RNTesterUnitTests:arm64+0x1a75250) #8 std::__1::__function::__value_func<void (facebook::react::JSExecutor*)>::operator()[abi:ue170006](facebook::react::JSExecutor*&&) const <null> (RNTesterUnitTests:arm64+0x1abac9c) #9 std::__1::function<void (facebook::react::JSExecutor*)>::operator()(facebook::react::JSExecutor*) const <null> (RNTesterUnitTests:arm64+0x1aba9d0) #10 facebook::react::NativeToJsBridge::runOnExecutorQueue(std::__1::function<void (facebook::react::JSExecutor*)>&&)::$_8::operator()() const <null> (RNTesterUnitTests:arm64+0x1aba8d4) #11 decltype(std::declval<facebook::react::NativeToJsBridge::runOnExecutorQueue(std::__1::function<void (facebook::react::JSExecutor*)>&&)::$_8&>()()) std::__1::__invoke[abi:ue170006]<facebook::react::NativeToJsBridge::runOnExecutorQueue(std::__1::function<void (facebook::react::JSExecutor*)>&&)::$_8&>(facebook::react::NativeToJsBridge::runOnExecutorQueue(std::__1::function<void (facebook::react::JSExecutor*)>&&)::$_8&) <null> (RNTesterUnitTests:arm64+0x1aba6d4) #12 void std::__1::__invoke_void_return_wrapper<void, true>::__call[abi:ue170006]<facebook::react::NativeToJsBridge::runOnExecutorQueue(std::__1::function<void (facebook::react::JSExecutor*)>&&)::$_8&>(facebook::react::NativeToJsBridge::runOnExecutorQueue(std::__1::function<void (facebook::react::JSExecutor*)>&&)::$_8&) <null> (RNTesterUnitTests:arm64+0x1aba4f8) #13 std::__1::__function::__alloc_func<facebook::react::NativeToJsBridge::runOnExecutorQueue(std::__1::function<void (facebook::react::JSExecutor*)>&&)::$_8, std::__1::allocator<facebook::react::NativeToJsBridge::runOnExecutorQueue(std::__1::function<void (facebook::react::JSExecutor*)>&&)::$_8>, void ()>::operator()[abi:ue170006]() <null> (RNTesterUnitTests:arm64+0x1aba45c) #14 std::__1::__function::__func<facebook::react::NativeToJsBridge::runOnExecutorQueue(std::__1::function<void (facebook::react::JSExecutor*)>&&)::$_8, std::__1::allocator<facebook::react::NativeToJsBridge::runOnExecutorQueue(std::__1::function<void (facebook::react::JSExecutor*)>&&)::$_8>, void ()>::operator()() <null> (RNTesterUnitTests:arm64+0x1ab4918) #15 std::__1::__function::__value_func<void ()>::operator()[abi:ue170006]() const <null> (RNTesterUnitTests:arm64+0x3ce2e4) #16 std::__1::function<void ()>::operator()() const <null> (RNTesterUnitTests:arm64+0x3cdfd0) #17 facebook::react::tryAndReturnError(std::__1::function<void ()> const&) <null> (RNTesterUnitTests:arm64+0x4af18c) #18 facebook::react::RCTMessageThread::tryFunc(std::__1::function<void ()> const&) <null> (RNTesterUnitTests:arm64+0x51595c) #19 facebook::react::RCTMessageThread::runOnQueue(std::__1::function<void ()>&&)::$_1::operator()() const <null> (RNTesterUnitTests:arm64+0x529df0) #20 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) #21 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) #22 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) #23 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) #24 std::__1::__function::__value_func<void ()>::operator()[abi:ue170006]() const <null> (RNTesterUnitTests:arm64+0x3ce2e4) #25 std::__1::function<void ()>::operator()() const <null> (RNTesterUnitTests:arm64+0x3cdfd0) #26 invocation function for block in facebook::react::RCTMessageThread::runAsync(std::__1::function<void ()>) <null> (RNTesterUnitTests:arm64+0x515384) #27 __CFRUNLOOP_IS_CALLING_OUT_TO_A_BLOCK__ <null> (CoreFoundation:arm64+0x8dc0c) #28 __NSThread__start__ <null> (Foundation:arm64+0x645c60) Previous write of size 8 at 0x00011a602690 by main thread: #0 std::__1::__function::__value_func<void (facebook::react::ReactMarker::ReactMarkerId, char const*)>::swap[abi:ue170006](std::__1::__function::__value_func<void (facebook::react::ReactMarker::ReactMarkerId, char const*)>&) <null> (RNTesterUnitTests:arm64+0x43b078) #1 std::__1::function<void (facebook::react::ReactMarker::ReactMarkerId, char const*)>::swap(std::__1::function<void (facebook::react::ReactMarker::ReactMarkerId, char const*)>&) <null> (RNTesterUnitTests:arm64+0x433100) #2 std::__1::function<void (facebook::react::ReactMarker::ReactMarkerId, char const*)>& std::__1::function<void (facebook::react::ReactMarker::ReactMarkerId, char const*)>::operator=<registerPerformanceLoggerHooks(RCTPerformanceLogger*)::$_1, void>(registerPerformanceLoggerHooks(RCTPerformanceLogger*)::$_1&&) <null> (RNTesterUnitTests:arm64+0x432d50) #3 registerPerformanceLoggerHooks(RCTPerformanceLogger*) <null> (RNTesterUnitTests:arm64+0x4170fc) #4 -[RCTCxxBridge initWithParentBridge:] <null> (RNTesterUnitTests:arm64+0x416504) #5 -[RCTBridge setUp] <null> (RNTesterUnitTests:arm64+0x3bf6f4) #6 -[RCTBridge initWithDelegate:bundleURL:moduleProvider:launchOptions:] <null> (RNTesterUnitTests:arm64+0x3bc540) #7 -[RCTBridge initWithBundleURL:moduleProvider:launchOptions:] <null> (RNTesterUnitTests:arm64+0x3bc124) #8 -[RCTImageLoaderTests testImageLoaderUsesImageURLLoaderWithHighestPriority] <null> (RNTesterUnitTests:arm64+0x7de8) #9 __invoking___ <null> (CoreFoundation:arm64+0x13371c) Location is global 'facebook::react::ReactMarker::logTaggedMarkerImpl' at 0x00011a602678 (RNTesterUnitTests+0x438a690) Thread T34 (tid=11229216, 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 -[RCTImageLoaderTests testImageLoaderUsesImageDecoderWithHighestPriority] <null> (RNTesterUnitTests:arm64+0xbe8c) #6 __invoking___ <null> (CoreFoundation:arm64+0x13371c) ``` The proposed solution is to wrap `logTaggedMarkerImpl` in a class that has a static getter and setter wherein a read/write lock is employed. It is my understanding that `logTaggedMarkerImpl` is read several times, but only assigned rarely, and thus it seems appropriate with a read/write lock. The getter and setter functions are also inlineable, such that one should not need to make an extra function call when obtaining the `logTaggedMarkerImpl` instance. In order to reproduce my findings and verify fix: * Clone this branch * Run setup code as described in README * Execute `git revert -n 6599883 83a2a3c` * 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 `logTaggedMarkerImpl`. 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. ## Changelog: [iOS][Fixed] Data race related to read/write on `ReactMarker::logTaggedMarkerImpl` Pull Request resolved: #45557 Test Plan: I believe there are existing tests that will cover the proposed changes. Reviewed By: cipolleschi Differential Revision: D60525080 Pulled By: dmytrorykun fbshipit-source-id: 78b0ce2a660af0e29909ff68c018698a9a1e29f8
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Description
RCTCxxBridge.Loading
is not thread safe. This is problematic as it is accessed on the JS thread, and while this example only shows it's accessed as well on the main thread in a test, it is also accessed e.g. here (RCTRootView.m:314-327
).Thus, two threads can simultaneously access it in production.
Steps to reproduce
RNTesterPods.xcworkspace
RNTester
andRNTesterUnitTests
RCTComponentPropsTests
with Runtime issue breakpoint enabled.React Native Version
0.74.3
Affected Platforms
Runtime - iOS
Output of
npx react-native info
Stacktrace or Logs
Reproducer
https://github.com/hakonk/react-native/#main
Screenshots and Videos
No response
The text was updated successfully, but these errors were encountered: