[RUM] Catch errors thrown by user callbacks (#745)

* Add catchErrors function

* Use catchErrors to wrap beforeSend

* limitModification does not need to catch errors anymore

* Use catchErrors to wrap onNewLocation

* Use catchErrors to wrap onReady callbacks before initilization

* Improve error message

Co-authored-by: Benoît <[email protected]>

* 👌 Move catchErrors to buildConfiguration

* 👌 Rename to catchUserErrors

* 👌 Refactor the code

* 👌 Test catching errors on onReady callbacks before init

* Fix typo

* Fix tests

* Refactor code

Co-authored-by: Bastien Caudan <[email protected]>

* Move onNewLocation wrapping to startRumEventCollection

This is needed to make the test able to pass its own Lifecycle object
and test that onNewLocation catches user errors after being wrapped.

The stopViewCollection is needed because startViewCollection attaches a
listener to history changes, and if not removed, it will be executed on
all following tests and generates unwanted console.errors

* 👌 Test catching errors of beforeSend and onNewLocation callbacks

Co-authored-by: Benoît <[email protected]>
Co-authored-by: Bastien Caudan <[email protected]>

packages/core/src/boot/init.spec.ts

@@ -0,0 +1,48 @@
+import { defineGlobal } from './init'
+
+describe('defineGlobal', () => {
+  it('adds new property to the global object', () => {
+    const myGlobal = {} as any
+    const value = 'my value'
+    defineGlobal(myGlobal, 'foo', value)
+    expect(myGlobal.foo).toBe(value)
+  })
+
+  it('overrides property if exists on the global object', () => {
+    const myGlobal = { foo: 'old value' }
+    const value = 'my value'
+    defineGlobal(myGlobal, 'foo', value)
+    expect(myGlobal.foo).toBe(value)
+  })
+
+  it('run the queued callbacks on the old value', () => {
+    const fn1 = jasmine.createSpy()
+    const fn2 = jasmine.createSpy()
+    const myGlobal: any = {
+      foo: {
+        q: [fn1, fn2],
+      },
+    }
+    const value = 'my value'
+    defineGlobal(myGlobal, 'foo', value)
+    expect(myGlobal.foo).toBe(value)
+    expect(fn1).toHaveBeenCalled()
+    expect(fn2).toHaveBeenCalled()
+  })
+
+  it('catches the errors thrown by the queued callbacks', () => {
+    const myError = 'Ooops!'
+    const onReady = () => {
+      throw myError
+    }
+    const myGlobal: any = {
+      foo: {
+        q: [onReady],
+      },
+    }
+    const consoleErrorSpy = spyOn(console, 'error')
+
+    defineGlobal(myGlobal, 'foo', {})
+    expect(consoleErrorSpy).toHaveBeenCalledWith('onReady callback threw an error:', myError)
+  })
+})

packages/core/src/boot/init.ts

@@ -2,6 +2,7 @@ import { areCookiesAuthorized, CookieOptions } from '../browser/cookie'
 import { buildConfiguration, UserConfiguration } from '../domain/configuration'
 import { setDebugMode, startInternalMonitoring } from '../domain/internalMonitoring'
 import { Datacenter } from '../domain/transportConfiguration'
+import { catchUserErrors } from '../tools/catchUserErrors'
 
 export function makePublicApi<T>(stub: T): T & { onReady(callback: () => void): void } {
   const publicApi = {
@@ -31,7 +32,7 @@ export function defineGlobal<Global, Name extends keyof Global>(global: Global,
   const existingGlobalVariable: { q?: Array<() => void> } | undefined = global[name]
   global[name] = api
   if (existingGlobalVariable && existingGlobalVariable.q) {
-    existingGlobalVariable.q.forEach((fn) => fn())
+    existingGlobalVariable.q.forEach((fn) => catchUserErrors(fn, 'onReady callback threw an error:')())
   }
 }
 

packages/core/src/domain/configuration.spec.ts

@@ -1,3 +1,4 @@
+import { RumEvent } from '../../../rum/src'
 import { BuildEnv, BuildMode } from '../boot/init'
 import { buildConfiguration } from './configuration'
 import { Datacenter } from './transportConfiguration'
@@ -31,4 +32,33 @@ describe('configuration', () => {
       expect(configuration.cookieOptions).toEqual({ secure: false, crossSite: false, domain: jasmine.any(String) })
     })
   })
+
+  describe('beforeSend', () => {
+    it('should be undefined when beforeSend is missing on user configuration', () => {
+      const configuration = buildConfiguration({ clientToken }, usEnv)
+      expect(configuration.beforeSend).toBeUndefined()
+    })
+
+    it('should return the same result as the original', () => {
+      const beforeSend = (event: RumEvent) => {
+        if (event.view.url === '/foo') {
+          return false
+        }
+      }
+      const configuration = buildConfiguration({ clientToken, beforeSend }, usEnv)
+      expect(configuration.beforeSend!({ view: { url: '/foo' } })).toBeFalse()
+      expect(configuration.beforeSend!({ view: { url: '/bar' } })).toBeUndefined()
+    })
+
+    it('should catch errors and log them', () => {
+      const myError = 'Ooops!'
+      const beforeSend = () => {
+        throw myError
+      }
+      const configuration = buildConfiguration({ clientToken, beforeSend }, usEnv)
+      const consoleErrorSpy = spyOn(console, 'error')
+      expect(configuration.beforeSend!(null)).toBeUndefined()
+      expect(consoleErrorSpy).toHaveBeenCalledWith('beforeSend threw an error:', myError)
+    })
+  })
 })

