From d5c73c176497597d9683376c975e6906b5ab2acc Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 20 May 2022 12:56:11 +0000 Subject: [PATCH 1/6] Add integration tests for multiple event listener attachments --- .../subject.js | 23 +++++++++++++ .../test.ts | 34 +++++++++++++++++++ .../instrumentation/eventListener/subject.js | 5 +++ .../instrumentation/eventListener/test.ts | 24 +++++++++++++ .../suites/public-api/instrumentation/init.js | 7 ++++ 5 files changed, 93 insertions(+) create mode 100644 packages/integration-tests/suites/public-api/instrumentation/eventListener-instrumentation-behaviour/subject.js create mode 100644 packages/integration-tests/suites/public-api/instrumentation/eventListener-instrumentation-behaviour/test.ts create mode 100644 packages/integration-tests/suites/public-api/instrumentation/eventListener/subject.js create mode 100644 packages/integration-tests/suites/public-api/instrumentation/eventListener/test.ts create mode 100644 packages/integration-tests/suites/public-api/instrumentation/init.js 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..399fbec67720 --- /dev/null +++ b/packages/integration-tests/suites/public-api/instrumentation/eventListener-instrumentation-behaviour/subject.js @@ -0,0 +1,23 @@ +// Simple function event listener +const eventListener1 = () => { + // Trigger something that puppeteer can listen to - like console.log + console.log('eventListener1'); +}; + +// Attach event listener twice +window.addEventListener('click', eventListener1); +window.addEventListener('click', eventListener1); + +// Event listener that has handleEvent() method: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#listener +class EventHandlerClass { + handleEvent() { + // Trigger something that puppeteer can listen to - like console.log + console.log('eventListener2'); + } +} + +const eventListener2 = new EventHandlerClass(); + +// Attach event listener twice +window.addEventListener('click', eventListener2, { passive: true, capture: false }); +window.addEventListener('click', eventListener2, { passive: true, capture: false }); 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..8ae76404fda6 --- /dev/null +++ b/packages/integration-tests/suites/public-api/instrumentation/eventListener-instrumentation-behaviour/test.ts @@ -0,0 +1,34 @@ +import { expect } from '@playwright/test'; + +import { sentryTest } from '../../../../utils/fixtures'; + +sentryTest('should attach the same event listener only once', async ({ getLocalTestPath, page }) => { + const url = await getLocalTestPath({ testDir: __dirname }); + await page.goto(url); + + const testCompletionPromise = new Promise(resolve => { + let eventListener1Calls = 0; + let eventListener2Calls = 0; + + page.on('console', async msg => { + const msgText = msg.text(); + + if (msgText === 'eventListener1') { + eventListener1Calls = eventListener1Calls + 1; + } else if (msgText === 'eventListener2') { + eventListener2Calls = eventListener2Calls + 1; + } else if (msgText === 'done') { + expect(eventListener1Calls).toBe(2); + expect(eventListener2Calls).toBe(2); + resolve(); + } + }); + }); + + // Trigger event listeners twice and signal completion afterwards + await page.evaluate('document.body.click()'); + await page.evaluate('document.body.click()'); + await page.evaluate('console.log("done")'); + + return testCompletionPromise; +}); 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..a103c3247b08 --- /dev/null +++ b/packages/integration-tests/suites/public-api/instrumentation/eventListener/test.ts @@ -0,0 +1,24 @@ +import { expect } from '@playwright/test'; +import { Event } from '@sentry/types'; + +import { sentryTest } from '../../../../utils/fixtures'; +import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers'; + +sentryTest('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', +}); From 7dffe5043cc0300c3e3016974a15e5a36c508f76 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 20 May 2022 15:11:14 +0000 Subject: [PATCH 2/6] minor change --- .../eventListener-instrumentation-behaviour/subject.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 index 399fbec67720..710a7f58f987 100644 --- 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 @@ -19,5 +19,5 @@ class EventHandlerClass { const eventListener2 = new EventHandlerClass(); // Attach event listener twice -window.addEventListener('click', eventListener2, { passive: true, capture: false }); -window.addEventListener('click', eventListener2, { passive: true, capture: false }); +window.addEventListener('click', eventListener2); +window.addEventListener('click', eventListener2); From b7862ae66a5f960b69243543e74fe9af94e5abf1 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 23 May 2022 08:28:40 +0000 Subject: [PATCH 3/6] Change tests to use exposeFunction --- .../subject.js | 6 ++-- .../test.ts | 29 +++++++------------ 2 files changed, 12 insertions(+), 23 deletions(-) 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 index 710a7f58f987..2d81e155d84f 100644 --- 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 @@ -1,7 +1,6 @@ // Simple function event listener const eventListener1 = () => { - // Trigger something that puppeteer can listen to - like console.log - console.log('eventListener1'); + testCallback1(); }; // Attach event listener twice @@ -11,8 +10,7 @@ window.addEventListener('click', eventListener1); // Event listener that has handleEvent() method: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#listener class EventHandlerClass { handleEvent() { - // Trigger something that puppeteer can listen to - like console.log - console.log('eventListener2'); + testCallback2(); } } 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 index 8ae76404fda6..03f4339840fc 100644 --- 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 @@ -6,29 +6,20 @@ sentryTest('should attach the same event listener only once', async ({ getLocalT const url = await getLocalTestPath({ testDir: __dirname }); await page.goto(url); - const testCompletionPromise = new Promise(resolve => { - let eventListener1Calls = 0; - let eventListener2Calls = 0; - - page.on('console', async msg => { - const msgText = msg.text(); + let testCallback1Calls = 0; + await page.exposeFunction('testCallback1', () => { + testCallback1Calls = testCallback1Calls + 1; + }); - if (msgText === 'eventListener1') { - eventListener1Calls = eventListener1Calls + 1; - } else if (msgText === 'eventListener2') { - eventListener2Calls = eventListener2Calls + 1; - } else if (msgText === 'done') { - expect(eventListener1Calls).toBe(2); - expect(eventListener2Calls).toBe(2); - resolve(); - } - }); + let testCallback2Calls = 0; + await page.exposeFunction('testCallback2', () => { + testCallback2Calls = testCallback2Calls + 1; }); - // Trigger event listeners twice and signal completion afterwards + // Trigger event listeners twice await page.evaluate('document.body.click()'); await page.evaluate('document.body.click()'); - await page.evaluate('console.log("done")'); - return testCompletionPromise; + expect(testCallback1Calls).toBe(2); + expect(testCallback2Calls).toBe(2); }); From c8e2ea9c709c697752cce62d94e7dd909eabc241 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 23 May 2022 09:13:31 +0000 Subject: [PATCH 4/6] Improve tests --- .../subject.js | 16 ++++---- .../test.ts | 37 +++++++++--------- .../eventListener-wrapping/subject.js | 18 +++++++++ .../eventListener-wrapping/test.ts | 38 +++++++++++++++++++ .../instrumentation/eventListener/test.ts | 35 +++++++++-------- 5 files changed, 103 insertions(+), 41 deletions(-) create mode 100644 packages/integration-tests/suites/public-api/instrumentation/eventListener-wrapping/subject.js create mode 100644 packages/integration-tests/suites/public-api/instrumentation/eventListener-wrapping/test.ts 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 index 2d81e155d84f..32f1d09378b0 100644 --- 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 @@ -1,21 +1,21 @@ // Simple function event listener -const eventListener1 = () => { - testCallback1(); +const functionListener = () => { + functionListenerCallback(); }; // Attach event listener twice -window.addEventListener('click', eventListener1); -window.addEventListener('click', eventListener1); +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() { - testCallback2(); + objectListenerCallback(); } } -const eventListener2 = new EventHandlerClass(); +const objectListener = new EventHandlerClass(); // Attach event listener twice -window.addEventListener('click', eventListener2); -window.addEventListener('click', eventListener2); +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 index 03f4339840fc..b6d2e6fa9231 100644 --- 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 @@ -2,24 +2,27 @@ import { expect } from '@playwright/test'; import { sentryTest } from '../../../../utils/fixtures'; -sentryTest('should attach the same event listener only once', async ({ getLocalTestPath, page }) => { - const url = await getLocalTestPath({ testDir: __dirname }); - await page.goto(url); +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 testCallback1Calls = 0; - await page.exposeFunction('testCallback1', () => { - testCallback1Calls = testCallback1Calls + 1; - }); + let functionListenerCalls = 0; + await page.exposeFunction('functionListenerCallback', () => { + functionListenerCalls = functionListenerCalls + 1; + }); - let testCallback2Calls = 0; - await page.exposeFunction('testCallback2', () => { - testCallback2Calls = testCallback2Calls + 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()'); + // Trigger event listeners twice + await page.evaluate('document.body.click()'); + await page.evaluate('document.body.click()'); - expect(testCallback1Calls).toBe(2); - expect(testCallback2Calls).toBe(2); -}); + expect(functionListenerCalls).toBe(2); + expect(objectListenerCalls).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/test.ts b/packages/integration-tests/suites/public-api/instrumentation/eventListener/test.ts index a103c3247b08..71eb3e7023b3 100644 --- a/packages/integration-tests/suites/public-api/instrumentation/eventListener/test.ts +++ b/packages/integration-tests/suites/public-api/instrumentation/eventListener/test.ts @@ -4,21 +4,24 @@ import { Event } from '@sentry/types'; import { sentryTest } from '../../../../utils/fixtures'; import { getFirstSentryEnvelopeRequest } from '../../../../utils/helpers'; -sentryTest('should capture an error thrown in an event handler', async ({ getLocalTestPath, page }) => { - const url = await getLocalTestPath({ testDir: __dirname }); +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); + 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), - }, - }); -}); + 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), + }, + }); + }, +); From 21e23a2f26d6708f6d022c73172732a3df54274e Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 23 May 2022 12:30:14 +0000 Subject: [PATCH 5/6] Add integration tests to check for preservation of `this` --- .../subject.js | 24 +++++++++++++++++++ .../eventListener-this-preservation/test.ts | 24 +++++++++++++++++++ 2 files changed, 48 insertions(+) create mode 100644 packages/integration-tests/suites/public-api/instrumentation/eventListener-this-preservation/subject.js create mode 100644 packages/integration-tests/suites/public-api/instrumentation/eventListener-this-preservation/test.ts 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); +}); From 9818be51480ec91962036f3cadb8198c824c32cf Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 23 May 2022 12:31:28 +0000 Subject: [PATCH 6/6] Fix bug --- packages/browser/src/helpers.ts | 7 ++++--- packages/browser/src/integrations/trycatch.ts | 8 +++++++- 2 files changed, 11 insertions(+), 4 deletions(-) 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',