Skip to content

Commit

Permalink
ref(browser): Replace TracekitStackFrame with Sentry StackFrame (#4523)
Browse files Browse the repository at this point in the history 
- Replaces all usages of `TracekitStackFrame` with the Sentry `StackFrame` and renames the fields appropriately. 
- Merge Opera regex tests in same function as others to remove duplication
- Makes some simplifications because `undefined` is now used instead of `null`
- Tightens up some `Error` types
  • Loading branch information
@timfish
timfish authored Feb 11, 2022
1 parent eddfcd3 commit 2c47a8b
Show file tree
Hide file tree
Showing 11 changed files with 482 additions and 519 deletions.
26 changes: 12 additions & 14 deletions packages/browser/src/parsers.ts
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Event, Exception, StackFrame } from '@sentry/types';
import { extractExceptionKeysForMessage, isEvent, normalizeToSize } from '@sentry/utils';

import { computeStackTrace, StackFrame as TraceKitStackFrame, StackTrace as TraceKitStackTrace } from './tracekit';
import { computeStackTrace, StackTrace as TraceKitStackTrace } from './tracekit';

const STACKTRACE_LIMIT = 50;

Expand Down Expand Up @@ -80,15 +80,15 @@ export function eventFromStacktrace(stacktrace: TraceKitStackTrace): Event {
/**
* @hidden
*/
export function prepareFramesForEvent(stack: TraceKitStackFrame[]): StackFrame[] {
if (!stack || !stack.length) {
export function prepareFramesForEvent(stack: StackFrame[]): StackFrame[] {
if (!stack.length) {
return [];
}

let localStack = stack;

const firstFrameFunction = localStack[0].func || '';
const lastFrameFunction = localStack[localStack.length - 1].func || '';
const firstFrameFunction = localStack[0].function || '';
const lastFrameFunction = localStack[localStack.length - 1].function || '';

// If stack starts with one of our API calls, remove it (starts, meaning it's the top of the stack - aka last call)
if (firstFrameFunction.indexOf('captureMessage') !== -1 || firstFrameFunction.indexOf('captureException') !== -1) {
Expand All @@ -103,14 +103,12 @@ export function prepareFramesForEvent(stack: TraceKitStackFrame[]): StackFrame[]
// The frame where the crash happened, should be the last entry in the array
return localStack
.slice(0, STACKTRACE_LIMIT)
.map(
(frame: TraceKitStackFrame): StackFrame => ({
colno: frame.column === null ? undefined : frame.column,
filename: frame.url || localStack[0].url,
function: frame.func || '?',
in_app: true,
lineno: frame.line === null ? undefined : frame.line,
}),
)
.map(frame => ({
filename: frame.filename || localStack[0].filename,
function: frame.function || '?',
lineno: frame.lineno,
colno: frame.colno,
in_app: true,
}))
.reverse();
}
197 changes: 55 additions & 142 deletions packages/browser/src/tracekit.ts
View file
Original file line number Diff line number Diff line change
@@ -1,27 +1,12 @@
import { StackFrame } from '@sentry/types';

/**
* This was originally forked from https://github.com/occ/TraceKit, but has since been
* largely modified and is now maintained as part of Sentry JS SDK.
*/

/* eslint-disable @typescript-eslint/no-unsafe-member-access, max-lines */

/**
* An object representing a single stack frame.
* {Object} StackFrame
* {string} url The JavaScript or HTML file URL.
* {string} func The function name, or empty for anonymous functions (if guessing did not work).
* {string[]?} args The arguments passed to the function, if known.
* {number=} line The line number, if known.
* {number=} column The column number, if known.
* {string[]} context An array of source code lines; the middle element corresponds to the correct line#.
*/
export interface StackFrame {
url: string;
func: string;
line: number | null;
column: number | null;
}

/**
* An object representing a JavaScript stack trace.
* {Object} StackTrace
Expand All @@ -32,9 +17,7 @@ export interface StackFrame {
export interface StackTrace {
name: string;
message: string;
mechanism?: string;
stack: StackFrame[];
failed?: boolean;
}

// global reference to slice
Expand All @@ -54,11 +37,13 @@ const geckoEval = /(\S+) line (\d+)(?: > eval line \d+)* > eval/i;
const chromeEval = /\((\S*)(?::(\d+))(?::(\d+))\)/;
// 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;
const opera10Regex = / line (\d+).*script (?:in )?(\S+)(?:: in function (\S+))?$/i;
const opera11Regex =
/ line (\d+), column (\d+)\s*(?:in (?:<anonymous function: ([^>]+)>|([^)]+))\(.*\))? in (.*):\s*$/i;

/** JSDoc */
// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types
export function computeStackTrace(ex: any): StackTrace {
let stack = null;
export function computeStackTrace(ex: Error & { framesToPop?: number; stacktrace?: string }): StackTrace {
let frames: StackFrame[] = [];
let popSize = 0;

if (ex) {
Expand All @@ -70,50 +55,52 @@ export function computeStackTrace(ex: any): StackTrace {
}

try {
// This must be tried first because Opera 10 *destroys*
// its stacktrace property if you try to access the stack
// property first!!
stack = computeStackTraceFromStacktraceProp(ex);
if (stack) {
return popFrames(stack, popSize);
}
// 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 || '';

frames = parseFrames(stacktrace);
} catch (e) {
// no-empty
}

try {
stack = computeStackTraceFromStackProp(ex);
if (stack) {
return popFrames(stack, popSize);
}
} catch (e) {
// no-empty
if (frames.length && popSize > 0) {
frames = frames.slice(popSize);
}

return {
message: extractMessage(ex),
name: ex && ex.name,
stack: [],
failed: true,
stack: frames,
};
}

/** JSDoc */
// eslint-disable-next-line @typescript-eslint/no-explicit-any, complexity
function computeStackTraceFromStackProp(ex: any): StackTrace | null {
if (!ex || !ex.stack) {
return null;
}

const stack = [];
const lines = ex.stack.split('\n');
// eslint-disable-next-line complexity
function parseFrames(stackString: string): StackFrame[] {
const frames: StackFrame[] = [];
const lines = stackString.split('\n');
let isEval;
let submatch;
let parts;
let element;
let element: StackFrame | undefined;

for (const line of lines) {
if ((parts = chrome.exec(line))) {
if ((parts = opera10Regex.exec(line))) {
element = {
filename: parts[2],
function: parts[3] || UNKNOWN_FUNCTION,
lineno: +parts[1],
};
} else if ((parts = opera11Regex.exec(line))) {
element = {
filename: parts[5],
function: parts[3] || parts[4] || UNKNOWN_FUNCTION,
lineno: +parts[1],
colno: +parts[2],
};
} else if ((parts = chrome.exec(line))) {
isEval = parts[2] && parts[2].indexOf('eval') === 0; // start of line
if (isEval && (submatch = chromeEval.exec(parts[2]))) {
// throw out eval line/column and use top-most line/column number
Expand All @@ -124,20 +111,20 @@ function computeStackTraceFromStackProp(ex: any): StackTrace | null {

// Kamil: One more hack won't hurt us right? Understanding and adding more rules on top of these regexps right now
// would be way too time consuming. (TODO: Rewrite whole RegExp to be more readable)
const [func, url] = extractSafariExtensionDetails(parts[1] || UNKNOWN_FUNCTION, parts[2]);
const [func, filename] = extractSafariExtensionDetails(parts[1] || UNKNOWN_FUNCTION, parts[2]);

element = {
url,
func,
line: parts[3] ? +parts[3] : null,
column: parts[4] ? +parts[4] : null,
filename,
function: func,
lineno: parts[3] ? +parts[3] : undefined,
colno: parts[4] ? +parts[4] : undefined,
};
} else if ((parts = winjs.exec(line))) {
element = {
url: parts[2],
func: parts[1] || UNKNOWN_FUNCTION,
line: +parts[3],
column: parts[4] ? +parts[4] : null,
filename: parts[2],
function: parts[1] || UNKNOWN_FUNCTION,
lineno: +parts[3],
colno: parts[4] ? +parts[4] : undefined,
};
} else if ((parts = gecko.exec(line))) {
isEval = parts[3] && parts[3].indexOf(' > eval') > -1;
Expand All @@ -149,86 +136,24 @@ function computeStackTraceFromStackProp(ex: any): StackTrace | null {
parts[5] = ''; // no column when eval
}

let url = parts[3];
let filename = parts[3];
let func = parts[1] || UNKNOWN_FUNCTION;
[func, url] = extractSafariExtensionDetails(func, url);
[func, filename] = extractSafariExtensionDetails(func, filename);

element = {
url,
func,
line: parts[4] ? +parts[4] : null,
column: parts[5] ? +parts[5] : null,
filename,
function: func,
lineno: parts[4] ? +parts[4] : undefined,
colno: parts[5] ? +parts[5] : undefined,
};
} else {
continue;
}

stack.push(element);
}

if (!stack.length) {
return null;
frames.push(element);
}

return {
message: extractMessage(ex),
name: ex.name,
stack,
};
}

/** JSDoc */
// eslint-disable-next-line @typescript-eslint/no-explicit-any
function computeStackTraceFromStacktraceProp(ex: any): StackTrace | null {
if (!ex || !ex.stacktrace) {
return null;
}
// 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;
const opera10Regex = / line (\d+).*script (?:in )?(\S+)(?:: in function (\S+))?$/i;
const opera11Regex =
/ line (\d+), column (\d+)\s*(?:in (?:<anonymous function: ([^>]+)>|([^)]+))\(.*\))? in (.*):\s*$/i;
const lines = stacktrace.split('\n');
const stack = [];
let parts;

for (let line = 0; line < lines.length; line += 2) {
let element = null;
if ((parts = opera10Regex.exec(lines[line]))) {
element = {
url: parts[2],
func: parts[3],
line: +parts[1],
column: null,
};
} else if ((parts = opera11Regex.exec(lines[line]))) {
element = {
url: parts[5],
func: parts[3] || parts[4],
line: +parts[1],
column: +parts[2],
};
}

if (element) {
if (!element.func && element.line) {
element.func = UNKNOWN_FUNCTION;
}
stack.push(element);
}
}

if (!stack.length) {
return null;
}

return {
message: extractMessage(ex),
name: ex.name,
stack,
};
return frames;
}

/**
Expand All @@ -251,30 +176,18 @@ function computeStackTraceFromStacktraceProp(ex: any): StackTrace | null {
* Unfortunatelly "just" changing RegExp is too complicated now and making it pass all tests
* and fix this case seems like an impossible, or at least way too time-consuming task.
*/
const extractSafariExtensionDetails = (func: string, url: string): [string, string] => {
const extractSafariExtensionDetails = (func: string, filename: string): [string, string] => {
const isSafariExtension = func.indexOf('safari-extension') !== -1;
const isSafariWebExtension = func.indexOf('safari-web-extension') !== -1;

return isSafariExtension || isSafariWebExtension
? [
func.indexOf('@') !== -1 ? func.split('@')[0] : UNKNOWN_FUNCTION,
isSafariExtension ? `safari-extension:${url}` : `safari-web-extension:${url}`,
isSafariExtension ? `safari-extension:${filename}` : `safari-web-extension:${filename}`,
]
: [func, url];
: [func, filename];
};

/** Remove N number of frames from the stack */
function popFrames(stacktrace: StackTrace, popSize: number): StackTrace {
try {
return {
...stacktrace,
stack: stacktrace.stack.slice(popSize),
};
} catch (e) {
return stacktrace;
}
}

/**
* There are cases where stacktrace.message is an Event object
* https://github.com/getsentry/sentry-javascript/issues/1949
Expand Down
26 changes: 13 additions & 13 deletions packages/browser/test/unit/parsers.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ describe('Parsers', () => {
describe('removed top frame if its internally reserved word (public API)', () => {
it('reserved captureException', () => {
const stack = [
{ context: ['x'], column: 1, line: 4, url: 'anything.js', func: 'captureException', args: [] },
{ context: ['x'], column: 1, line: 3, url: 'anything.js', func: 'foo', args: [] },
{ context: ['x'], column: 1, line: 2, url: 'anything.js', func: 'bar', args: [] },
{ colno: 1, lineno: 4, filename: 'anything.js', function: 'captureException' },
{ colno: 1, lineno: 3, filename: 'anything.js', function: 'foo' },
{ colno: 1, lineno: 2, filename: 'anything.js', function: 'bar' },
];

// Should remove `captureException` as its a name considered "internal"
Expand All @@ -20,9 +20,9 @@ describe('Parsers', () => {

it('reserved captureMessage', () => {
const stack = [
{ context: ['x'], column: 1, line: 4, url: 'anything.js', func: 'captureMessage', args: [] },
{ context: ['x'], column: 1, line: 3, url: 'anything.js', func: 'foo', args: [] },
{ context: ['x'], column: 1, line: 2, url: 'anything.js', func: 'bar', args: [] },
{ colno: 1, lineno: 4, filename: 'anything.js', function: 'captureMessage' },
{ colno: 1, lineno: 3, filename: 'anything.js', function: 'foo' },
{ colno: 1, lineno: 2, filename: 'anything.js', function: 'bar' },
];

// Should remove `captureMessage` as its a name considered "internal"
Expand All @@ -37,9 +37,9 @@ describe('Parsers', () => {
describe('removed bottom frame if its internally reserved word (internal API)', () => {
it('reserved sentryWrapped', () => {
const stack = [
{ context: ['x'], column: 1, line: 3, url: 'anything.js', func: 'foo', args: [] },
{ context: ['x'], column: 1, line: 2, url: 'anything.js', func: 'bar', args: [] },
{ context: ['x'], column: 1, line: 1, url: 'anything.js', func: 'sentryWrapped', args: [] },
{ colno: 1, lineno: 3, filename: 'anything.js', function: 'foo' },
{ colno: 1, lineno: 2, filename: 'anything.js', function: 'bar' },
{ colno: 1, lineno: 1, filename: 'anything.js', function: 'sentryWrapped' },
];

// Should remove `sentryWrapped` as its a name considered "internal"
Expand All @@ -53,10 +53,10 @@ describe('Parsers', () => {

it('removed top and bottom frame if they are internally reserved words', () => {
const stack = [
{ context: ['x'], column: 1, line: 4, url: 'anything.js', func: 'captureMessage', args: [] },
{ context: ['x'], column: 1, line: 3, url: 'anything.js', func: 'foo', args: [] },
{ context: ['x'], column: 1, line: 2, url: 'anything.js', func: 'bar', args: [] },
{ context: ['x'], column: 1, line: 1, url: 'anything.js', func: 'sentryWrapped', args: [] },
{ colno: 1, lineno: 4, filename: 'anything.js', function: 'captureMessage' },
{ colno: 1, lineno: 3, filename: 'anything.js', function: 'foo' },
{ colno: 1, lineno: 2, filename: 'anything.js', function: 'bar' },
{ colno: 1, lineno: 1, filename: 'anything.js', function: 'sentryWrapped' },
];

// Should remove `captureMessage` and `sentryWrapped` as its a name considered "internal"
Expand Down
Loading

0 comments on commit 2c47a8b

Please sign in to comment.