Skip to content

Commit

Permalink
Fixing threading-related hang during initialization in urgent mode
Browse files Browse the repository at this point in the history
Co-authored-by: wowlocal <kh.misha.9@icloud.com>
  • Loading branch information
samedson and wowlocal committed May 31, 2023
1 parent a2230e2 commit 06e64b6
Show file tree
Hide file tree
Showing 4 changed files with 107 additions and 11 deletions.
15 changes: 12 additions & 3 deletions Crashlytics/Crashlytics/Controllers/FIRCLSReportUploader.m
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,18 @@ - (void)prepareAndSubmitReport:(FIRCLSInternalReport *)report
FIRCLSApplicationActivityDefault, @"Crashlytics Crash Report Processing", ^{
// Check to see if the FID has rotated before we construct the payload
// so that the payload has an updated value.
[self.installIDModel regenerateInstallIDIfNeededWithBlock:^(NSString *_Nonnull newFIID) {
self.fiid = [newFIID copy];
}];
//
// If we're in urgent mode, this will be running on the main thread. Since
// the FIID callback is run on the main thread, this call can deadlock in
// urgent mode. Since urgent mode happens when the app is in a crash loop,
// we can safely assume users aren't rotating their FIID, so this can be skipped.
if (!urgent) {
[self.installIDModel regenerateInstallIDIfNeededWithBlock:^(NSString *_Nonnull newFIID) {
self.fiid = [newFIID copy];
}];
} else {
FIRCLSWarningLog(@"Crashlytics skipped rotating the Install ID during urgent mode because it is run on the main thread, which can't succeed. This can happen if the app crashed the last run and Crashlytics is uploading urgently.");
}

// Run on-device symbolication before packaging if we should process
if (shouldProcess) {
Expand Down
12 changes: 12 additions & 0 deletions Crashlytics/UnitTests/Data/GoogleService-Info.plist
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<key>API_KEY</key>
<string>ATestTestTestTestTestTestTestTestTest00</string>
<key>PROJECT_ID</key>
<string>test-1aaaa</string>
<key>GOOGLE_APP_ID</key>
<string>1:632950151350:ios:d5b0d08d4f00f4b1</string>
</dict>
</plist>
7 changes: 5 additions & 2 deletions Crashlytics/UnitTests/FIRCLSInstallIdentifierModelTests.m
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,11 @@ - (void)testCreateUUIDAndRotate {
[[FIRCLSInstallIdentifierModel alloc] initWithInstallations:iid];
XCTAssertNotNil(model.installID);

BOOL didRotate = [model regenerateInstallIDIfNeededWithBlock:^(NSString *_Nonnull fiid){
}];
BOOL __block didRotate = true;
// dispatch_async_and_wait(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^(void){
didRotate = [model regenerateInstallIDIfNeededWithBlock:^(NSString *_Nonnull fiid){
}];
// });
sleep(1);

XCTAssertFalse(didRotate);
Expand Down
84 changes: 78 additions & 6 deletions Crashlytics/UnitTests/FIRCLSReportUploaderTests.m
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,12 @@ - (void)setUp {

- (void)tearDown {
self.uploader = nil;
[FIRApp resetApps];

[super tearDown];
}

- (void)setupUploaderWithInstallations:(FIRMockInstallations *)installations {
- (void)setupUploaderWithInstallations:(FIRInstallations *)installations {
// Allow nil values only in tests
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wnonnull"
Expand All @@ -95,17 +96,33 @@ - (void)setupUploaderWithInstallations:(FIRMockInstallations *)installations {
self.uploader = [[FIRCLSReportUploader alloc] initWithManagerData:self.managerData];
}

- (NSString*)resourcePath {
#if SWIFT_PACKAGE
NSBundle* bundle = SWIFTPM_MODULE_BUNDLE;
return [bundle.resourcePath stringByAppendingPathComponent:@"Data"];
#else
NSBundle *bundle = [NSBundle bundleForClass:[self class]];
return bundle.resourcePath;
#endif
}

- (FIRCLSInternalReport *)createReport {
NSString *path = [self.fileManager.activePath stringByAppendingPathComponent:@"pkg_uuid"];
FIRCLSInternalReport *report = [[FIRCLSInternalReport alloc] initWithPath:path];
self.fileManager.moveItemAtPathResult = [NSNumber numberWithInt:1];
return report;
}

#pragma mark - Tests

- (void)testPrepareReport {
FIRCLSInternalReport *report = [[FIRCLSInternalReport alloc] initWithPath:[self packagePath]];
self.fileManager.moveItemAtPathResult = [NSNumber numberWithInt:1];
FIRCLSInternalReport *report = [self createReport];

XCTAssertNil(self.uploader.fiid);

[self.uploader prepareAndSubmitReport:report
dataCollectionToken:FIRCLSDataCollectionToken.validToken
asUrgent:YES
asUrgent:NO
withProcessing:YES];

XCTAssertEqual(self.uploader.fiid, TestFIID);
Expand All @@ -115,9 +132,64 @@ - (void)testPrepareReport {
[self.fileManager.moveItemAtPath_destDir containsString:self.fileManager.preparedPath]);
}

- (void)testPrepareReportOnMainThread {
NSString *pathToPlist =
[[self resourcePath] stringByAppendingPathComponent:@"GoogleService-Info.plist"];
FIROptions *options = [[FIROptions alloc] initWithContentsOfFile:pathToPlist];

[FIRApp configureWithName:@"__FIRAPP_DEFAULT" options:options];
XCTAssertNotNil([FIRApp defaultApp], @"configureWithName must have been initialized");

FIRInstallations *installations = [FIRInstallations installationsWithApp:[FIRApp defaultApp]];
FIRCLSInternalReport *report = [self createReport];
[self setupUploaderWithInstallations:installations];

/*
if a report is urgent report will be processed on the Main Thread
otherwise, it will be dispatched to a NSOperationQueue (see `FIRCLSExistingReportManager.m:230`)
This test checks if `prepareAndSubmitReport` finishes in a reasonable time.
*/

NSOperationQueue *queue = [NSOperationQueue new];

// target call will block the main thread, so we need a background thread
// that will wait on a semaphore for a timeout
dispatch_semaphore_t backgroundWaiter = dispatch_semaphore_create(0);
[queue addOperationWithBlock:^{
intptr_t result = dispatch_semaphore_wait(backgroundWaiter,
dispatch_time(DISPATCH_TIME_NOW, 1 * NSEC_PER_SEC));
BOOL exitBecauseOfTimeout = result != 0;
XCTAssertFalse(exitBecauseOfTimeout, @"Main Thread was blocked for more than 1 second");
}];

// Urgent (on the Main thread)
[self.uploader prepareAndSubmitReport:report
dataCollectionToken:FIRCLSDataCollectionToken.validToken
asUrgent:YES
withProcessing:YES];
dispatch_semaphore_signal(backgroundWaiter);

// Not urgent (on a background thread)
XCTestExpectation *expectation =
[self expectationWithDescription:@"wait for a preparation to complete"];

[queue addOperationWithBlock:^{
[self.uploader prepareAndSubmitReport:report
dataCollectionToken:FIRCLSDataCollectionToken.validToken
asUrgent:YES
withProcessing:YES];
[expectation fulfill];
}];

[self waitForExpectationsWithTimeout:1
handler:^(NSError *_Nullable error) {
XCTAssertNil(error, @"expectations failed: %@", error);
}];
}

- (void)test_NilFIID_DoesNotCrash {
FIRCLSInternalReport *report = [[FIRCLSInternalReport alloc] initWithPath:[self packagePath]];
self.fileManager.moveItemAtPathResult = [NSNumber numberWithInt:1];
FIRCLSInternalReport *report = [self createReport];

self.mockInstallations = [[FIRMockInstallations alloc]
initWithError:[NSError errorWithDomain:@"TestDomain" code:-1 userInfo:nil]];
Expand Down

0 comments on commit 06e64b6

Please sign in to comment.