🐛 [RUMF-1203] fix stopSessionReplayRecording instrumentation clean…

…up (#1442)

* 🐛 [RUMF-1203] use `instrumentMethod` for CSSStyleSheet methods

* ♻️ [RUMF-1203] simplify input observer a bit

* ✨ [RUMF-1203] implement a instrumentSetter similar to instrumentMethod

* 🐛 [RUMF-1203] replace `hookSetter` with the new `instrumentSetter`

packages/core/src/index.ts

@@ -44,7 +44,7 @@ export * from './tools/timeUtils'
 export * from './tools/utils'
 export * from './tools/createEventRateLimiter'
 export * from './tools/browserDetection'
-export { instrumentMethod, instrumentMethodAndCallOriginal } from './tools/instrumentMethod'
+export { instrumentMethod, instrumentMethodAndCallOriginal, instrumentSetter } from './tools/instrumentMethod'
 export {
   ErrorSource,
   ErrorHandling,

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

@@ -1,4 +1,7 @@
-import { instrumentMethod } from './instrumentMethod'
+import type { Clock } from '../../test/specHelper'
+import { mockClock } from '../../test/specHelper'
+import { instrumentMethod, instrumentSetter } from './instrumentMethod'
+import { noop } from './utils'
 
 describe('instrumentMethod', () => {
   it('replaces the original method', () => {
@@ -104,3 +107,155 @@ describe('instrumentMethod', () => {
     object.method = () => originalMethod() + 2
   }
 })
+
+describe('instrumentSetter', () => {
+  let clock: Clock
+  beforeEach(() => {
+    clock = mockClock()
+  })
+  afterEach(() => {
+    clock.cleanup()
+  })
+
+  it('replaces the original setter', () => {
+    const originalSetter = () => {
+      // do nothing particular, only used to test if this setter gets replaced
+    }
+    const object = {} as { foo: number }
+    Object.defineProperty(object, 'foo', { set: originalSetter, configurable: true })
+
+    instrumentSetter(object, 'foo', noop)
+
+    // eslint-disable-next-line @typescript-eslint/unbound-method
+    expect(Object.getOwnPropertyDescriptor(object, 'foo')!.set).not.toBe(originalSetter)
+  })
+
+  it('skips instrumentation if there is no original setter', () => {
+    const object = { foo: 1 }
+
+    instrumentSetter(object, 'foo', noop)
+    // eslint-disable-next-line @typescript-eslint/unbound-method
+    expect(Object.getOwnPropertyDescriptor(object, 'foo')!.set).toBeUndefined()
+  })
+
+  it('skips instrumentation if the descriptor is not configurable', () => {
+    const originalSetter = () => {
+      // do nothing particular, only used to test if this setter gets replaced
+    }
+    const object = {} as { foo: number }
+    Object.defineProperty(object, 'foo', { set: originalSetter, configurable: false })
+
+    instrumentSetter(object, 'foo', noop)
+    // eslint-disable-next-line @typescript-eslint/unbound-method
+    expect(Object.getOwnPropertyDescriptor(object, 'foo')!.set).toBe(originalSetter)
+  })
+
+  it('calls the original setter', () => {
+    const originalSetterSpy = jasmine.createSpy()
+    const object = {} as { foo: number }
+    Object.defineProperty(object, 'foo', { set: originalSetterSpy, configurable: true })
+
+    instrumentSetter(object, 'foo', noop)
+
+    object.foo = 1
+    expect(originalSetterSpy).toHaveBeenCalledOnceWith(1)
+  })
+
+  it('calls the instrumentation asynchronously', () => {
+    const instrumentationSetterSpy = jasmine.createSpy()
+    const object = {} as { foo: number }
+    Object.defineProperty(object, 'foo', { set: noop, configurable: true })
+
+    instrumentSetter(object, 'foo', instrumentationSetterSpy)
+
+    object.foo = 1
+    expect(instrumentationSetterSpy).not.toHaveBeenCalled()
+    clock.tick(0)
+    expect(instrumentationSetterSpy).toHaveBeenCalledOnceWith(object, 1)
+  })
+
+  it('allows other instrumentations from third parties', () => {
+    const object = {} as { foo: number }
+    Object.defineProperty(object, 'foo', { set: noop, configurable: true })
+    const instrumentationSetterSpy = jasmine.createSpy()
+    instrumentSetter(object, 'foo', instrumentationSetterSpy)
+
+    const thirdPartyInstrumentationSpy = thirdPartyInstrumentation(object)
+
+    object.foo = 2
+    expect(thirdPartyInstrumentationSpy).toHaveBeenCalledOnceWith(2)
+    clock.tick(0)
+    expect(instrumentationSetterSpy).toHaveBeenCalledOnceWith(object, 2)
+  })
+
+  describe('stop()', () => {
+    it('restores the original behavior', () => {
+      const object = {} as { foo: number }
+      const originalSetter = () => {
+        // do nothing particular, only used to test if this setter gets replaced
+      }
+      Object.defineProperty(object, 'foo', { set: originalSetter, configurable: true })
+      const { stop } = instrumentSetter(object, 'foo', noop)
+
+      stop()
+
+      // eslint-disable-next-line @typescript-eslint/unbound-method
+      expect(Object.getOwnPropertyDescriptor(object, 'foo')!.set).toBe(originalSetter)
+    })
+
+    it('does not call the instrumentation anymore', () => {
+      const object = {} as { foo: number }
+      Object.defineProperty(object, 'foo', { set: noop, configurable: true })
+      const instrumentationSetterSpy = jasmine.createSpy()
+      const { stop } = instrumentSetter(object, 'foo', instrumentationSetterSpy)
+
+      stop()
+
+      object.foo = 2
+
+      expect(instrumentationSetterSpy).not.toHaveBeenCalled()
+    })
+
+    describe('when the method has been instrumented by a third party', () => {
+      it('should not break the third party instrumentation', () => {
+        const object = {} as { foo: number }
+        Object.defineProperty(object, 'foo', { set: noop, configurable: true })
+        const { stop } = instrumentSetter(object, 'foo', noop)
+
+        const thirdPartyInstrumentationSpy = thirdPartyInstrumentation(object)
+
+        stop()
+
+        // eslint-disable-next-line @typescript-eslint/unbound-method
+        expect(Object.getOwnPropertyDescriptor(object, 'foo')!.set).toBe(thirdPartyInstrumentationSpy)
+      })
+
+      it('does not call the instrumentation', () => {
+        const object = {} as { foo: number }
+        Object.defineProperty(object, 'foo', { set: noop, configurable: true })
+        const instrumentationSetterSpy = jasmine.createSpy()
+        const { stop } = instrumentSetter(object, 'foo', noop)
+
+        thirdPartyInstrumentation(object)
+
+        stop()
+
+        object.foo = 2
+
+        expect(instrumentationSetterSpy).not.toHaveBeenCalled()
+      })
+    })
+  })
+
+  function thirdPartyInstrumentation(object: { foo: number }) {
+    // eslint-disable-next-line @typescript-eslint/unbound-method
+    const originalSetter = Object.getOwnPropertyDescriptor(object, 'foo')!.set
+    const thirdPartyInstrumentationSpy = jasmine.createSpy().and.callFake(function (this: any, value) {
+      if (originalSetter) {
+        originalSetter.call(this, value)
+      }
+    })
+    Object.defineProperty(object, 'foo', { set: thirdPartyInstrumentationSpy })
+    return thirdPartyInstrumentationSpy
+  }
+})

packages/core/src/tools/instrumentMethod.ts

@@ -1,4 +1,5 @@
-import { callMonitored } from '../domain/internalMonitoring'
+import { callMonitored, monitor } from '../domain/internalMonitoring'
+import { noop } from './utils'
 
 export function instrumentMethod<OBJECT extends { [key: string]: any }, METHOD extends keyof OBJECT>(
   object: OBJECT,
@@ -35,8 +36,8 @@ export function instrumentMethodAndCallOriginal<OBJECT extends { [key: string]:
     before,
     after,
   }: {
-    before?: (this: OBJECT, ...args: Parameters<OBJECT[METHOD]>) => ReturnType<OBJECT[METHOD]>
-    after?: (this: OBJECT, ...args: Parameters<OBJECT[METHOD]>) => ReturnType<OBJECT[METHOD]>
+    before?: (this: OBJECT, ...args: Parameters<OBJECT[METHOD]>) => void
+    after?: (this: OBJECT, ...args: Parameters<OBJECT[METHOD]>) => void
   }
 ) {
   return instrumentMethod(
@@ -65,3 +66,43 @@ export function instrumentMethodAndCallOriginal<OBJECT extends { [key: string]:
       }
   )
 }
+
+export function instrumentSetter<OBJECT extends { [key: string]: any }, PROPERTY extends keyof OBJECT>(
+  object: OBJECT,
+  property: PROPERTY,
+  after: (thisObject: OBJECT, value: OBJECT[PROPERTY]) => void
+) {
+  const originalDescriptor = Object.getOwnPropertyDescriptor(object, property)
+  if (!originalDescriptor || !originalDescriptor.set || !originalDescriptor.configurable) {
+    return { stop: noop }
+  }
+
+  let instrumentation = (thisObject: OBJECT, value: OBJECT[PROPERTY]) => {
+    // put hooked setter into event loop to avoid of set latency
+    setTimeout(
+      monitor(() => {
+        after(thisObject, value)
+      }),
+      0
+    )
+  }
+
+  const instrumentationWrapper = function (this: OBJECT, value: OBJECT[PROPERTY]) {
+    originalDescriptor.set!.call(this, value)
+    instrumentation(this, value)
+  }
+
+  Object.defineProperty(object, property, {
+    set: instrumentationWrapper,
+  })
+
+  return {
+    stop: () => {
+      if (Object.getOwnPropertyDescriptor(object, property)?.set === instrumentationWrapper) {
+        Object.defineProperty(object, property, originalDescriptor)
+      } else {
+        instrumentation = noop
+      }
+    },
+  }
+}

packages/rum/src/domain/record/observer.ts

@@ -1,21 +1,20 @@
 import type { DefaultPrivacyLevel } from '@datadog/browser-core'
 import {
+  instrumentSetter,
+  instrumentMethodAndCallOriginal,
   assign,
   monitor,
-  callMonitored,
   throttle,
   DOM_EVENT,
   addEventListeners,
   addEventListener,
-  includes,
   noop,
 } from '@datadog/browser-core'
 import { NodePrivacyLevel } from '../../constants'
 import { getNodePrivacyLevel, shouldMaskNode } from './privacy'
 import { getElementInputValue, getSerializedNodeId, hasSerializedNode } from './serializationUtils'
 import type {
   FocusCallback,
-  HookResetter,
   InputCallback,
   InputState,
   ListenerHandler,
@@ -32,7 +31,7 @@ import type {
   MouseInteractionParam,
 } from './types'
 import { IncrementalSource, MediaInteractions, MouseInteractions } from './types'
-import { forEach, hookSetter, isTouchEvent } from './utils'
+import { forEach, isTouchEvent } from './utils'
 import type { MutationController } from './mutationObserver'
 import { startMutationObserver } from './mutationObserver'
 
@@ -201,18 +200,12 @@ function initViewportResizeObserver(cb: ViewportResizeCallback): ListenerHandler
   return addEventListener(window, DOM_EVENT.RESIZE, updateDimension, { capture: true, passive: true }).stop
 }
 
-export const INPUT_TAGS = ['INPUT', 'TEXTAREA', 'SELECT']
-const lastInputStateMap: WeakMap<EventTarget, InputState> = new WeakMap()
 export function initInputObserver(cb: InputCallback, defaultPrivacyLevel: DefaultPrivacyLevel): ListenerHandler {
-  function eventHandler(event: { target: EventTarget | null }) {
-    const target = event.target as HTMLInputElement | HTMLTextAreaElement
+  const lastInputStateMap: WeakMap<Node, InputState> = new WeakMap()
+
+  function onElementChange(target: HTMLInputElement | HTMLTextAreaElement | HTMLSelectElement) {
     const nodePrivacyLevel = getNodePrivacyLevel(target, defaultPrivacyLevel)
-    if (
-      !target ||
-      !target.tagName ||
-      !includes(INPUT_TAGS, target.tagName) ||
-      nodePrivacyLevel === NodePrivacyLevel.HIDDEN
-    ) {
+    if (nodePrivacyLevel === NodePrivacyLevel.HIDDEN) {
       return
     }
 
@@ -272,73 +265,64 @@ export function initInputObserver(cb: InputCallback, defaultPrivacyLevel: Defaul
     }
   }
 
-  const { stop: stopEventListeners } = addEventListeners(document, [DOM_EVENT.INPUT, DOM_EVENT.CHANGE], eventHandler, {
-    capture: true,
-    passive: true,
-  })
+  const { stop: stopEventListeners } = addEventListeners(
+    document,
+    [DOM_EVENT.INPUT, DOM_EVENT.CHANGE],
+    (event) => {
+      if (
+        event.target instanceof HTMLInputElement ||
+        event.target instanceof HTMLTextAreaElement ||
+        event.target instanceof HTMLSelectElement
+      ) {
+        onElementChange(event.target)
+      }
+    },
+    {
+      capture: true,
+      passive: true,
+    }
+  )
 
-  const propertyDescriptor = Object.getOwnPropertyDescriptor(HTMLInputElement.prototype, 'value')
-  const hookProperties: Array<[HTMLElement, string]> = [
-    [HTMLInputElement.prototype, 'value'],
-    [HTMLInputElement.prototype, 'checked'],
-    [HTMLSelectElement.prototype, 'value'],
-    [HTMLTextAreaElement.prototype, 'value'],
-    // Some UI library use selectedIndex to set select value
-    [HTMLSelectElement.prototype, 'selectedIndex'],
+  const instrumentationStoppers = [
+    instrumentSetter(HTMLInputElement.prototype, 'value', onElementChange),
+    instrumentSetter(HTMLInputElement.prototype, 'checked', onElementChange),
+    instrumentSetter(HTMLSelectElement.prototype, 'value', onElementChange),
+    instrumentSetter(HTMLTextAreaElement.prototype, 'value', onElementChange),
+    instrumentSetter(HTMLSelectElement.prototype, 'selectedIndex', onElementChange),
   ]
 
-  const hookResetters: HookResetter[] = []
-  if (propertyDescriptor && propertyDescriptor.set) {
-    hookResetters.push(
-      ...hookProperties.map((p) =>
-        hookSetter<HTMLElement>(p[0], p[1], {
-          set: monitor(function () {
-            // mock to a normal event
-            eventHandler({ target: this })
-          }),
-        })
-      )
-    )
-  }
-
   return () => {
-    hookResetters.forEach((h) => h())
+    instrumentationStoppers.forEach((stopper) => stopper.stop())
     stopEventListeners()
   }
 }
 
 function initStyleSheetObserver(cb: StyleSheetRuleCallback): ListenerHandler {
-  // eslint-disable-next-line @typescript-eslint/unbound-method
-  const insertRule = CSSStyleSheet.prototype.insertRule
-  CSSStyleSheet.prototype.insertRule = function (this: CSSStyleSheet, rule: string, index?: number) {
-    callMonitored(() => {
+  const { stop: restoreInsertRule } = instrumentMethodAndCallOriginal(CSSStyleSheet.prototype, 'insertRule', {
+    before(rule, index) {
       if (hasSerializedNode(this.ownerNode!)) {
         cb({
           id: getSerializedNodeId(this.ownerNode),
           adds: [{ rule, index }],
         })
       }
-    })
-    return insertRule.call(this, rule, index)
-  }
+    },
+  })
 
-  // eslint-disable-next-line @typescript-eslint/unbound-method
-  const deleteRule = CSSStyleSheet.prototype.deleteRule
-  CSSStyleSheet.prototype.deleteRule = function (this: CSSStyleSheet, index: number) {
-    callMonitored(() => {
+  const { stop: restoreDeleteRule } = instrumentMethodAndCallOriginal(CSSStyleSheet.prototype, 'deleteRule', {
+    before(index) {
       if (hasSerializedNode(this.ownerNode!)) {
         cb({
           id: getSerializedNodeId(this.ownerNode),
           removes: [{ index }],
         })
       }
-    })
-    return deleteRule.call(this, index)
-  }
+    },
+  })
 
   return () => {
-    CSSStyleSheet.prototype.insertRule = insertRule
-    CSSStyleSheet.prototype.deleteRule = deleteRule
+    restoreInsertRule()
+    restoreDeleteRule()
   }
 }