SentryParser counts Error message lines in error.framesToPop #9656

Activity

1. krystofwoldrich

   added

   Type: Bug

   on Nov 27, 2023
2. github-actions

   added

   Package: Browser

   on Nov 27, 2023
3. lforst

   added

   Meta: Breaking

   on Nov 27, 2023
4. lforst

   added this to the 8.0.0 milestone on Nov 27, 2023
5. AbhiPrasad commented on Nov 27, 2023

   

   AbhiPrasad

   on Nov 27, 2023

   Contributor

   More actions

   Oh shoot - good catch cc @timfish
6. timfish commented on Nov 30, 2023

   

   timfish

   on Nov 30, 2023

   Last edited by timfish

   Collaborator

   More actions

   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:
7. krystofwoldrich commented on Jan 10, 2024

   

   krystofwoldrich

   on Jan 10, 2024

   Last edited by krystofwoldrich

   ContributorAuthor

   More actions

   @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.

   👍React with 👍1Reacted by Tim Fish
8. timfish commented on Jan 11, 2024

   

   timfish

   on Jan 11, 2024

   Collaborator

   More actions

   @krystofwoldrich many thanks for the detailed explanation and bearing with me 🙂
9. timfish commented on Jan 11, 2024

   

   timfish

   on Jan 11, 2024

   Last edited by timfish

   Collaborator

   More actions

   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.
10. krystofwoldrich commented on Feb 8, 2024

    

    krystofwoldrich

    on Feb 8, 2024

    ContributorAuthor

    More actions

    @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.
11. krystofwoldrich

    closed this as completedin #10560on Mar 6, 2024