Synchronize access to dict in FLEXNetworkRecorder (#3457)

Summary:
All our read/write to `identifierDict` in FLEXNetworkRecorder are done in their own thread-safe queue except one read and one write call.

Those two non-thread-safe read/write calls are causing ThreadSanitizer crashes in `react-native-macOS` when we consume flipper. By adding these calls to the `dispatch_async(self.queue` a few lines lower in the method, their access becomes thread safe as well.

Never having worked on Flipper I don't have much context on what FLEXNetworkRecorder actually does and the git history for this file shows the bug has existed since the "Initial commit" so it's unclear to me if we've purposefully not included these calls in the dispatch queue for some reason.

That said, I'd propose this as a fix as the thread sanitizing crash no longer repros for me downstream and I don't see anything immediately obvious for why this can't be in the self.queue as well.

## Changelog

Fix thread sanitizer crash in FLEXNetworkRecorder.

Pull Request resolved: https://github.com/facebook/flipper/pull/3457

Test Plan:
Running react-native-macOS with the ThreadSanitizer consistently hits this race condition on a library object in Flipper with the error below.

```
WARNING: ThreadSanitizer: race on NSMutableDictionary (pid=32575)
  Read-only access of NSMutableDictionary at 0x000133ae5c60 by thread T11:
    #0 -[__NSDictionaryM objectForKeyedSubscript:] <null>:130036204 (CoreFoundation:arm64+0x1897d8)
    https://github.com/facebook/flipper/issues/1 __85-[FLEXNetworkRecorder recordRequestWillBeSentWithRequestID:request:redirectResponse:]_block_invoke FLEXNetworkRecorder.mm:130 (RNTester:arm64+0x1007afc48)
    https://github.com/facebook/flipper/issues/2 __tsan::invoke_and_release_block(void*) <null>:130036204 (libclang_rt.tsan_iossim_dynamic.dylib:arm64+0x70514)
    https://github.com/facebook/flipper/issues/3 _dispatch_client_callout <null>:130036204 (libdispatch.dylib:arm64+0x581c)

  Previous modifying access of NSMutableDictionary at 0x000133ae5c60 by thread T1:
    #0 -[__NSDictionaryM setObject:forKeyedSubscript:] <null>:130036204 (CoreFoundation:arm64+0x188808)
    https://github.com/facebook/flipper/issues/1 -[FLEXNetworkRecorder recordRequestWillBeSentWithRequestID:request:redirectResponse:] FLEXNetworkRecorder.mm:118 (RNTester:arm64+0x1007af754)
    https://github.com/facebook/flipper/issues/2 __73-[FLEXNetworkObserver(NSURLSessionTaskHelpers) URLSessionTaskWillResume:]_block_invoke FLEXNetworkObserver.mm:1690 (RNTester:arm64+0x1007adc70)
    https://github.com/facebook/flipper/issues/3 __tsan::invoke_and_release_block(void*) <null>:130036204 (libclang_rt.tsan_iossim_dynamic.dylib:arm64+0x70514)
    https://github.com/facebook/flipper/issues/4 _dispatch_client_callout <null>:130036204 (libdispatch.dylib:arm64+0x581c)
```
After moving the only non-thread safe read/write call into the appropriate dispatch queues I'm not longer able to repro this thread access crash after many attempts.

Reviewed By: fabiomassimo

Differential Revision: D34388079

Pulled By: lblasa

fbshipit-source-id: 2e654d601bc6a7d8d78d9a824e0aee66889b7fb9
This commit is contained in:
Chris Hogan
2022-02-22 10:00:28 -08:00
committed by Facebook GitHub Bot
parent bed23c586b
commit 4c91382e09

View File

@@ -114,10 +114,11 @@ NSString* const kFLEXNetworkRecorderResponseCacheLimitDefaultsKey =
- (void)recordRequestWillBeSentWithRequestID:(NSString*)requestID - (void)recordRequestWillBeSentWithRequestID:(NSString*)requestID
request:(NSURLRequest*)request request:(NSURLRequest*)request
redirectResponse:(NSURLResponse*)redirectResponse { redirectResponse:(NSURLResponse*)redirectResponse {
NSDate* requestDate = [NSDate date];
dispatch_async(self.queue, ^{
if (![self.identifierDict objectForKey:requestID]) { if (![self.identifierDict objectForKey:requestID]) {
self.identifierDict[requestID] = [NSNumber random]; self.identifierDict[requestID] = [NSNumber random];
} }
NSDate* startDate = [NSDate date];
if (redirectResponse) { if (redirectResponse) {
[self recordResponseReceivedWithRequestID:requestID [self recordResponseReceivedWithRequestID:requestID
@@ -125,7 +126,6 @@ NSString* const kFLEXNetworkRecorderResponseCacheLimitDefaultsKey =
[self recordLoadingFinishedWithRequestID:requestID responseBody:nil]; [self recordLoadingFinishedWithRequestID:requestID responseBody:nil];
} }
dispatch_async(self.queue, ^{
SKRequestInfo* info = [[SKRequestInfo alloc] SKRequestInfo* info = [[SKRequestInfo alloc]
initWithIdentifier:self.identifierDict[requestID].longLongValue initWithIdentifier:self.identifierDict[requestID].longLongValue
timestamp:[NSDate timestamp] timestamp:[NSDate timestamp]
@@ -136,7 +136,7 @@ NSString* const kFLEXNetworkRecorderResponseCacheLimitDefaultsKey =
FLEXNetworkTransaction* transaction = [FLEXNetworkTransaction new]; FLEXNetworkTransaction* transaction = [FLEXNetworkTransaction new];
transaction.requestID = requestID; transaction.requestID = requestID;
transaction.request = request; transaction.request = request;
transaction.startTime = startDate; transaction.startTime = requestDate;
[self.orderedTransactions insertObject:transaction atIndex:0]; [self.orderedTransactions insertObject:transaction atIndex:0];
[self.networkTransactionsForRequestIdentifiers setObject:transaction [self.networkTransactionsForRequestIdentifiers setObject:transaction