fix: Use exception stack when available

For thrown exceptions, use the exception stack trace rather than a
generated one to allow notify() to be called from a different thread or
handler.

CHANGELOG.md

@@ -1,6 +1,20 @@
 Changelog
 =========
 
+## TBD
+
+### Bug fixes
+
+* Fix generating an incorrect stacktrace used when logging an exception to
+  Bugsnag from a location other than the original call site (for example, from a
+  logging function or across threads).  If an exception was raised/thrown, then
+  the resulting Bugsnag report from `notify()` will now use the `NSException`
+  instance's call stack addresses to construct the stacktrace, ignoring depth.
+  This fixes an issue in macOS exception reporting where `reportException` is
+  reporting the handler code stacktrace rather than the reported exception
+  stack.
+  [#334](https://github.com/bugsnag/bugsnag-cocoa/pull/334)
+
 ## 5.19.0 (2019-02-28)
 
 Note for Carthage users: this release updates the Xcode configuration to the settings recommended by Xcode 10.

Source/BugsnagCrashReport.h

@@ -181,7 +181,8 @@ __deprecated_msg("Use toJson: instead.");
  */
 @property(readonly, copy, nonnull) NSDictionary *overrides;
 /**
- *  Number of frames to discard at the top of the stacktrace
+ *  Number of frames to discard at the top of the generated stacktrace.
+ *  Stacktraces from raised exceptions are unaffected.
  */
 @property(readwrite) NSUInteger depth;
 /**

Source/BugsnagCrashSentry.h

@@ -20,6 +20,7 @@
 
 - (void)reportUserException:(NSString *)reportName
                      reason:(NSString *)reportMessage
+          originalException:(NSException *)ex
                handledState:(NSDictionary *)handledState
                    appState:(NSDictionary *)appState
           callbackOverrides:(NSDictionary *)overrides

Source/BugsnagCrashSentry.m

@@ -43,6 +43,7 @@ - (void)install:(BugsnagConfiguration *)config
 
 - (void)reportUserException:(NSString *)reportName
                      reason:(NSString *)reportMessage
+          originalException:(NSException *)ex
                handledState:(NSDictionary *)handledState
                    appState:(NSDictionary *)appState
           callbackOverrides:(NSDictionary *)overrides
@@ -52,6 +53,7 @@ - (void)reportUserException:(NSString *)reportName
 
     [[BSG_KSCrash sharedInstance] reportUserException:reportName
                                                reason:reportMessage
+                                    originalException:ex
                                          handledState:handledState
                                              appState:appState
                                     callbackOverrides:overrides

Source/BugsnagNotifier.m

@@ -443,8 +443,10 @@ - (void)notifyError:(NSError *)error
         [BugsnagHandledState handledStateWithSeverityReason:HandledError
                                                    severity:BSGSeverityWarning
                                                   attrValue:error.domain];
-    [self notify:NSStringFromClass([error class])
-             message:error.localizedDescription
+    NSException *wrapper = [NSException exceptionWithName:NSStringFromClass([error class])
+                                                   reason:error.localizedDescription
+                                                 userInfo:error.userInfo];
+    [self notify:wrapper
         handledState:state
                block:^(BugsnagCrashReport *_Nonnull report) {
                  NSMutableDictionary *metadata = [report.metaData mutableCopy];
@@ -472,20 +474,14 @@ - (void)notifyException:(NSException *)exception
         handledStateWithSeverityReason:UserSpecifiedSeverity
                               severity:severity
                              attrValue:nil];
-    [self notify:exception.name ?: NSStringFromClass([exception class])
-             message:exception.reason
-        handledState:state
-               block:block];
+    [self notify:exception handledState:state block:block];
 }
 
 - (void)notifyException:(NSException *)exception
                   block:(void (^)(BugsnagCrashReport *))block {
     BugsnagHandledState *state =
         [BugsnagHandledState handledStateWithSeverityReason:HandledException];
-    [self notify:exception.name ?: NSStringFromClass([exception class])
-             message:exception.reason
-        handledState:state
-               block:block];
+    [self notify:exception handledState:state block:block];
 }
 
 - (void)internalClientNotify:(NSException *_Nonnull)exception
@@ -506,20 +502,14 @@ - (void)internalClientNotify:(NSException *_Nonnull)exception
                               severity:BSGParseSeverity(severity)
                              attrValue:logLevel];
 
-    [self notify:exception.name ?: NSStringFromClass([exception class])
-             message:exception.reason
-        handledState:state
-               block:^(BugsnagCrashReport *_Nonnull report) {
-                 if (block) {
-                     block(report);
-                 }
-               }];
+    [self notify:exception handledState:state block:block];
 }
 
-- (void)notify:(NSString *)exceptionName
-         message:(NSString *)message
+- (void)notify:(NSException *)exception
     handledState:(BugsnagHandledState *_Nonnull)handledState
            block:(void (^)(BugsnagCrashReport *))block {
+    NSString *exceptionName = exception.name ?: NSStringFromClass([exception class]);
+    NSString *message = exception.reason;
     [self.sessionTracker handleHandledErrorEvent];
 
     BugsnagCrashReport *report = [[BugsnagCrashReport alloc]
@@ -550,6 +540,7 @@ - (void)notify:(NSString *)exceptionName
 
     [self.crashSentry reportUserException:reportName
                                    reason:reportMessage
+                        originalException:exception
                              handledState:[handledState toJson]
                                  appState:[self.state toDictionary]
                         callbackOverrides:report.overrides

Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.h

@@ -173,6 +173,7 @@ typedef enum {
  *
  * @param name The exception name (for namespacing exception types).
  * @param reason A description of why the exception occurred
+ * @param exception The exception which was thrown (if any)
  * @param handledState The severity, reason, and handled-ness of the report
  * @param appState breadcrumbs and other app environmental info
  * @param overrides Report fields overridden by callbacks, collated in the
@@ -185,6 +186,7 @@ typedef enum {
  */
 - (void)reportUserException:(NSString *)name
                      reason:(NSString *)reason
+          originalException:(NSException *)exception
                handledState:(NSDictionary *)handledState
                    appState:(NSDictionary *)appState
           callbackOverrides:(NSDictionary *)overrides

Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.m

@@ -347,6 +347,7 @@ - (void)deleteAllReports {
 
 - (void)reportUserException:(NSString *)name
                      reason:(NSString *)reason
+          originalException:(NSException *)exception
                handledState:(NSDictionary *)handledState
                    appState:(NSDictionary *)appState
           callbackOverrides:(NSDictionary *)overrides
@@ -356,7 +357,17 @@ - (void)reportUserException:(NSString *)name
            terminateProgram:(BOOL)terminateProgram {
     const char *cName = [name cStringUsingEncoding:NSUTF8StringEncoding];
     const char *cReason = [reason cStringUsingEncoding:NSUTF8StringEncoding];
+    NSArray *addresses = [exception callStackReturnAddresses];
+    NSUInteger numFrames = [addresses count];
+    uintptr_t *callstack = malloc(numFrames * sizeof(*callstack));
+    for (NSUInteger i = 0; i < numFrames; i++) {
+        callstack[i] = [addresses[i] unsignedLongValue];
+    }
+    if (numFrames > 0) {
+        depth = 0; // reset depth if the stack does not need to be generated
+    }
     bsg_kscrash_reportUserException(cName, cReason,
+                                    callstack, numFrames,
                                     [handledState[@"currentSeverity"] UTF8String],
                                     [self encodeAsJSONString:handledState],
                                     [self encodeAsJSONString:overrides],

Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.c

@@ -286,6 +286,8 @@ void bsg_kscrash_setCrashNotifyCallback(
 }
 
 void bsg_kscrash_reportUserException(const char *name, const char *reason,
+                                     uintptr_t *stackAddresses,
+                                     unsigned long stackLength,
                                      const char *severity,
                                      const char *handledState,
                                      const char *overrides,
@@ -294,7 +296,11 @@ void bsg_kscrash_reportUserException(const char *name, const char *reason,
                                      const char *config,
                                      int discardDepth,
                                      bool terminateProgram) {
-    bsg_kscrashsentry_reportUserException(name, reason, severity, handledState, overrides,
+    bsg_kscrashsentry_reportUserException(name, reason,
+                                          stackAddresses,
+                                          stackLength,
+                                          severity,
+                                          handledState, overrides,
                                           metadata, appState, config, discardDepth,
                                           terminateProgram);
 }

Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.h

@@ -176,6 +176,8 @@ void bsg_kscrash_setCrashNotifyCallback(
  *
  * @param name The exception name (for namespacing exception types).
  * @param reason A description of why the exception occurred.
+ * @param stackAddresses An array of addresses or NULL
+ * @param stackLength The number of addresses in stackAddresses
  * @param handledState The severity, reason, and handled-ness of the report
  * @param appState breadcrumbs and other app environmental info
  * @param overrides Report fields overridden by callbacks, collated in the
@@ -187,6 +189,8 @@ void bsg_kscrash_setCrashNotifyCallback(
  * Terminate the program instead.
  */
 void bsg_kscrash_reportUserException(const char *name, const char *reason,
+                                     uintptr_t *stackAddresses,
+                                     unsigned long stackLength,
                                      const char *severity,
                                      const char *handledState,
                                      const char *overrides,

Source/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry_User.c

@@ -47,7 +47,11 @@ void bsg_kscrashsentry_uninstallUserExceptionHandler(void) {
     bsg_g_context = NULL;
 }
 
-void bsg_kscrashsentry_reportUserException(const char *name, const char *reason,
+
+void bsg_kscrashsentry_reportUserException(const char *name,
+                                           const char *reason,
+                                           uintptr_t *stackAddresses,
+                                           unsigned long stackLength,
                                            const char *severity,
                                            const char *handledState,
                                            const char *overrides,
@@ -67,23 +71,30 @@ void bsg_kscrashsentry_reportUserException(const char *name, const char *reason,
             bsg_kscrashsentry_suspendThreads();
         }
 
-        BSG_KSLOG_DEBUG("Fetching call stack.");
-        int callstackCount = 100;
-        uintptr_t callstack[callstackCount];
-        callstackCount = backtrace((void **)callstack, callstackCount);
-        if (callstackCount <= 0) {
-            BSG_KSLOG_ERROR("backtrace() returned call stack length of %d",
-                            callstackCount);
-            callstackCount = 0;
+
+        if (stackAddresses != NULL && stackLength > 0) {
+            bsg_g_context->stackTrace = stackAddresses;
+            bsg_g_context->stackTraceLength = (int)stackLength;
+        } else {
+            BSG_KSLOG_DEBUG("Fetching call stack.");
+            int callstackCount = 100;
+            uintptr_t callstack[callstackCount];
+            callstackCount = backtrace((void **)callstack, callstackCount);
+            if (callstackCount <= 0) {
+                BSG_KSLOG_ERROR("backtrace() returned call stack length of %d",
+                                callstackCount);
+                callstackCount = 0;
+            }
+            BSG_KSLOG_DEBUG("Filling out stack context entries.");
+            bsg_g_context->stackTrace = callstack;
+            bsg_g_context->stackTraceLength = callstackCount;
         }
 
         BSG_KSLOG_DEBUG("Filling out context.");
         bsg_g_context->crashType = BSG_KSCrashTypeUserReported;
         bsg_g_context->offendingThread = bsg_ksmachthread_self();
         bsg_g_context->registersAreValid = false;
         bsg_g_context->crashReason = reason;
-        bsg_g_context->stackTrace = callstack;
-        bsg_g_context->stackTraceLength = callstackCount;
         bsg_g_context->userException.name = name;
         bsg_g_context->userException.handledState = handledState;
         bsg_g_context->userException.overrides = overrides;

Source/KSCrash/Source/KSCrash/Recording/Sentry/BSG_KSCrashSentry_User.h

@@ -54,7 +54,8 @@ void bsg_kscrashsentry_uninstallUserExceptionHandler(void);
  * @param name The exception name (for namespacing exception types).
  *
  * @param reason A description of why the exception occurred.
- *
+ * @param stackAddresses An array of addresses or NULL
+ * @param stackLength The number of addresses in stackAddresses
  * @param handledState The severity, reason, and handled-ness of the report
  * @param appState breadcrumbs and other app environmental info
  * @param overrides Report fields overridden by callbacks, collated in the
@@ -67,6 +68,8 @@ void bsg_kscrashsentry_uninstallUserExceptionHandler(void);
  * Terminate the program instead.
  */
     void bsg_kscrashsentry_reportUserException(const char *name, const char *reason,
+                                               uintptr_t *stackAddresses,
+                                               unsigned long stackLength,
                                                const char *severity,
                                                const char *handledState,
                                                const char *overrides,

features/fixtures/ios-swift-cocoapods/iOSTestApp.xcodeproj/project.pbxproj

@@ -7,6 +7,7 @@
 	objects = {
 
 /* Begin PBXBuildFile section */
+		8A32DB8222424E3000EDD92F /* NSExceptionShiftScenario.m in Sources */ = {isa = PBXBuildFile; fileRef = 8A32DB8122424E3000EDD92F /* NSExceptionShiftScenario.m */; };
 		8A840FBA21AF5C450041DBFA /* SwiftAssertion.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8A840FB921AF5C450041DBFA /* SwiftAssertion.swift */; };
 		8A98400320FD11BF0023ECD1 /* AutoSessionCustomVersionScenario.m in Sources */ = {isa = PBXBuildFile; fileRef = 8A98400220FD11BF0023ECD1 /* AutoSessionCustomVersionScenario.m */; };
 		8AB8866420404DD30003E444 /* AppDelegate.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8AB8866320404DD30003E444 /* AppDelegate.swift */; };
@@ -58,6 +59,8 @@
 
 /* Begin PBXFileReference section */
 		4994F05E0421A0B037DD2CC5 /* Pods_iOSTestApp.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = Pods_iOSTestApp.framework; sourceTree = BUILT_PRODUCTS_DIR; };
+		8A32DB8022424E3000EDD92F /* NSExceptionShiftScenario.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = NSExceptionShiftScenario.h; sourceTree = "<group>"; };
+		8A32DB8122424E3000EDD92F /* NSExceptionShiftScenario.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = NSExceptionShiftScenario.m; sourceTree = "<group>"; };
 		8A840FB921AF5C450041DBFA /* SwiftAssertion.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = SwiftAssertion.swift; sourceTree = "<group>"; };
 		8A98400120FD11BF0023ECD1 /* AutoSessionCustomVersionScenario.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = AutoSessionCustomVersionScenario.h; sourceTree = "<group>"; };
 		8A98400220FD11BF0023ECD1 /* AutoSessionCustomVersionScenario.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = AutoSessionCustomVersionScenario.m; sourceTree = "<group>"; };
@@ -204,6 +207,8 @@
 		F42953DE2BB41023C0B07F41 /* scenarios */ = {
 			isa = PBXGroup;
 			children = (
+				8A32DB8022424E3000EDD92F /* NSExceptionShiftScenario.h */,
+				8A32DB8122424E3000EDD92F /* NSExceptionShiftScenario.m */,
 				8A840FB921AF5C450041DBFA /* SwiftAssertion.swift */,
 				F42957FA1A3724BFBDC22E14 /* NSExceptionScenario.swift */,
 				F4295F595986A279FA3BDEA7 /* UserEmailScenario.swift */,
@@ -410,6 +415,7 @@
 				F4295836C8AF75547C675E8D /* ReleasedObjectScenario.m in Sources */,
 				F42958881D3F34A76CADE4EC /* SwiftCrash.swift in Sources */,
 				F42955DB6D08642528917FAB /* CxxExceptionScenario.mm in Sources */,
+				8A32DB8222424E3000EDD92F /* NSExceptionShiftScenario.m in Sources */,
 				F42954B7318A02824C65C514 /* ObjCMsgSendScenario.m in Sources */,
 				F42953498545B853CC0B635E /* NullPointerScenario.m in Sources */,
 				8A840FBA21AF5C450041DBFA /* SwiftAssertion.swift in Sources */,

features/fixtures/ios-swift-cocoapods/iOSTestApp/scenarios/NSExceptionShiftScenario.h

@@ -0,0 +1,17 @@
+//
+//  NSExceptionShiftScenario.h
+//  macOSTestApp
+//
+//  Created by Delisa on 3/20/19.
+//  Copyright © 2019 Bugsnag Inc. All rights reserved.
+//
+
+#import "Scenario.h"
+
+NS_ASSUME_NONNULL_BEGIN
+
+@interface NSExceptionShiftScenario : Scenario
+
+@end
+
+NS_ASSUME_NONNULL_END

features/fixtures/ios-swift-cocoapods/iOSTestApp/scenarios/NSExceptionShiftScenario.m

@@ -0,0 +1,29 @@
+#import "NSExceptionShiftScenario.h"
+#import <Bugsnag/Bugsnag.h>
+
+@implementation NSExceptionShiftScenario
+
+- (void)startBugsnag {
+    self.config.shouldAutoCaptureSessions = NO;
+    [super startBugsnag];
+}
+
+- (void)run {
+    [self causeAnException];
+}
+
+- (void)causeAnException {
+    @try {
+        @throw [NSException exceptionWithName:@"Tertiary failure"
+                                       reason:@"invalid invariant"
+                                     userInfo:nil];
+    } @catch (NSException *exception) {
+        [self shouldNotBeInStacktrace:exception];
+    }
+}
+
+- (void)shouldNotBeInStacktrace:(NSException *)exception {
+    [Bugsnag notify:exception];
+}
+
+@end

features/handled_errors.feature

@@ -27,3 +27,17 @@ Scenario: Reporting a handled exception
     And the payload field "events" is an array with 1 element
     And the exception "errorClass" equals "HandledExceptionScenario"
     And the exception "message" equals "Message: HandledExceptionScenario"
+
+Scenario: Reporting a handled exception's stacktrace
+    When I run "NSExceptionShiftScenario"
+    Then I should receive a request
+    And the request is a valid for the error reporting API
+    And the "Bugsnag-API-Key" header equals "a35a2a72bd230ac0aa0f52715bbdc6aa"
+    And the payload notifier name is correct
+    And the payload field "events" is an array with 1 element
+    And the exception "errorClass" equals "Tertiary failure"
+    And the exception "message" equals "invalid invariant"
+    And the "method" of stack frame 0 equals "__exceptionPreprocess"
+    And the "method" of stack frame 1 equals "objc_exception_throw"
+    And the "method" of stack frame 2 equals "-[NSExceptionShiftScenario causeAnException]"
+    And the "method" of stack frame 3 equals "-[NSExceptionShiftScenario run]"