From 5cc872aa3044c7708914e9742a5983ef82b16b06 Mon Sep 17 00:00:00 2001 From: Krystof Woldrich Date: Wed, 7 Feb 2024 22:28:11 +0100 Subject: [PATCH 1/4] fix(v8/utils): Stack parser skip frames (not lines of stack string) --- .../browser/test/unit/tracekit/react.test.ts | 22 ++++--------------- packages/utils/src/stacktrace.ts | 7 +++--- 2 files changed, 7 insertions(+), 22 deletions(-) diff --git a/packages/browser/test/unit/tracekit/react.test.ts b/packages/browser/test/unit/tracekit/react.test.ts index d949a4dee0eb..5fd40dc19ca5 100644 --- a/packages/browser/test/unit/tracekit/react.test.ts +++ b/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: @@ -37,14 +37,7 @@ describe('Tracekit - React Tests', () => { lineno: 1, 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: @@ -132,14 +125,7 @@ describe('Tracekit - React Tests', () => { lineno: 1, colno: 21841, in_app: true, - }, - { - filename: 'http://localhost:5000/static/js/foo.chunk.js', - function: '?', - lineno: 1, - colno: 21738, - in_app: true, - }, + } ], }, }); diff --git a/packages/utils/src/stacktrace.ts b/packages/utils/src/stacktrace.ts index 917b46daa5d1..5c84dfd8ee3c 100644 --- a/packages/utils/src/stacktrace.ts +++ b/packages/utils/src/stacktrace.ts @@ -24,8 +24,7 @@ export function createStackParser(...parsers: StackLineParser[]): StackParser { const frames: StackFrame[] = []; const lines = stack.split('\n'); - for (let i = skipFirst; i < lines.length; i++) { - const line = lines[i]; + for (const line of lines) { // 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 // input size. Huge strings can result in hangs/Denial of Service: @@ -53,12 +52,12 @@ export function createStackParser(...parsers: StackLineParser[]): StackParser { } } - if (frames.length >= STACKTRACE_FRAME_LIMIT) { + if (frames.length >= STACKTRACE_FRAME_LIMIT + skipFirst) { break; } } - return stripSentryFramesAndReverse(frames); + return stripSentryFramesAndReverse(frames.slice(skipFirst)); }; } From 9a9a4f76b54df1635665c3af889f02f95b83662c Mon Sep 17 00:00:00 2001 From: Krystof Woldrich Date: Wed, 7 Feb 2024 23:27:36 +0100 Subject: [PATCH 2/4] fix tests add two params lines to skip and frames to pop --- packages/browser/src/eventbuilder.ts | 36 +++++++++++++------ .../browser/test/unit/tracekit/react.test.ts | 4 +-- packages/types/src/stacktrace.ts | 2 +- packages/utils/src/stacktrace.ts | 9 ++--- 4 files changed, 34 insertions(+), 17 deletions(-) diff --git a/packages/browser/src/eventbuilder.ts b/packages/browser/src/eventbuilder.ts index 2956006b0ac7..b39baee2a3fe 100644 --- a/packages/browser/src/eventbuilder.ts +++ b/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; diff --git a/packages/browser/test/unit/tracekit/react.test.ts b/packages/browser/test/unit/tracekit/react.test.ts index 5fd40dc19ca5..55ffdc34c537 100644 --- a/packages/browser/test/unit/tracekit/react.test.ts +++ b/packages/browser/test/unit/tracekit/react.test.ts @@ -37,7 +37,7 @@ describe('Tracekit - React Tests', () => { lineno: 1, colno: 21841, in_app: true, - } + }, ], }, }); @@ -125,7 +125,7 @@ describe('Tracekit - React Tests', () => { lineno: 1, colno: 21841, in_app: true, - } + }, ], }, }); diff --git a/packages/types/src/stacktrace.ts b/packages/types/src/stacktrace.ts index c27cbf00a12c..59dff047ab21 100644 --- a/packages/types/src/stacktrace.ts +++ b/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]; diff --git a/packages/utils/src/stacktrace.ts b/packages/utils/src/stacktrace.ts index 5c84dfd8ee3c..6c9ad7208350 100644 --- a/packages/utils/src/stacktrace.ts +++ b/packages/utils/src/stacktrace.ts @@ -20,11 +20,12 @@ 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 (const line of lines) { + 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 // input size. Huge strings can result in hangs/Denial of Service: @@ -52,12 +53,12 @@ export function createStackParser(...parsers: StackLineParser[]): StackParser { } } - if (frames.length >= STACKTRACE_FRAME_LIMIT + skipFirst) { + if (frames.length >= STACKTRACE_FRAME_LIMIT + framesToPop) { break; } } - return stripSentryFramesAndReverse(frames.slice(skipFirst)); + return stripSentryFramesAndReverse(frames.slice(framesToPop)); }; } From 92bd4d23e9633b64e06670ee891b26da09f03813 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kry=C5=A1tof=20Wold=C5=99ich?= <31292499+krystofwoldrich@users.noreply.github.com> Date: Tue, 13 Feb 2024 13:41:16 +0100 Subject: [PATCH 3/4] Update MIGRATION.md --- MIGRATION.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/MIGRATION.md b/MIGRATION.md index f975b13c0f4d..120058e24caa 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -1,5 +1,10 @@ # Upgrading from 7.x to 8.x +## `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. + ## Removal of Severity Enum In v7 we deprecated the `Severity` enum in favor of using the `SeverityLevel` type. In v8 we removed the `Severity` From 99d8390f667e8ad31a9b1fd62fbd4941162fab04 Mon Sep 17 00:00:00 2001 From: Krystof Woldrich Date: Wed, 6 Mar 2024 11:20:14 +0100 Subject: [PATCH 4/4] fix prettier --- MIGRATION.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/MIGRATION.md b/MIGRATION.md index 6b60f3997958..0d09fa68c0c1 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -311,8 +311,8 @@ 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. +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`