From 0f051f603492f97a85e051f112d96180352bba77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=A5kon=20Knutzen?= <2263015+hakonk@users.noreply.github.com> Date: Tue, 16 Jul 2024 12:30:43 -0700 Subject: [PATCH] Add missing lock around assignment of cancellation handler in `RCTImageLoader` (#45454) Summary: When running the entire unit test suite of `RNTesterPods` with `TSan`, I saw that occasionally a data race was detected on line 843 of `RCTImageLoader`. It seems the completion handler that does contain the lock around `cancelLoad` is called concurrently with the value being assigned on line 843. Here there is no lock in place. Here is the output of `TSan` when I comment out my fix: ``` WARNING: ThreadSanitizer: data race (pid=72490) Write of size 8 at 0x000144151ce8 by main thread: #0 -[RCTImageLoader loadImageWithURLRequest:size:scale:clipped:resizeMode:priority:attribution:progressBlock:partialLoadBlock:completionBlock:] (RNTesterUnitTests:arm64+0x16e8030) https://github.com/facebook/react-native/issues/1 -[RCTImageLoader loadImageWithURLRequest:size:scale:clipped:resizeMode:priority:progressBlock:partialLoadBlock:completionBlock:] (RNTesterUnitTests:arm64+0x16df8dc) https://github.com/facebook/react-native/issues/2 -[RCTImageLoader loadImageWithURLRequest:size:scale:clipped:resizeMode:progressBlock:partialLoadBlock:completionBlock:] (RNTesterUnitTests:arm64+0x16df534) https://github.com/facebook/react-native/issues/3 -[RCTImageLoaderTests testImageLoaderUsesImageURLLoaderWithHighestPriority] (RNTesterUnitTests:arm64+0x7cb8) https://github.com/facebook/react-native/issues/4 __invoking___ (CoreFoundation:arm64+0x13371c) Previous write of size 8 at 0x000144151ce8 by thread T4 (mutexes: write M0): #0 __140-[RCTImageLoader loadImageWithURLRequest:size:scale:clipped:resizeMode:priority:attribution:progressBlock:partialLoadBlock:completionBlock:]_block_invoke_2 (RNTesterUnitTests:arm64+0x16e8894) https://github.com/facebook/react-native/issues/1 __139-[RCTImageLoader _loadImageOrDataWithURLRequest:size:scale:resizeMode:priority:attribution:progressBlock:partialLoadBlock:completionBlock:]_block_invoke (RNTesterUnitTests:arm64+0x16e3430) https://github.com/facebook/react-native/issues/2 __139-[RCTImageLoader _loadImageOrDataWithURLRequest:size:scale:resizeMode:priority:attribution:progressBlock:partialLoadBlock:completionBlock:]_block_invoke_3 (RNTesterUnitTests:arm64+0x16e52a8) https://github.com/facebook/react-native/issues/3 __75-[RCTImageLoaderTests testImageLoaderUsesImageURLLoaderWithHighestPriority]_block_invoke_2 (RNTesterUnitTests:arm64+0x7f24) https://github.com/facebook/react-native/issues/4 -[RCTConcreteImageURLLoader loadImageForURL:size:scale:resizeMode:progressHandler:partialLoadHandler:completionHandler:] (RNTesterUnitTests:arm64+0x6c470) https://github.com/facebook/react-native/issues/5 __139-[RCTImageLoader _loadImageOrDataWithURLRequest:size:scale:resizeMode:priority:attribution:progressBlock:partialLoadBlock:completionBlock:]_block_invoke.172 (RNTesterUnitTests:arm64+0x16e4964) https://github.com/facebook/react-native/issues/6 __tsan::invoke_and_release_block(void*) (libclang_rt.tsan_iossim_dynamic.dylib:arm64+0x77ee0) https://github.com/facebook/react-native/issues/7 _dispatch_client_callout (libdispatch.dylib:arm64+0x3974) Location is heap block of size 48 at 0x000144151cc0 allocated by main thread: #0 malloc (libclang_rt.tsan_iossim_dynamic.dylib:arm64+0x4fa48) https://github.com/facebook/react-native/issues/1 _malloc_type_malloc_outlined (libsystem_malloc.dylib:arm64+0xf3ec) https://github.com/facebook/react-native/issues/2 _call_copy_helpers_excp (libsystem_blocks.dylib:arm64+0x10b4) https://github.com/facebook/react-native/issues/3 -[RCTImageLoader loadImageWithURLRequest:size:scale:clipped:resizeMode:priority:progressBlock:partialLoadBlock:completionBlock:] (RNTesterUnitTests:arm64+0x16df8dc) https://github.com/facebook/react-native/issues/4 -[RCTImageLoader loadImageWithURLRequest:size:scale:clipped:resizeMode:progressBlock:partialLoadBlock:completionBlock:] (RNTesterUnitTests:arm64+0x16df534) https://github.com/facebook/react-native/issues/5 -[RCTImageLoaderTests testImageLoaderUsesImageURLLoaderWithHighestPriority] (RNTesterUnitTests:arm64+0x7cb8) https://github.com/facebook/react-native/issues/6 __invoking___ (CoreFoundation:arm64+0x13371c) Mutex M0 (0x000108f316e8) created at: #0 pthread_mutex_init (libclang_rt.tsan_iossim_dynamic.dylib:arm64+0x2cc98) https://github.com/facebook/react-native/issues/1 -[NSLock init] (Foundation:arm64+0x5ca14c) https://github.com/facebook/react-native/issues/2 -[RCTImageLoader loadImageWithURLRequest:size:scale:clipped:resizeMode:priority:progressBlock:partialLoadBlock:completionBlock:] (RNTesterUnitTests:arm64+0x16df8dc) https://github.com/facebook/react-native/issues/3 -[RCTImageLoader loadImageWithURLRequest:size:scale:clipped:resizeMode:progressBlock:partialLoadBlock:completionBlock:] (RNTesterUnitTests:arm64+0x16df534) https://github.com/facebook/react-native/issues/4 -[RCTImageLoaderTests testImageLoaderUsesImageURLLoaderWithHighestPriority] (RNTesterUnitTests:arm64+0x7cb8) https://github.com/facebook/react-native/issues/5 __invoking___ (CoreFoundation:arm64+0x13371c) Thread T4 (tid=10935088, running) is a GCD worker thread ``` ## Changelog: [iOS][Fixed] Data race in `RCTImageLoader` related to assignment of cancellation block. Pull Request resolved: https://github.com/facebook/react-native/pull/45454 Test Plan: There are already tests in place for `RCTImageLoader`. I hope these will cover the fix. Reviewed By: realsoelynn Differential Revision: D59816000 Pulled By: zeyap fbshipit-source-id: f959d472eb60f83f39ced6711ee395949ab37e7c --- packages/react-native/Libraries/Image/RCTImageLoader.mm | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/react-native/Libraries/Image/RCTImageLoader.mm b/packages/react-native/Libraries/Image/RCTImageLoader.mm index dac4ecc5fe40ac..d6db050e35c36e 100644 --- a/packages/react-native/Libraries/Image/RCTImageLoader.mm +++ b/packages/react-native/Libraries/Image/RCTImageLoader.mm @@ -840,7 +840,9 @@ - (RCTImageURLLoaderRequest *)loadImageWithURLRequest:(NSURLRequest *)imageURLRe progressBlock:progressBlock partialLoadBlock:partialLoadBlock completionBlock:completionHandler]; + [cancelLoadLock lock]; cancelLoad = loaderRequest.cancellationBlock; + [cancelLoadLock unlock]; return [[RCTImageURLLoaderRequest alloc] initWithRequestId:loaderRequest.requestId imageURL:imageURLRequest.URL cancellationBlock:cancellationBlock];