feat: reduce jserrors wrapping and remove onerror use (#614)

Author: patrickhousley

src/features/jserrors/aggregate/compute-stack-trace.js

@@ -233,7 +233,7 @@ function computeStackTraceBySourceAndLine (ex) {
       mode: 'sourceline',
       name: className,
       message: ex.message,
-      stackString: getClassName(ex) + ': ' + ex.message + '\n    in evaluated code',
+      stackString: className + ': ' + ex.message + '\n    in evaluated code',
       frames: [{
         func: 'evaluated code'
       }]

src/features/jserrors/instrument/debug.js

@@ -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)
+        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 @@ export class Instrument extends InstrumentBase {
   /** 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)
+    }
+
+    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)
     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)
   }
 }

src/features/jserrors/instrument/index.js

@@ -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'
+    this.message = message
+    this.sourceURL = filename
+    this.line = lineno
+    this.column = colno
+  }
+}

src/features/jserrors/instrument/uncaught-error.js

@@ -127,7 +127,7 @@ export function setAPI (agentIdentifier, forceDrain) {
           try {
             return cb.apply(this, arguments)
           } catch (err) {
-            tracerEE.emit('fn-err', [arguments, this, typeof err == 'string' ? new Error(err) : err], contextStore)
+            tracerEE.emit('fn-err', [arguments, this, err], contextStore)
             // the error came from outside the agent, so don't swallow
             throw err
           } finally {

src/loaders/api/api.js

@@ -30,8 +30,7 @@ testDriver.test('reporting uncaught errors', supported, function (t, browser, ro
     const expectedErrorMessages = [
       { message: 'original onerror', tested: false },
       { message: 'uncaught error', tested: false },
-      { message: 'fake', tested: false },
-      { message: 'original return false', tested: false }
+      { message: 'original return abc', tested: false }
     ]
     actualErrors.forEach(err => {
       const targetError = expectedErrorMessages.find(x => x.message === err.params.message)
@@ -43,7 +42,7 @@ testDriver.test('reporting uncaught errors', supported, function (t, browser, ro
       if (err.params.message === 'fake') t.ok(err.params.exceptionClass !== 'Error', `fake error has correct exceptionClass (${err.params.exceptionClass})`)
       else t.ok(err.params.exceptionClass === 'Error', `error has correct exceptionClass (${err.params.exceptionClass})`)
     })
-    t.ok(expectedErrorMessages.every(x => x.tested), 'All expected error messages were found')
+    t.ok(expectedErrorMessages.every(x => x.tested), `All expected error messages were found ${JSON.stringify(expectedErrorMessages.filter(x => !x.tested))}`)
     t.end()
   }).catch(fail)
 

tests/functional/err/uncaught.test.js

@@ -49,7 +49,7 @@ testDriver.test('unhandledPromiseRejections are caught and are readable', suppor
       t.ok(!!err.params.stack_trace, 'stack_trace exists')
       t.ok(!!err.params.stackHash, 'stackHash exists')
     })
-    t.ok(expectedErrorMessages.every(x => x.tested), `All expected error messages were found ${expectedErrorMessages.filter(x => !x.tested)}`)
+    t.ok(expectedErrorMessages.every(x => x.tested), `All expected error messages were found ${JSON.stringify(expectedErrorMessages.filter(x => !x.tested))}`)
     t.end()
   }).catch(fail)
 

tests/functional/err/unhandled-rejection-readable.test.js

@@ -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))
       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')

tests/functional/spa/errors.test.js

@@ -9,6 +9,6 @@
   "author": "",
   "license": "ISC",
   "dependencies": {
-    "@newrelic/browser-agent": "file:../../../temp/newrelic-browser-agent-1.235.0.tgz"
+    "@newrelic/browser-agent": "file:../../../temp/newrelic-browser-agent-1.236.0.tgz"
   }
 }

tools/test-builds/browser-agent-wrapper/package.json

@@ -9,6 +9,6 @@
     "author": "",
     "license": "ISC",
     "dependencies": {
-        "@newrelic/browser-agent": "file:../../../temp/newrelic-browser-agent-1.235.0.tgz"
+        "@newrelic/browser-agent": "file:../../../temp/newrelic-browser-agent-1.236.0.tgz"
     }
 }

tools/test-builds/browser-tests/package.json

@@ -9,6 +9,6 @@
   "author": "",
   "license": "ISC",
   "dependencies": {
-    "@newrelic/browser-agent": "file:../../../temp/newrelic-browser-agent-1.235.0.tgz"
+    "@newrelic/browser-agent": "file:../../../temp/newrelic-browser-agent-1.236.0.tgz"
   }
 }

tools/test-builds/raw-src-wrapper/package.json

@@ -4,7 +4,7 @@
   "main": "index.js",
   "license": "MIT",
   "dependencies": {
-    "@newrelic/browser-agent": "file:../../../temp/newrelic-browser-agent-1.235.0.tgz",
+    "@newrelic/browser-agent": "file:../../../temp/newrelic-browser-agent-1.236.0.tgz",
     "react": "18.2.0",
     "react-dom": "18.2.0"
   },