Fix possible deadlock in urgent state of report uploader (#9826)

Add forgotten semaphore_signal calls at early return

Author: wowlocal

Crashlytics/Crashlytics/Controllers/FIRCLSReportUploader.m

@@ -191,12 +191,14 @@ - (void)uploadPackagedReportAtPath:(NSString *)path
            if (!wasWritten) {
              FIRCLSErrorLog(
                  @"Failed to send crash report due to failure writing GoogleDataTransport event");
+             dispatch_semaphore_signal(semaphore);
              return;
            }
 
            if (error) {
              FIRCLSErrorLog(@"Failed to send crash report due to GoogleDataTransport error: %@",
                             error.localizedDescription);
+             dispatch_semaphore_signal(semaphore);
              return;
            }
 

Crashlytics/UnitTests/FIRCLSReportUploaderTests.m

@@ -59,6 +59,7 @@ - (void)setUp {
   self.mockDataTransport = [[FIRMockGDTCORTransport alloc] initWithMappingID:@"1206"
                                                                 transformers:nil
                                                                       target:kGDTCORTargetCSH];
+  self.mockDataTransport.sendDataEvent_wasWritten = YES;
   self.fileManager = [[FIRCLSTempMockFileManager alloc] init];
 
   id fakeApp = [[FIRAppFake alloc] init];
@@ -114,17 +115,46 @@ - (void)testUrgentUploadPackagedReportWithPath {
   [self runUploadPackagedReportWithUrgency:YES];
 }
 
-- (void)testUploadPackagedReportWithoutDataCollectionToken {
-  [self setUpForUpload];
+- (void)testUrgentWaitUntillUpload {
+  self.mockDataTransport.async = YES;
+
+  [self runUploadPackagedReportWithUrgency:YES];
+
+  XCTAssertNotNil(self.mockDataTransport.sendDataEvent_event);
+}
+
+- (void)testUrgentWaitUntillUploadWithError {
+  self.mockDataTransport.async = YES;
+  self.mockDataTransport.sendDataEvent_error = [[NSError alloc] initWithDomain:@"domain"
+                                                                          code:1
+                                                                      userInfo:nil];
+
+  [self.uploader uploadPackagedReportAtPath:[self packagePath]
+                        dataCollectionToken:FIRCLSDataCollectionToken.validToken
+                                   asUrgent:YES];
+
+  XCTAssertNotNil(self.mockDataTransport.sendDataEvent_event);
+}
+
+- (void)testUrgentWaitUntillUploadWithWritingError {
+  self.mockDataTransport.async = YES;
+  self.mockDataTransport.sendDataEvent_wasWritten = NO;
 
+  [self.uploader uploadPackagedReportAtPath:[self packagePath]
+                        dataCollectionToken:FIRCLSDataCollectionToken.validToken
+                                   asUrgent:YES];
+
+  XCTAssertNotNil(self.mockDataTransport.sendDataEvent_event);
+}
+
+- (void)testUploadPackagedReportWithoutDataCollectionToken {
   [self.uploader uploadPackagedReportAtPath:[self packagePath] dataCollectionToken:nil asUrgent:NO];
 
   // Ensure we don't hand off an event to GDT
   XCTAssertNil(self.mockDataTransport.sendDataEvent_event);
 }
 
 - (void)testUploadPackagedReportNotGDTWritten {
-  [self setUpForUpload];
   self.mockDataTransport.sendDataEvent_wasWritten = NO;
 
   [self.uploader uploadPackagedReportAtPath:[self packagePath] dataCollectionToken:nil asUrgent:NO];
@@ -134,7 +164,6 @@ - (void)testUploadPackagedReportNotGDTWritten {
 }
 
 - (void)testUploadPackagedReportGDTError {
-  [self setUpForUpload];
   self.mockDataTransport.sendDataEvent_error = [[NSError alloc] initWithDomain:@"domain"
                                                                           code:1
                                                                       userInfo:nil];
@@ -152,8 +181,6 @@ - (NSString *)packagePath {
 }
 
 - (void)runUploadPackagedReportWithUrgency:(BOOL)urgent {
-  [self setUpForUpload];
-
   [self.uploader uploadPackagedReportAtPath:[self packagePath]
                         dataCollectionToken:FIRCLSDataCollectionToken.validToken
                                    asUrgent:urgent];
@@ -162,8 +189,4 @@ - (void)runUploadPackagedReportWithUrgency:(BOOL)urgent {
   XCTAssertEqualObjects(self.fileManager.removedItemAtPath_path, [self packagePath]);
 }
 
-- (void)setUpForUpload {
-  self.mockDataTransport.sendDataEvent_wasWritten = YES;
-}
-
 @end

Crashlytics/UnitTests/Mocks/FIRMockGDTCoreTransport.h

@@ -24,6 +24,7 @@ NS_ASSUME_NONNULL_BEGIN
 @property(nullable, nonatomic, strong) GDTCOREvent *sendDataEvent_event;
 @property(nullable, nonatomic, strong) NSError *sendDataEvent_error;
 @property(nonatomic) BOOL sendDataEvent_wasWritten;
+@property(assign, nonatomic) BOOL async;
 
 - (instancetype)initWithMappingID:(NSString *)mappingID
                      transformers:(nullable NSArray<id<GDTCOREventTransformer>> *)transformers

Crashlytics/UnitTests/Mocks/FIRMockGDTCoreTransport.m

@@ -22,14 +22,24 @@ - (instancetype)initWithMappingID:(NSString *)mappingID
   _mappingID = mappingID;
   _target = target;
   _sendDataEvent_wasWritten = YES;
+  _async = NO;
 
   return [super initWithMappingID:mappingID transformers:transformers target:target];
 }
 
 - (void)sendDataEvent:(GDTCOREvent *)event
            onComplete:(void (^)(BOOL wasWritten, NSError *error))completion {
-  self.sendDataEvent_event = event;
-  completion(self.sendDataEvent_wasWritten, self.sendDataEvent_error);
+  dispatch_block_t sendData = ^() {
+    self.sendDataEvent_event = event;
+    completion(self.sendDataEvent_wasWritten, self.sendDataEvent_error);
+  };
+
+  if (self.async) {
+    dispatch_after(DISPATCH_TIME_NOW, dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0),
+                   sendData);
+  } else {
+    sendData();
+  }
 }
 
 @end