🐛 [RUM-3599] do not define undefined instrumented method (#2662)

* 🐛 [RUM-3599] do not define undefined instrumented method

* use the polyfilled `startsWith`

* 👌📝 add some documentation regarding event handlers

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

@@ -28,15 +28,23 @@ describe('instrumentMethod', () => {
     expect(instrumentationSpy).toHaveBeenCalledBefore(originalSpy)
   })
 
-  it('sets a method originally undefined', () => {
+  it('does not set a method originally undefined', () => {
     const object: { method?: () => number } = {}
 
+    instrumentMethod(object, 'method', noop)
+
+    expect(object.method).toBeUndefined()
+  })
+
+  it('sets an event handler even if it was originally undefined', () => {
+    const object: { onevent?: () => void } = {}
+
     const instrumentationSpy = jasmine.createSpy()
-    instrumentMethod(object, 'method', instrumentationSpy)
+    instrumentMethod(object, 'onevent', instrumentationSpy)
 
-    expect(object.method).toBeDefined()
-    object.method!()
+    expect(object.onevent).toBeDefined()
 
+    object.onevent!()
     expect(instrumentationSpy).toHaveBeenCalled()
   })
 
@@ -133,27 +141,34 @@ describe('instrumentMethod', () => {
       })
 
       it('should not throw errors if original method was undefined', () => {
-        const object: { method?: () => number } = {}
+        const object: { onevent?: () => number } = {}
         const instrumentationStub = () => 2
-        const { stop } = instrumentMethod(object, 'method', instrumentationStub)
+        const { stop } = instrumentMethod(object, 'onevent', instrumentationStub)
 
         thirdPartyInstrumentation(object)
 
         stop()
 
-        expect(object.method).not.toThrow()
+        expect(object.onevent).not.toThrow()
       })
     })
   })
 
-  function thirdPartyInstrumentation(object: { method?: () => number }) {
+  function thirdPartyInstrumentation(object: { method?: () => number; onevent?: () => void }) {
     const originalMethod = object.method
     if (typeof originalMethod === 'function') {
       object.method = () => {
         originalMethod()
         return THIRD_PARTY_RESULT
       }
     }
+
+    const originalOnEvent = object.onevent
+    object.onevent = () => {
+      if (originalOnEvent) {
+        originalOnEvent()
+      }
+    }
   }
 })
 

packages/core/src/tools/instrumentMethod.ts

@@ -1,7 +1,7 @@
 import { setTimeout } from './timer'
 import { callMonitored } from './monitor'
 import { noop } from './utils/functionUtils'
-import { arrayFrom } from './utils/polyfills'
+import { arrayFrom, startsWith } from './utils/polyfills'
 
 /**
  * Object passed to the callback of an instrumented method call. See `instrumentMethod` for more
@@ -35,11 +35,23 @@ type PostCallCallback<TARGET extends { [key: string]: any }, METHOD extends keyo
  * Instruments a method on a object, calling the given callback before the original method is
  * invoked. The callback receives an object with information about the method call.
  *
+ * This function makes sure that we are "good citizens" regarding third party instrumentations: when
+ * removing the instrumentation, the original method is usually restored, but if a third party
+ * instrumentation was set after ours, we keep it in place and just replace our instrumentation with
+ * a noop.
+ *
  * Note: it is generally better to instrument methods that are "owned" by the object instead of ones
  * that are inherited from the prototype chain. Example:
  * * do:    `instrumentMethod(Array.prototype, 'push', ...)`
  * * don't: `instrumentMethod([], 'push', ...)`
  *
+ * This method is also used to set event handler properties (ex: window.onerror = ...), as it has
+ * the same requirements as instrumenting a method:
+ * * if the event handler is already set by a third party, we need to call it and not just blindly
+ * override it.
+ * * if the event handler is set by a third party after us, we need to keep it in place when
+ * removing ours.
+ *
  * @example
  *
  *  instrumentMethod(window, 'fetch', ({ target, parameters, onPostCall }) => {
@@ -50,12 +62,20 @@ type PostCallCallback<TARGET extends { [key: string]: any }, METHOD extends keyo
  *    })
  *  })
  */
-export function instrumentMethod<TARGET extends { [key: string]: any }, METHOD extends keyof TARGET>(
+export function instrumentMethod<TARGET extends { [key: string]: any }, METHOD extends keyof TARGET & string>(
   targetPrototype: TARGET,
   method: METHOD,
   onPreCall: (this: null, callInfos: InstrumentedMethodCall<TARGET, METHOD>) => void
 ) {
-  const original = targetPrototype[method]
+  let original = targetPrototype[method]
+
+  if (typeof original !== 'function') {
+    if (startsWith(method, 'on')) {
+      original = noop as TARGET[METHOD]
+    } else {
+      return { stop: noop }
+    }
+  }
 
   let instrumentation = createInstrumentedMethod(original, onPreCall)
 
@@ -86,7 +106,6 @@ function createInstrumentedMethod<TARGET extends { [key: string]: any }, METHOD
   // eslint-disable-next-line @typescript-eslint/no-unsafe-return
   return function (this: TARGET) {
     const parameters = arrayFrom(arguments) as Parameters<TARGET[METHOD]>
-    let result
 
     let postCallCallback: PostCallCallback<TARGET, METHOD> | undefined
 
@@ -100,10 +119,8 @@ function createInstrumentedMethod<TARGET extends { [key: string]: any }, METHOD
       },
     ])
 
-    if (typeof original === 'function') {
-      // eslint-disable-next-line @typescript-eslint/no-unsafe-call
-      result = original.apply(this, parameters)
-    }
+    // eslint-disable-next-line @typescript-eslint/no-unsafe-call
+    const result = original.apply(this, parameters)
 
     if (postCallCallback) {
       callMonitored(postCallCallback, null, [result])