Fix backgrounding in GDTStorage (#3627) (#3632)

* Don't end a background task twice

* Ensure that setting backgroundID is atomic

* Remove @synchronizeds, use a volatile variable instead

* Don't run NSCoding methods on the queue

All calls utilizing NSKeyedArchiver archiveXXXX should be called from on the storage queue, but it shouldn't enqueue itself.

* Style

* Update CHANGELOG

* Change PR #s to issue #s

Author: maksymmalyhin

GoogleDataTransport/CHANGELOG.md

@@ -1,6 +1,7 @@
 # v1.1.2
 - Add initial support for iOS 13.
 - Add initial support for Catalyst.
+- Backgrounding in GDTStorage is fixed. (#3623 and #3625)
 
 # v1.1.1
 - Fixes a crash in GDTUploadPackage and GDTStorage. (#3547)

GoogleDataTransport/GDTLibrary/GDTStorage.m

@@ -76,10 +76,12 @@ - (instancetype)init {
 - (void)storeEvent:(GDTEvent *)event {
   [self createEventDirectoryIfNotExists];
 
-  __block GDTBackgroundIdentifier bgID = GDTBackgroundIdentifierInvalid;
-  if (_runningInBackground) {
-    bgID = [[GDTApplication sharedApplication] beginBackgroundTaskWithExpirationHandler:^{
-      [[GDTApplication sharedApplication] endBackgroundTask:bgID];
+  if (_backgroundID == GDTBackgroundIdentifierInvalid) {
+    _backgroundID = [[GDTApplication sharedApplication] beginBackgroundTaskWithExpirationHandler:^{
+      if (self->_backgroundID != GDTBackgroundIdentifierInvalid) {
+        [[GDTApplication sharedApplication] endBackgroundTask:self->_backgroundID];
+        self->_backgroundID = GDTBackgroundIdentifierInvalid;
+      }
     }];
   }
 
@@ -110,7 +112,7 @@ - (void)storeEvent:(GDTEvent *)event {
     }
 
     // If running in the background, save state to disk and end the associated background task.
-    if (bgID != GDTBackgroundIdentifierInvalid) {
+    if (self->_backgroundID != GDTBackgroundIdentifierInvalid) {
       if (@available(macOS 10.13, iOS 11.0, tvOS 11.0, *)) {
         NSData *data = [NSKeyedArchiver archivedDataWithRootObject:self
                                              requiringSecureCoding:YES
@@ -121,7 +123,8 @@ - (void)storeEvent:(GDTEvent *)event {
         [NSKeyedArchiver archiveRootObject:self toFile:[GDTStorage archivePath]];
 #endif
       }
-      [[GDTApplication sharedApplication] endBackgroundTask:bgID];
+      [[GDTApplication sharedApplication] endBackgroundTask:self->_backgroundID];
+      self->_backgroundID = GDTBackgroundIdentifierInvalid;
     }
   });
 }
@@ -210,27 +213,34 @@ - (void)appWillForeground:(GDTApplication *)app {
     [NSKeyedUnarchiver unarchiveObjectWithFile:[GDTStorage archivePath]];
 #endif
   }
-  self->_runningInBackground = NO;
 }
 
 - (void)appWillBackground:(GDTApplication *)app {
-  self->_runningInBackground = YES;
-  if (@available(macOS 10.13, iOS 11.0, tvOS 11.0, *)) {
-    NSData *data = [NSKeyedArchiver archivedDataWithRootObject:self
-                                         requiringSecureCoding:YES
-                                                         error:nil];
-    [data writeToFile:[GDTStorage archivePath] atomically:YES];
-  } else {
+  if (_backgroundID == GDTBackgroundIdentifierInvalid) {
+    _backgroundID = [[GDTApplication sharedApplication] beginBackgroundTaskWithExpirationHandler:^{
+      if (self->_backgroundID != GDTBackgroundIdentifierInvalid) {
+        [[GDTApplication sharedApplication] endBackgroundTask:self->_backgroundID];
+        self->_backgroundID = GDTBackgroundIdentifierInvalid;
+      }
+    }];
+  }
+  dispatch_async(_storageQueue, ^{
+    if (@available(macOS 10.13, iOS 11.0, tvOS 11.0, *)) {
+      NSData *data = [NSKeyedArchiver archivedDataWithRootObject:self
+                                           requiringSecureCoding:YES
+                                                           error:nil];
+      [data writeToFile:[GDTStorage archivePath] atomically:YES];
+    } else {
 #if !defined(TARGET_OS_MACCATALYST)
-    [NSKeyedArchiver archiveRootObject:self toFile:[GDTStorage archivePath]];
+      [NSKeyedArchiver archiveRootObject:self toFile:[GDTStorage archivePath]];
 #endif
-  }
-  // Create an immediate background task to run until the end of the current queue of work.
-  __block GDTBackgroundIdentifier bgID = [app beginBackgroundTaskWithExpirationHandler:^{
-    [app endBackgroundTask:bgID];
-  }];
+    }
+  });
   dispatch_async(_storageQueue, ^{
-    [app endBackgroundTask:bgID];
+    if (self->_backgroundID != GDTBackgroundIdentifierInvalid) {
+      [app endBackgroundTask:self->_backgroundID];
+      self->_backgroundID = GDTBackgroundIdentifierInvalid;
+    }
   });
 }
 
@@ -283,25 +293,19 @@ - (instancetype)initWithCoder:(NSCoder *)aDecoder {
 
 - (void)encodeWithCoder:(NSCoder *)aCoder {
   GDTStorage *sharedInstance = [self.class sharedInstance];
-  dispatch_queue_t storageQueue = sharedInstance.storageQueue;
-  if (!storageQueue) {
-    return;
+  NSMutableOrderedSet<GDTStoredEvent *> *storedEvents = sharedInstance->_storedEvents;
+  if (storedEvents) {
+    [aCoder encodeObject:storedEvents forKey:kGDTStorageStoredEventsKey];
+  }
+  NSMutableDictionary<NSNumber *, NSMutableSet<GDTStoredEvent *> *> *targetToEventSet =
+      sharedInstance->_targetToEventSet;
+  if (targetToEventSet) {
+    [aCoder encodeObject:targetToEventSet forKey:kGDTStorageTargetToEventSetKey];
+  }
+  GDTUploadCoordinator *uploadCoordinator = sharedInstance->_uploadCoordinator;
+  if (uploadCoordinator) {
+    [aCoder encodeObject:uploadCoordinator forKey:kGDTStorageUploadCoordinatorKey];
   }
-  dispatch_sync(storageQueue, ^{
-    NSMutableOrderedSet<GDTStoredEvent *> *storedEvents = sharedInstance->_storedEvents;
-    if (storedEvents) {
-      [aCoder encodeObject:storedEvents forKey:kGDTStorageStoredEventsKey];
-    }
-    NSMutableDictionary<NSNumber *, NSMutableSet<GDTStoredEvent *> *> *targetToEventSet =
-        sharedInstance->_targetToEventSet;
-    if (targetToEventSet) {
-      [aCoder encodeObject:targetToEventSet forKey:kGDTStorageTargetToEventSetKey];
-    }
-    GDTUploadCoordinator *uploadCoordinator = sharedInstance->_uploadCoordinator;
-    if (uploadCoordinator) {
-      [aCoder encodeObject:uploadCoordinator forKey:kGDTStorageUploadCoordinatorKey];
-    }
-  });
 }
 
 @end

GoogleDataTransport/GDTLibrary/Private/GDTStorage_Private.h

@@ -35,9 +35,8 @@ NS_ASSUME_NONNULL_BEGIN
 /** The upload coordinator instance used by this storage instance. */
 @property(nonatomic) GDTUploadCoordinator *uploadCoordinator;
 
-/** If YES, every call to -storeLog results in background task and serializes the singleton to disk.
- */
-@property(nonatomic, readonly) BOOL runningInBackground;
+/** The background identifier, not invalid if running in the background. */
+@property(nonatomic) GDTBackgroundIdentifier backgroundID;
 
 /** Returns the path to the keyed archive of the singleton. This is where the singleton is saved
  * to disk during certain app lifecycle events.

GoogleDataTransport/GDTLibrary/Public/GDTPlatform.h

@@ -42,7 +42,7 @@ FOUNDATION_EXPORT NSString *const kGDTApplicationWillTerminateNotification;
 BOOL GDTReachabilityFlagsContainWWAN(SCNetworkReachabilityFlags flags);
 
 /** A typedef identify background identifiers. */
-typedef NSUInteger GDTBackgroundIdentifier;
+typedef volatile NSUInteger GDTBackgroundIdentifier;
 
 /** A background task's invalid sentinel value. */
 FOUNDATION_EXPORT const GDTBackgroundIdentifier GDTBackgroundIdentifierInvalid;

GoogleDataTransport/GDTTests/Lifecycle/GDTLifecycleTest.m

@@ -107,7 +107,6 @@ - (void)testBackgrounding {
 
   NSNotificationCenter *notifCenter = [NSNotificationCenter defaultCenter];
   [notifCenter postNotificationName:kGDTApplicationDidEnterBackgroundNotification object:nil];
-  XCTAssertTrue([GDTStorage sharedInstance].runningInBackground);
   XCTAssertTrue([GDTUploadCoordinator sharedInstance].runningInBackground);
   GDTWaitForBlock(
       ^BOOL {
@@ -144,7 +143,6 @@ - (void)testForegrounding {
 
   [[NSRunLoop currentRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:1.0]];
   [notifCenter postNotificationName:kGDTApplicationWillEnterForegroundNotification object:nil];
-  XCTAssertFalse([GDTStorage sharedInstance].runningInBackground);
   XCTAssertFalse([GDTUploadCoordinator sharedInstance].runningInBackground);
   GDTWaitForBlock(
       ^BOOL {

GoogleDataTransport/GDTTests/Unit/GDTStorageTest.m

@@ -34,7 +34,7 @@
 #import "GDTTests/Common/Categories/GDTRegistrar+Testing.h"
 #import "GDTTests/Common/Categories/GDTStorage+Testing.h"
 
-static NSInteger target = 1337;
+static NSInteger target = kGDTTargetCCT;
 
 @interface GDTStorageTest : GDTTestCase
 
@@ -63,6 +63,8 @@ - (void)setUp {
 
 - (void)tearDown {
   [super tearDown];
+  dispatch_sync([GDTStorage sharedInstance].storageQueue, ^{
+                });
   // Destroy these objects before the next test begins.
   self.testBackend = nil;
   self.testPrioritizer = nil;
@@ -262,16 +264,18 @@ - (void)testNSSecureCoding {
   event.dataObjectTransportBytes = [@"testString" dataUsingEncoding:NSUTF8StringEncoding];
   XCTAssertNoThrow([[GDTStorage sharedInstance] storeEvent:event]);
   event = nil;
-  NSData *storageData;
-  if (@available(macOS 10.13, iOS 11.0, tvOS 11.0, *)) {
-    storageData = [NSKeyedArchiver archivedDataWithRootObject:[GDTStorage sharedInstance]
-                                        requiringSecureCoding:YES
-                                                        error:nil];
-  } else {
+  __block NSData *storageData;
+  dispatch_sync([GDTStorage sharedInstance].storageQueue, ^{
+    if (@available(macOS 10.13, iOS 11.0, tvOS 11.0, *)) {
+      storageData = [NSKeyedArchiver archivedDataWithRootObject:[GDTStorage sharedInstance]
+                                          requiringSecureCoding:YES
+                                                          error:nil];
+    } else {
 #if !defined(TARGET_OS_MACCATALYST)
-    storageData = [NSKeyedArchiver archivedDataWithRootObject:[GDTStorage sharedInstance]];
+      storageData = [NSKeyedArchiver archivedDataWithRootObject:[GDTStorage sharedInstance]];
 #endif
-  }
+    }
+  });
   dispatch_sync([GDTStorage sharedInstance].storageQueue, ^{
     XCTAssertNotNil([[GDTStorage sharedInstance].storedEvents lastObject]);
   });
@@ -300,16 +304,18 @@ - (void)testNSSecureCodingWithSharedInstance {
   event.clockSnapshot = [GDTClock snapshot];
   XCTAssertNoThrow([[GDTStorage sharedInstance] storeEvent:event]);
   event = nil;
-  NSData *storageData;
-  if (@available(macOS 10.13, iOS 11.0, tvOS 11.0, *)) {
-    storageData = [NSKeyedArchiver archivedDataWithRootObject:[GDTStorage sharedInstance]
-                                        requiringSecureCoding:YES
-                                                        error:nil];
-  } else {
+  __block NSData *storageData;
+  dispatch_sync([GDTStorage sharedInstance].storageQueue, ^{
+    if (@available(macOS 10.13, iOS 11.0, tvOS 11.0, *)) {
+      storageData = [NSKeyedArchiver archivedDataWithRootObject:[GDTStorage sharedInstance]
+                                          requiringSecureCoding:YES
+                                                          error:nil];
+    } else {
 #if !defined(TARGET_OS_MACCATALYST)
-    storageData = [NSKeyedArchiver archivedDataWithRootObject:[GDTStorage sharedInstance]];
+      storageData = [NSKeyedArchiver archivedDataWithRootObject:[GDTStorage sharedInstance]];
 #endif
-  }
+    }
+  });
   dispatch_sync([GDTStorage sharedInstance].storageQueue, ^{
     XCTAssertNotNil([[GDTStorage sharedInstance].storedEvents lastObject]);
   });
@@ -354,4 +360,18 @@ - (void)testQoSTierFast {
   });
 }
 
+/** Tests a deadlock condition in which a background signal is sent during -storeEvent:. */
+- (void)testStoreEventDeadlockFromBackgrounding {
+  [GDTStorage sharedInstance].backgroundID = 1234;
+  for (int i = 0; i < 10000; i++) {
+    GDTEvent *event = [[GDTEvent alloc] initWithMappingID:@"404" target:kGDTTargetCCT];
+    event.dataObjectTransportBytes = [@"testString" dataUsingEncoding:NSUTF8StringEncoding];
+    event.qosTier = GDTEventQoSFast;
+    event.clockSnapshot = [GDTClock snapshot];
+    [[GDTStorage sharedInstance] storeEvent:event];
+  }
+  dispatch_sync(dispatch_get_global_queue(QOS_CLASS_USER_INTERACTIVE, 0), ^{
+                });
+}
+
 @end

GoogleDataTransportCCTSupport/GDTCCTLibrary/GDTCCTUploader.m

@@ -192,7 +192,10 @@ - (void)packageExpired:(GDTUploadPackage *)package {
 
 - (void)appWillBackground:(GDTApplication *)app {
   _backgroundID = [app beginBackgroundTaskWithExpirationHandler:^{
-    [app endBackgroundTask:self->_backgroundID];
+    if (self->_backgroundID != GDTBackgroundIdentifierInvalid) {
+      [app endBackgroundTask:self->_backgroundID];
+      self->_backgroundID = GDTBackgroundIdentifierInvalid;
+    }
   }];
 }