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

Author: krystofwoldrich

MIGRATION.md

@@ -309,6 +309,11 @@ const replay = Sentry.getIntegration(Replay);
 const replay = getClient().getIntegrationByName('Replay');
 ```
 
+#### `framesToPop` applies to parsed frames
+
+Error with `framesToPop` property will have the specified number of frames removed from the top of the stack. This
+changes compared to the v7 where the property `framesToPop` was used to remove top n lines from the stack string.
+
 #### `tracingOrigins` has been replaced by `tracePropagationTargets`
 
 `tracingOrigins` is now removed in favor of the `tracePropagationTargets` option. The `tracePropagationTargets` option

packages/browser/src/eventbuilder.ts

@@ -106,10 +106,11 @@ export function parseStackFrames(
   // reliably in other circumstances.
   const stacktrace = ex.stacktrace || ex.stack || '';
 
-  const popSize = getPopSize(ex);
+  const skipLines = getSkipFirstStackStringLines(ex);
+  const framesToPop = getPopFirstTopFrames(ex);
 
   try {
-    return stackParser(stacktrace, popSize);
+    return stackParser(stacktrace, skipLines, framesToPop);
   } catch (e) {
     // no-empty
   }
@@ -120,15 +121,30 @@ export function parseStackFrames(
 // 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;
-    }
+/**
+ * Certain known React errors contain links that would be falsely
+ * parsed as frames. This function check for these errors and
+ * returns number of the stack string lines to skip.
+ */
+function getSkipFirstStackStringLines(ex: Error): number {
+  if (ex && reactMinifiedRegexp.test(ex.message)) {
+    return 1;
+  }
 
-    if (reactMinifiedRegexp.test(ex.message)) {
-      return 1;
-    }
+  return 0;
+}
+
+/**
+ * If error has `framesToPop` property, it means that the
+ * creator tells us the first x frames will be useless
+ * and should be discarded. Typically error from wrapper function
+ * which don't point to the actual location in the developer's code.
+ *
+ * Example: https://github.com/zertosh/invariant/blob/master/invariant.js#L46
+ */
+function getPopFirstTopFrames(ex: Error & { framesToPop?: unknown }): number {
+  if (typeof ex.framesToPop === 'number') {
+    return ex.framesToPop;
   }
 
   return 0;

packages/browser/test/unit/tracekit/react.test.ts

@@ -2,7 +2,7 @@ import { exceptionFromError } from '../../../src/eventbuilder';
 import { defaultStackParser as parser } from '../../../src/stack-parsers';
 
 describe('Tracekit - React Tests', () => {
-  it('should correctly parse Invariant Violation errors and use framesToPop to drop info message', () => {
+  it('should correctly parse Invariant Violation errors and use framesToPop to drop the invariant frame', () => {
     const REACT_INVARIANT_VIOLATION_EXCEPTION = {
       framesToPop: 1,
       message:
@@ -38,13 +38,6 @@ describe('Tracekit - React Tests', () => {
             colno: 21841,
             in_app: true,
           },
-          {
-            filename: 'http://localhost:5000/static/js/foo.chunk.js',
-            function: '?',
-            lineno: 1,
-            colno: 21738,
-            in_app: true,
-          },
         ],
       },
     });
@@ -97,7 +90,7 @@ describe('Tracekit - React Tests', () => {
     });
   });
 
-  it('should not drop additional frame for production errors if framesToPop is still there', () => {
+  it('should drop invariant frame for production errors if framesToPop is present', () => {
     const REACT_PRODUCTION_ERROR = {
       framesToPop: 1,
       message:
@@ -133,13 +126,6 @@ describe('Tracekit - React Tests', () => {
             colno: 21841,
             in_app: true,
           },
-          {
-            filename: 'http://localhost:5000/static/js/foo.chunk.js',
-            function: '?',
-            lineno: 1,
-            colno: 21738,
-            in_app: true,
-          },
         ],
       },
     });

packages/types/src/stacktrace.ts

@@ -6,6 +6,6 @@ export interface Stacktrace {
   frames_omitted?: [number, number];
 }
 
-export type StackParser = (stack: string, skipFirst?: number) => StackFrame[];
+export type StackParser = (stack: string, skipFirstLines?: number, framesToPop?: number) => StackFrame[];
 export type StackLineParserFn = (line: string) => StackFrame | undefined;
 export type StackLineParser = [number, StackLineParserFn];

packages/utils/src/stacktrace.ts

@@ -16,11 +16,11 @@ const STRIP_FRAME_REGEXP = /captureMessage|captureException/;
 export function createStackParser(...parsers: StackLineParser[]): StackParser {
   const sortedParsers = parsers.sort((a, b) => a[0] - b[0]).map(p => p[1]);
 
-  return (stack: string, skipFirst: number = 0): StackFrame[] => {
+  return (stack: string, skipFirstLines: number = 0, framesToPop: number = 0): StackFrame[] => {
     const frames: StackFrame[] = [];
     const lines = stack.split('\n');
 
-    for (let i = skipFirst; i < lines.length; i++) {
+    for (let i = skipFirstLines; i < lines.length; i++) {
       const line = lines[i];
       // Ignore lines over 1kb as they are unlikely to be stack frames.
       // Many of the regular expressions use backtracking which results in run time that increases exponentially with
@@ -49,12 +49,12 @@ export function createStackParser(...parsers: StackLineParser[]): StackParser {
         }
       }
 
-      if (frames.length >= STACKTRACE_FRAME_LIMIT) {
+      if (frames.length >= STACKTRACE_FRAME_LIMIT + framesToPop) {
         break;
       }
     }
 
-    return stripSentryFramesAndReverse(frames);
+    return stripSentryFramesAndReverse(frames.slice(framesToPop));
   };
 }