review-comment: Synchronize access to shared network details data

Synchronize objc_get/setAssociatedObject and SentryNetwokDetailsData
locking on the NSURLSessionTask instance.

`setState` - iOS internal API, undocumented threading model
  - calls captureRequestDetails
`completionHandler` - callback queue (thread) configured by app.
  - calls captureResponseDetails

captureRequestDetails
To avoid introducing blocking on apple's thread, we use the lock to
objc_getAssociated(task,..) but drop the lock before processing the request
(SentryNetworkRequestDetails:setRequest).

captureResponseDetails
However we need the lock while processing response data
(SentryNetworkRequestDetails:setResponse) b/c (later) it will race with
calls to `SentryNetworkRequestDetails:serialize`. The completionHandler
is on the app's network response path, so adding some thread contention here
(worst-case) is less noticable (an i/o request just finished).

Author: 43jay

Sources/Sentry/SentryNetworkTracker.m

@@ -606,32 +606,36 @@ - (void)captureResponseDetails:(NSData *)data
         return;
     }
 
-    SentryReplayNetworkDetails *details = objc_getAssociatedObject(task, &SentryNetworkDetailsKey);
-    if (!details) {
-        SENTRY_LOG_WARN(@"[NetworkCapture] No SentryReplayNetworkDetails found for %@ - "
-                         @"skipping response capture",
-                         urlString);
-        return;
-    }
+    @synchronized(task) {
+        SentryReplayNetworkDetails *details
+            = objc_getAssociatedObject(task, &SentryNetworkDetailsKey);
+        if (!details) {
+            SENTRY_LOG_WARN(@"[NetworkCapture] No SentryReplayNetworkDetails found for %@ - "
+                             @"skipping response capture",
+                             urlString);
+            return;
+        }
 
-    NSInteger statusCode = 0;
-    NSDictionary *allHeaders = nil;
-    NSString *contentType = nil;
-    if ([response isKindOfClass:[NSHTTPURLResponse class]]) {
-        NSHTTPURLResponse *httpResponse = (NSHTTPURLResponse *)response;
-        statusCode = httpResponse.statusCode;
-        allHeaders = httpResponse.allHeaderFields;
-        contentType = httpResponse.allHeaderFields[@"Content-Type"];
-    }
+        NSInteger statusCode = 0;
+        NSDictionary *allHeaders = nil;
+        NSString *contentType = nil;
+        if ([response isKindOfClass:[NSHTTPURLResponse class]]) {
+            NSHTTPURLResponse *httpResponse = (NSHTTPURLResponse *)response;
+            statusCode = httpResponse.statusCode;
+            allHeaders = httpResponse.allHeaderFields;
+            contentType = httpResponse.allHeaderFields[@"Content-Type"];
+        }
 
-    NSData *bodyData = (options.sessionReplay.networkCaptureBodies && data.length > 0) ? data : nil;
+        NSData *bodyData
+            = (options.sessionReplay.networkCaptureBodies && data.length > 0) ? data : nil;
 
-    [details setResponseWithStatusCode:statusCode
-                                  size:@(data ? data.length : 0)
-                              bodyData:bodyData
-                           contentType:contentType
-                            allHeaders:allHeaders
-                     configuredHeaders:options.sessionReplay.networkResponseHeaders];
+        [details setResponseWithStatusCode:statusCode
+                                      size:@(data ? data.length : 0)
+                                  bodyData:bodyData
+                               contentType:contentType
+                                allHeaders:allHeaders
+                         configuredHeaders:options.sessionReplay.networkResponseHeaders];
+    }
 }
 
 - (void)captureRequestDetails:(NSURLSessionTask *)sessionTask
@@ -642,16 +646,18 @@ - (void)captureRequestDetails:(NSURLSessionTask *)sessionTask
         return;
     }
 
-    SentryReplayNetworkDetails *existingDetails
-        = objc_getAssociatedObject(sessionTask, &SentryNetworkDetailsKey);
-    if (existingDetails) {
-        return;
-    }
-
     NSURLRequest *request = sessionTask.currentRequest;
+    SentryReplayNetworkDetails *details;
 
-    SentryReplayNetworkDetails *details =
-        [[SentryReplayNetworkDetails alloc] initWithMethod:request.HTTPMethod ?: @"GET"];
+    @synchronized(sessionTask) {
+        if (objc_getAssociatedObject(sessionTask, &SentryNetworkDetailsKey)) {
+            return;
+        }
+        details =
+            [[SentryReplayNetworkDetails alloc] initWithMethod:request.HTTPMethod ?: @"GET"];
+        objc_setAssociatedObject(
+            sessionTask, &SentryNetworkDetailsKey, details, OBJC_ASSOCIATION_RETAIN_NONATOMIC);
+    }
 
     // Prefer originalRequest.HTTPBody: currentRequest may reflect redirects, and its HTTPBody may be nil on in-flight tasks.
     NSData *bodyData
@@ -664,9 +670,6 @@ - (void)captureRequestDetails:(NSURLSessionTask *)sessionTask
                     contentType:request.allHTTPHeaderFields[@"Content-Type"]
                      allHeaders:request.allHTTPHeaderFields
               configuredHeaders:networkRequestHeaders];
-
-    objc_setAssociatedObject(
-        sessionTask, &SentryNetworkDetailsKey, details, OBJC_ASSOCIATION_RETAIN_NONATOMIC);
 }
 #endif // SENTRY_TARGET_REPLAY_SUPPORTED
 

Sources/Swift/Integrations/SessionReplay/SentryReplayNetworkDetails.swift

@@ -16,6 +16,11 @@ enum NetworkBodyWarning: String {
 /// ObjC callers (SentryNetworkTracker) create this object and populate it
 /// via `setRequest`/`setResponse`. Swift callers (SentrySRDefaultBreadcrumbConverter)
 /// consume it via `serialize()`.
+///
+/// - Important: `setRequest` and `setResponse` can be called concurrently from
+///   `SentryNetworkTracker` because they write to independent properties.
+///   Adding shared mutable state between will require adding synchronization.
+
 @objc
 @_spi(Private) public class SentryReplayNetworkDetails: NSObject {