From 4ac28abd5c34d4fc006e9c169d863b95d2f799ef Mon Sep 17 00:00:00 2001 From: Nick Dowell Date: Wed, 21 Apr 2021 10:00:45 +0100 Subject: [PATCH] Fix thread sending behaviour --- Bugsnag/Client/BugsnagClient.m | 13 +++++-------- Bugsnag/Payload/BugsnagError+Private.h | 2 +- Bugsnag/Payload/BugsnagError.m | 8 ++------ Bugsnag/Payload/BugsnagEvent.m | 15 +++++++++------ CHANGELOG.md | 7 +++++++ Tests/BugsnagErrorTest.m | 6 +++--- features/barebone_tests.feature | 9 ++++----- features/threads.feature | 20 +++++++------------- 8 files changed, 38 insertions(+), 42 deletions(-) diff --git a/Bugsnag/Client/BugsnagClient.m b/Bugsnag/Client/BugsnagClient.m index ed4be8145..aa9e793b2 100644 --- a/Bugsnag/Client/BugsnagClient.m +++ b/Bugsnag/Client/BugsnagClient.m @@ -66,6 +66,7 @@ #import "BugsnagSession+Private.h" #import "BugsnagSessionTracker+Private.h" #import "BugsnagSessionTrackingApiClient.h" +#import "BugsnagStackframe+Private.h" #import "BugsnagStateEvent.h" #import "BugsnagSystemState.h" #import "BugsnagThread+Private.h" @@ -870,18 +871,14 @@ - (void)notify:(NSException *)exception NSArray *callStack = exception.callStackReturnAddresses; if (!callStack.count) { + // If the NSException was not raised by the Objective-C runtime, it will be missing a call stack. + // Use the current call stack instead. callStack = BSGArraySubarrayFromIndex(NSThread.callStackReturnAddresses, depth); } BOOL recordAllThreads = self.configuration.sendThreads == BSGThreadSendPolicyAlways; - NSArray *threads = [BugsnagThread allThreads:recordAllThreads callStackReturnAddresses:callStack]; + NSArray *threads = recordAllThreads ? [BugsnagThread allThreads:YES callStackReturnAddresses:callStack] : @[]; - NSArray *stacktrace = nil; - for (BugsnagThread *thread in threads) { - if (thread.errorReportingThread) { - stacktrace = thread.stacktrace; - break; - } - } + NSArray *stacktrace = [BugsnagStackframe stackframesWithCallStackReturnAddresses:callStack]; BugsnagError *error = [[BugsnagError alloc] initWithErrorClass:exception.name ?: NSStringFromClass([exception class]) errorMessage:exception.reason ?: @"" diff --git a/Bugsnag/Payload/BugsnagError+Private.h b/Bugsnag/Payload/BugsnagError+Private.h index ec65da698..a9ec9ebf5 100644 --- a/Bugsnag/Payload/BugsnagError+Private.h +++ b/Bugsnag/Payload/BugsnagError+Private.h @@ -14,7 +14,7 @@ NS_ASSUME_NONNULL_BEGIN @interface BugsnagError () -- (instancetype)initWithEvent:(NSDictionary *)event errorReportingThread:(nullable BugsnagThread *)thread; +- (instancetype)initWithKSCrashReport:(NSDictionary *)event stacktrace:(NSArray *)stacktrace; - (instancetype)initWithErrorClass:(NSString *)errorClass errorMessage:(NSString *)errorMessage diff --git a/Bugsnag/Payload/BugsnagError.m b/Bugsnag/Payload/BugsnagError.m index 1f302a0c9..4b7858e54 100644 --- a/Bugsnag/Payload/BugsnagError.m +++ b/Bugsnag/Payload/BugsnagError.m @@ -83,11 +83,7 @@ @implementation BugsnagError @dynamic type; -- (instancetype)initWithErrorReportingThread:(BugsnagThread *)thread { - return [self initWithEvent:@{} errorReportingThread:thread]; -} - -- (instancetype)initWithEvent:(NSDictionary *)event errorReportingThread:(BugsnagThread *)thread { +- (instancetype)initWithKSCrashReport:(NSDictionary *)event stacktrace:(NSArray *)stacktrace { if (self = [super init]) { NSDictionary *error = [event valueForKeyPath:@"crash.error"]; NSString *errorType = error[BSGKeyType]; @@ -96,7 +92,7 @@ - (instancetype)initWithEvent:(NSDictionary *)event errorReportingThread:(Bugsna _typeString = BSGSerializeErrorType(BSGErrorTypeCocoa); if (![[event valueForKeyPath:@"user.state.didOOM"] boolValue]) { - _stacktrace = thread.stacktrace; + _stacktrace = stacktrace; } } return self; diff --git a/Bugsnag/Payload/BugsnagEvent.m b/Bugsnag/Payload/BugsnagEvent.m index 05aab39ee..6af0e0250 100644 --- a/Bugsnag/Payload/BugsnagEvent.m +++ b/Bugsnag/Payload/BugsnagEvent.m @@ -310,12 +310,9 @@ - (instancetype)initWithKSCrashData:(NSDictionary *)event { // generate threads/error info NSArray *binaryImages = event[@"binary_images"]; NSArray *threadDict = [event valueForKeyPath:@"crash.threads"]; - NSMutableArray *threads = [BugsnagThread threadsFromArray:threadDict - binaryImages:binaryImages - depth:depth - errorType:errorType]; + NSArray *threads = [BugsnagThread threadsFromArray:threadDict binaryImages:binaryImages depth:depth errorType:errorType]; - BugsnagThread *errorReportingThread; + BugsnagThread *errorReportingThread = nil; for (BugsnagThread *thread in threads) { if (thread.errorReportingThread) { errorReportingThread = thread; @@ -323,7 +320,13 @@ - (instancetype)initWithKSCrashData:(NSDictionary *)event { } } - NSArray *errors = @[[[BugsnagError alloc] initWithEvent:event errorReportingThread:errorReportingThread]]; + NSArray *errors = @[[[BugsnagError alloc] initWithKSCrashReport:event stacktrace:errorReportingThread.stacktrace ?: @[]]]; + + // KSCrash captures only the offending thread when sendThreads = BSGThreadSendPolicyNever. + // The BugsnagEvent should not contain threads in this case, only the stacktrace. + if (threads.count == 1) { + threads = @[]; + } if (errorReportingThread.crashInfoMessage) { [errors[0] updateWithCrashInfoMessage:(NSString * _Nonnull)errorReportingThread.crashInfoMessage]; diff --git a/CHANGELOG.md b/CHANGELOG.md index 6054134c6..682f3289e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,13 @@ Changelog ========= +## TBD + +### Bug fixes + +* `event.threads` will now be empty, rather than containing a single thread, if `sendThreads` dictates that threads should not be sent. + [#1077](https://github.com/bugsnag/bugsnag-cocoa/pull/1077) + ## 6.9.0 (2021-04-21) ### Enhancements diff --git a/Tests/BugsnagErrorTest.m b/Tests/BugsnagErrorTest.m index 996c37fbe..ba18fde06 100644 --- a/Tests/BugsnagErrorTest.m +++ b/Tests/BugsnagErrorTest.m @@ -65,7 +65,7 @@ - (void)setUp { - (void)testErrorLoad { BugsnagThread *thread = [self findErrorReportingThread:self.event]; - BugsnagError *error = [[BugsnagError alloc] initWithEvent:self.event errorReportingThread:thread]; + BugsnagError *error = [[BugsnagError alloc] initWithKSCrashReport:self.event stacktrace:thread.stacktrace]; XCTAssertEqualObjects(@"Foo Exception", error.errorClass); XCTAssertEqualObjects(@"Foo overload", error.errorMessage); XCTAssertEqual(BSGErrorTypeCocoa, error.type); @@ -79,7 +79,7 @@ - (void)testErrorLoad { - (void)testToDictionary { BugsnagThread *thread = [self findErrorReportingThread:self.event]; - BugsnagError *error = [[BugsnagError alloc] initWithEvent:self.event errorReportingThread:thread]; + BugsnagError *error = [[BugsnagError alloc] initWithKSCrashReport:self.event stacktrace:thread.stacktrace]; NSDictionary *dict = [error toDictionary]; XCTAssertEqualObjects(@"Foo Exception", dict[@"errorClass"]); XCTAssertEqualObjects(@"Foo overload", dict[@"message"]); @@ -141,7 +141,7 @@ - (void)testErrorMessageParse { - (void)testStacktraceOverride { BugsnagThread *thread = [self findErrorReportingThread:self.event]; - BugsnagError *error = [[BugsnagError alloc] initWithEvent:self.event errorReportingThread:thread]; + BugsnagError *error = [[BugsnagError alloc] initWithKSCrashReport:self.event stacktrace:thread.stacktrace]; XCTAssertNotNil(error.stacktrace); XCTAssertEqual(1, error.stacktrace.count); error.stacktrace = @[]; diff --git a/features/barebone_tests.feature b/features/barebone_tests.feature index 4ccc8e704..8ffbb0498 100644 --- a/features/barebone_tests.feature +++ b/features/barebone_tests.feature @@ -61,10 +61,6 @@ Feature: Barebone tests And the event "severity" equals "warning" And the event "severityReason.type" equals "handledException" And the event "severityReason.unhandledOverridden" is true - And the event "threads.0.errorReportingThread" is true - And the event "threads.0.id" equals "0" - And the event "threads.0.name" equals "com.apple.main-thread" - And the event "threads.0.stacktrace.0.method" matches "BareboneTestHandledScenario" And the event "unhandled" is true And the event "user.email" equals "foobar@example.com" And the event "user.id" equals "foobar" @@ -79,7 +75,7 @@ Feature: Barebone tests And the error payload field "events.0.device.freeMemory" is an integer And the error payload field "events.0.device.model" matches the test device model And the error payload field "events.0.device.totalMemory" is an integer - And the error payload field "events.0.threads" is an array with 1 elements + And the error payload field "events.0.threads" is an array with 0 elements And the "method" of stack frame 0 matches "BareboneTestHandledScenario" And I discard the oldest error @@ -158,6 +154,8 @@ Feature: Barebone tests And the error payload field "events.0.device.freeMemory" is an integer And the error payload field "events.0.device.model" matches the test device model And the error payload field "events.0.device.totalMemory" is an integer + And the error payload field "events.0.threads" is a non-empty array + And the error payload field "events.0.threads.1" is not null And the "method" of stack frame 0 matches "(assertionFailure|)" @skip_macos @@ -246,3 +244,4 @@ Feature: Barebone tests And the error payload field "events.0.device.freeMemory" is null And the error payload field "events.0.device.model" matches the test device model And the error payload field "events.0.device.totalMemory" is an integer + And the error payload field "events.0.threads" is an array with 0 elements diff --git a/features/threads.feature b/features/threads.feature index 9f43e622f..75dd8415b 100644 --- a/features/threads.feature +++ b/features/threads.feature @@ -1,4 +1,4 @@ -Feature: Handled Errors and Exceptions +Feature: Threads Background: Given I clear all persistent data @@ -10,6 +10,8 @@ Feature: Handled Errors and Exceptions And the event "unhandled" is false And the error payload field "events" is an array with 1 elements And the exception "message" equals "HandledErrorThreadSendAlwaysScenario" + And the error payload field "events.0.threads" is a non-empty array + And the error payload field "events.0.threads.1" is not null And the thread information is valid for the event Scenario: Threads are captured for unhandled errors by default @@ -20,6 +22,8 @@ Feature: Handled Errors and Exceptions And the event "unhandled" is true And the error payload field "events" is an array with 1 elements And the exception "message" equals "UnhandledErrorThreadSendAlwaysScenario" + And the error payload field "events.0.threads" is a non-empty array + And the error payload field "events.0.threads.1" is not null And the thread information is valid for the event Scenario: Threads are not captured for handled errors when sendThreads is set to unhandled_only @@ -29,12 +33,7 @@ Feature: Handled Errors and Exceptions And the event "unhandled" is false And the error payload field "events" is an array with 1 elements And the exception "errorClass" equals "HandledErrorThreadSendUnhandledOnlyScenario" - And the error payload field "events.0.threads" is an array with 1 elements - And the error payload field "events.0.threads.0.errorReportingThread" is true - And the error payload field "events.0.threads.0.id" is not null - And the error payload field "events.0.threads.0.name" equals "com.apple.main-thread" - And the error payload field "events.0.threads.0.type" equals "cocoa" - And the thread information is valid for the event + And the error payload field "events.0.threads" is an array with 0 elements Scenario: Threads are not captured for unhandled errors when sendThreads is set to never When I run "UnhandledErrorThreadSendNeverScenario" and relaunch the app @@ -44,9 +43,4 @@ Feature: Handled Errors and Exceptions And the event "unhandled" is true And the error payload field "events" is an array with 1 elements And the exception "message" equals "UnhandledErrorThreadSendNeverScenario" - And the error payload field "events.0.threads" is an array with 1 elements - And the error payload field "events.0.threads.0.errorReportingThread" is true - And the error payload field "events.0.threads.0.id" is not null - And the error payload field "events.0.threads.0.name" is null - And the error payload field "events.0.threads.0.type" equals "cocoa" - And the thread information is valid for the event + And the error payload field "events.0.threads" is an array with 0 elements