[iOS][Fixed] Data race related to reading/writing to AllocationTestModule.valid

[hakonk]

The fix entails making AllocationTestModule.valid an Objective-C atomic property and funneling access to the ivar via the synthesized property getter and setter.

Summary:

While the data race was present in test code, it would make it more difficult to spot more severe data races with the TSan. Also, getting rid of a data race is always good.

Changelog:

[iOS][Fixed] - Data race related to access of AllocationTestModule.valid

Test Plan:

RCTAllocationTests will test the implementation of AllocationTestModule.

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	20,315,971	-1,037
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	23,513,287	-362
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: e320ab4
Branch: main

[javache]

It's unclear what problem this PR is solving,. If access to this property needs to be atomic (but it's unclear why), consider using @property (atomic, [...] instead of the current nonatomic.

[hakonk]

Running this test with TSan enabled and runtime issues breakpoint in Xcode will reveal concurrent read/write to the mentioned property, which is in this case classified as a data race by TSan. I haven't studied the reasons for concurrent access, but it seems to be related to code fundamental to how RCTBridge works.

That said, I can certainly alter the proposed changes to make use of the atomic property attribute. Will come back to you soon with suggested changes.

[hakonk]

Here is the TSan output when valid is not atomic:

==================
WARNING: ThreadSanitizer: data race (pid=75074)
  Write of size 1 at 0x000106a04348 by thread T8:
    #0 -[AllocationTestModule setValid:] <null> (RNTesterUnitTests:arm64+0x582c0)
    #1 -[AllocationTestModule invalidate] <null> (RNTesterUnitTests:arm64+0x58034)
    #2 __26-[RCTCxxBridge invalidate]_block_invoke_2 <null> (RNTesterUnitTests:arm64+0x41adf4)
    #3 __tsan::invoke_and_release_block(void*) <null> (libclang_rt.tsan_iossim_dynamic.dylib:arm64+0x77ee0)
    #4 _dispatch_client_callout <null> (libdispatch.dylib:arm64+0x3974)

  Previous read of size 1 at 0x000106a04348 by main thread:
    #0 -[AllocationTestModule isValid] <null> (RNTesterUnitTests:arm64+0x581b4)
    #1 -[RCTAllocationTests testModulesAreInvalidated] <null> (RNTesterUnitTests:arm64+0x59d60)
    #2 __invoking___ <null> (CoreFoundation:arm64+0x13371c)

  Location is heap block of size 16 at 0x000106a04340 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 __invoking___ <null> (CoreFoundation:arm64+0x13371c)

  Thread T8 (tid=9283099, running) is a GCD worker thread

Seems like it is related to invalidating the bridge.

I have now resorted to making use of an atomic Objective-C property. Please see the changes @javache

[javache]

Thanks! Please update the PR description to make it reflect the changes you've made.

[facebook-github-bot]

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

[facebook-github-bot]

@javache merged this pull request in dba25fa.

[github-actions]

This pull request was successfully merged by @hakonk in dba25fa.

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