diff --git a/packages/browser/src/integrations/breadcrumbs.ts b/packages/browser/src/integrations/breadcrumbs.ts index e0f7cc511bec..1a485bd51576 100644 --- a/packages/browser/src/integrations/breadcrumbs.ts +++ b/packages/browser/src/integrations/breadcrumbs.ts @@ -184,6 +184,7 @@ export class Breadcrumbs implements Integration { { event: handlerData.event, name: handlerData.name, + global: handlerData.global, }, ); } diff --git a/packages/browser/test/integration/suites/breadcrumbs.js b/packages/browser/test/integration/suites/breadcrumbs.js index 6e8422af683c..3ca054bf4e95 100644 --- a/packages/browser/test/integration/suites/breadcrumbs.js +++ b/packages/browser/test/integration/suites/breadcrumbs.js @@ -446,7 +446,7 @@ describe("breadcrumbs", function() { }); }); - it("should bail out if accessing the `type` and `target` properties of an event throw an exception", function() { + it("should bail out if accessing the `target` property of an event throws an exception", function() { // see: https://github.com/getsentry/sentry-javascript/issues/768 return runInSandbox(sandbox, function() { // click @@ -454,7 +454,6 @@ describe("breadcrumbs", function() { function kaboom() { throw new Error("lol"); } - Object.defineProperty(click, "type", { get: kaboom }); Object.defineProperty(click, "target", { get: kaboom }); var input = document.querySelector(".a"); // leaf node @@ -500,6 +499,121 @@ describe("breadcrumbs", function() { }); }); + it("should correctly capture multiple consecutive breadcrumbs if they are of different type", function() { + return runInSandbox(sandbox, function() { + var input = document.getElementsByTagName("input")[0]; + + var clickHandler = function() {}; + input.addEventListener("click", clickHandler); + var keypressHandler = function() {}; + input.addEventListener("keypress", keypressHandler); + + input.dispatchEvent(new MouseEvent("click")); + input.dispatchEvent(new KeyboardEvent("keypress")); + + Sentry.captureMessage("test"); + }).then(function(summary) { + if (IS_LOADER) { + // The async loader doesn't wrap event listeners, but we should receive the event without breadcrumbs + assert.lengthOf(summary.events, 1); + } else { + // Breadcrumb should be captured by the global event listeners, not a specific one + assert.equal(summary.breadcrumbs.length, 2); + assert.equal(summary.breadcrumbs[0].category, "ui.click"); + assert.equal( + summary.breadcrumbs[0].message, + 'body > form#foo-form > input[name="foo"]' + ); + assert.equal(summary.breadcrumbs[1].category, "ui.input"); + assert.equal( + summary.breadcrumbs[0].message, + 'body > form#foo-form > input[name="foo"]' + ); + assert.equal(summary.breadcrumbHints[0].global, false); + assert.equal(summary.breadcrumbHints[1].global, false); + assert.isUndefined(summary.events[0].exception); + } + }); + }); + + it("should debounce multiple consecutive identical breadcrumbs but allow for switching to a different type", function() { + return runInSandbox(sandbox, function() { + var input = document.getElementsByTagName("input")[0]; + + var clickHandler = function() {}; + input.addEventListener("click", clickHandler); + var keypressHandler = function() {}; + input.addEventListener("keypress", keypressHandler); + + input.dispatchEvent(new MouseEvent("click")); + input.dispatchEvent(new MouseEvent("click")); + input.dispatchEvent(new MouseEvent("click")); + input.dispatchEvent(new KeyboardEvent("keypress")); + input.dispatchEvent(new KeyboardEvent("keypress")); + input.dispatchEvent(new KeyboardEvent("keypress")); + + Sentry.captureMessage("test"); + }).then(function(summary) { + if (IS_LOADER) { + // The async loader doesn't wrap event listeners, but we should receive the event without breadcrumbs + assert.lengthOf(summary.events, 1); + } else { + // Breadcrumb should be captured by the global event listeners, not a specific one + assert.equal(summary.breadcrumbs.length, 2); + assert.equal(summary.breadcrumbs[0].category, "ui.click"); + assert.equal( + summary.breadcrumbs[0].message, + 'body > form#foo-form > input[name="foo"]' + ); + assert.equal(summary.breadcrumbs[1].category, "ui.input"); + assert.equal( + summary.breadcrumbs[0].message, + 'body > form#foo-form > input[name="foo"]' + ); + assert.equal(summary.breadcrumbHints[0].global, false); + assert.equal(summary.breadcrumbHints[1].global, false); + assert.isUndefined(summary.events[0].exception); + } + }); + }); + + it("should debounce multiple consecutive identical breadcrumbs but allow for switching to a different target", function() { + return runInSandbox(sandbox, function() { + var input = document.querySelector("#foo-form input"); + var div = document.querySelector("#foo-form div"); + + var clickHandler = function() {}; + input.addEventListener("click", clickHandler); + div.addEventListener("click", clickHandler); + + input.dispatchEvent(new MouseEvent("click")); + div.dispatchEvent(new MouseEvent("click")); + + Sentry.captureMessage("test"); + }).then(function(summary) { + if (IS_LOADER) { + // The async loader doesn't wrap event listeners, but we should receive the event without breadcrumbs + assert.lengthOf(summary.events, 1); + } else { + // Breadcrumb should be captured by the global event listeners, not a specific one + assert.equal(summary.breadcrumbs.length, 2); + assert.equal(summary.breadcrumbs[0].category, "ui.click"); + assert.equal( + summary.breadcrumbs[0].message, + 'body > form#foo-form > input[name="foo"]' + ); + assert.equal(summary.breadcrumbs[1].category, "ui.click"); + assert.equal( + summary.breadcrumbs[1].message, + "body > form#foo-form > div.contenteditable" + ); + assert.equal(summary.breadcrumbHints[0].global, false); + assert.equal(summary.breadcrumbHints[1].global, false); + assert.isUndefined(summary.events[0].exception); + } + }); + }); + it( optional( "should flush keypress breadcrumbs when an error is thrown", @@ -659,6 +773,42 @@ describe("breadcrumbs", function() { }); }); + it("should remove breadcrumb instrumentation when all event listeners are detached", function() { + return runInSandbox(sandbox, function() { + var input = document.getElementsByTagName("input")[0]; + + var clickHandler = function() {}; + var otherClickHandler = function() {}; + input.addEventListener("click", clickHandler); + input.addEventListener("click", otherClickHandler); + input.removeEventListener("click", clickHandler); + input.removeEventListener("click", otherClickHandler); + + var keypressHandler = function() {}; + var otherKeypressHandler = function() {}; + input.addEventListener("keypress", keypressHandler); + input.addEventListener("keypress", otherKeypressHandler); + input.removeEventListener("keypress", keypressHandler); + input.removeEventListener("keypress", otherKeypressHandler); + + input.dispatchEvent(new MouseEvent("click")); + input.dispatchEvent(new KeyboardEvent("keypress")); + + Sentry.captureMessage("test"); + }).then(function(summary) { + if (IS_LOADER) { + // The async loader doesn't wrap event listeners, but we should receive the event without breadcrumbs + assert.lengthOf(summary.events, 1); + } else { + // Breadcrumb should be captured by the global event listeners, not a specific one + assert.equal(summary.breadcrumbs.length, 2); + assert.equal(summary.breadcrumbHints[0].global, true); + assert.equal(summary.breadcrumbHints[1].global, true); + assert.isUndefined(summary.events[0].exception); + } + }); + }); + it( optional( "should record history.[pushState|replaceState] changes as navigation breadcrumbs", diff --git a/packages/utils/src/instrument.ts b/packages/utils/src/instrument.ts index 2c5e4e419fa6..d4263e1a9ab9 100644 --- a/packages/utils/src/instrument.ts +++ b/packages/utils/src/instrument.ts @@ -357,179 +357,233 @@ function instrumentHistory(): void { fill(global.history, 'replaceState', historyReplacementFunction); } -/** JSDoc */ -function instrumentDOM(): void { - if (!('document' in global)) { - return; +const debounceDuration = 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 + */ +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; } - // Capture breadcrumbs from any click that is unhandled / bubbled up all the way - // to the document. Do this before we instrument addEventListener. - global.document.addEventListener('click', domEventHandler('click', triggerHandlers.bind(null, 'dom')), false); - global.document.addEventListener('keypress', keypressEventHandler(triggerHandlers.bind(null, 'dom')), false); + // If both events have different type, then user definitely performed two separate actions. e.g. click + keypress. + if (previous.type !== current.type) { + return true; + } - // After hooking into document bubbled up click and keypresses events, we also hook into user handled click & keypresses. - ['EventTarget', 'Node'].forEach((target: string) => { - /* eslint-disable @typescript-eslint/no-unsafe-member-access */ - const proto = (global as any)[target] && (global as any)[target].prototype; + 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; + } + } catch (e) { + // just accessing `target` property can throw an exception in some rare circumstances + // see: https://github.com/getsentry/sentry-javascript/issues/838 + } - // eslint-disable-next-line no-prototype-builtins - if (!proto || !proto.hasOwnProperty || !proto.hasOwnProperty('addEventListener')) { - return; + // 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; +} + +/** + * Decide whether an event should be captured. + * @param event event to be captured + */ +function shouldSkipDOMEvent(event: Event): boolean { + // We are only interested in filtering `keypress` events for now. + if (event.type !== 'keypress') { + return false; + } + + try { + const target = event.target as HTMLElement; + + if (!target || !target.tagName) { + return true; } - /* eslint-enable @typescript-eslint/no-unsafe-member-access */ - - fill(proto, 'addEventListener', function( - original: () => void, - ): ( - eventName: string, - fn: EventListenerOrEventListenerObject, - options?: boolean | AddEventListenerOptions, - ) => void { - return function( - this: any, - eventName: string, - fn: EventListenerOrEventListenerObject, - options?: boolean | AddEventListenerOptions, - ): (eventName: string, fn: EventListenerOrEventListenerObject, capture?: boolean, secure?: boolean) => void { - if (fn && (fn as EventListenerObject).handleEvent) { - if (eventName === 'click') { - fill(fn, 'handleEvent', function(innerOriginal: () => void): (caughtEvent: Event) => void { - return function(this: any, event: Event): (event: Event) => void { - domEventHandler('click', triggerHandlers.bind(null, 'dom'))(event); - return innerOriginal.call(this, event); - }; - }); - } - if (eventName === 'keypress') { - fill(fn, 'handleEvent', function(innerOriginal: () => void): (caughtEvent: Event) => void { - return function(this: any, event: Event): (event: Event) => void { - keypressEventHandler(triggerHandlers.bind(null, 'dom'))(event); - return innerOriginal.call(this, event); - }; - }); - } - } else { - if (eventName === 'click') { - domEventHandler('click', triggerHandlers.bind(null, 'dom'), true)(this); - } - if (eventName === 'keypress') { - keypressEventHandler(triggerHandlers.bind(null, 'dom'))(this); - } - } - return original.call(this, eventName, fn, options); - }; - }); + // 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; + } + } catch (e) { + // just accessing `target` property can throw an exception in some rare circumstances + // see: https://github.com/getsentry/sentry-javascript/issues/838 + } - fill(proto, 'removeEventListener', function( - original: () => void, - ): ( - this: any, - eventName: string, - fn: EventListenerOrEventListenerObject, - options?: boolean | EventListenerOptions, - ) => () => void { - return function( - this: any, - eventName: string, - fn: EventListenerOrEventListenerObject, - options?: boolean | EventListenerOptions, - ): () => void { - try { - original.call(this, eventName, ((fn as unknown) as WrappedFunction).__sentry_wrapped__, options); - } catch (e) { - // ignore, accessing __sentry_wrapped__ will throw in some Selenium environments - } - return original.call(this, eventName, fn, options); - }; - }); - }); + return true; } -const debounceDuration: number = 1000; -let debounceTimer: number = 0; -let keypressTimeout: number | undefined; -let lastCapturedEvent: Event | undefined; - /** * Wraps addEventListener to capture UI breadcrumbs - * @param name the event name (e.g. "click") * @param handler function that will be triggered - * @param debounce decides whether it should wait till another event loop + * @param globalListener indicates whether event was captured by the global event listener * @returns wrapped breadcrumb events handler * @hidden */ -function domEventHandler(name: string, handler: Function, debounce: boolean = false): (event: Event) => void { +function makeDOMEventHandler(handler: Function, globalListener: boolean = false): (event: Event) => void { return (event: Event): void => { - // reset keypress timeout; e.g. triggering a 'click' after - // a 'keypress' will reset the keypress debounce so that a new - // set of keypresses can be recorded - keypressTimeout = undefined; // 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 the event. + // event (e.g. event propagation through node ancestors). + // Ignore if we've already captured that event. if (!event || lastCapturedEvent === event) { return; } - lastCapturedEvent = event; - - if (debounceTimer) { - clearTimeout(debounceTimer); + // We always want to skip _some_ events. + if (shouldSkipDOMEvent(event)) { + return; } - if (debounce) { - debounceTimer = setTimeout(() => { - handler({ event, name }); + 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 that's the case, emit the previous event and store locally the newly-captured DOM event. + else if (shouldShortcircuitPreviousDebounce(lastCapturedEvent, event)) { + handler({ + event: event, + name, + global: globalListener, }); - } else { - handler({ event, name }); + lastCapturedEvent = event; } + + // Start a new debounce timer that will prevent us from capturing multiple events that should be grouped together. + clearTimeout(debounceTimerID); + debounceTimerID = global.setTimeout(() => { + debounceTimerID = undefined; + }, debounceDuration); }; } -/** - * Wraps addEventListener to capture keypress UI events - * @param handler function that will be triggered - * @returns wrapped keypress events handler - * @hidden - */ -function keypressEventHandler(handler: Function): (event: Event) => void { - // TODO: if somehow user switches keypress target before - // debounce timeout is triggered, we will only capture - // a single breadcrumb from the FIRST target (acceptable?) - return (event: Event): void => { - let target; +type AddEventListener = ( + type: string, + listener: EventListenerOrEventListenerObject, + options?: boolean | AddEventListenerOptions, +) => void; +type RemoveEventListener = ( + type: string, + listener: EventListenerOrEventListenerObject, + options?: boolean | EventListenerOptions, +) => void; + +type InstrumentedElement = Element & { + __sentry_instrumentation_handlers__?: { + [key in 'click' | 'keypress']?: { + handler?: Function; + /** The number of custom listeners attached to this element */ + refCount: number; + }; + }; +}; - try { - target = event.target; - } catch (e) { - // just accessing event properties can throw an exception in some rare circumstances - // see: https://github.com/getsentry/raven-js/issues/838 +/** JSDoc */ +function instrumentDOM(): void { + if (!('document' in global)) { + return; + } + + // Make it so that any click or keypress that is unhandled / bubbled up all the way to the document triggers our dom + // handlers. (Normally we have only one, which captures a breadcrumb for each click or keypress.) Do this before + // we instrument `addEventListener` so that we don't end up attaching this handler twice. + const triggerDOMHandler = triggerHandlers.bind(null, 'dom'); + const globalDOMEventHandler = makeDOMEventHandler(triggerDOMHandler, true); + global.document.addEventListener('click', globalDOMEventHandler, false); + global.document.addEventListener('keypress', globalDOMEventHandler, false); + + // After hooking into click and keypress events bubbled up to `document`, we also hook into user-handled + // clicks & keypresses, by adding an event listener of our own to any element to which they add a listener. That + // way, whenever one of their handlers is triggered, ours will be, too. (This is needed because their handler + // could potentially prevent the event from bubbling up to our global listeners. This way, our handler are still + // guaranteed to fire at least once.) + ['EventTarget', 'Node'].forEach((target: string) => { + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + const proto = (global as any)[target] && (global as any)[target].prototype; + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, no-prototype-builtins + if (!proto || !proto.hasOwnProperty || !proto.hasOwnProperty('addEventListener')) { return; } - const tagName = target && (target as HTMLElement).tagName; + fill(proto, 'addEventListener', function(originalAddEventListener: AddEventListener): AddEventListener { + return function( + this: Element, + type: string, + listener: EventListenerOrEventListenerObject, + options?: boolean | AddEventListenerOptions, + ): AddEventListener { + if (type === 'click' || type == 'keypress') { + const el = this as InstrumentedElement; + const handlers = (el.__sentry_instrumentation_handlers__ = el.__sentry_instrumentation_handlers__ || {}); + const handlerForType = (handlers[type] = handlers[type] || { refCount: 0 }); + + if (!handlerForType.handler) { + const handler = makeDOMEventHandler(triggerDOMHandler); + handlerForType.handler = handler; + originalAddEventListener.call(this, type, handler, options); + } - // only consider keypress events on actual input elements - // this will disregard keypresses targeting body (e.g. tabbing - // through elements, hotkeys, etc) - if (!tagName || (tagName !== 'INPUT' && tagName !== 'TEXTAREA' && !(target as HTMLElement).isContentEditable)) { - return; - } + handlerForType.refCount += 1; + } - // record first keypress in a series, but ignore subsequent - // keypresses until debounce clears - if (!keypressTimeout) { - domEventHandler('input', handler)(event); - } - clearTimeout(keypressTimeout); + return originalAddEventListener.call(this, type, listener, options); + }; + }); - keypressTimeout = (setTimeout(() => { - keypressTimeout = undefined; - }, debounceDuration) as any) as number; - }; + fill(proto, 'removeEventListener', function(originalRemoveEventListener: RemoveEventListener): RemoveEventListener { + return function( + this: Element, + type: string, + listener: EventListenerOrEventListenerObject, + options?: boolean | EventListenerOptions, + ): () => void { + if (type === 'click' || type == 'keypress') { + try { + const el = this as InstrumentedElement; + const handlers = el.__sentry_instrumentation_handlers__ || {}; + const handlerForType = handlers[type]; + + if (handlerForType) { + handlerForType.refCount -= 1; + // If there are no longer any custom handlers of the current type on this element, we can remove ours, too. + if (handlerForType.refCount <= 0) { + originalRemoveEventListener.call(this, type, handlerForType.handler, options); + handlerForType.handler = undefined; + delete handlers[type]; // eslint-disable-line @typescript-eslint/no-dynamic-delete + } + + // If there are no longer any custom handlers of any type on this element, cleanup everything. + if (Object.keys(handlers).length === 0) { + delete el.__sentry_instrumentation_handlers__; + } + } + } catch (e) { + // accessing dom properties is always fragile + } + } + + return originalRemoveEventListener.call(this, type, listener, options); + }; + }); + }); } let _oldOnErrorHandler: OnErrorEventHandler = null;