From 41e2c5a178c372c2b2cc59d596f306ee4482cac3 Mon Sep 17 00:00:00 2001 From: Paul Zabelin Date: Sun, 25 Jun 2023 21:47:33 -0700 Subject: [PATCH 1/6] trim stack trace for non-fatal errors --- Crashlytics/Crashlytics/Components/FIRCLSUserLogging.h | 4 +++- Crashlytics/Crashlytics/Components/FIRCLSUserLogging.m | 7 ++++++- Crashlytics/Crashlytics/FIRCrashlytics.m | 8 +++++++- .../Public/FirebaseCrashlytics/FIRCrashlytics.h | 2 ++ 4 files changed, 18 insertions(+), 3 deletions(-) diff --git a/Crashlytics/Crashlytics/Components/FIRCLSUserLogging.h b/Crashlytics/Crashlytics/Components/FIRCLSUserLogging.h index e0cadd483fb..980cd57cbf2 100644 --- a/Crashlytics/Crashlytics/Components/FIRCLSUserLogging.h +++ b/Crashlytics/Crashlytics/Components/FIRCLSUserLogging.h @@ -81,7 +81,9 @@ void FIRCLSUserLoggingRecordUserKeysAndValues(NSDictionary* keysAndValues); void FIRCLSUserLoggingRecordInternalKeyValue(NSString* key, id value); void FIRCLSUserLoggingWriteInternalKeyValue(NSString* key, NSString* value); -void FIRCLSUserLoggingRecordError(NSError* error, NSDictionary* additionalUserInfo); +void FIRCLSUserLoggingRecordError(NSError* error, + NSDictionary* additionalUserInfo, + NSUInteger skipStacks); NSDictionary* FIRCLSUserLoggingGetCompactedKVEntries(FIRCLSUserLoggingKVStorage* storage, bool decodeHex); diff --git a/Crashlytics/Crashlytics/Components/FIRCLSUserLogging.m b/Crashlytics/Crashlytics/Components/FIRCLSUserLogging.m index 31b4deef1e9..0e046d81167 100644 --- a/Crashlytics/Crashlytics/Components/FIRCLSUserLogging.m +++ b/Crashlytics/Crashlytics/Components/FIRCLSUserLogging.m @@ -379,7 +379,8 @@ static void FIRCLSUserLoggingWriteError(FIRCLSFile *file, } void FIRCLSUserLoggingRecordError(NSError *error, - NSDictionary *additionalUserInfo) { + NSDictionary *additionalUserInfo, + NSUInteger numberOfStacksToSkip) { if (!error) { return; } @@ -391,6 +392,10 @@ void FIRCLSUserLoggingRecordError(NSError *error, // record the stacktrace and timestamp here, so we // are as close as possible to the user's log statement NSArray *addresses = [NSThread callStackReturnAddresses]; + if (numberOfStacksToSkip > 0 && [addresses count] >= numberOfStacksToSkip) { + NSRange range = NSMakeRange(0, numberOfStacksToSkip); + addresses = [addresses subarrayWithRange:range]; + } uint64_t timestamp = time(NULL); FIRCLSUserLoggingWriteAndCheckABFiles( diff --git a/Crashlytics/Crashlytics/FIRCrashlytics.m b/Crashlytics/Crashlytics/FIRCrashlytics.m index 4d112cddad4..c9064daff88 100644 --- a/Crashlytics/Crashlytics/FIRCrashlytics.m +++ b/Crashlytics/Crashlytics/FIRCrashlytics.m @@ -94,6 +94,7 @@ @interface FIRCrashlytics () *)userInfo { - FIRCLSUserLoggingRecordError(error, userInfo); + FIRCLSUserLoggingRecordError(error, userInfo, self.frameStacksToSkip); } - (void)recordExceptionModel:(FIRExceptionModel *)exceptionModel { @@ -391,6 +393,10 @@ - (void)recordOnDemandExceptionModel:(FIRExceptionModel *)exceptionModel { usingExistingReportManager:self.existingReportManager]; } +- (void)setNumberOfStackFramesToSkipForNotFatalErrors:(NSUInteger)frames { + self.frameStacksToSkip = frames; +} + #pragma mark - FIRSessionsSubscriber - (void)onSessionChanged:(FIRSessionDetails *_Nonnull)session { diff --git a/Crashlytics/Crashlytics/Public/FirebaseCrashlytics/FIRCrashlytics.h b/Crashlytics/Crashlytics/Public/FirebaseCrashlytics/FIRCrashlytics.h index 1ace1bed780..4921414cf7d 100644 --- a/Crashlytics/Crashlytics/Public/FirebaseCrashlytics/FIRCrashlytics.h +++ b/Crashlytics/Crashlytics/Public/FirebaseCrashlytics/FIRCrashlytics.h @@ -243,6 +243,8 @@ NS_SWIFT_NAME(Crashlytics) */ - (void)deleteUnsentReports; +- (void)setNumberOfStackFramesToSkipForNotFatalErrors:(NSUInteger)frames; + @end NS_ASSUME_NONNULL_END From 2c64991803d44c75b1058caf0fc05529c9de2247 Mon Sep 17 00:00:00 2001 From: Paul Zabelin Date: Sun, 25 Jun 2023 22:09:38 -0700 Subject: [PATCH 2/6] fix array with subrange --- Crashlytics/Crashlytics/Components/FIRCLSUserLogging.m | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Crashlytics/Crashlytics/Components/FIRCLSUserLogging.m b/Crashlytics/Crashlytics/Components/FIRCLSUserLogging.m index 0e046d81167..c9f120dff3a 100644 --- a/Crashlytics/Crashlytics/Components/FIRCLSUserLogging.m +++ b/Crashlytics/Crashlytics/Components/FIRCLSUserLogging.m @@ -392,8 +392,8 @@ void FIRCLSUserLoggingRecordError(NSError *error, // record the stacktrace and timestamp here, so we // are as close as possible to the user's log statement NSArray *addresses = [NSThread callStackReturnAddresses]; - if (numberOfStacksToSkip > 0 && [addresses count] >= numberOfStacksToSkip) { - NSRange range = NSMakeRange(0, numberOfStacksToSkip); + if (numberOfStacksToSkip > 0 && [addresses count] > numberOfStacksToSkip) { + NSRange range = NSMakeRange(numberOfStacksToSkip, [addresses count] - numberOfStacksToSkip); addresses = [addresses subarrayWithRange:range]; } uint64_t timestamp = time(NULL); From d287a15de3953c80565060879bc78742a3a218aa Mon Sep 17 00:00:00 2001 From: Paul Zabelin Date: Mon, 26 Jun 2023 01:30:05 -0700 Subject: [PATCH 3/6] fix: style --- Crashlytics/Crashlytics/FIRCrashlytics.m | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Crashlytics/Crashlytics/FIRCrashlytics.m b/Crashlytics/Crashlytics/FIRCrashlytics.m index c9064daff88..fef3ab3e7be 100644 --- a/Crashlytics/Crashlytics/FIRCrashlytics.m +++ b/Crashlytics/Crashlytics/FIRCrashlytics.m @@ -379,7 +379,7 @@ - (void)recordError:(NSError *)error { } - (void)recordError:(NSError *)error userInfo:(NSDictionary *)userInfo { - FIRCLSUserLoggingRecordError(error, userInfo, self.frameStacksToSkip); + FIRCLSUserLoggingRecordError(error, userInfo, self.frameStacksToSkip); } - (void)recordExceptionModel:(FIRExceptionModel *)exceptionModel { From 5e0ced51a0cc7960033d431822cd8ce51e53c5d1 Mon Sep 17 00:00:00 2001 From: Paul Zabelin Date: Sun, 9 Jul 2023 03:31:40 -0700 Subject: [PATCH 4/6] update unit tests --- Crashlytics/UnitTests/FIRCLSLoggingTests.m | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Crashlytics/UnitTests/FIRCLSLoggingTests.m b/Crashlytics/UnitTests/FIRCLSLoggingTests.m index a5c72f5c73c..064b4692f73 100644 --- a/Crashlytics/UnitTests/FIRCLSLoggingTests.m +++ b/Crashlytics/UnitTests/FIRCLSLoggingTests.m @@ -365,7 +365,7 @@ - (void)testLoggedError { code:-1 userInfo:@{@"key1" : @"value", @"key2" : @"value2"}]; - FIRCLSUserLoggingRecordError(error, @{@"additional" : @"key"}); + FIRCLSUserLoggingRecordError(error, @{@"additional" : @"key"}, 0); NSArray* errors = [self errorAContents]; @@ -405,7 +405,7 @@ - (void)testWritingMaximumNumberOfLoggedErrors { userInfo:@{@"key1" : @"value", @"key2" : @"value2"}]; for (size_t i = 0; i < _firclsContext.readonly->logging.errorStorage.maxEntries; ++i) { - FIRCLSUserLoggingRecordError(error, nil); + FIRCLSUserLoggingRecordError(error, nil, 0); } NSArray* errors = [self errorAContents]; @@ -414,7 +414,7 @@ - (void)testWritingMaximumNumberOfLoggedErrors { // at this point, if we log one more, we should expect a roll over to the next file - FIRCLSUserLoggingRecordError(error, nil); + FIRCLSUserLoggingRecordError(error, nil, 0); XCTAssertEqual([[self errorAContents] count], 8, @""); XCTAssertEqual([[self errorBContents] count], 1, @""); @@ -422,7 +422,7 @@ - (void)testWritingMaximumNumberOfLoggedErrors { // and our next entry should continue into the B file - FIRCLSUserLoggingRecordError(error, nil); + FIRCLSUserLoggingRecordError(error, nil, 0); XCTAssertEqual([[self errorAContents] count], 8, @""); XCTAssertEqual([[self errorBContents] count], 2, @""); @@ -432,7 +432,7 @@ - (void)testWritingMaximumNumberOfLoggedErrors { - (void)testLoggedErrorWithNullsInAdditionalInfo { NSError* error = [NSError errorWithDomain:@"Domain" code:-1 userInfo:nil]; - FIRCLSUserLoggingRecordError(error, @{@"null-key" : [NSNull null]}); + FIRCLSUserLoggingRecordError(error, @{@"null-key" : [NSNull null]}, 0); NSArray* errors = [self errorAContents]; From a4d5c5f652c0f6c17243a3b6b0388814288097b8 Mon Sep 17 00:00:00 2001 From: Paul Zabelin Date: Sun, 9 Jul 2023 04:15:42 -0700 Subject: [PATCH 5/6] add unit test for skipping stack frames --- Crashlytics/UnitTests/FIRCLSLoggingTests.m | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/Crashlytics/UnitTests/FIRCLSLoggingTests.m b/Crashlytics/UnitTests/FIRCLSLoggingTests.m index 064b4692f73..d44a5e3171c 100644 --- a/Crashlytics/UnitTests/FIRCLSLoggingTests.m +++ b/Crashlytics/UnitTests/FIRCLSLoggingTests.m @@ -455,4 +455,25 @@ - (void)testLoggedErrorWithNullsInAdditionalInfo { XCTAssertEqualObjects(additionalEntries[0], entryOne, @""); } +- (void)testSkippingFramesInStackTrace { + NSError* error = [NSError errorWithDomain:@"Domain" code:-1 userInfo:nil]; + FIRCLSUserLoggingRecordError(error, @{}, 0); + + NSUInteger numberOfStacksToSkip = 2; + FIRCLSUserLoggingRecordError(error, @{}, numberOfStacksToSkip); + + NSArray* errors = [self errorAContents]; + + NSArray* firstTrace = [errors[0] valueForKeyPath:@"error.stacktrace"]; + NSArray* secondTrace = [errors[1] valueForKeyPath:@"error.stacktrace"]; + + XCTAssertEqual([secondTrace count], [firstTrace count] - 2, + @"second error should have have 2 frames less"); + XCTAssertEqual(secondTrace[0], firstTrace[2], @"should have same stack traces starting with 2"); + XCTAssertEqual(secondTrace[1], firstTrace[3], @"should have same stack traces starting with 2"); + XCTAssertEqualObjects(secondTrace, + [firstTrace subarrayWithRange:NSMakeRange(2, [secondTrace count])], + @"should have same stack traces with first 2 removed"); +} + @end From 329e3bf40979f055aa9ab53f52eb367bc08facee Mon Sep 17 00:00:00 2001 From: Paul Zabelin Date: Sun, 9 Jul 2023 23:47:37 -0700 Subject: [PATCH 6/6] fIx: typo use NSUInteger rename stackFramesToSkip --- Crashlytics/Crashlytics/FIRCrashlytics.m | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Crashlytics/Crashlytics/FIRCrashlytics.m b/Crashlytics/Crashlytics/FIRCrashlytics.m index fef3ab3e7be..4beb6574c6a 100644 --- a/Crashlytics/Crashlytics/FIRCrashlytics.m +++ b/Crashlytics/Crashlytics/FIRCrashlytics.m @@ -94,7 +94,7 @@ @interface FIRCrashlytics () *)userInfo { - FIRCLSUserLoggingRecordError(error, userInfo, self.frameStacksToSkip); + FIRCLSUserLoggingRecordError(error, userInfo, self.stackFramesToSkip); } - (void)recordExceptionModel:(FIRExceptionModel *)exceptionModel { @@ -394,7 +394,7 @@ - (void)recordOnDemandExceptionModel:(FIRExceptionModel *)exceptionModel { } - (void)setNumberOfStackFramesToSkipForNotFatalErrors:(NSUInteger)frames { - self.frameStacksToSkip = frames; + self.stackFramesToSkip = frames; } #pragma mark - FIRSessionsSubscriber