-
Notifications
You must be signed in to change notification settings - Fork 40
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.
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
feat: reduce jserrors wrapping and remove onerror use #614
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,64 +5,55 @@ | |
|
||
import { handle } from '../../../common/event-emitter/handle' | ||
import { now } from '../../../common/timing/now' | ||
import { getOrSet } from '../../../common/util/get-or-set' | ||
import { wrapRaf, wrapTimer, wrapEvents, wrapXhr } from '../../../common/wrap' | ||
import './debug' | ||
import { InstrumentBase } from '../../utils/instrument-base' | ||
import { FEATURE_NAME, NR_ERR_PROP } from '../constants' | ||
import { FEATURE_NAME } from '../constants' | ||
import { FEATURE_NAMES } from '../../../loaders/features/features' | ||
import { globalScope } from '../../../common/constants/runtime' | ||
import { eventListenerOpts } from '../../../common/event-listener/event-listener-opts' | ||
import { getRuntime } from '../../../common/config/config' | ||
import { stringify } from '../../../common/util/stringify' | ||
import { UncaughtError } from './uncaught-error' | ||
|
||
export class Instrument extends InstrumentBase { | ||
static featureName = FEATURE_NAME | ||
|
||
#seenErrors = new Set() | ||
|
||
constructor (agentIdentifier, aggregator, auto = true) { | ||
super(agentIdentifier, aggregator, FEATURE_NAME, auto) | ||
// skipNext counter to keep track of uncaught | ||
// errors that will be the same as caught errors. | ||
this.skipNext = 0 | ||
|
||
try { | ||
// this try-catch can be removed when IE11 is completely unsupported & gone | ||
this.removeOnAbort = new AbortController() | ||
} catch (e) {} | ||
|
||
const thisInstrument = this | ||
thisInstrument.ee.on('fn-start', function (args, obj, methodName) { | ||
if (thisInstrument.abortHandler) thisInstrument.skipNext += 1 | ||
}) | ||
thisInstrument.ee.on('fn-err', function (args, obj, err) { | ||
if (thisInstrument.abortHandler && !err[NR_ERR_PROP]) { | ||
getOrSet(err, NR_ERR_PROP, function getVal () { | ||
return true | ||
}) | ||
this.thrown = true | ||
handle('err', [err, now()], undefined, FEATURE_NAMES.jserrors, thisInstrument.ee) | ||
} | ||
}) | ||
thisInstrument.ee.on('fn-end', function () { | ||
if (!thisInstrument.abortHandler) return | ||
if (!this.thrown && thisInstrument.skipNext > 0) thisInstrument.skipNext -= 1 | ||
// Capture function errors early in case the spa feature is loaded | ||
this.ee.on('fn-err', (args, obj, error) => { | ||
if (!this.abortHandler || this.#seenErrors.has(error)) return | ||
this.#seenErrors.add(error) | ||
|
||
handle('err', [this.#castError(error), now()], undefined, FEATURE_NAMES.jserrors, this.ee) | ||
}) | ||
thisInstrument.ee.on('internal-error', function (e) { | ||
handle('ierr', [e, now(), true], undefined, FEATURE_NAMES.jserrors, thisInstrument.ee) | ||
|
||
this.ee.on('internal-error', (error) => { | ||
if (!this.abortHandler) return | ||
handle('ierr', [this.#castError(error), now(), true], undefined, FEATURE_NAMES.jserrors, this.ee) | ||
}) | ||
|
||
// Replace global error handler with our own. | ||
this.origOnerror = globalScope.onerror | ||
globalScope.onerror = this.onerrorHandler.bind(this) | ||
globalScope.addEventListener('unhandledrejection', (promiseRejectionEvent) => { | ||
if (!this.abortHandler) return | ||
|
||
globalScope.addEventListener('unhandledrejection', (e) => { | ||
/** rejections can contain data of any type -- this is an effort to keep the message human readable */ | ||
const err = castReasonToError(e.reason) | ||
handle('err', [err, now(), false, { unhandledPromiseRejection: 1 }], undefined, FEATURE_NAMES.jserrors, this.ee) | ||
handle('err', [this.#castPromiseRejectionEvent(promiseRejectionEvent), now(), false, { unhandledPromiseRejection: 1 }], undefined, FEATURE_NAMES.jserrors, this.ee) | ||
}, eventListenerOpts(false, this.removeOnAbort?.signal)) | ||
|
||
wrapRaf(this.ee) | ||
wrapTimer(this.ee) | ||
wrapEvents(this.ee) | ||
if (getRuntime(agentIdentifier).xhrWrappable) wrapXhr(this.ee) | ||
globalScope.addEventListener('error', (errorEvent) => { | ||
if (!this.abortHandler) return | ||
if (this.#seenErrors.has(errorEvent.error)) { | ||
this.#seenErrors.delete(errorEvent.error) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would you mind adding a comment or some here about the process behind this / why it's done? for others |
||
return | ||
} | ||
|
||
handle('err', [this.#castErrorEvent(errorEvent), now()], undefined, FEATURE_NAMES.jserrors, this.ee) | ||
}, eventListenerOpts(false, this.removeOnAbort?.signal)) | ||
|
||
this.abortHandler = this.#abort // we also use this as a flag to denote that the feature is active or on and handling errors | ||
this.importAggregator() | ||
|
@@ -71,67 +62,55 @@ | |
/** Restoration and resource release tasks to be done if JS error loader is being aborted. Unwind changes to globals. */ | ||
#abort () { | ||
this.removeOnAbort?.abort() | ||
this.#seenErrors.clear() | ||
this.abortHandler = undefined // weakly allow this abort op to run only once | ||
} | ||
|
||
#castError (error) { | ||
if (error instanceof Error) { | ||
return error | ||
} | ||
|
||
if (typeof error.message !== 'undefined') { | ||
return new UncaughtError(error.message, error.filename, error.lineno, error.colno) | ||
} | ||
Comment on lines
+74
to
+76
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a case when this is called with an error that's not an Error instance yet has a message? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Likewise for castErrorEvent usage There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
return new UncaughtError(error) | ||
} | ||
|
||
/** | ||
* FF and Android browsers do not provide error info to the 'error' event callback, | ||
* so we must use window.onerror | ||
* @param {string} message | ||
* @param {string} filename | ||
* @param {number} lineno | ||
* @param {number} column | ||
* @param {Error | *} errorObj | ||
* @returns | ||
* Attempts to convert a PromiseRejectionEvent object to an Error object | ||
* @param {PromiseRejectionEvent} unhandledRejectionEvent The unhandled promise rejection event | ||
* @returns {Error} An Error object with the message as the casted reason | ||
*/ | ||
onerrorHandler (message, filename, lineno, column, errorObj) { | ||
if (typeof this.origOnerror === 'function') this.origOnerror(...arguments) | ||
|
||
try { | ||
if (this.skipNext) this.skipNext -= 1 | ||
else handle('err', [errorObj || new UncaughtException(message, filename, lineno), now()], undefined, FEATURE_NAMES.jserrors, this.ee) | ||
} catch (e) { | ||
#castPromiseRejectionEvent (promiseRejectionEvent) { | ||
let prefix = 'Unhandled Promise Rejection: ' | ||
if (promiseRejectionEvent?.reason instanceof Error) { | ||
try { | ||
handle('ierr', [e, now(), true], undefined, FEATURE_NAMES.jserrors, this.ee) | ||
} catch (err) { | ||
// do nothing | ||
promiseRejectionEvent.reason.message = prefix + promiseRejectionEvent.reason.message | ||
return promiseRejectionEvent.reason | ||
} catch (e) { | ||
return promiseRejectionEvent.reason | ||
} | ||
} | ||
return false // maintain default behavior of the error event of Window | ||
} | ||
} | ||
|
||
/** | ||
* | ||
* @param {string} message | ||
* @param {string} filename | ||
* @param {number} lineno | ||
*/ | ||
function UncaughtException (message, filename, lineno) { | ||
this.message = message || 'Uncaught error with no additional information' | ||
this.sourceURL = filename | ||
this.line = lineno | ||
} | ||
|
||
/** | ||
* Attempts to cast an unhandledPromiseRejection reason (reject(...)) to an Error object | ||
* @param {*} reason - The reason property from an unhandled promise rejection | ||
* @returns {Error} - An Error object with the message as the casted reason | ||
*/ | ||
function castReasonToError (reason) { | ||
let prefix = 'Unhandled Promise Rejection: ' | ||
if (reason instanceof Error) { | ||
if (typeof promiseRejectionEvent.reason === 'undefined') return new Error(prefix) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should keep in mind that when we create a new Error, the stack trace is going to point back to this code (us), so users will likely first come to us saying there's some error being thrown in our code. That said, if they call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code was not changed, just moved. That said, I also questioned why we were creating new Error instances here for the same reason. I thought about changing this to use the |
||
try { | ||
reason.message = prefix + reason.message | ||
return reason | ||
return new Error(prefix + stringify(promiseRejectionEvent.reason)) | ||
} catch (e) { | ||
return reason | ||
return new Error(promiseRejectionEvent.reason) | ||
} | ||
} | ||
if (typeof reason === 'undefined') return new Error(prefix) | ||
try { | ||
return new Error(prefix + stringify(reason)) | ||
} catch (err) { | ||
return new Error(prefix) | ||
|
||
/** | ||
* Attempts to convert an ErrorEvent object to an Error object | ||
* @param {ErrorEvent} errorEvent The error event | ||
* @returns {Error|UncaughtError} The error event converted to an Error object | ||
*/ | ||
#castErrorEvent (errorEvent) { | ||
if (errorEvent.error instanceof Error) { | ||
return errorEvent.error | ||
} | ||
|
||
return new UncaughtError(errorEvent.message, errorEvent.filename, errorEvent.lineno, errorEvent.colno) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
/** | ||
* Represents an uncaught non Error type error. This class does | ||
* not extend the Error class to prevent an invalid stack trace | ||
* from being created. Use this class to cast thrown errors that | ||
* do not use the Error class (strings, etc) to an object. | ||
*/ | ||
export class UncaughtError { | ||
constructor (message, filename, lineno, colno) { | ||
this.name = 'UncaughtError' | ||
cwli24 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
this.message = message | ||
this.sourceURL = filename | ||
this.line = lineno | ||
this.column = colno | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -160,9 +160,6 @@ testDriver.test('error in custom tracer', function (t, browser, router) { | |
}) | ||
|
||
testDriver.test('string error in custom tracer', function (t, browser, router) { | ||
// This tests throwing a string inside a custom tracer. It shows that in a specific case, the | ||
// agent will double count the error because the error is first caught in the custom node, re-thrown, and caught again in the click event listener. | ||
// This behavior only happens in ie11, other browsers ignore the string error and only generate 1 error. | ||
waitForPageLoadAnInitialCalls(browser, router, 'spa/errors/captured-custom-string.html') | ||
.then(() => { | ||
return clickPageAndWaitForEventsAndErrors(t, browser, router) | ||
|
@@ -171,8 +168,7 @@ testDriver.test('string error in custom tracer', function (t, browser, router) { | |
// check that errors payload did not include the error | ||
const errors = getErrorsFromResponse(errorData, browser) | ||
|
||
if (browser.match('ie@>=11')) t.equal(errors.length, 2, 'should have 2 errors (1 String Class, 1 Error Class)') | ||
else t.equal(errors.length, 1, 'should have 1 errors') | ||
t.equal(errors.length, 1, 'should have 1 errors') | ||
|
||
let { body, query } = eventData | ||
let interactionTree = (body && body.length ? body : querypack.decode(query.e))[0] | ||
|
@@ -183,6 +179,7 @@ testDriver.test('string error in custom tracer', function (t, browser, router) { | |
var nodeId = interactionTree.children[0].nodeId | ||
|
||
var error = errors[0] | ||
console.log(JSON.stringify(error)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remnant |
||
t.equal(error.params.message, 'some error') | ||
t.equal(error.params.browserInteractionId, interactionId, 'should have the correct interaction id') | ||
t.equal(error.params.parentNodeId, nodeId, 'has the correct node id') | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove NR_ERR_PROP from the constants file too?