[iOS] Fix data races in RCTImageLoader and RCTNetworkTask with shared atomic counters

[hakonk]

Summary:

In order to fix the data races described in #44715, I propose a simple solution by leveraging shared counter functions wherein std::atomic is the backing for the integer values.

Changelog:

[iOS] [Fixed] - Implement shared atomic counters and replace static integers in RCTImageLoader and RCTNetworkTask that were accessed concurrently, which in some cases lead to data races.

Test Plan:

Added unit tests for the counters in RCTSharedCounterTests.

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	20,315,962	-1,046
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	23,513,278	-371
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: e320ab4
Branch: main

[cipolleschi]

Great catch and smart solution!
I left some questions and a few minor changes requested.

[cipolleschi]

This change should be reverted.

[hakonk]

8c7547e

[cipolleschi]

I like the approach, but I'm a bit confused by the API namig.
We have an API that creates an atomicCounter and that returns a function that returns the counter an increment it.

It's a bit confusing that we use the RCTCreateAtomicNSUIntegerCounter to actually return a function that is getAndIncrementCounter.
By calling RCTCreateAtomicNSUIntegerCounter I expect to get a counter as a result not a function to increment a counter. 🤔

Can we think of a better name for this API?

[hakonk]

In the case of RCTNetworkTask:24, we create a static counter like so:

static const auto generateRequestId = RCTCreateAtomicNSUIntegerCounter();

That is, the counter function is not static unless it is held statically. In turn, calling on generateRequestId will yield an incremented value. I believe it is more appropriate to name the variable that holds the block generateRequestId, as calling this block indeed increments the counter, while RCTCreateAtomicNSUIntegerCounter creates a new counter block where count is initialized to 0 for each call.

[cipolleschi]

I think that the approach is very clever, but talking about memory management, when is the std::atomic destroyed?

Because as far as I can see, the count is captured by the closure. So I'd say that when there are no closures that keep the reference alive, the shared pointer will be deleted. Correct?

Can we add a test to make sure that this is actually happening?

[hakonk]

My understanding is that this is correct. I have attempted to allow for testing this via this commit

[hakonk]

Hi @cipolleschi, thanks for the feedback! I will look into the comments and think of improvements. Will push changes when I have the time.

[hakonk]

@cipolleschi please see the proposed changes above

[cipolleschi]

Thanks for verifying that the memory is deallocated.
For maintainability I'd rather remove the spy code, so the code is clearer and easier to understand and maintain.
Thanks for putting extra work in verifying the memory management! ❤️

[cipolleschi]

(also, remember to rebase on top of main, please! 🙏 )

[javache]

Since we already use Objective-C++ quite widely across the codebase, can we just replace this with std::atomic?

[hakonk]

Since we already use Objective-C++ quite widely across the codebase, can we just replace this with std::atomic?

We certainly can. My approach was to generalize the solution, seeing it as one could make use of atomic counter functions for cases to come. I believe testing becomes easier as well, as one does not have to for example instantiate a class wherein a static count variable is used.

Is it more preferable from your point of view not to introduce the counters?

[javache]

I'd rather rely on standard C++ fundamentals and use those directly, rather than using wrappers which involve shared_ptr, which have their own.

In pure Objective-C, we should also be able to use OSAtomicIncrement64 as a much simpler solution.

[hakonk]

I'd rather rely on standard C++ fundamentals and use those directly, rather than using wrappers which involve shared_ptr, which have their own.

In pure Objective-C, we should also be able to use OSAtomicIncrement64 as a much simpler solution.

@javache I have revised according to your review. Please see changes.

[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 ffc16fc.

[github-actions]

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

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