packages/core/src/domain/configuration.ts

@@ -1,5 +1,6 @@
 import { BuildEnv } from '../boot/init'
 import { CookieOptions, getCurrentSite } from '../browser/cookie'
+import { catchUserErrors } from '../tools/catchUserErrors'
 import { includes, ONE_KILO_BYTE, ONE_SECOND } from '../tools/utils'
 import { computeTransportConfiguration, Datacenter } from './transportConfiguration'
 
@@ -105,7 +106,8 @@ export function buildConfiguration(userConfiguration: UserConfiguration, buildEn
     : []
 
   const configuration: Configuration = {
-    beforeSend: userConfiguration.beforeSend,
+    beforeSend:
+      userConfiguration.beforeSend && catchUserErrors(userConfiguration.beforeSend, 'beforeSend threw an error:'),
     cookieOptions: buildCookieOptions(userConfiguration),
     isEnabled: (feature: string) => includes(enableExperimentalFeatures, feature),
     service: userConfiguration.service,

packages/core/src/index.ts

@@ -43,6 +43,7 @@ export {
   resetFetchProxy,
 } from './browser/fetchProxy'
 export { BoundedBuffer } from './tools/boundedBuffer'
+export { catchUserErrors } from './tools/catchUserErrors'
 export { createContextManager } from './tools/contextManager'
 export { limitModification } from './tools/limitModification'
 

packages/core/src/tools/catchUserErrors.spec.ts

@@ -0,0 +1,18 @@
+import { catchUserErrors } from './catchUserErrors'
+
+describe('catchUserErrors', () => {
+  it('returns the same result as the original function', () => {
+    const wrappedFn = catchUserErrors((a: number, b: number) => a + b, 'Error during callback')
+    expect(wrappedFn(10, 2)).toBe(12)
+  })
+
+  it('logs errors using console.error and returns undefined', () => {
+    const consoleErrorSpy = spyOn(console, 'error')
+    const myError = 'Ooops!'
+    const wrappedFn = catchUserErrors(() => {
+      throw myError
+    }, 'Error during callback')
+    expect(wrappedFn()).toBe(undefined)
+    expect(consoleErrorSpy).toHaveBeenCalledWith('Error during callback', myError)
+  })
+})

packages/core/src/tools/catchUserErrors.ts

@@ -0,0 +1,9 @@
+export function catchUserErrors<Args extends any[], R>(fn: (...args: Args) => R, errorMsg: string) {
+  return (...args: Args) => {
+    try {
+      return fn(...args)
+    } catch (err) {
+      console.error(errorMsg, err)
+    }
+  }
+}

packages/core/src/tools/limitModification.spec.ts

@@ -92,21 +92,4 @@ describe('limitModification', () => {
       foo: { bar: 'qux' },
     })
   })
-
-  it('should catch and log modifier exception', () => {
-    const object = { foo: { bar: 'bar' }, qux: 'qux' }
-    const modifier = (candidate: any) => {
-      candidate.qux = 'modified'
-      candidate.foo.qux.bar = 'will throw'
-    }
-    const errorSpy = spyOn(console, 'error')
-
-    limitModification(object, ['foo.bar', 'qux'], modifier)
-
-    expect(errorSpy).toHaveBeenCalled()
-    expect(object).toEqual({
-      foo: { bar: 'bar' },
-      qux: 'qux',
-    })
-  })
 })

packages/core/src/tools/limitModification.ts

