diff --git a/packages/browser/src/helpers.ts b/packages/browser/src/helpers.ts index 9bf1d5631288..870756066df4 100644 --- a/packages/browser/src/helpers.ts +++ b/packages/browser/src/helpers.ts @@ -32,7 +32,8 @@ export function ignoreNextOnError(): void { * Instruments the given function and sends an event to Sentry every time the * function throws an exception. * - * @param fn A function to wrap. + * @param fn A function to wrap. It is generally safe to pass an unbound function, because the returned wrapper always + * has a correct `this` context. * @returns The wrapped function. * @hidden */ @@ -75,8 +76,8 @@ export function wrap( } /* eslint-disable prefer-rest-params */ - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const sentryWrapped: WrappedFunction = function (this: any): void { + // It is important that `sentryWrapped` is not an arrow function to preserve the context of `this` + const sentryWrapped: WrappedFunction = function (this: unknown): void { const args = Array.prototype.slice.call(arguments); try { diff --git a/packages/browser/src/integrations/trycatch.ts b/packages/browser/src/integrations/trycatch.ts index 84391d238b65..b6b293d70c37 100644 --- a/packages/browser/src/integrations/trycatch.ts +++ b/packages/browser/src/integrations/trycatch.ts @@ -208,7 +208,13 @@ function _wrapEventTarget(target: string): void { ): (eventName: string, fn: EventListenerObject, capture?: boolean, secure?: boolean) => void { try { if (typeof fn.handleEvent === 'function') { - fn.handleEvent = wrap(fn.handleEvent.bind(fn), { + // ESlint disable explanation: + // First, it is generally safe to call `wrap` with an unbound function. Furthermore, using `.bind()` would + // introduce a bug here, because bind returns a new function that doesn't have our + // flags(like __sentry_original__) attached. `wrap` checks for those flags to avoid unnecessary wrapping. + // Without those flags, every call to addEventListener wraps the function again, causing a memory leak. + // eslint-disable-next-line @typescript-eslint/unbound-method + fn.handleEvent = wrap(fn.handleEvent, { mechanism: { data: { function: 'handleEvent', diff --git a/packages/integration-tests/suites/public-api/instrumentation/eventListener-instrumentation-behaviour/subject.js b/packages/integration-tests/suites/public-api/instrumentation/eventListener-instrumentation-behaviour/subject.js new file mode 100644 index 000000000000..32f1d09378b0 --- /dev/null +++ b/packages/integration-tests/suites/public-api/instrumentation/eventListener-instrumentation-behaviour/subject.js @@ -0,0 +1,21 @@ +// Simple function event listener +const functionListener = () => { + functionListenerCallback(); +}; + +// Attach event listener twice +window.addEventListener('click', functionListener); +window.addEventListener('click', functionListener); + +// Event listener that has handleEvent() method: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#listener +class EventHandlerClass { + handleEvent() { + objectListenerCallback(); + } +} + +const objectListener = new EventHandlerClass(); + +// Attach event listener twice +window.addEventListener('click', objectListener); +window.addEventListener('click', objectListener); diff --git a/packages/integration-tests/suites/public-api/instrumentation/eventListener-instrumentation-behaviour/test.ts b/packages/integration-tests/suites/public-api/instrumentation/eventListener-instrumentation-behaviour/test.ts new file mode 100644 index 000000000000..b6d2e6fa9231 --- /dev/null +++ b/packages/integration-tests/suites/public-api/instrumentation/eventListener-instrumentation-behaviour/test.ts @@ -0,0 +1,28 @@ +import { expect } from '@playwright/test'; + +import { sentryTest } from '../../../../utils/fixtures'; + +sentryTest( + 'Event listener instrumentation should attach the same event listener only once', + async ({ getLocalTestPath, page }) => { + const url = await getLocalTestPath({ testDir: __dirname }); + await page.goto(url); + + let functionListenerCalls = 0; + await page.exposeFunction('functionListenerCallback', () => { + functionListenerCalls = functionListenerCalls + 1; + }); + + let objectListenerCalls = 0; + await page.exposeFunction('objectListenerCallback', () => { + objectListenerCalls = objectListenerCalls + 1; + }); + + // Trigger event listeners twice + await page.evaluate('document.body.click()'); + await page.evaluate('document.body.click()'); + + expect(functionListenerCalls).toBe(2); + expect(objectListenerCalls).toBe(2); + }, +); diff --git a/packages/integration-tests/suites/public-api/instrumentation/eventListener-this-preservation/subject.js b/packages/integration-tests/suites/public-api/instrumentation/eventListener-this-preservation/subject.js new file mode 100644 index 000000000000..7f5d2cc290e1 --- /dev/null +++ b/packages/integration-tests/suites/public-api/instrumentation/eventListener-this-preservation/subject.js @@ -0,0 +1,24 @@ +const btn = document.createElement('button'); +btn.id = 'btn'; +document.body.appendChild(btn); + +const functionListener = function () { + functionCallback(this.constructor.name); +}; + +class EventHandlerClass { + handleEvent() { + classInstanceCallback(this.constructor.name); + } +} +const objectListener = new EventHandlerClass(); + +// Attach event listeners a few times for good measure + +btn.addEventListener('click', functionListener); +btn.addEventListener('click', functionListener); +btn.addEventListener('click', functionListener); + +btn.addEventListener('click', objectListener); +btn.addEventListener('click', objectListener); +btn.addEventListener('click', objectListener); diff --git a/packages/integration-tests/suites/public-api/instrumentation/eventListener-this-preservation/test.ts b/packages/integration-tests/suites/public-api/instrumentation/eventListener-this-preservation/test.ts new file mode 100644 index 000000000000..e6304858eed4 --- /dev/null +++ b/packages/integration-tests/suites/public-api/instrumentation/eventListener-this-preservation/test.ts @@ -0,0 +1,24 @@ +import { expect } from '@playwright/test'; + +import { sentryTest } from '../../../../utils/fixtures'; + +sentryTest('Event listener instrumentation preserves "this" context', async ({ getLocalTestPath, page }) => { + const url = await getLocalTestPath({ testDir: __dirname }); + await page.goto(url); + + let assertions = 0; + + await page.exposeFunction('functionCallback', (thisInstanceName: unknown) => { + expect(thisInstanceName).toBe('HTMLButtonElement'); + assertions = assertions + 1; + }); + + await page.exposeFunction('classInstanceCallback', (thisInstanceName: unknown) => { + expect(thisInstanceName).toBe('EventHandlerClass'); + assertions = assertions + 1; + }); + + await page.evaluate('document.getElementById("btn").click()'); + + expect(assertions).toBe(2); +}); diff --git a/packages/integration-tests/suites/public-api/instrumentation/eventListener-wrapping/subject.js b/packages/integration-tests/suites/public-api/instrumentation/eventListener-wrapping/subject.js new file mode 100644 index 000000000000..289068737dc2 --- /dev/null +++ b/packages/integration-tests/suites/public-api/instrumentation/eventListener-wrapping/subject.js @@ -0,0 +1,18 @@ +// Simple function event listener +const functionListener = () => { + reportFunctionListenerStackHeight(new Error().stack.split('\n').length); +}; + +// Event listener that has handleEvent() method: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#listener +class EventHandlerClass { + handleEvent() { + reportObjectListenerStackHeight(new Error().stack.split('\n').length); + } +} + +const objectListener = new EventHandlerClass(); + +window.attachListeners = function () { + window.addEventListener('click', functionListener); + window.addEventListener('click', objectListener); +}; diff --git a/packages/integration-tests/suites/public-api/instrumentation/eventListener-wrapping/test.ts b/packages/integration-tests/suites/public-api/instrumentation/eventListener-wrapping/test.ts new file mode 100644 index 000000000000..cf77132c98df --- /dev/null +++ b/packages/integration-tests/suites/public-api/instrumentation/eventListener-wrapping/test.ts @@ -0,0 +1,38 @@ +import { expect } from '@playwright/test'; + +import { sentryTest } from '../../../../utils/fixtures'; + +sentryTest( + 'Event listener instrumentation should not wrap event listeners multiple times', + async ({ getLocalTestPath, page }) => { + const url = await getLocalTestPath({ testDir: __dirname }); + await page.goto(url); + + const functionListenerStackHeights: number[] = []; + const objectListenerStackHeights: number[] = []; + + await page.exposeFunction('reportFunctionListenerStackHeight', (height: number) => { + functionListenerStackHeights.push(height); + }); + + await page.exposeFunction('reportObjectListenerStackHeight', (height: number) => { + objectListenerStackHeights.push(height); + }); + + // Attach initial listeners + await page.evaluate('window.attachListeners()'); + await page.evaluate('document.body.click()'); + + await page.evaluate('window.attachListeners()'); + await page.evaluate('window.attachListeners()'); + await page.evaluate('window.attachListeners()'); + await page.evaluate('document.body.click()'); + + expect(functionListenerStackHeights).toHaveLength(2); + expect(objectListenerStackHeights).toHaveLength(2); + + // check if all error stack traces are the same height + expect(functionListenerStackHeights.every((val, _i, arr) => val === arr[0])).toBeTruthy(); + expect(objectListenerStackHeights.every((val, _i, arr) => val === arr[0])).toBeTruthy(); + }, +); diff --git a/packages/integration-tests/suites/public-api/instrumentation/eventListener/subject.js b/packages/integration-tests/suites/public-api/instrumentation/eventListener/subject.js new file mode 100644 index 000000000000..a5e052885a4a --- /dev/null +++ b/packages/integration-tests/suites/public-api/instrumentation/eventListener/subject.js @@ -0,0 +1,5 @@ +window.addEventListener('click', () => { + throw new Error('event_listener_error'); +}); + +document.body.click(); diff --git a/packages/integration-tests/suites/public-api/instrumentation/eventListener/test.ts b/packages/integration-tests/suites/public-api/instrumentation/eventListener/test.ts new file mode 100644 index 000000000000..71eb3e7023b3 --- /dev/null +++ b/packages/integration-tests/suites/public-api/instrumentation/eventListener/test.ts @@ -0,0 +1,27 @@ +import { expect } from '@playwright/test'; +import { Event } from '@sentry/types'; + +import { sentryTest } from '../../../../utils/fixtures'; +import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers'; + +sentryTest( + 'Event listener instrumentation should capture an error thrown in an event handler', + async ({ getLocalTestPath, page }) => { + const url = await getLocalTestPath({ testDir: __dirname }); + + const eventData = await getFirstSentryEnvelopeRequest(page, url); + + expect(eventData.exception?.values).toHaveLength(1); + expect(eventData.exception?.values?.[0]).toMatchObject({ + type: 'Error', + value: 'event_listener_error', + mechanism: { + type: 'instrument', + handled: true, + }, + stacktrace: { + frames: expect.any(Array), + }, + }); + }, +); diff --git a/packages/integration-tests/suites/public-api/instrumentation/init.js b/packages/integration-tests/suites/public-api/instrumentation/init.js new file mode 100644 index 000000000000..d8c94f36fdd0 --- /dev/null +++ b/packages/integration-tests/suites/public-api/instrumentation/init.js @@ -0,0 +1,7 @@ +import * as Sentry from '@sentry/browser'; + +window.Sentry = Sentry; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', +});