Merge pull request #1077 from bugsnag/nickdowell/fix-send-threads

[PLAT-4706] Fix thread sending behaviour

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<NSNumber *> *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<BugsnagStackframe *> *stacktrace = nil;
-    for (BugsnagThread *thread in threads) {
-        if (thread.errorReportingThread) {
-            stacktrace = thread.stacktrace;
-            break;
-        }
-    }
+    NSArray<BugsnagStackframe *> *stacktrace = [BugsnagStackframe stackframesWithCallStackReturnAddresses:callStack];
 
     BugsnagError *error = [[BugsnagError alloc] initWithErrorClass:exception.name ?: NSStringFromClass([exception class])
                                                       errorMessage:exception.reason ?: @""

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<BugsnagStackframe *> *)stacktrace;
 
 - (instancetype)initWithErrorClass:(NSString *)errorClass
                       errorMessage:(NSString *)errorMessage

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<BugsnagStackframe *> *)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;

Bugsnag/Payload/BugsnagEvent.m

@@ -310,20 +310,23 @@ - (instancetype)initWithKSCrashData:(NSDictionary *)event {
     // generate threads/error info
     NSArray *binaryImages = event[@"binary_images"];
     NSArray *threadDict = [event valueForKeyPath:@"crash.threads"];
-    NSMutableArray<BugsnagThread *> *threads = [BugsnagThread threadsFromArray:threadDict
-                                                                  binaryImages:binaryImages
-                                                                         depth:depth
-                                                                     errorType:errorType];
+    NSArray<BugsnagThread *> *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;
             break;
         }
     }
 
-    NSArray<BugsnagError *> *errors = @[[[BugsnagError alloc] initWithEvent:event errorReportingThread:errorReportingThread]];
+    NSArray<BugsnagError *> *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];

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

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 = @[];

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|<redacted>)"
 
   @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

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