fix(utils): Avoid keeping a reference of last used event (#9387)

mydea · web-flow · commit d1515b9ee333 · 2023-10-30T12:58:44.000+01:00
As discussed some time ago, this changes the dom instrumentation to avoid keeping any reference to an event in the module scope. Instead, we put a `_sentryId` non-enumerable property on the event target (if possible) and check based on this. I also added some tests to verify the expected behavior, also for future changes here. Fixes #9204
diff --git a/packages/browser-integration-tests/suites/integrations/Breadcrumbs/dom/click/template.html b/packages/browser-integration-tests/suites/integrations/Breadcrumbs/dom/click/template.html
@@ -0,0 +1,11 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8" />
+    <title></title>
+  </head>
+  <body>
+    <button id="button1" type="button">Button 1</button>
+    <button id="button2" type="button">Button 2</button>
+  </body>
+</html>
diff --git a/packages/browser-integration-tests/suites/integrations/Breadcrumbs/dom/click/test.ts b/packages/browser-integration-tests/suites/integrations/Breadcrumbs/dom/click/test.ts
@@ -0,0 +1,59 @@
+import { expect } from '@playwright/test';
+import type { Event } from '@sentry/types';
+
+import { sentryTest } from '../../../../../utils/fixtures';
+import { getFirstSentryEnvelopeRequest } from '../../../../../utils/helpers';
+
+sentryTest('captures Breadcrumb for clicks & debounces them for a second', async ({ getLocalTestUrl, page }) => {
+  const url = await getLocalTestUrl({ testDir: __dirname });
+
+  await page.route('**/foo', route => {
+    return route.fulfill({
+      status: 200,
+      body: JSON.stringify({
+        userNames: ['John', 'Jane'],
+      }),
+      headers: {
+        'Content-Type': 'application/json',
+      },
+    });
+  });
+
+  const promise = getFirstSentryEnvelopeRequest<Event>(page);
+
+  await page.goto(url);
+
+  await page.click('#button1');
+  // not debounced because other target
+  await page.click('#button2');
+  // This should be debounced
+  await page.click('#button2');
+
+  // Wait a second for the debounce to finish
+  await page.waitForTimeout(1000);
+  await page.click('#button2');
+
+  await page.evaluate('Sentry.captureException("test exception")');
+
+  const eventData = await promise;
+
+  expect(eventData.exception?.values).toHaveLength(1);
+
+  expect(eventData.breadcrumbs).toEqual([
+    {
+      timestamp: expect.any(Number),
+      category: 'ui.click',
+      message: 'body > button#button1[type="button"]',
+    },
+    {
+      timestamp: expect.any(Number),
+      category: 'ui.click',
+      message: 'body > button#button2[type="button"]',
+    },
+    {
+      timestamp: expect.any(Number),
+      category: 'ui.click',
+      message: 'body > button#button2[type="button"]',
+    },
+  ]);
+});
diff --git a/packages/browser-integration-tests/suites/integrations/Breadcrumbs/dom/init.js b/packages/browser-integration-tests/suites/integrations/Breadcrumbs/dom/init.js
@@ -0,0 +1,10 @@
+import * as Sentry from '@sentry/browser';
+
+window.Sentry = Sentry;
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  defaultIntegrations: false,
+  integrations: [new Sentry.Integrations.Breadcrumbs()],
+  sampleRate: 1,
+});
diff --git a/packages/browser-integration-tests/suites/integrations/Breadcrumbs/dom/textInput/template.html b/packages/browser-integration-tests/suites/integrations/Breadcrumbs/dom/textInput/template.html
@@ -0,0 +1,11 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8" />
+    <title></title>
+  </head>
+  <body>
+    <input id="input1" type="text" />
+    <input id="input2" type="text" />
+  </body>
+</html>
diff --git a/packages/browser-integration-tests/suites/integrations/Breadcrumbs/dom/textInput/test.ts b/packages/browser-integration-tests/suites/integrations/Breadcrumbs/dom/textInput/test.ts
@@ -0,0 +1,66 @@
+import { expect } from '@playwright/test';
+import type { Event } from '@sentry/types';
+
+import { sentryTest } from '../../../../../utils/fixtures';
+import { getFirstSentryEnvelopeRequest } from '../../../../../utils/helpers';
+
+sentryTest('captures Breadcrumb for events on inputs & debounced them', async ({ getLocalTestUrl, page }) => {
+  const url = await getLocalTestUrl({ testDir: __dirname });
+
+  await page.route('**/foo', route => {
+    return route.fulfill({
+      status: 200,
+      body: JSON.stringify({
+        userNames: ['John', 'Jane'],
+      }),
+      headers: {
+        'Content-Type': 'application/json',
+      },
+    });
+  });
+
+  const promise = getFirstSentryEnvelopeRequest<Event>(page);
+
+  await page.goto(url);
+
+  await page.click('#input1');
+  // Not debounced because other event type
+  await page.type('#input1', 'John', { delay: 1 });
+  // This should be debounced
+  await page.type('#input1', 'Abby', { delay: 1 });
+  // not debounced because other target
+  await page.type('#input2', 'Anne', { delay: 1 });
+
+  // Wait a second for the debounce to finish
+  await page.waitForTimeout(1000);
+  await page.type('#input2', 'John', { delay: 1 });
+
+  await page.evaluate('Sentry.captureException("test exception")');
+
+  const eventData = await promise;
+
+  expect(eventData.exception?.values).toHaveLength(1);
+
+  expect(eventData.breadcrumbs).toEqual([
+    {
+      timestamp: expect.any(Number),
+      category: 'ui.click',
+      message: 'body > input#input1[type="text"]',
+    },
+    {
+      timestamp: expect.any(Number),
+      category: 'ui.input',
+      message: 'body > input#input1[type="text"]',
+    },
+    {
+      timestamp: expect.any(Number),
+      category: 'ui.input',
+      message: 'body > input#input2[type="text"]',
+    },
+    {
+      timestamp: expect.any(Number),
+      category: 'ui.input',
+      message: 'body > input#input2[type="text"]',
+    },
+  ]);
+});
diff --git a/packages/utils/src/instrument.ts b/packages/utils/src/instrument.ts
@@ -12,6 +12,7 @@ import type {
 import { isString } from './is';
 import type { ConsoleLevel } from './logger';
 import { CONSOLE_LEVELS, logger, originalConsoleMethods } from './logger';
+import { uuid4 } from './misc';
 import { addNonEnumerableProperty, fill } from './object';
 import { getFunctionName } from './stacktrace';
 import { supportsHistory, supportsNativeFetch } from './supports';
@@ -404,21 +405,24 @@ function instrumentHistory(): void {
 
 const DEBOUNCE_DURATION = 1000;
 let debounceTimerID: number | undefined;
-let lastCapturedEvent: Event | undefined;
+let lastCapturedEventType: string | undefined;
+let lastCapturedEventTargetId: string | undefined;
+
+type SentryWrappedTarget = HTMLElement & { _sentryId?: string };
 
 /**
- * Check whether two DOM events are similar to eachother. For example, two click events on the same button.
+ * Check whether the event is similar to the last captured one. For example, two click events on the same button.
  */
-function areSimilarDomEvents(a: Event, b: Event): boolean {
+function isSimilarToLastCapturedEvent(event: Event): boolean {
   // If both events have different type, then user definitely performed two separate actions. e.g. click + keypress.
-  if (a.type !== b.type) {
+  if (event.type !== lastCapturedEventType) {
     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 (a.target !== b.target) {
+    if (!event.target || (event.target as SentryWrappedTarget)._sentryId !== lastCapturedEventTargetId) {
       return false;
     }
   } catch (e) {
@@ -436,30 +440,33 @@ function areSimilarDomEvents(a: Event, b: Event): boolean {
  * Decide whether an event should be captured.
  * @param event event to be captured
  */
-function shouldSkipDOMEvent(event: Event): boolean {
+function shouldSkipDOMEvent(eventType: string, target: SentryWrappedTarget | null): boolean {
   // We are only interested in filtering `keypress` events for now.
-  if (event.type !== 'keypress') {
+  if (eventType !== 'keypress') {
     return false;
   }
 
-  try {
-    const target = event.target as HTMLElement;
+  if (!target || !target.tagName) {
+    return true;
+  }
 
-    if (!target || !target.tagName) {
-      return true;
-    }
+  // Only consider keypress events on actual input elements. This will disregard keypresses targeting body
+  // e.g.tabbing through elements, hotkeys, etc.
+  if (target.tagName === 'INPUT' || target.tagName === 'TEXTAREA' || target.isContentEditable) {
+    return false;
+  }
 
-    // Only consider keypress events on actual input elements. This will disregard keypresses targeting body
-    // e.g.tabbing through elements, hotkeys, etc.
-    if (target.tagName === 'INPUT' || target.tagName === 'TEXTAREA' || target.isContentEditable) {
-      return false;
-    }
+  return true;
+}
+
+function getEventTarget(event: Event): SentryWrappedTarget | null {
+  try {
+    return event.target as SentryWrappedTarget | null;
   } catch (e) {
     // just accessing `target` property can throw an exception in some rare circumstances
     // see: https://github.com/getsentry/sentry-javascript/issues/838
+    return null;
   }
-
-  return true;
 }
 
 /**
@@ -478,32 +485,41 @@ function makeDOMEventHandler(handler: Function, globalListener: boolean = false)
       return;
     }
 
+    const target = getEventTarget(event);
+
     // We always want to skip _some_ events.
-    if (shouldSkipDOMEvent(event)) {
+    if (shouldSkipDOMEvent(event.type, target)) {
       return;
     }
 
     // Mark event as "seen"
     addNonEnumerableProperty(event, '_sentryCaptured', true);
 
+    if (target && !target._sentryId) {
+      // Add UUID to event target so we can identify if
+      addNonEnumerableProperty(target, '_sentryId', uuid4());
+    }
+
     const name = event.type === 'keypress' ? 'input' : event.type;
 
     // 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.
-    if (lastCapturedEvent === undefined || !areSimilarDomEvents(lastCapturedEvent, event)) {
+    if (!isSimilarToLastCapturedEvent(event)) {
       handler({
         event: event,
         name,
         global: globalListener,
       });
-      lastCapturedEvent = event;
+      lastCapturedEventType = event.type;
+      lastCapturedEventTargetId = target ? target._sentryId : undefined;
     }
 
     // Start a new debounce timer that will prevent us from capturing multiple events that should be grouped together.
     clearTimeout(debounceTimerID);
     debounceTimerID = WINDOW.setTimeout(() => {
-      lastCapturedEvent = undefined;
+      lastCapturedEventTargetId = undefined;
+      lastCapturedEventType = undefined;
     }, DEBOUNCE_DURATION);
   };
 }
diff --git a/rollup/plugins/bundlePlugins.js b/rollup/plugins/bundlePlugins.js
@@ -131,6 +131,9 @@ export function makeTerserPlugin() {
           '_meta',
           // Object we inject debug IDs into with bundler plugins
           '_sentryDebugIds',
+          // These are used by instrument.ts in utils for identifying HTML elements & events
+          '_sentryCaptured',
+          '_sentryId',
         ],
       },
     },
