Skip to content

Commit

Permalink
Add missing lock around assignment of cancellation handler in `RCTIma…
Browse files Browse the repository at this point in the history
…geLoader` (#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:] <null> (RNTesterUnitTests:arm64+0x16e8030)
    #1 -[RCTImageLoader loadImageWithURLRequest:size:scale:clipped:resizeMode:priority:progressBlock:partialLoadBlock:completionBlock:] <null> (RNTesterUnitTests:arm64+0x16df8dc)
    #2 -[RCTImageLoader loadImageWithURLRequest:size:scale:clipped:resizeMode:progressBlock:partialLoadBlock:completionBlock:] <null> (RNTesterUnitTests:arm64+0x16df534)
    #3 -[RCTImageLoaderTests testImageLoaderUsesImageURLLoaderWithHighestPriority] <null> (RNTesterUnitTests:arm64+0x7cb8)
    #4 __invoking___ <null> (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 <null> (RNTesterUnitTests:arm64+0x16e8894)
    #1 __139-[RCTImageLoader _loadImageOrDataWithURLRequest:size:scale:resizeMode:priority:attribution:progressBlock:partialLoadBlock:completionBlock:]_block_invoke <null> (RNTesterUnitTests:arm64+0x16e3430)
    #2 __139-[RCTImageLoader _loadImageOrDataWithURLRequest:size:scale:resizeMode:priority:attribution:progressBlock:partialLoadBlock:completionBlock:]_block_invoke_3 <null> (RNTesterUnitTests:arm64+0x16e52a8)
    #3 __75-[RCTImageLoaderTests testImageLoaderUsesImageURLLoaderWithHighestPriority]_block_invoke_2 <null> (RNTesterUnitTests:arm64+0x7f24)
    #4 -[RCTConcreteImageURLLoader loadImageForURL:size:scale:resizeMode:progressHandler:partialLoadHandler:completionHandler:] <null> (RNTesterUnitTests:arm64+0x6c470)
    #5 __139-[RCTImageLoader _loadImageOrDataWithURLRequest:size:scale:resizeMode:priority:attribution:progressBlock:partialLoadBlock:completionBlock:]_block_invoke.172 <null> (RNTesterUnitTests:arm64+0x16e4964)
    #6 __tsan::invoke_and_release_block(void*) <null> (libclang_rt.tsan_iossim_dynamic.dylib:arm64+0x77ee0)
    #7 _dispatch_client_callout <null> (libdispatch.dylib:arm64+0x3974)

  Location is heap block of size 48 at 0x000144151cc0 allocated by main thread:
    #0 malloc <null> (libclang_rt.tsan_iossim_dynamic.dylib:arm64+0x4fa48)
    #1 _malloc_type_malloc_outlined <null> (libsystem_malloc.dylib:arm64+0xf3ec)
    #2 _call_copy_helpers_excp <null> (libsystem_blocks.dylib:arm64+0x10b4)
    #3 -[RCTImageLoader loadImageWithURLRequest:size:scale:clipped:resizeMode:priority:progressBlock:partialLoadBlock:completionBlock:] <null> (RNTesterUnitTests:arm64+0x16df8dc)
    #4 -[RCTImageLoader loadImageWithURLRequest:size:scale:clipped:resizeMode:progressBlock:partialLoadBlock:completionBlock:] <null> (RNTesterUnitTests:arm64+0x16df534)
    #5 -[RCTImageLoaderTests testImageLoaderUsesImageURLLoaderWithHighestPriority] <null> (RNTesterUnitTests:arm64+0x7cb8)
    #6 __invoking___ <null> (CoreFoundation:arm64+0x13371c)

  Mutex M0 (0x000108f316e8) created at:
    #0 pthread_mutex_init <null> (libclang_rt.tsan_iossim_dynamic.dylib:arm64+0x2cc98)
    #1 -[NSLock init] <null> (Foundation:arm64+0x5ca14c)
    #2 -[RCTImageLoader loadImageWithURLRequest:size:scale:clipped:resizeMode:priority:progressBlock:partialLoadBlock:completionBlock:] <null> (RNTesterUnitTests:arm64+0x16df8dc)
    #3 -[RCTImageLoader loadImageWithURLRequest:size:scale:clipped:resizeMode:progressBlock:partialLoadBlock:completionBlock:] <null> (RNTesterUnitTests:arm64+0x16df534)
    #4 -[RCTImageLoaderTests testImageLoaderUsesImageURLLoaderWithHighestPriority] <null> (RNTesterUnitTests:arm64+0x7cb8)
    #5 __invoking___ <null> (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: #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
  • Loading branch information
hakonk authored and facebook-github-bot committed Jul 16, 2024
1 parent fb9ce76 commit 0f051f6
Showing 1 changed file with 2 additions and 0 deletions.
2 changes: 2 additions & 0 deletions packages/react-native/Libraries/Image/RCTImageLoader.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down

0 comments on commit 0f051f6

Please sign in to comment.