SentryParser counts Error message lines in `error.framesToPop`

[krystofwoldrich]

Capture an error with framesToPop property set to 1.

Changing this would be a breaking change and could affect issues grouping.

Expected Result

The top frame will be removed. We skip parsed frames instead of stack lines.

Actual Result

Nothing is removed. The skipped line is an error message

sentry-javascript/packages/utils/src/stacktrace.ts

Line 23 in 739d904

return (stack: string, skipFirst: number = 0): StackFrame[] => {

[AbhiPrasad]

Oh shoot - good catch cc @timfish

[timfish]

I'm just trying to understand what's not working as expected here.

The skipped line is an error message

skipFirst is specifically used to skip the error message.

The logic to handle framesToPop is here:

sentry-javascript/packages/browser/src/eventbuilder.ts

Lines 91 to 127 in 59db749

	/** Parses stack frames from an error */
	export function parseStackFrames(
	stackParser: StackParser,
	ex: Error & { framesToPop?: number; stacktrace?: string },
	): StackFrame[] {
	// Access and store the stacktrace property before doing ANYTHING
	// else to it because Opera is not very good at providing it
	// reliably in other circumstances.
	const stacktrace = ex.stacktrace || ex.stack || '';
	const popSize = getPopSize(ex);
	try {
	return stackParser(stacktrace, popSize);
	} catch (e) {
	// no-empty
	}
	return [];
	}
	// Based on our own mapping pattern - https://github.com/getsentry/sentry/blob/9f08305e09866c8bd6d0c24f5b0aabdd7dd6c59c/src/sentry/lang/javascript/errormapping.py#L83-L108
	const reactMinifiedRegexp = /Minified React error #\d+;/i;
	function getPopSize(ex: Error & { framesToPop?: number }): number {
	if (ex) {
	if (typeof ex.framesToPop === 'number') {
	return ex.framesToPop;
	}
	if (reactMinifiedRegexp.test(ex.message)) {
	return 1;
	}
	}
	return 0;
	}

We've got a couple of tests that use framesToPop, are these wrong or incomplete?

sentry-javascript/packages/browser/test/unit/tracekit/react.test.ts

Lines 4 to 8 in 59db749

	describe('Tracekit - React Tests', () => {
	it('should correctly parse Invariant Violation errors and use framesToPop to drop info message', () => {
	const REACT_INVARIANT_VIOLATION_EXCEPTION = {
	framesToPop: 1,
	message:

[krystofwoldrich]

@timfish The skipFirst is now used to skip the first x lines of the error stack string which is true, but I believe it should not skip the lines of the error stack string but actual parsed frames.

The value passed to the skipFirst is framesToPop, if set to one I would expect the first frame to be skipped but all frames are present because only the message from the stack string is skipped.

sentry-javascript/packages/browser/test/unit/tracekit/react.test.ts

Lines 4 to 8 in 59db749

	describe('Tracekit - React Tests', () => {
	it('should correctly parse Invariant Violation errors and use framesToPop to drop info message', () => {
	const REACT_INVARIANT_VIOLATION_EXCEPTION = {
	framesToPop: 1,
	message:

The tests I believe are wrong as they expect no frames to be skipped when framesToPop is set to 1.

Skipping the lines of the string is unpredictable as the message can have multiple lines.

[timfish]

@krystofwoldrich many thanks for the detailed explanation and bearing with me 🙂

[timfish]

It looks like the framesToPop property on the error doesn't make a difference with the current parser.

If we look waaaay back when this test was added:
2979b2c

And browser the source at that point:
https://github.com/getsentry/sentry-javascript/tree/2979b2c5b09a80980a8765272dde246564c82dfd

We can see that we used tracekit virtually unmodified from when it was forked and I can see that at that point it did parse the frames first and then pop a frame:

sentry-javascript/packages/browser/src/tracekit.ts

Lines 880 to 883 in 2979b2c

	stack = _computeStackTraceFromStacktraceProp(ex);
	if (stack) {
	return popFrames(stack, popSize);
	}

But even back then, 4 frames were expected in the test case:

sentry-javascript/packages/browser/test/unit/tracekit/custom.test.ts

Lines 305 to 336 in 2979b2c

	{
	args: [],
	column: 21738,
	context: null,
	func: '?',
	line: 1,
	url: 'http://localhost:5000/static/js/foo.chunk.js',
	},
	{
	args: [],
	column: 21841,
	context: null,
	func: 'a',
	line: 1,
	url: 'http://localhost:5000/static/js/foo.chunk.js',
	},
	{
	args: [],
	column: 68735,
	context: null,
	func: 'ho',
	line: 1,
	url: 'http://localhost:5000/static/js/foo.chunk.js',
	},
	{
	args: [],
	column: 980,
	context: null,
	func: 'f',
	line: 1,
	url: 'http://localhost:5000/',
	},

I think the current stack line parser regex is stricter than back in 2019 and since we drop lines that could not be parsed, this automatically strips the Invariant Violation: Minified React error line from the frames. This is why the tests continued to pass after many modifications and updates to how stack parsing works. We could (and probably should) strip out all the framesToPop code and the parser tests would all continue to pass.

Do you have an example stack string where the parsing isn't working as expected? We currently have a single test case that hasn't been modified in 4 years and has continued to pass.

[krystofwoldrich]

@timfish I've opened a PR as it might be easier to resolve this than talking theory.

• fix(v8/utils): Stack parser skip frames (not lines of stack string) #10560

I believe there were two things mixed from the beginning.

1. Skipping a line of the stack string which otherwise would be parsed as a valid frame although it's not a frame.

   • Example: Minified React error #31; visit https://reactjs.org/docs/error-decoder.html?invariant=31&args[]=object%20with%20keys%20%7B%7D&args[]= for

2. Skipping the actual frame, React uses invariant.js which sets framesToPop=1 to remove the invariant frame as this will be the same in all the errors.