ref(parser): limit max stack lines we parse #7410
Conversation
size-limit report 📦
|
packages/utils/src/stacktrace.ts
Outdated
| @@ -79,7 +86,7 @@ export function stripSentryFramesAndReverse(stack: StackFrame[]): StackFrame[] { | |||
|
|
|||
| // The frame where the crash happened, should be the last entry in the array | |||
| return localStack | |||
| .slice(0, STACKTRACE_LIMIT) | |||
There was a problem hiding this comment.
@AbhiPrasad @timfish am I right to assume that the slice should no longer be required as everything will run through the stack parser on L19? Not sure if we can remove it, or if there is some other code path that could call reverseSentryFrames fn. I kept it as the fn is exported, so better be safe than sorry here
There was a problem hiding this comment.
yeah, we can probably remove the slice, but perhaps someone depends on this behaviour.
We could just mark it to be removed in the next major version with a TODO.
There was a problem hiding this comment.
We do use stripSentryFramesAndReverse in the Electron SDK so we can combine two parsers:
https://github.com/getsentry/sentry-electron/blob/0d2eb16bf8daf9e330da85a9a823d20c80ad16ac/src/renderer/stack-parse.ts
However as long as I remember when updating, I can update the above code to drop out of the for loop at STACKTRACE_LIMIT
There was a problem hiding this comment.
A further search across GitHub suggests this is only used in the Electron SDK. I've opened an issue to track this so feel free to remove the slice here and I'll fix this downstream on the next javascript sdks update!
There was a problem hiding this comment.
I am somewhat still not super happy with removing it entirely - it is relatively low cost and provides safety. The risk/reward does not seem worth it to me. If we are to remove it then I think we should do it in the next major just in case someone out there relies on this behavior that our search did not show missed.
I would however propose we make the slice conditional and only do it if the input exceeds the max array size, that seems like a good middle ground without breaking anything.
Thoughts?
There was a problem hiding this comment.
Actually, my point is moot because the input is a ReadonlyArray<StackFrame> and since our implementation is reversing the array, a copy is required regardless so that we do not end up mutating the input.
I made the changes to use readonly type in #7417 as it seemed reasonable and safer (the implementation was already considering it as readonly).
It becomes a larger change in terms of how we expect the function to be called and what safety guarantees we want, e.g. if we only expect it to be called once (which seems to be the case), then we might not care about readonly type.
For now, I would opt to keep this as is for now and port the for loop changes to electron - this way we just avoid parsing data that will be discarded anyways and keep some extra safety in stripSentryFramesAndReverse
JonasBa
force-pushed
the
jb/ref/parser-limit
branch
from
57c3c64
to
fb1abb4
Compare
Enforce the stacktrace frame limit when we parse frames