Don't leak upload packages or deadlock on lifecycle events (#5382)

* Don't leak upload packages or deadlock on lifecycle events

GDTCORUploadPackages would call targetToStorage in -initWithTarget. When this method was running during app terminate, this would cause a deadlock because calling targetToStorage is re-entrant while GDTCORRegistrar is sending lifecycle events to all the instances it's tracking.

NSTimer's invalidate method is also called whenever a package is completed, retried, or expired, without regard to the presence of a package handler.

A unit test is added to ensure that GDTCORUploadPackage's are not leaked by a retain

TSAN caught no issues

* Don't mark GDTCORUploadPackage -init as unavailable

GoogleDataTransport/GDTCORLibrary/GDTCORRegistrar.m

@@ -122,63 +122,66 @@
 #pragma mark - GDTCORLifecycleProtocol
 
 - (void)appWillBackground:(nonnull GDTCORApplication *)app {
-  dispatch_async(_registrarQueue, ^{
-    for (id<GDTCORUploader> uploader in [self->_targetToUploader allValues]) {
-      if ([uploader respondsToSelector:@selector(appWillBackground:)]) {
-        [uploader appWillBackground:app];
-      }
+  NSArray<id<GDTCORUploader>> *uploaders = [self.targetToUploader allValues];
+  for (id<GDTCORUploader> uploader in uploaders) {
+    if ([uploader respondsToSelector:@selector(appWillBackground:)]) {
+      [uploader appWillBackground:app];
     }
-    for (id<GDTCORPrioritizer> prioritizer in [self->_targetToPrioritizer allValues]) {
-      if ([prioritizer respondsToSelector:@selector(appWillBackground:)]) {
-        [prioritizer appWillBackground:app];
-      }
+  }
+  NSArray<id<GDTCORPrioritizer>> *prioritizers = [self.targetToPrioritizer allValues];
+  for (id<GDTCORPrioritizer> prioritizer in prioritizers) {
+    if ([prioritizer respondsToSelector:@selector(appWillBackground:)]) {
+      [prioritizer appWillBackground:app];
     }
-    for (id<GDTCORStorageProtocol> storage in [self->_targetToStorage allValues]) {
-      if ([storage respondsToSelector:@selector(appWillBackground:)]) {
-        [storage appWillBackground:app];
-      }
+  }
+  NSArray<id<GDTCORStorageProtocol>> *storages = [self.targetToStorage allValues];
+  for (id<GDTCORStorageProtocol> storage in storages) {
+    if ([storage respondsToSelector:@selector(appWillBackground:)]) {
+      [storage appWillBackground:app];
     }
-  });
+  }
 }
 
 - (void)appWillForeground:(nonnull GDTCORApplication *)app {
-  dispatch_async(_registrarQueue, ^{
-    for (id<GDTCORUploader> uploader in [self->_targetToUploader allValues]) {
-      if ([uploader respondsToSelector:@selector(appWillForeground:)]) {
-        [uploader appWillForeground:app];
-      }
+  NSArray<id<GDTCORUploader>> *uploaders = [self.targetToUploader allValues];
+  for (id<GDTCORUploader> uploader in uploaders) {
+    if ([uploader respondsToSelector:@selector(appWillForeground:)]) {
+      [uploader appWillForeground:app];
     }
-    for (id<GDTCORPrioritizer> prioritizer in [self->_targetToPrioritizer allValues]) {
-      if ([prioritizer respondsToSelector:@selector(appWillForeground:)]) {
-        [prioritizer appWillForeground:app];
-      }
+  }
+  NSArray<id<GDTCORPrioritizer>> *prioritizers = [self.targetToPrioritizer allValues];
+  for (id<GDTCORPrioritizer> prioritizer in prioritizers) {
+    if ([prioritizer respondsToSelector:@selector(appWillForeground:)]) {
+      [prioritizer appWillForeground:app];
     }
-    for (id<GDTCORStorageProtocol> storage in [self->_targetToStorage allValues]) {
-      if ([storage respondsToSelector:@selector(appWillForeground:)]) {
-        [storage appWillForeground:app];
-      }
+  }
+  NSArray<id<GDTCORStorageProtocol>> *storages = [self.targetToStorage allValues];
+  for (id<GDTCORStorageProtocol> storage in storages) {
+    if ([storage respondsToSelector:@selector(appWillForeground:)]) {
+      [storage appWillForeground:app];
     }
-  });
+  }
 }
 
 - (void)appWillTerminate:(nonnull GDTCORApplication *)app {
-  dispatch_sync(_registrarQueue, ^{
-    for (id<GDTCORUploader> uploader in [self->_targetToUploader allValues]) {
-      if ([uploader respondsToSelector:@selector(appWillTerminate:)]) {
-        [uploader appWillTerminate:app];
-      }
+  NSArray<id<GDTCORUploader>> *uploaders = [self.targetToUploader allValues];
+  for (id<GDTCORUploader> uploader in uploaders) {
+    if ([uploader respondsToSelector:@selector(appWillTerminate:)]) {
+      [uploader appWillTerminate:app];
     }
-    for (id<GDTCORPrioritizer> prioritizer in [self->_targetToPrioritizer allValues]) {
-      if ([prioritizer respondsToSelector:@selector(appWillTerminate:)]) {
-        [prioritizer appWillTerminate:app];
-      }
+  }
+  NSArray<id<GDTCORPrioritizer>> *prioritizers = [self.targetToPrioritizer allValues];
+  for (id<GDTCORPrioritizer> prioritizer in prioritizers) {
+    if ([prioritizer respondsToSelector:@selector(appWillTerminate:)]) {
+      [prioritizer appWillTerminate:app];
     }
-    for (id<GDTCORStorageProtocol> storage in [self->_targetToStorage allValues]) {
-      if ([storage respondsToSelector:@selector(appWillTerminate:)]) {
-        [storage appWillTerminate:app];
-      }
+  }
+  NSArray<id<GDTCORStorageProtocol>> *storages = [self.targetToStorage allValues];
+  for (id<GDTCORStorageProtocol> storage in storages) {
+    if ([storage respondsToSelector:@selector(appWillTerminate:)]) {
+      [storage appWillTerminate:app];
     }
-  });
+  }
 }
 
 @end

GoogleDataTransport/GDTCORLibrary/GDTCORUploadPackage.m

@@ -23,6 +23,30 @@
 #import "GDTCORLibrary/Private/GDTCORUploadCoordinator.h"
 #import "GDTCORLibrary/Private/GDTCORUploadPackage_Private.h"
 
+/** A class that holds a weak reference to an upload package, for use by the package's NSTimer. */
+@interface GDTCORUploadPackageTimerHolder : NSObject
+
+/** The upload package. */
+@property(weak, nonatomic) GDTCORUploadPackage *package;
+
+@end
+
+@implementation GDTCORUploadPackageTimerHolder
+
+/** Calls checkIfPackageIsExpired on the package if non-nil. Invalidates if the package is nil.
+ *
+ * @param timer The timer instance calling this method.
+ */
+- (void)timerFired:(NSTimer *)timer {
+  if (_package) {
+    [_package checkIfPackageIsExpired];
+  } else {
+    [timer invalidate];
+  }
+}
+
+@end
+
 @implementation GDTCORUploadPackage {
   /** If YES, the package's -completeDelivery method has been called. */
   BOOL _isDelivered;
@@ -41,9 +65,11 @@
     _storage = [GDTCORRegistrar sharedInstance].targetToStorage[@(target)];
     _deliverByTime = [GDTCORClock clockSnapshotInTheFuture:180000];
     _handler = [GDTCORUploadCoordinator sharedInstance];
+    GDTCORUploadPackageTimerHolder *holder = [[GDTCORUploadPackageTimerHolder alloc] init];
+    holder.package = self;
     _expirationTimer = [NSTimer scheduledTimerWithTimeInterval:5.0
-                                                        target:self
-                                                      selector:@selector(checkIfPackageIsExpired:)
+                                                        target:holder
+                                                      selector:@selector(timerFired:)
                                                       userInfo:nil
                                                        repeats:YES];
   }
@@ -52,7 +78,18 @@
 }
 
 - (instancetype)copy {
-  GDTCORUploadPackage *newPackage = [[GDTCORUploadPackage alloc] initWithTarget:_target];
+  GDTCORUploadPackage *newPackage = [[GDTCORUploadPackage alloc] init];
+  newPackage->_target = _target;
+  newPackage->_storage = _storage;
+  newPackage->_deliverByTime = _deliverByTime;
+  newPackage->_handler = _handler;
+  GDTCORUploadPackageTimerHolder *holder = [[GDTCORUploadPackageTimerHolder alloc] init];
+  holder.package = newPackage;
+  newPackage->_expirationTimer = [NSTimer scheduledTimerWithTimeInterval:5.0
+                                                                  target:holder
+                                                                selector:@selector(timerFired:)
+                                                                userInfo:nil
+                                                                 repeats:YES];
   newPackage->_events = [_events copy];
   GDTCORLogDebug(@"Copying UploadPackage %@ to %@", self, newPackage);
   return newPackage;
@@ -76,9 +113,9 @@
                    @"It's an API violation to call -completeDelivery twice.");
   }
   _isDelivered = YES;
+  [_expirationTimer invalidate];
   if (!_isHandled && _handler &&
       [_handler respondsToSelector:@selector(packageDelivered:successful:)]) {
-    [_expirationTimer invalidate];
     _isHandled = YES;
     [_handler packageDelivered:[self copy] successful:YES];
   }
@@ -86,20 +123,20 @@
 }
 
 - (void)retryDeliveryInTheFuture {
+  [_expirationTimer invalidate];
   if (!_isHandled && _handler &&
       [_handler respondsToSelector:@selector(packageDelivered:successful:)]) {
-    [_expirationTimer invalidate];
     _isHandled = YES;
     [_handler packageDelivered:[self copy] successful:NO];
   }
   GDTCORLogDebug(@"Upload package will retry in the future: %@", self);
 }
 
-- (void)checkIfPackageIsExpired:(NSTimer *)timer {
+- (void)checkIfPackageIsExpired {
   if ([[GDTCORClock snapshot] isAfter:_deliverByTime]) {
+    [_expirationTimer invalidate];
     if (_handler && [_handler respondsToSelector:@selector(packageExpired:)]) {
       _isHandled = YES;
-      [_expirationTimer invalidate];
       GDTCORLogDebug(@"Upload package expired: %@", self);
       [_handler packageExpired:self];
     }

GoogleDataTransport/GDTCORLibrary/Private/GDTCORUploadPackage_Private.h

@@ -24,4 +24,7 @@
 /** A handler that will receive callbacks for certain events. */
 @property(nonatomic) id<NSSecureCoding, GDTCORUploadPackageProtocol> handler;
 
+/** Checks if the package is expired and calls -packageExpired: on the handler if necessary. */
+- (void)checkIfPackageIsExpired;
+
 @end

GoogleDataTransport/GDTCORLibrary/Public/GDTCORUploadPackage.h

@@ -62,10 +62,7 @@
  * @param target The target/destination of this package.
  * @return An instance of this class.
  */
-- (instancetype)initWithTarget:(GDTCORTarget)target NS_DESIGNATED_INITIALIZER;
-
-// Please use the designated initializer.
-- (instancetype)init NS_UNAVAILABLE;
+- (instancetype)initWithTarget:(GDTCORTarget)target;
 
 /** Completes delivery of the package.
  *

GoogleDataTransport/GDTCORTests/Unit/GDTCORUploadPackageTest.m

@@ -142,4 +142,15 @@
   [self waitForExpectations:@[ expectation ] timeout:30];
 }
 
+/** Tests that the upload package is not leaked by using an NSTimer. */
+- (void)testNoMemoryLeak {
+  __weak GDTCORUploadPackage *weakPackage;
+  @autoreleasepool {
+    GDTCORUploadPackage *package = [[GDTCORUploadPackage alloc] initWithTarget:kGDTCORTargetTest];
+    weakPackage = package;
+    package = nil;
+  }
+  XCTAssertNil(weakPackage);
+}
+
 @end