-
Notifications
You must be signed in to change notification settings - Fork 130
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
Conversation
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.
6aaeed5
to
2fb60c2
Compare
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.
I left a few comments inline, once those are resolved/addressed I'd be happy with the implementation.
@@ -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. |
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.
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 comment
The 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 comment
The 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.
block(report); | ||
} | ||
}]; | ||
[self notify:exception handledState:state block:block]; |
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.
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 comment
The 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.
Goal
Improve the debugging experience for caught
NSException
s by providing the exception stack rather than a stack generated at the timenotify()
is called.Design
If there is a stacktrace attached to an exception, uses the exception stack rather than generating a new one by passing
-[NSException callStackReturnAddresses]
as auintptr_t
array toreportUserException
. If the exception has not been thrown or the call stack addresses array is null, a stacktrace is generated as before.Changeset
depth
argument to only affect generated stacktracesTests
Review
Outstanding Questions