@@ -11,13 +11,7 @@ export function limitModification<T extends Context, Result>(
   modifier: (object: T) => Result
 ): Result | undefined {
   const clone = deepClone(object)
-  let result
-  try {
-    result = modifier(clone)
-  } catch (e) {
-    console.error(e)
-    return
-  }
+  const result = modifier(clone)
   modifiableFieldPaths.forEach((path) => {
     const originalValue = get(object, path)
     const newValue = get(clone, path)

packages/rum-core/src/boot/rum.spec.ts

@@ -5,7 +5,7 @@ import { RumPerformanceNavigationTiming } from '../browser/performanceCollection
 import { LifeCycle, LifeCycleEventType } from '../domain/lifeCycle'
 import { SESSION_KEEP_ALIVE_INTERVAL, THROTTLE_VIEW_UPDATE_PERIOD } from '../domain/rumEventsCollection/view/trackViews'
 import { RumEvent } from '../rumEvent.types'
-import { startRumEventCollection } from './rum'
+import { NewLocationListener, startRumEventCollection } from './rum'
 
 function collectServerEvents(lifeCycle: LifeCycle) {
   const serverRumEvents: RumEvent[] = []
@@ -184,3 +184,42 @@ describe('rum view url', () => {
     expect(serverRumEvents[0].view.url).toEqual('http://foo.com/')
   })
 })
+
+describe('rum onNewLocation', () => {
+  let setupBuilder: TestSetupBuilder
+  let serverRumEvents: RumEvent[]
+  let onNewLocation: NewLocationListener
+
+  beforeEach(() => {
+    setupBuilder = setup().beforeBuild(({ applicationId, location, lifeCycle, configuration, session }) => {
+      serverRumEvents = collectServerEvents(lifeCycle)
+      return startRumEventCollection(
+        applicationId,
+        location,
+        lifeCycle,
+        configuration,
+        session,
+        () => ({
+          context: {},
+          user: {},
+        }),
+        onNewLocation
+      )
+    })
+  })
+
+  afterEach(() => {
+    setupBuilder.cleanup()
+  })
+
+  it('should catch errors thrown by onNewLocation', () => {
+    const myError = 'Ooops!'
+    onNewLocation = () => {
+      throw myError
+    }
+    const consoleErrorSpy = spyOn(console, 'error')
+    setupBuilder.withFakeLocation('http://foo.com/').build()
+    expect(serverRumEvents[0].view.url).toEqual('http://foo.com/')
+    expect(consoleErrorSpy).toHaveBeenCalledWith('onNewLocation threw an error:', myError)
+  })
+})

packages/rum-core/src/boot/rum.ts

@@ -1,4 +1,4 @@
-import { combine, commonInit, Configuration } from '@datadog/browser-core'
+import { catchUserErrors, combine, commonInit, Configuration } from '@datadog/browser-core'
 import { startDOMMutationCollection } from '../browser/domMutationCollection'
 import { startPerformanceCollection } from '../browser/performanceCollection'
 import { startRumAssembly } from '../domain/assembly'
@@ -87,7 +87,11 @@ export function startRumEventCollection(
   startRumAssembly(applicationId, configuration, lifeCycle, session, parentContexts, getCommonContext)
   startLongTaskCollection(lifeCycle)
   startResourceCollection(lifeCycle, session)
-  const { addTiming } = startViewCollection(lifeCycle, location, onNewLocation)
+  const { addTiming, stop: stopViewCollection } = startViewCollection(
+    lifeCycle,
+    location,
+    onNewLocation && catchUserErrors(onNewLocation, 'onNewLocation threw an error:')
+  )
   const { addError } = startErrorCollection(lifeCycle, configuration)
   const { addAction } = startActionCollection(lifeCycle, configuration)
 
@@ -99,6 +103,7 @@ export function startRumEventCollection(
     addTiming,
 
     stop() {
+      stopViewCollection()
       // prevent batch from previous tests to keep running and send unwanted requests
       // could be replaced by stopping all the component when they will all have a stop method
       batch.stop()

packages/rum-core/src/domain/rumEventsCollection/view/trackViews.spec.ts

@@ -1,4 +1,4 @@
-import { Context } from '../../../../../core/src'
+import { catchUserErrors, Context } from '../../../../../core/src'
 import { RumEvent } from '../../../../../rum/src'
 import { setup, TestSetupBuilder } from '../../../../test/specHelper'
 import { NewLocationListener } from '../../../boot/rum'
@@ -223,7 +223,7 @@ describe('rum use onNewLocation callback to rename/ignore views', () => {
       .withFakeLocation('/foo')
       .beforeBuild(({ location, lifeCycle }) => {
         lifeCycle.subscribe(LifeCycleEventType.VIEW_UPDATED, handler)
-        return trackViews(location, lifeCycle, onNewLocation)
+        return trackViews(location, lifeCycle, catchUserErrors(onNewLocation, 'onNewLocation threw an error:'))
       })
   })
 

packages/rum-core/src/domain/rumEventsCollection/view/trackViews.ts

@@ -41,7 +41,6 @@ export function trackViews(
   lifeCycle: LifeCycle,
   onNewLocation: NewLocationListener = () => undefined
 ) {
-  onNewLocation = wrapOnNewLocation(onNewLocation)
   const startOrigin = 0
   const initialView = newView(
     lifeCycle,
@@ -345,15 +344,3 @@ function sanitizeTiming(name: string) {
   }
   return sanitized
 }
-
-function wrapOnNewLocation(onNewLocation: NewLocationListener): NewLocationListener {
-  return (newLocation, oldLocation) => {
-    let result
-    try {
-      result = onNewLocation(newLocation, oldLocation)
-    } catch (err) {
-      console.error('onNewLocation threw an error:', err)
-    }
-    return result
-  }
-}