-
Notifications
You must be signed in to change notification settings - Fork 129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Use exception stack when available #334
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this alter the depth of the stack, as there's no longer an extra block? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, technically, however for how we use it, no. This function is only used in cocoa apps in react native, where a custom stacktrace is attached, disregarding depth altogether. And as it isn't a public interface, there should be no other effects. |
||
} | ||
|
||
- (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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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++) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does this perform with very large stacktraces e.g. recursion gone wrong? The previous implementation has a limit of 100 frames. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unlimited stack errors can’t be caught and notified. This implementation is more in line with uncaught NSException handling. |
||
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], | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also update our docs so they state this.