[iOS][Fix] Add missing lock around assignment of cancellation handler in RCTImageLoader

[hakonk]

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.

Test Plan:

There are already tests in place for RCTImageLoader. I hope these will cover the fix.

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	21,367,499	-83,322
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	24,564,231	-82,984
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: 944d40f
Branch: main

[facebook-github-bot]

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

[facebook-github-bot]

@zeyap merged this pull request in 0f051f6.

[github-actions]

This pull request was successfully merged by @hakonk in 0f051f6

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