Skip to content

Commit 1f44dd5

Browse files
kinyoklionkinyoklion
authored
fix: Fix a bug where the incorrect src lines may have been captured. (#792)
Resolves a bug where the srcLine and before/after context would have been populated incorrectly. We would assume the src line would be in the middle of the source context, but when a number of lines greater than the context window don't exist, it will not always be in the middle. The fix extends TraceKit with a srcStart field that indicates the line the context window starts on. This can then be used to calculate the offset to the originating line.
1 parent 243888d commit 1f44dd5

File tree

4 files changed

+247
-18
lines changed

4 files changed

+247
-18
lines changed

‎packages/telemetry/browser-telemetry/__tests__/stack/StackParser.test.ts‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,8 @@ describe('given source lines', () => {
5252
describe('given an input stack frame', () => {
5353
const inputFrame = {
5454
context: ['1234567890', 'ABCDEFGHIJ', 'the src line', '0987654321', 'abcdefghij'],
55+
line: 3,
56+
srcStart: 1,
5557
column: 0,
5658
};
5759

@@ -124,6 +126,56 @@ describe('given an input stack frame', () => {
124126
});
125127
});
126128

129+
it('can handle an origin before the context window', () => {
130+
// This isn't expected, but we just want to make sure it is handle gracefully.
131+
const inputFrame = {
132+
context: ['1234567890', 'ABCDEFGHIJ', 'the src line', '0987654321', 'abcdefghij'],
133+
line: 3,
134+
srcStart: 5,
135+
column: 0,
136+
};
137+
138+
expect(
139+
getSrcLines(inputFrame, {
140+
enabled: true,
141+
source: {
142+
beforeLines: 1,
143+
afterLines: 1,
144+
maxLineLength: 280,
145+
},
146+
}),
147+
).toMatchObject({
148+
srcBefore: [],
149+
srcLine: '1234567890',
150+
srcAfter: ['ABCDEFGHIJ'],
151+
});
152+
});
153+
154+
it('can handle an origin after the context window', () => {
155+
// This isn't expected, but we just want to make sure it is handle gracefully.
156+
const inputFrame = {
157+
context: ['1234567890', 'ABCDEFGHIJ', 'the src line', '0987654321', 'abcdefghij'],
158+
line: 100,
159+
srcStart: 5,
160+
column: 0,
161+
};
162+
163+
expect(
164+
getSrcLines(inputFrame, {
165+
enabled: true,
166+
source: {
167+
beforeLines: 1,
168+
afterLines: 1,
169+
maxLineLength: 280,
170+
},
171+
}),
172+
).toMatchObject({
173+
srcBefore: ['0987654321'],
174+
srcLine: 'abcdefghij',
175+
srcAfter: [],
176+
});
177+
});
178+
127179
it('returns an empty stack when stack parsing is disabled', () => {
128180
expect(
129181
parse(new Error('test'), {

‎packages/telemetry/browser-telemetry/__tests__/vendor/TraceKit/TraceKit.test.ts‎

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,4 +114,123 @@ describe('TraceKit', function () {
114114
expect(stackFrames.stack[0].url).toEqual('<anonymous>');
115115
});
116116
});
117+
118+
describe('given mock source code, xhr, and domain', () => {
119+
// Mock source code that will be fetched
120+
const mockSource =
121+
'function foo() {\n' +
122+
' console.log("line 2");\n' +
123+
' throw new Error("error on line 3");\n' +
124+
' console.log("line 4");\n' +
125+
'}\n' +
126+
'foo();';
127+
128+
// Mock XMLHttpRequest
129+
const mockXHR = {
130+
open: jest.fn(),
131+
send: jest.fn(),
132+
responseText: mockSource,
133+
};
134+
135+
// @ts-ignore - we know this is incomplete
136+
window.XMLHttpRequest = jest.fn(() => mockXHR);
137+
138+
window.document.domain = 'localhost';
139+
140+
it('should populate srcStart and context from source code with firefox style stack trace', () => {
141+
const traceKit = getTraceKit();
142+
traceKit.remoteFetching = true;
143+
traceKit.linesOfContext = 10;
144+
145+
const error = new Error('error on line 3');
146+
// Firefox style stack trace
147+
error.stack =
148+
'foo/<@http://localhost:8081/assets/index-BvsURM3r.js:3:2\n' +
149+
'@http://localhost:8081/assets/index-BvsURM3r.js:6:0';
150+
const stackFrames = traceKit.computeStackTrace(error);
151+
152+
expect(stackFrames).toBeTruthy();
153+
expect(stackFrames.stack[0]).toEqual({
154+
url: 'http://localhost:8081/assets/index-BvsURM3r.js',
155+
func: 'foo/<',
156+
args: [],
157+
line: 3,
158+
column: 2,
159+
context: [
160+
'function foo() {',
161+
' console.log("line 2");',
162+
' throw new Error("error on line 3");',
163+
' console.log("line 4");',
164+
'}',
165+
'foo();',
166+
],
167+
srcStart: 1,
168+
});
169+
});
170+
171+
it('should populate srcStart and context from source code with chrome style stack trace', () => {
172+
const traceKit = getTraceKit();
173+
traceKit.remoteFetching = true;
174+
traceKit.linesOfContext = 10;
175+
176+
const error = new Error('error on line 3');
177+
// Chrome style stack trace
178+
error.stack =
179+
'Error: error on line 3\n' +
180+
' at foo (http://localhost:8081/assets/index-BvsURM3r.js:3:2)\n' +
181+
' at http://localhost:8081/assets/index-BvsURM3r.js:6:0';
182+
const stackFrames = traceKit.computeStackTrace(error);
183+
184+
expect(stackFrames).toBeTruthy();
185+
expect(stackFrames.stack[0]).toEqual({
186+
url: 'http://localhost:8081/assets/index-BvsURM3r.js',
187+
func: 'foo',
188+
args: [],
189+
line: 3,
190+
column: 2,
191+
context: [
192+
'function foo() {',
193+
' console.log("line 2");',
194+
' throw new Error("error on line 3");',
195+
' console.log("line 4");',
196+
'}',
197+
'foo();',
198+
],
199+
srcStart: 1,
200+
});
201+
});
202+
203+
it('should populate srcStart and context from source code with opera style stack trace', () => {
204+
const traceKit = getTraceKit();
205+
traceKit.remoteFetching = true;
206+
traceKit.linesOfContext = 10;
207+
208+
const error = new Error('error on line 3');
209+
// Opera style stack trace
210+
// @ts-ignore - Opera does what it wants.
211+
error.stacktrace =
212+
'Error initially occurred at line 3, column 2 in foo() in http://localhost:8081/assets/index-BvsURM3r.js:\n' +
213+
'throw new Error("error on line 3");\n' +
214+
'called from line 6, column 1 in foo() in http://localhost:8081/assets/index-BvsURM3r.js:';
215+
const stackFrames = traceKit.computeStackTrace(error);
216+
217+
expect(stackFrames).toBeTruthy();
218+
expect(stackFrames.stack[0]).toEqual({
219+
url: 'http://localhost:8081/assets/index-BvsURM3r.js',
220+
func: 'foo',
221+
args: [],
222+
line: 3,
223+
column: 2,
224+
context: [
225+
'function foo() {',
226+
' console.log("line 2");',
227+
' throw new Error("error on line 3");',
228+
' console.log("line 4");',
229+
'}',
230+
'foo();',
231+
],
232+
srcStart: 1,
233+
});
234+
});
235+
});
117236
});

‎packages/telemetry/browser-telemetry/src/stack/StackParser.ts‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,18 @@ export function processUrlToFileName(input: string, origin: string): string {
4747
return cleaned;
4848
}
4949

50+
/**
51+
* Clamp a value to be between an inclusive max an minimum.
52+
*
53+
* @param min The inclusive minimum value.
54+
* @param max The inclusive maximum value.
55+
* @param value The value to clamp.
56+
* @returns The clamped value in range [min, max].
57+
*/
58+
function clamp(min: number, max: number, value: number): number {
59+
return Math.min(max, Math.max(min, value));
60+
}
61+
5062
export interface TrimOptions {
5163
/**
5264
* The maximum length of the trimmed line.
@@ -130,6 +142,8 @@ export function getSrcLines(
130142
// as we can.
131143
context?: string[] | null;
132144
column?: number | null;
145+
srcStart?: number | null;
146+
line?: number | null;
133147
},
134148
options: ParsedStackOptions,
135149
): {
@@ -168,7 +182,8 @@ export function getSrcLines(
168182
0,
169183
);
170184

171-
const origin = Math.floor(context.length / 2);
185+
const origin = clamp(0, context.length - 1, (inFrame?.line ?? 0) - (inFrame.srcStart ?? 0));
186+
172187
return {
173188
// The lines immediately preceeding the origin line.
174189
srcBefore: getLines(origin - options.source.beforeLines, origin, context, trimmer),
@@ -204,8 +219,9 @@ export default function parse(error: Error, options: ParsedStackOptions): StackT
204219
const frames: StackFrame[] = parsed.stack.reverse().map((inFrame) => ({
205220
fileName: processUrlToFileName(inFrame.url, window.location.origin),
206221
function: inFrame.func,
207-
line: inFrame.line,
208-
col: inFrame.column,
222+
// Strip the nulls so we only ever return undefined.
223+
line: inFrame.line ?? undefined,
224+
col: inFrame.column ?? undefined,
209225
...getSrcLines(inFrame, options),
210226
}));
211227
return {

0 commit comments

Comments
 (0)