Skip to content

Commit 68b39b8

Browse files
samedsonsamedson
authored
Fixing threading-related hang during initialization in urgent mode (#11216)
1 parent 9143c4b commit 68b39b8

File tree

4 files changed

+108
-9
lines changed

4 files changed

+108
-9
lines changed

‎Crashlytics/CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
1+
# Unreleased
2+
- [fixed] Fixed a threading-related hang during initialization in urgent mode (#11216)
3+
14
# 10.10.0
25
- [changed] Removed references to deprecated CTCarrier API in FirebaseSessions. (#11144)
36
- [fixed] Fix Xcode 14.3 Analyzer issue. (#11228)

‎Crashlytics/Crashlytics/Controllers/FIRCLSReportUploader.m

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -89,9 +89,21 @@ - (void)prepareAndSubmitReport:(FIRCLSInternalReport *)report
8989
FIRCLSApplicationActivityDefault, @"Crashlytics Crash Report Processing", ^{
9090
// Check to see if the FID has rotated before we construct the payload
9191
// so that the payload has an updated value.
92-
[self.installIDModel regenerateInstallIDIfNeededWithBlock:^(NSString *_Nonnull newFIID) {
93-
self.fiid = [newFIID copy];
94-
}];
92+
//
93+
// If we're in urgent mode, this will be running on the main thread. Since
94+
// the FIID callback is run on the main thread, this call can deadlock in
95+
// urgent mode. Since urgent mode happens when the app is in a crash loop,
96+
// we can safely assume users aren't rotating their FIID, so this can be skipped.
97+
if (!urgent) {
98+
[self.installIDModel regenerateInstallIDIfNeededWithBlock:^(NSString *_Nonnull newFIID) {
99+
self.fiid = [newFIID copy];
100+
}];
101+
} else {
102+
FIRCLSWarningLog(
103+
@"Crashlytics skipped rotating the Install ID during urgent mode because it is run "
104+
@"on the main thread, which can't succeed. This can happen if the app crashed the "
105+
@"last run and Crashlytics is uploading urgently.");
106+
}
95107

96108
// Run on-device symbolication before packaging if we should process
97109
if (shouldProcess) {

‎Crashlytics/UnitTests/Data/GoogleService-Info.plist

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
<?xml version="1.0" encoding="UTF-8"?>
2+
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
3+
<plist version="1.0">
4+
<dict>
5+
<key>API_KEY</key>
6+
<string>ATestTestTestTestTestTestTestTestTest00</string>
7+
<key>PROJECT_ID</key>
8+
<string>test-1aaaa</string>
9+
<key>GOOGLE_APP_ID</key>
10+
<string>1:632950151350:ios:d5b0d08d4f00f4b1</string>
11+
</dict>
12+
</plist>

‎Crashlytics/UnitTests/FIRCLSReportUploaderTests.m

Lines changed: 78 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -74,11 +74,12 @@ - (void)setUp {
7474

7575
- (void)tearDown {
7676
self.uploader = nil;
77+
[FIRApp resetApps];
7778

7879
[super tearDown];
7980
}
8081

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

99+
- (NSString *)resourcePath {
100+
#if SWIFT_PACKAGE
101+
NSBundle *bundle = SWIFTPM_MODULE_BUNDLE;
102+
return [bundle.resourcePath stringByAppendingPathComponent:@"Data"];
103+
#else
104+
NSBundle *bundle = [NSBundle bundleForClass:[self class]];
105+
return bundle.resourcePath;
106+
#endif
107+
}
108+
109+
- (FIRCLSInternalReport *)createReport {
110+
NSString *path = [self.fileManager.activePath stringByAppendingPathComponent:@"pkg_uuid"];
111+
FIRCLSInternalReport *report = [[FIRCLSInternalReport alloc] initWithPath:path];
112+
self.fileManager.moveItemAtPathResult = [NSNumber numberWithInt:1];
113+
return report;
114+
}
115+
98116
#pragma mark - Tests
99117

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

104121
XCTAssertNil(self.uploader.fiid);
105122

106123
[self.uploader prepareAndSubmitReport:report
107124
dataCollectionToken:FIRCLSDataCollectionToken.validToken
108-
asUrgent:YES
125+
asUrgent:NO
109126
withProcessing:YES];
110127

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

135+
- (void)testPrepareReportOnMainThread {
136+
NSString *pathToPlist =
137+
[[self resourcePath] stringByAppendingPathComponent:@"GoogleService-Info.plist"];
138+
FIROptions *options = [[FIROptions alloc] initWithContentsOfFile:pathToPlist];
139+
140+
[FIRApp configureWithName:@"__FIRAPP_DEFAULT" options:options];
141+
XCTAssertNotNil([FIRApp defaultApp], @"configureWithName must have been initialized");
142+
143+
FIRInstallations *installations = [FIRInstallations installationsWithApp:[FIRApp defaultApp]];
144+
FIRCLSInternalReport *report = [self createReport];
145+
[self setupUploaderWithInstallations:installations];
146+
147+
/*
148+
if a report is urgent report will be processed on the Main Thread
149+
otherwise, it will be dispatched to a NSOperationQueue (see `FIRCLSExistingReportManager.m:230`)
150+
151+
This test checks if `prepareAndSubmitReport` finishes in a reasonable time.
152+
*/
153+
154+
NSOperationQueue *queue = [NSOperationQueue new];
155+
156+
// target call will block the main thread, so we need a background thread
157+
// that will wait on a semaphore for a timeout
158+
dispatch_semaphore_t backgroundWaiter = dispatch_semaphore_create(0);
159+
[queue addOperationWithBlock:^{
160+
intptr_t result = dispatch_semaphore_wait(backgroundWaiter,
161+
dispatch_time(DISPATCH_TIME_NOW, 1 * NSEC_PER_SEC));
162+
BOOL exitBecauseOfTimeout = result != 0;
163+
XCTAssertFalse(exitBecauseOfTimeout, @"Main Thread was blocked for more than 1 second");
164+
}];
165+
166+
// Urgent (on the Main thread)
167+
[self.uploader prepareAndSubmitReport:report
168+
dataCollectionToken:FIRCLSDataCollectionToken.validToken
169+
asUrgent:YES
170+
withProcessing:YES];
171+
dispatch_semaphore_signal(backgroundWaiter);
172+
173+
// Not urgent (on a background thread)
174+
XCTestExpectation *expectation =
175+
[self expectationWithDescription:@"wait for a preparation to complete"];
176+
177+
[queue addOperationWithBlock:^{
178+
[self.uploader prepareAndSubmitReport:report
179+
dataCollectionToken:FIRCLSDataCollectionToken.validToken
180+
asUrgent:YES
181+
withProcessing:YES];
182+
[expectation fulfill];
183+
}];
184+
185+
[self waitForExpectationsWithTimeout:1
186+
handler:^(NSError *_Nullable error) {
187+
XCTAssertNil(error, @"expectations failed: %@", error);
188+
}];
189+
}
190+
118191
- (void)test_NilFIID_DoesNotCrash {
119-
FIRCLSInternalReport *report = [[FIRCLSInternalReport alloc] initWithPath:[self packagePath]];
120-
self.fileManager.moveItemAtPathResult = [NSNumber numberWithInt:1];
192+
FIRCLSInternalReport *report = [self createReport];
121193

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

0 commit comments

Comments
 (0)