Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref(browser): Replace TracekitStackFrame with Sentry StackFrame #4523

Merged
merged 6 commits into from
Feb 11, 2022
Merged

ref(browser): Replace TracekitStackFrame with Sentry StackFrame #4523

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0ec6249
Refactor tests and minor improvements
timfish Feb 7, 2022
5563471
Remove TraceKitStackFrame
timfish Feb 8, 2022
4db0290
Remove undefined
timfish Feb 8, 2022
8e09b5e
Merge remote-tracking branch 'upstream/master' into remove-tracekitst…
timfish Feb 10, 2022
87c5628
Merge opera parser to slim down code
timfish Feb 10, 2022
245eee9
Lil tidy
timfish Feb 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 12 additions & 14 deletions packages/browser/src/parsers.ts
View file
Open in desktop
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
Open in desktop
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[];
timfish marked this conversation as resolved.
Show resolved Hide resolved
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],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is super byte size warrior, but perhaps to save on the repeated un-minifiable object props being defined when creating the element obj, we can extract into a function that takes args for each of these?

We do sacrifice readability of this file a little, and the savings might not justify it tbh. Thoughts?

// TS will complain about this
function createStackFrame(filename, function, lineno, colno?: StackFrame['colno']): StackFrame {
  const element = {
    filename,
    function,
    lineno,
  }

  if (colno) element.colno = colno;

  return element;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that would mean the fields would only need to be defined once all the parsers!

I'll add that to the next PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a plan.

};
} 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
Open in desktop
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