Fix SDK crash when Error.prepareStackTrace returns a non-string #254
Conversation
| orig['prepareStackTrace'] = (e, s) => { | ||
| // Remove some AI and Zone methods from the stack trace | ||
| // Otherwise we leave side-effects | ||
| s.splice(0, 3); |
There was a problem hiding this comment.
Does s always have three methods? Is it possible that we are removing too much?
There was a problem hiding this comment.
I don't expect the first 3 methods to change without a code change on our side, since they're all basically passing around our error handlers and zone error handlers. (In other words, the whole top of the stack is just AI SDK and Zone) If we ever change how our error handling works we'll have to adjust this number.
There was a problem hiding this comment.
I've added a UT to check if this number is correct
| @@ -168,7 +168,25 @@ export class CorrelationContextManager { | |||
| return AppInsightsAsyncCorrelatedErrorWrapper.apply(Object.create(AppInsightsAsyncCorrelatedErrorWrapper.prototype), arguments); | |||
| } | |||
|
|
|||
| // Is this object set to rewrite the stack? | |||
| // If so, we should turn off some Zone stuff that is prone to break | |||
| var stackRewrite = orig['stackRewrite']; | |||
There was a problem hiding this comment.
Make "strackRewrite" and "prepare" constants?
There was a problem hiding this comment.
I think that might be confusing for this use case. Perhaps I should use dot notation instead and cast to any. The only reason I'm using this bracket notation is to avoid typescript errors on these hidden properties and methods
| orig['prepareStackTrace'] = (e, s) => { | ||
| // Remove some AI and Zone methods from the stack trace | ||
| // Otherwise we leave side-effects | ||
| s.splice(0, 3); |
There was a problem hiding this comment.
what is 3? Trying to see how hard it will be for someone unfamiliar with zone to support this later
There was a problem hiding this comment.
3 here is the number of methods we add to the stack trace. The library that we crash on will still fail (though not crash!) if we don't remove ourselves from the trace
There was a problem hiding this comment.
I've added a UT to check if this number is correct. If we ever add more methods that make this magic number incorrect, the UT will fail, making it obvious this needs to change.
| orig.apply(this, arguments); | ||
|
|
||
| // Restore Zone stack rewriting settings | ||
| orig['stackRewrite'] = stackRewrite; |
There was a problem hiding this comment.
do we want to wrap it into try...finally to restore orig always?
There was a problem hiding this comment.
Good idea! I'm not sure how the program can really proceed if apply fails however, since it cannot at that point construct the error object. Apply failing is what caused the original crash as well. Any ideas on what object to return if we do get an exception?
This turns off Zone stack-rewriting which can't handle non-default stack trace formats. This resolves an incompatibility with "caller", which when used in conjunction with this SDK would crash upon use.