fix(utils): Dereference DOM events after they have servered their purpose (#9224)

Author: lforst

packages/utils/src/instrument.ts

@@ -12,7 +12,7 @@ import type {
 import { isString } from './is';
 import type { ConsoleLevel } from './logger';
 import { CONSOLE_LEVELS, logger, originalConsoleMethods } from './logger';
-import { fill } from './object';
+import { addNonEnumerableProperty, fill } from './object';
 import { getFunctionName } from './stacktrace';
 import { supportsHistory, supportsNativeFetch } from './supports';
 import { getGlobalObject, GLOBAL_OBJ } from './worldwide';
@@ -400,31 +400,24 @@ function instrumentHistory(): void {
   fill(WINDOW.history, 'replaceState', historyReplacementFunction);
 }
 
-const debounceDuration = 1000;
+const DEBOUNCE_DURATION = 1000;
 let debounceTimerID: number | undefined;
 let lastCapturedEvent: Event | undefined;
 
 /**
- * Decide whether the current event should finish the debounce of previously captured one.
- * @param previous previously captured event
- * @param current event to be captured
+ * Check whether two DOM events are similar to eachother. For example, two click events on the same button.
  */
-function shouldShortcircuitPreviousDebounce(previous: Event | undefined, current: Event): boolean {
-  // If there was no previous event, it should always be swapped for the new one.
-  if (!previous) {
-    return true;
-  }
-
+function areSimilarDomEvents(a: Event, b: Event): boolean {
   // If both events have different type, then user definitely performed two separate actions. e.g. click + keypress.
-  if (previous.type !== current.type) {
-    return true;
+  if (a.type !== b.type) {
+    return false;
   }
 
   try {
     // If both events have the same type, it's still possible that actions were performed on different targets.
     // e.g. 2 clicks on different buttons.
-    if (previous.target !== current.target) {
-      return true;
+    if (a.target !== b.target) {
+      return false;
     }
   } catch (e) {
     // just accessing `target` property can throw an exception in some rare circumstances
@@ -434,7 +427,7 @@ function shouldShortcircuitPreviousDebounce(previous: Event | undefined, current
   // If both events have the same type _and_ same `target` (an element which triggered an event, _not necessarily_
   // to which an event listener was attached), we treat them as the same action, as we want to capture
   // only one breadcrumb. e.g. multiple clicks on the same button, or typing inside a user input box.
-  return false;
+  return true;
 }
 
 /**
@@ -475,11 +468,11 @@ function shouldSkipDOMEvent(event: Event): boolean {
  * @hidden
  */
 function makeDOMEventHandler(handler: Function, globalListener: boolean = false): (event: Event) => void {
-  return (event: Event): void => {
+  return (event: Event & { _sentryCaptured?: true }): void => {
     // It's possible this handler might trigger multiple times for the same
     // event (e.g. event propagation through node ancestors).
     // Ignore if we've already captured that event.
-    if (!event || lastCapturedEvent === event) {
+    if (!event || event['_sentryCaptured']) {
       return;
     }
 
@@ -488,20 +481,15 @@ function makeDOMEventHandler(handler: Function, globalListener: boolean = false)
       return;
     }
 
+    // Mark event as "seen"
+    addNonEnumerableProperty(event, '_sentryCaptured', true);
+
     const name = event.type === 'keypress' ? 'input' : event.type;
 
-    // If there is no debounce timer, it means that we can safely capture the new event and store it for future comparisons.
-    if (debounceTimerID === undefined) {
-      handler({
-        event: event,
-        name,
-        global: globalListener,
-      });
-      lastCapturedEvent = event;
-    }
-    // If there is a debounce awaiting, see if the new event is different enough to treat it as a unique one.
+    // If there is no last captured event, it means that we can safely capture the new event and store it for future comparisons.
+    // If there is a last captured event, see if the new event is different enough to treat it as a unique one.
     // If that's the case, emit the previous event and store locally the newly-captured DOM event.
-    else if (shouldShortcircuitPreviousDebounce(lastCapturedEvent, event)) {
+    if (lastCapturedEvent === undefined || !areSimilarDomEvents(lastCapturedEvent, event)) {
       handler({
         event: event,
         name,
@@ -513,8 +501,8 @@ function makeDOMEventHandler(handler: Function, globalListener: boolean = false)
     // Start a new debounce timer that will prevent us from capturing multiple events that should be grouped together.
     clearTimeout(debounceTimerID);
     debounceTimerID = WINDOW.setTimeout(() => {
-      debounceTimerID = undefined;
-    }, debounceDuration);
+      lastCapturedEvent = undefined;
+    }, DEBOUNCE_DURATION);
   };
 }
 

packages/utils/src/object.ts

@@ -42,7 +42,7 @@ export function fill(source: { [key: string]: any }, name: string, replacementFa
  * @param name The name of the property to be set
  * @param value The value to which to set the property
  */
-export function addNonEnumerableProperty(obj: { [key: string]: unknown }, name: string, value: unknown): void {
+export function addNonEnumerableProperty(obj: object, name: string, value: unknown): void {
   try {
     Object.defineProperty(obj, name, {
       // enumerable: false, // the default, so we can save on bundle size by not explicitly setting it