Resolve a possible race condition (#8512)

Pass the session details as a part of the session updated notification. This avoids the sessionDetails object missing race condition when trying to update the session details to the traces.

Attempt to fix #8495.

Author: visumickey

FirebasePerformance/CHANGELOG.md

@@ -1,5 +1,6 @@
 # Version 8.6.0
 * Add Firebase Performance support for Swift Package Manager. (#6528)
+* Fix a crash due to a race condition. (#8485)
 
 # Version 8.2.0
 * Update log messages with proper log levels.

FirebasePerformance/Sources/AppActivity/FPRSessionManager+Private.h

@@ -22,7 +22,7 @@ NS_ASSUME_NONNULL_BEGIN
 @interface FPRSessionManager ()
 
 /** The current active session managed by the session manager. Modifiable for unit tests */
-@property(nonatomic, nullable, readwrite) FPRSessionDetails *sessionDetails;
+@property(atomic, nullable, readwrite) FPRSessionDetails *sessionDetails;
 
 /**
  * Checks if the currently active session is beyond maximum allowed time. If so renew the session,

FirebasePerformance/Sources/AppActivity/FPRSessionManager.h

@@ -19,14 +19,17 @@
 /* Notification name when the session Id gets updated. */
 FOUNDATION_EXTERN NSString *_Nonnull const kFPRSessionIdUpdatedNotification;
 
+/* Notification name when the session Id gets updated. */
+FOUNDATION_EXTERN NSString *_Nonnull const kFPRSessionIdNotificationKey;
+
 /** This class manages the current active sessionId of the application and provides mechanism for
  *  propagating the session Id.
  */
 NS_EXTENSION_UNAVAILABLE("Firebase Performance is not supported for extensions.")
 @interface FPRSessionManager : NSObject
 
 /** The current active session managed by the session manager. */
-@property(nonatomic, readonly, nonnull) FPRSessionDetails *sessionDetails;
+@property(atomic, readonly, nonnull) FPRSessionDetails *sessionDetails;
 
 /**
  * The notification center managed by the session manager. All the notifications by the session

FirebasePerformance/Sources/AppActivity/FPRSessionManager.m

@@ -22,6 +22,7 @@
 #import <UIKit/UIKit.h>
 
 NSString *const kFPRSessionIdUpdatedNotification = @"kFPRSessionIdUpdatedNotification";
+NSString *const kFPRSessionIdNotificationKey = @"kFPRSessionIdNotificationKey";
 
 @interface FPRSessionManager ()
 
@@ -104,8 +105,12 @@ - (void)updateSessionId:(NSNotification *)notification {
   FPRSessionDetails *sessionInfo = [[FPRSessionDetails alloc] initWithSessionId:sessionIdString
                                                                         options:sessionOptions];
   self.sessionDetails = sessionInfo;
+  NSMutableDictionary<NSString *, FPRSessionDetails *> *userInfo =
+      [[NSMutableDictionary alloc] init];
+  [userInfo setObject:sessionInfo forKey:kFPRSessionIdNotificationKey];
   [self.sessionNotificationCenter postNotificationName:kFPRSessionIdUpdatedNotification
-                                                object:self];
+                                                object:self
+                                              userInfo:[userInfo copy]];
 }
 
 /**

FirebasePerformance/Sources/Instrumentation/FPRNetworkTrace.m

@@ -50,8 +50,11 @@ @interface FPRNetworkTrace ()
 /** @brief Serial queue to manage the updation of session Ids. */
 @property(nonatomic, readwrite) dispatch_queue_t sessionIdSerialQueue;
 
-/** Updates the current trace with the current session details. */
-- (void)updateTraceWithCurrentSession;
+/**
+ * Updates the current trace with the current session details.
+ * @param sessionDetails Updated session details of the currently active session.
+ */
+- (void)updateTraceWithCurrentSession:(FPRSessionDetails *)sessionDetails;
 
 @end
 
@@ -137,14 +140,20 @@ - (NSString *)description {
   return [NSString stringWithFormat:@"Request: %@", _URLRequest];
 }
 
-- (void)updateTraceWithCurrentSession {
+- (void)sessionChanged:(NSNotification *)notification {
   if (self.traceStarted && !self.traceCompleted) {
-    dispatch_async(self.sessionIdSerialQueue, ^{
-      FPRSessionManager *sessionManager = [FPRSessionManager sharedInstance];
-      FPRSessionDetails *sessionDetails = [sessionManager.sessionDetails copy];
-      if (sessionDetails) {
-        [self.activeSessions addObject:sessionDetails];
-      }
+    NSDictionary<NSString *, FPRSessionDetails *> *userInfo = notification.userInfo;
+    FPRSessionDetails *sessionDetails = [userInfo valueForKey:kFPRSessionIdNotificationKey];
+    if (sessionDetails) {
+      [self updateTraceWithCurrentSession:sessionDetails];
+    }
+  }
+}
+
+- (void)updateTraceWithCurrentSession:(FPRSessionDetails *)sessionDetails {
+  if (sessionDetails != nil) {
+    dispatch_sync(self.sessionIdSerialQueue, ^{
+      [self.activeSessions addObject:sessionDetails];
     });
   }
 }
@@ -191,9 +200,9 @@ - (void)start {
     [self checkpointState:FPRNetworkTraceCheckpointStateInitiated];
 
     FPRSessionManager *sessionManager = [FPRSessionManager sharedInstance];
-    [self updateTraceWithCurrentSession];
+    [self updateTraceWithCurrentSession:[sessionManager.sessionDetails copy]];
     [sessionManager.sessionNotificationCenter addObserver:self
-                                                 selector:@selector(updateTraceWithCurrentSession)
+                                                 selector:@selector(sessionChanged:)
                                                      name:kFPRSessionIdUpdatedNotification
                                                    object:sessionManager];
   }

FirebasePerformance/Sources/Timer/FIRTrace.m

@@ -49,8 +49,11 @@ @interface FIRTrace ()
 /** Stops an active stage that is currently active. */
 - (void)stopActiveStage;
 
-/** Updates the current trace with the session id. */
-- (void)updateTraceWithSessionId;
+/**
+ * Updates the current trace with the session id.
+ * @param sessionDetails Updated session details of the currently active session.
+ */
+- (void)updateTraceWithSessionId:(FPRSessionDetails *)sessionDetails;
 
 @end
 
@@ -139,9 +142,9 @@ - (void)start {
     self.backgroundActivityTracker = [[FPRTraceBackgroundActivityTracker alloc] init];
     FPRSessionManager *sessionManager = [FPRSessionManager sharedInstance];
     if (!self.isStage) {
-      [self updateTraceWithSessionId];
+      [self updateTraceWithSessionId:[sessionManager.sessionDetails copy]];
       [sessionManager.sessionNotificationCenter addObserver:self
-                                                   selector:@selector(updateTraceWithSessionId)
+                                                   selector:@selector(sessionChanged:)
                                                        name:kFPRSessionIdUpdatedNotification
                                                      object:sessionManager];
     }
@@ -400,18 +403,22 @@ - (void)removeAttribute:(NSString *)attribute {
 
 #pragma mark - Utility methods
 
-- (void)updateTraceWithSessionId {
+- (void)sessionChanged:(NSNotification *)notification {
   if ([self isTraceActive]) {
-    dispatch_async(self.sessionIdSerialQueue, ^{
-      FPRSessionManager *sessionManager = [FPRSessionManager sharedInstance];
-      FPRSessionDetails *sessionDetails = [sessionManager.sessionDetails copy];
-      if (sessionDetails) {
-        [self.activeSessions addObject:sessionDetails];
-      }
-    });
+    NSDictionary<NSString *, FPRSessionDetails *> *userInfo = notification.userInfo;
+    FPRSessionDetails *sessionDetails = [userInfo valueForKey:kFPRSessionIdNotificationKey];
+    if (sessionDetails) {
+      [self updateTraceWithSessionId:sessionDetails];
+    }
   }
 }
 
+- (void)updateTraceWithSessionId:(FPRSessionDetails *)sessionDetails {
+  dispatch_sync(self.sessionIdSerialQueue, ^{
+    [self.activeSessions addObject:sessionDetails];
+  });
+}
+
 - (BOOL)isTraceStarted {
   return self.startTime != nil;
 }

FirebasePerformance/Tests/Unit/FPRSessionManagerTest.m

@@ -91,4 +91,35 @@ - (void)testSessionIdUpdationThrowsNotification {
   XCTAssertNotEqual(sessionId, instance.sessionDetails.sessionId);
 }
 
+/** Validate that sessionId changes sends notifications with the session details. */
+- (void)testSessionIdUpdationSendsNotificationWithSessionDetails {
+  FPRSessionManager *instance = [FPRSessionManager sharedInstance];
+  [instance startTrackingAppStateChanges];
+  NSString *sessionId = instance.sessionDetails.sessionId;
+
+  __block BOOL containsSessionDetails = NO;
+  __block FPRSessionDetails *updatedSessionDetails = nil;
+  [instance.sessionNotificationCenter
+      addObserverForName:kFPRSessionIdUpdatedNotification
+                  object:instance
+                   queue:[NSOperationQueue mainQueue]
+              usingBlock:^(NSNotification *note) {
+                NSDictionary<NSString *, FPRSessionDetails *> *userInfo = note.userInfo;
+                FPRSessionDetails *sessionDetails =
+                    [userInfo valueForKey:kFPRSessionIdNotificationKey];
+                if (sessionDetails != nil) {
+                  containsSessionDetails = YES;
+                  updatedSessionDetails = sessionDetails;
+                }
+              }];
+
+  NSNotificationCenter *notificationCenter = [NSNotificationCenter defaultCenter];
+  [notificationCenter postNotificationName:UIApplicationWillEnterForegroundNotification
+                                    object:[UIApplication sharedApplication]];
+
+  XCTAssertTrue(containsSessionDetails);
+  XCTAssertNotEqual(sessionId, instance.sessionDetails.sessionId);
+  XCTAssertEqual(updatedSessionDetails.sessionId, instance.sessionDetails.sessionId);
+}
+
 @end