fix: Use depth in stacktrace length calc

CHANGELOG.md

@@ -1,6 +1,17 @@
 Changelog
 =========
 
+## TBD
+
+### Bug fixes
+
+* Fix trimming the stacktraces of handled error/exceptions using the
+  [`depth`](https://docs.bugsnag.com/platforms/ios/reporting-handled-exceptions/#depth)
+  property.
+  [Paul Zabelin](https://github.com/paulz)
+  [#363](https://github.com/bugsnag/bugsnag-cocoa/pull/363)
+
+
 ## 5.22.1 (2019-05-21)
 
 * Report correct app version in out-of-memory reports. Previously the bundle

Source/BugsnagCrashReport.m

@@ -290,7 +290,7 @@ - (instancetype)initWithKSReport:(NSDictionary *)report
                     [[BugsnagHandledState alloc] initWithDictionary:recordedState];
 
                 // only makes sense to use serialised value for handled exceptions
-                _depth = [[report valueForKeyPath:@"user.state.crash.depth"]
+                _depth = [[report valueForKeyPath:@"user.depth"]
                         unsignedIntegerValue];
             } else if (_errorType != nil) { // the event was unhandled.
                 BOOL isSignal = [BSGKeySignal isEqualToString:_errorType];

Tests/BugsnagCrashReportTests.m

@@ -16,88 +16,10 @@
 
 
 @interface BugsnagCrashReportTests : XCTestCase
-@property BugsnagCrashReport *report;
 @end
 
 @implementation BugsnagCrashReportTests
 
-- (void)setUp {
-    [super setUp];
-    NSBundle *bundle = [NSBundle bundleForClass:[self class]];
-    NSString *path = [bundle pathForResource:@"report" ofType:@"json"];
-    NSString *contents = [NSString stringWithContentsOfFile:path
-                                                   encoding:NSUTF8StringEncoding
-                                                      error:nil];
-    NSDictionary *dictionary = [NSJSONSerialization
-        JSONObjectWithData:[contents dataUsingEncoding:NSUTF8StringEncoding]
-                   options:0
-                     error:nil];
-    self.report = [[BugsnagCrashReport alloc] initWithKSReport:dictionary];
-}
-
-- (void)tearDown {
-    [super tearDown];
-    self.report = nil;
-}
-
-- (void)testReadReleaseStage {
-    XCTAssertEqualObjects(self.report.releaseStage, @"production");
-}
-
-- (void)testReadNotifyReleaseStages {
-    XCTAssertEqualObjects(self.report.notifyReleaseStages,
-                          (@[ @"production", @"development" ]));
-}
-
-- (void)testReadNotifyReleaseStagesSends {
-    XCTAssertTrue([self.report shouldBeSent]);
-}
-
-- (void)testAddMetadataAddsNewTab {
-    NSDictionary *metadata = @{@"color" : @"blue", @"beverage" : @"tea"};
-    [self.report addMetadata:metadata toTabWithName:@"user prefs"];
-    NSDictionary *prefs = self.report.metaData[@"user prefs"];
-    XCTAssertEqual(@"blue", prefs[@"color"]);
-    XCTAssertEqual(@"tea", prefs[@"beverage"]);
-    XCTAssert([prefs count] == 2);
-}
-
-- (void)testAddMetadataMergesExistingTab {
-    NSDictionary *oldMetadata = @{@"color" : @"red", @"food" : @"carrots"};
-    [self.report addMetadata:oldMetadata toTabWithName:@"user prefs"];
-    NSDictionary *metadata = @{@"color" : @"blue", @"beverage" : @"tea"};
-    [self.report addMetadata:metadata toTabWithName:@"user prefs"];
-    NSDictionary *prefs = self.report.metaData[@"user prefs"];
-    XCTAssertEqual(@"blue", prefs[@"color"]);
-    XCTAssertEqual(@"tea", prefs[@"beverage"]);
-    XCTAssertEqual(@"carrots", prefs[@"food"]);
-    XCTAssert([prefs count] == 3);
-}
-
-- (void)testAddAttributeAddsNewTab {
-    [self.report addAttribute:@"color"
-                    withValue:@"blue"
-                toTabWithName:@"prefs"];
-    NSDictionary *prefs = self.report.metaData[@"prefs"];
-    XCTAssertEqual(@"blue", prefs[@"color"]);
-}
-
-- (void)testAddAttributeOverridesExistingValue {
-    [self.report addAttribute:@"color" withValue:@"red" toTabWithName:@"prefs"];
-    [self.report addAttribute:@"color"
-                    withValue:@"blue"
-                toTabWithName:@"prefs"];
-    NSDictionary *prefs = self.report.metaData[@"prefs"];
-    XCTAssertEqual(@"blue", prefs[@"color"]);
-}
-
-- (void)testAddAttributeRemovesValue {
-    [self.report addAttribute:@"color" withValue:@"red" toTabWithName:@"prefs"];
-    [self.report addAttribute:@"color" withValue:nil toTabWithName:@"prefs"];
-    NSDictionary *prefs = self.report.metaData[@"prefs"];
-    XCTAssertNil(prefs[@"color"]);
-}
-
 - (void)testNotifyReleaseStagesSendsFromConfig {
     BugsnagConfiguration *config = [BugsnagConfiguration new];
     config.notifyReleaseStages = @[ @"foo" ];
@@ -496,15 +418,15 @@ - (void)testEmptyReport {
 
 - (void)testUnhandledReportDepth {
     // unhandled reports should calculate their own depth
-    NSDictionary *dict = @{@"user.state.crash.depth": @2};
+    NSDictionary *dict = @{@"user.depth": @2};
     BugsnagCrashReport *report = [[BugsnagCrashReport alloc] initWithKSReport:dict];
     XCTAssertEqual(report.depth, 0);
 }
 
 - (void)testHandledReportDepth {
     // handled reports should use the serialised depth
     BugsnagHandledState *state = [BugsnagHandledState handledStateWithSeverityReason:HandledException];
-    NSDictionary *dict = @{@"user.state.crash.depth": @2, @"user.handledState": [state toJson]};
+    NSDictionary *dict = @{@"user.depth": @2, @"user.handledState": [state toJson]};
     BugsnagCrashReport *report = [[BugsnagCrashReport alloc] initWithKSReport:dict];
     XCTAssertEqual(report.depth, 2);
 }
@@ -553,12 +475,6 @@ - (void)testNoReportMetaData {
     XCTAssertEqual(report.metaData.count, 0);
 }
 
-- (void)testAppVersion {
-    NSDictionary *dictionary = [self.report toJson];
-    XCTAssertEqualObjects(@"1.0", dictionary[@"app"][@"version"]);
-    XCTAssertEqualObjects(@"1", dictionary[@"app"][@"bundleVersion"]);
-}
-
 - (void)testAppVersionOverride {
     BugsnagCrashReport *overrideReport = [[BugsnagCrashReport alloc] initWithKSReport:@{
             @"system" : @{

Tests/BugsnagSinkTests.m

@@ -125,8 +125,8 @@ - (void)testEventSeverity {
     XCTAssertNotNil(event);
 
     NSString *severity = event[@"severity"];
-    XCTAssertTrue([event[@"unhandled"] boolValue]);
-    XCTAssertEqualObjects(severity, @"error");
+    XCTAssertFalse([event[@"unhandled"] boolValue]);
+    XCTAssertEqualObjects(severity, @"info");
 }
 
 - (void)testEventBreadcrumbs {

Tests/report.json

@@ -2081,6 +2081,13 @@
             "key": "value"
           }
         },
+        "handledState":{
+            "unhandled":false,
+            "currentSeverity":"info",
+            "severityReasonType":"handledError",
+            "originalSeverity":"warning"
+        },
+        "depth":7,
         "config": {
             "releaseStage": "production",
             "notifyReleaseStages": ["production", "development"],

features/fixtures/ios-swift-cocoapods/iOSTestApp/scenarios/HandledErrorOverrideScenario.swift

@@ -7,7 +7,9 @@ import Foundation
 import Bugsnag
 
 /**
- * Sends a handled Error to Bugsnag and overrides the exception name + message
+ Sends a handled Error to Bugsnag and overrides the exception name + message
+ Demonstrates adjusting report depth to exclude common error handling code from grouping
+ See: https://docs.bugsnag.com/platforms/ios-objc/reporting-handled-exceptions/#depth
  */
 class HandledErrorOverrideScenario: Scenario {
 
@@ -16,12 +18,21 @@ class HandledErrorOverrideScenario: Scenario {
       super.startBugsnag()
     }
 
-    override func run() {
-        let error = NSError(domain: "HandledErrorOverrideScenario", code: 100, userInfo: nil)
-        Bugsnag.notifyError(error, block: { report in
+    fileprivate func logError(_ error: Error)  {
+        Bugsnag.notifyError(error) { report in
             report.errorMessage = "Foo"
             report.errorClass = "Bar"
-        })
+            report.depth += 2
+        }
+    }
+
+    private func handleError(_ error: NSError)  {
+        logError(error)
+    }
+
+    override func run() {
+        let error = NSError(domain: "HandledErrorOverrideScenario", code: 100, userInfo: nil)
+        handleError(error)
     }
 
 }

features/handled_errors.feature

@@ -1,13 +1,19 @@
 Feature: Handled Errors and Exceptions
 
-Scenario: Override errorClass and message from a notifyError() callback
+Scenario: Override errorClass and message from a notifyError() callback and discard lines from stack
+
+    Discard 2 lines from the stacktrace, as we have single place to report and log errors, see
+    https://docs.bugsnag.com/platforms/ios-objc/reporting-handled-exceptions/#depth
+    This way top of the stacktrace is not logError but run
+
     When I run "HandledErrorOverrideScenario"
     Then I should receive a request
     And the request is a valid for the error reporting API
     And the exception "errorClass" equals "Bar"
     And the exception "message" equals "Foo"
     And the event "device.time" is within 30 seconds of the current timestamp
-    And the stack trace is an array with 22 stack frames
+    And the "method" of stack frame 0 demangles to "iOSTestApp.HandledErrorOverrideScenario.run() -> ()"
+    And the stack trace is an array with 15 stack frames
 
 Scenario: Reporting an NSError
     When I run "HandledErrorScenario"
@@ -18,7 +24,7 @@ Scenario: Reporting an NSError
     And the payload field "events" is an array with 1 element
     And the exception "errorClass" equals "NSError"
     And the exception "message" equals "The operation couldn’t be completed. (HandledErrorScenario error 100.)"
-    And the stack trace is an array with 22 stack frames
+    And the stack trace is an array with 15 stack frames
 
 Scenario: Reporting a handled exception
     When I run "HandledExceptionScenario"
@@ -29,7 +35,7 @@ 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"
-    And the stack trace is an array with 22 stack frames
+    And the stack trace is an array with 15 stack frames
 
 Scenario: Reporting a handled exception's stacktrace
     When I run "NSExceptionShiftScenario"

features/steps/crash_assertion_steps.rb

@@ -22,17 +22,17 @@
 def assert_exception_matches_12_1(exception, stacktrace)
   case stacktrace.first["method"]
   when "__pthread_kill"
-    assert_equal(exception["errorClass"], "SIGABRT")
-    assert_equal(stacktrace[1]["method"], "abort")
+    assert_equal("SIGABRT", exception["errorClass"])
+    assert_equal("abort", stacktrace[1]["method"])
   when "nanov2_allocate_from_block"
-    assert_equal(exception["errorClass"], "EXC_BAD_INSTRUCTION")
-    assert_equal(stacktrace[1]["method"], "nanov2_allocate")
-    assert_equal(stacktrace[15]["method"], "NSLog")
-    assert_equal(stacktrace[16]["method"], "-[CorruptMallocScenario run]")
+    assert_equal("EXC_BAD_INSTRUCTION", exception["errorClass"])
+    assert_equal("nanov2_allocate", stacktrace[1]["method"])
+    assert_equal("NSLog", stacktrace[15]["method"])
+    assert_equal("-[CorruptMallocScenario run]", stacktrace[16]["method"])
   when "notify_dump_status"
-    assert_equal(exception["errorClass"], "EXC_BAD_ACCESS")
-    assert_equal(stacktrace[10]["method"], "NSLog")
-    assert_equal(stacktrace[11]["method"], "-[CorruptMallocScenario run]")
+    assert_equal("EXC_BAD_ACCESS", exception["errorClass"])
+    assert_equal("NSLog", stacktrace[10]["method"])
+    assert_equal("-[CorruptMallocScenario run]", stacktrace[11]["method"])
   else
     fail("The exception does not reflect malloc corruption")
   end
@@ -58,16 +58,16 @@ def assert_exception_matches_11_2(exception, stacktrace)
       frame = 2
     end
 
-    assert_equal(stacktrace[frame]["method"], "notify_check")
-    assert_equal(stacktrace[frame + 1]["method"], "notify_check_tz")
-    assert_equal(stacktrace[frame + 2]["method"], "tzsetwall_basic")
-    assert_equal(stacktrace[frame + 3]["method"], "localtime_r")
-    assert_equal(stacktrace[frame + 4]["method"], "_populateBanner")
-    assert_equal(stacktrace[frame + 5]["method"], "_CFLogvEx2Predicate")
-    assert_equal(stacktrace[frame + 6]["method"], "_CFLogvEx3")
-    assert_equal(stacktrace[frame + 7]["method"], "_NSLogv")
-    assert_equal(stacktrace[frame + 8]["method"], "NSLog")
-    assert_equal(stacktrace[frame + 9]["method"], "-[CorruptMallocScenario run]")
+    assert_equal("notify_check", stacktrace[frame]["method"])
+    assert_equal("notify_check_tz", stacktrace[frame + 1]["method"])
+    assert_equal("tzsetwall_basic", stacktrace[frame + 2]["method"])
+    assert_equal("localtime_r", stacktrace[frame + 3]["method"])
+    assert_equal("_populateBanner", stacktrace[frame + 4]["method"])
+    assert_equal("_CFLogvEx2Predicate", stacktrace[frame + 5]["method"])
+    assert_equal("_CFLogvEx3", stacktrace[frame + 6]["method"])
+    assert_equal("_NSLogv", stacktrace[frame + 7]["method"])
+    assert_equal("NSLog", stacktrace[frame + 8]["method"])
+    assert_equal("-[CorruptMallocScenario run]", stacktrace[frame + 9]["method"])
   else
     fail("The exception does not reflect malloc corruption")
   end

features/steps/ios_steps.rb

@@ -120,9 +120,15 @@
   assert_not_nil(match, "No crumb matches the provided message")
 end
 
+Then("the {string} of stack frame {int} demangles to {string}") do |field, frame_index, expected_value|
+  value = read_key_path(find_request(0)[:body], "events.0.exceptions.0.stacktrace.#{frame_index}.#{field}")
+  demangled_value = `xcrun swift-demangle -compact '#{value}'`.chomp
+  assert_equal(expected_value, demangled_value)
+end
+
 Then("the stack trace is an array with {int} stack frames") do |expected_length|
   stack_trace = read_key_path(find_request(0)[:body], "events.0.exceptions.0.stacktrace")
-  assert_equal(stack_trace.length, expected_length)
+  assert_equal(expected_length,  stack_trace.length)
 end
 Then("the payload field {string} equals the device version") do |field|
   value = read_key_path(find_request(0)[:body], field)