[RUM] Catch errors thrown by user callbacks

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
-  }
-}