From 0a2a39777584f20af4a0795acd137e923fbf563e Mon Sep 17 00:00:00 2001 From: John Bley Date: Thu, 23 Jul 2020 08:37:17 -0400 Subject: [PATCH 1/9] fix: patch removeEventListener to properly remove patched functions --- .../src/userInteraction.ts | 37 +++++++++++++++++++ .../test/userInteraction.nozone.test.ts | 14 +++++++ 2 files changed, 51 insertions(+) diff --git a/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts b/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts index 13810326c4..f3d67292fa 100644 --- a/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts +++ b/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts @@ -48,6 +48,7 @@ export class UserInteractionPlugin extends BasePlugin { moduleName = this.component; private _spansData = new WeakMap(); private _zonePatched = false; + private _wrappedListeners = new WeakMap(); constructor() { super('@opentelemetry/plugin-user-interaction', VERSION); @@ -193,11 +194,36 @@ export class UserInteractionPlugin extends BasePlugin { return listener.apply(target, args); } }; + plugin._wrappedListeners.set(listener, patchedListener); return original.call(this, type, patchedListener, useCapture); }; }; } + /** + * This patches the removeEventListener of HTMLElement to handle the fact that + * we patched the original callbacks + * This is done when zone is not available + */ + private _patchRemoveEventListener() { + const plugin = this; + return (original: Function) => { + return function removeEventListenerPatched( + this: HTMLElement, + type: any, + listener: any, + useCapture: any + ) { + const wrappedListener = plugin._wrappedListeners.get(listener); + if (wrappedListener) { + return original.call(this, type, wrappedListener, useCapture); + } else { + return original.call(this, type, listener, useCapture); + } + }; + }; + } + /** * Patches the history api */ @@ -434,11 +460,22 @@ export class UserInteractionPlugin extends BasePlugin { 'removing previous patch from method addEventListener' ); } + if (isWrapped(HTMLElement.prototype.removeEventListener)) { + shimmer.unwrap(HTMLElement.prototype, 'removeEventListener'); + this._logger.debug( + 'removing previous patch from method removeEventListener' + ); + } shimmer.wrap( HTMLElement.prototype, 'addEventListener', this._patchElement() ); + shimmer.wrap( + HTMLElement.prototype, + 'removeEventListener', + this._patchRemoveEventListener() + ); } this._patchHistoryApi(); diff --git a/plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts b/plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts index 936a73dbef..4be1b455e6 100644 --- a/plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts +++ b/plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts @@ -89,6 +89,20 @@ describe('UserInteractionPlugin', () => { context.disable(); }); + it('should not break removeEventListener', () => { + let called = false; + const listener = function () { + called = true; + }; + document.body.addEventListener('testEvent', listener); + document.body.dispatchEvent(new Event('testEvent')); + assert.strictEqual(called, true); + called = false; + document.body.removeEventListener('testEvent', listener); + document.body.dispatchEvent(new Event('testEvent')); + assert.strictEqual(called, false); + }); + it('should handle task without async operation', () => { fakeInteraction(); assert.equal(exportSpy.args.length, 1, 'should export one span'); From 5117e4b1cb821f0fd158313a0315f8f164f06bed Mon Sep 17 00:00:00 2001 From: John Bley Date: Thu, 23 Jul 2020 09:21:13 -0400 Subject: [PATCH 2/9] chore: delete patched listener from weak map --- .../opentelemetry-plugin-user-interaction/src/userInteraction.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts b/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts index f3d67292fa..ea58b17855 100644 --- a/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts +++ b/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts @@ -216,6 +216,7 @@ export class UserInteractionPlugin extends BasePlugin { ) { const wrappedListener = plugin._wrappedListeners.get(listener); if (wrappedListener) { + plugin._wrappedListeners.delete(listener); return original.call(this, type, wrappedListener, useCapture); } else { return original.call(this, type, listener, useCapture); From e238549034222ff50c0721667bdf7cbd56a7d1f4 Mon Sep 17 00:00:00 2001 From: John Bley Date: Thu, 23 Jul 2020 08:37:17 -0400 Subject: [PATCH 3/9] fix: patch removeEventListener to properly remove patched functions --- .../src/userInteraction.ts | 37 +++++++++++++++++++ .../test/userInteraction.nozone.test.ts | 14 +++++++ 2 files changed, 51 insertions(+) diff --git a/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts b/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts index 13810326c4..f3d67292fa 100644 --- a/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts +++ b/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts @@ -48,6 +48,7 @@ export class UserInteractionPlugin extends BasePlugin { moduleName = this.component; private _spansData = new WeakMap(); private _zonePatched = false; + private _wrappedListeners = new WeakMap(); constructor() { super('@opentelemetry/plugin-user-interaction', VERSION); @@ -193,11 +194,36 @@ export class UserInteractionPlugin extends BasePlugin { return listener.apply(target, args); } }; + plugin._wrappedListeners.set(listener, patchedListener); return original.call(this, type, patchedListener, useCapture); }; }; } + /** + * This patches the removeEventListener of HTMLElement to handle the fact that + * we patched the original callbacks + * This is done when zone is not available + */ + private _patchRemoveEventListener() { + const plugin = this; + return (original: Function) => { + return function removeEventListenerPatched( + this: HTMLElement, + type: any, + listener: any, + useCapture: any + ) { + const wrappedListener = plugin._wrappedListeners.get(listener); + if (wrappedListener) { + return original.call(this, type, wrappedListener, useCapture); + } else { + return original.call(this, type, listener, useCapture); + } + }; + }; + } + /** * Patches the history api */ @@ -434,11 +460,22 @@ export class UserInteractionPlugin extends BasePlugin { 'removing previous patch from method addEventListener' ); } + if (isWrapped(HTMLElement.prototype.removeEventListener)) { + shimmer.unwrap(HTMLElement.prototype, 'removeEventListener'); + this._logger.debug( + 'removing previous patch from method removeEventListener' + ); + } shimmer.wrap( HTMLElement.prototype, 'addEventListener', this._patchElement() ); + shimmer.wrap( + HTMLElement.prototype, + 'removeEventListener', + this._patchRemoveEventListener() + ); } this._patchHistoryApi(); diff --git a/plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts b/plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts index 936a73dbef..4be1b455e6 100644 --- a/plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts +++ b/plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts @@ -89,6 +89,20 @@ describe('UserInteractionPlugin', () => { context.disable(); }); + it('should not break removeEventListener', () => { + let called = false; + const listener = function () { + called = true; + }; + document.body.addEventListener('testEvent', listener); + document.body.dispatchEvent(new Event('testEvent')); + assert.strictEqual(called, true); + called = false; + document.body.removeEventListener('testEvent', listener); + document.body.dispatchEvent(new Event('testEvent')); + assert.strictEqual(called, false); + }); + it('should handle task without async operation', () => { fakeInteraction(); assert.equal(exportSpy.args.length, 1, 'should export one span'); From b56aa2b0178abbdd3088f91afc0e8bd697692aef Mon Sep 17 00:00:00 2001 From: John Bley Date: Thu, 23 Jul 2020 09:21:13 -0400 Subject: [PATCH 4/9] chore: delete patched listener from weak map --- .../opentelemetry-plugin-user-interaction/src/userInteraction.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts b/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts index f3d67292fa..ea58b17855 100644 --- a/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts +++ b/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts @@ -216,6 +216,7 @@ export class UserInteractionPlugin extends BasePlugin { ) { const wrappedListener = plugin._wrappedListeners.get(listener); if (wrappedListener) { + plugin._wrappedListeners.delete(listener); return original.call(this, type, wrappedListener, useCapture); } else { return original.call(this, type, listener, useCapture); From e6a0b9777ee0b46801ff5dd778002a9f7e78cd97 Mon Sep 17 00:00:00 2001 From: John Bley Date: Thu, 23 Jul 2020 14:38:54 -0400 Subject: [PATCH 5/9] chore: also handle type+element in removeEventListener --- .../src/userInteraction.ts | 44 +++++++++++++++++-- .../test/userInteraction.nozone.test.ts | 28 ++++++++++-- 2 files changed, 64 insertions(+), 8 deletions(-) diff --git a/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts b/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts index ea58b17855..8b65268cc6 100644 --- a/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts +++ b/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts @@ -48,7 +48,8 @@ export class UserInteractionPlugin extends BasePlugin { moduleName = this.component; private _spansData = new WeakMap(); private _zonePatched = false; - private _wrappedListeners = new WeakMap(); + // for addEventListener/removeEventListener state + private _wrappedListeners = new WeakMap>>(); constructor() { super('@opentelemetry/plugin-user-interaction', VERSION); @@ -166,6 +167,42 @@ export class UserInteractionPlugin extends BasePlugin { } } + private addPatchedListener(on: HTMLElement, type: String, listener: Function, wrappedListener: Function) { + let listener2Type = this._wrappedListeners.get(listener); + if (!listener2Type) { + listener2Type = new Map(); + this._wrappedListeners.set(listener, listener2Type); + } + let element2patched = listener2Type.get(type); + if (!element2patched) { + element2patched = new Map(); + listener2Type.set(type, element2patched); + } + element2patched.set(on, wrappedListener); + } + + private removePatchedListener(on: HTMLElement, type: String, listener: Function) : Function|undefined { + let listener2Type = this._wrappedListeners.get(listener); + if (!listener2Type) { + return undefined; + } + let element2patched = listener2Type.get(type); + if (!element2patched) { + return undefined; + } + const patched = element2patched.get(on); + if (patched) { + element2patched.delete(on); + if (element2patched.size === 0) { + listener2Type.delete(type); + if (listener2Type.size === 0) { + this._wrappedListeners.delete(listener); + } + } + } + return patched; + } + /** * This patches the addEventListener of HTMLElement to be able to * auto instrument the click events @@ -194,7 +231,7 @@ export class UserInteractionPlugin extends BasePlugin { return listener.apply(target, args); } }; - plugin._wrappedListeners.set(listener, patchedListener); + plugin.addPatchedListener(this, type, listener, patchedListener); return original.call(this, type, patchedListener, useCapture); }; }; @@ -214,9 +251,8 @@ export class UserInteractionPlugin extends BasePlugin { listener: any, useCapture: any ) { - const wrappedListener = plugin._wrappedListeners.get(listener); + const wrappedListener = plugin.removePatchedListener(this, type, listener); if (wrappedListener) { - plugin._wrappedListeners.delete(listener); return original.call(this, type, wrappedListener, useCapture); } else { return original.call(this, type, listener, useCapture); diff --git a/plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts b/plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts index 4be1b455e6..0a659a4bc6 100644 --- a/plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts +++ b/plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts @@ -94,12 +94,32 @@ describe('UserInteractionPlugin', () => { const listener = function () { called = true; }; - document.body.addEventListener('testEvent', listener); - document.body.dispatchEvent(new Event('testEvent')); + // add same listener three different ways + document.body.addEventListener('bodyEvent', listener); + document.body.addEventListener('bodyEvent2', listener); + document.addEventListener('docEvent', listener); + document.body.dispatchEvent(new Event('bodyEvent')); assert.strictEqual(called, true); called = false; - document.body.removeEventListener('testEvent', listener); - document.body.dispatchEvent(new Event('testEvent')); + // Remove first callback, second type should still fire + document.body.removeEventListener('bodyEvent', listener); + document.body.dispatchEvent(new Event('bodyEvent')); + assert.strictEqual(called, false); + document.body.dispatchEvent(new Event('bodyEvent2')); + assert.strictEqual(called, true); + called = false; + // Remove doc callback, body 2 should still fire + document.removeEventListener('docEvent', listener); + document.dispatchEvent(new Event('docEvent')); + assert.strictEqual(called, false); + document.body.dispatchEvent(new Event('bodyEvent2')); + assert.strictEqual(called, true); + called = false; + // Finally, remove the last one and nothing should fire + document.body.removeEventListener('bodyEvent2', listener); + document.body.dispatchEvent(new Event('bodyEvent')); + document.body.dispatchEvent(new Event('bodyEvent2')); + document.dispatchEvent(new Event('docEvent')); assert.strictEqual(called, false); }); From f790b627982b44be3f736d96edca8c2e669afd98 Mon Sep 17 00:00:00 2001 From: John Bley Date: Thu, 23 Jul 2020 14:43:56 -0400 Subject: [PATCH 6/9] chore: lint --- .../src/userInteraction.ts | 28 +++++++++++++++---- 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts b/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts index 8b65268cc6..644f5fef4a 100644 --- a/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts +++ b/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts @@ -49,7 +49,10 @@ export class UserInteractionPlugin extends BasePlugin { private _spansData = new WeakMap(); private _zonePatched = false; // for addEventListener/removeEventListener state - private _wrappedListeners = new WeakMap>>(); + private _wrappedListeners = new WeakMap< + Function, + Map> + >(); constructor() { super('@opentelemetry/plugin-user-interaction', VERSION); @@ -167,7 +170,12 @@ export class UserInteractionPlugin extends BasePlugin { } } - private addPatchedListener(on: HTMLElement, type: String, listener: Function, wrappedListener: Function) { + private addPatchedListener( + on: HTMLElement, + type: string, + listener: Function, + wrappedListener: Function + ) { let listener2Type = this._wrappedListeners.get(listener); if (!listener2Type) { listener2Type = new Map(); @@ -181,12 +189,16 @@ export class UserInteractionPlugin extends BasePlugin { element2patched.set(on, wrappedListener); } - private removePatchedListener(on: HTMLElement, type: String, listener: Function) : Function|undefined { - let listener2Type = this._wrappedListeners.get(listener); + private removePatchedListener( + on: HTMLElement, + type: string, + listener: Function + ): Function | undefined { + const listener2Type = this._wrappedListeners.get(listener); if (!listener2Type) { return undefined; } - let element2patched = listener2Type.get(type); + const element2patched = listener2Type.get(type); if (!element2patched) { return undefined; } @@ -251,7 +263,11 @@ export class UserInteractionPlugin extends BasePlugin { listener: any, useCapture: any ) { - const wrappedListener = plugin.removePatchedListener(this, type, listener); + const wrappedListener = plugin.removePatchedListener( + this, + type, + listener + ); if (wrappedListener) { return original.call(this, type, wrappedListener, useCapture); } else { From f529213ac570490590d0e12ee62d122f441600a5 Mon Sep 17 00:00:00 2001 From: John Bley Date: Thu, 23 Jul 2020 15:56:13 -0400 Subject: [PATCH 7/9] chore: handle double-registered listeners --- .../src/userInteraction.ts | 17 ++++++++++++++--- .../test/userInteraction.nozone.test.ts | 17 +++++++++++++++++ 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts b/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts index 644f5fef4a..c9a08cfd88 100644 --- a/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts +++ b/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts @@ -170,12 +170,15 @@ export class UserInteractionPlugin extends BasePlugin { } } + /** + * Returns true iff we should use the patched callback; false if it's already been patched + */ private addPatchedListener( on: HTMLElement, type: string, listener: Function, wrappedListener: Function - ) { + ): boolean { let listener2Type = this._wrappedListeners.get(listener); if (!listener2Type) { listener2Type = new Map(); @@ -186,9 +189,16 @@ export class UserInteractionPlugin extends BasePlugin { element2patched = new Map(); listener2Type.set(type, element2patched); } + if (element2patched.has(on)) { + return false; + } element2patched.set(on, wrappedListener); + return true; } + /** + * Returns the patched version of the callback (or undefined) + */ private removePatchedListener( on: HTMLElement, type: string, @@ -243,8 +253,9 @@ export class UserInteractionPlugin extends BasePlugin { return listener.apply(target, args); } }; - plugin.addPatchedListener(this, type, listener, patchedListener); - return original.call(this, type, patchedListener, useCapture); + if (plugin.addPatchedListener(this, type, listener, patchedListener)) { + return original.call(this, type, patchedListener, useCapture); + } }; }; } diff --git a/plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts b/plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts index 0a659a4bc6..3810c412a2 100644 --- a/plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts +++ b/plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts @@ -123,6 +123,23 @@ describe('UserInteractionPlugin', () => { assert.strictEqual(called, false); }); + it('should only not double-register a listener', () => { + let callCount = 0; + const listener = function () { + callCount++; + }; + // addEventListener semantics treat the second call as a no-op + document.body.addEventListener('bodyEvent', listener); + document.body.addEventListener('bodyEvent', listener); + document.body.dispatchEvent(new Event('bodyEvent')); + assert.strictEqual(callCount, 1); + // now ensure remove still works + callCount = 0; + document.body.removeEventListener('bodyEvent', listener); + document.body.dispatchEvent(new Event('bodyEvent')); + assert.strictEqual(callCount, 0); + }); + it('should handle task without async operation', () => { fakeInteraction(); assert.equal(exportSpy.args.length, 1, 'should export one span'); From bfb3d672cd6e5bf55a8683bec7d106b9c60556e3 Mon Sep 17 00:00:00 2001 From: John Bley Date: Thu, 23 Jul 2020 16:57:33 -0400 Subject: [PATCH 8/9] chore: also handle callbacks with once:true --- .../src/userInteraction.ts | 4 ++++ .../test/userInteraction.nozone.test.ts | 18 +++++++++++++++++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts b/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts index c9a08cfd88..cc5753e902 100644 --- a/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts +++ b/plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts @@ -239,8 +239,12 @@ export class UserInteractionPlugin extends BasePlugin { listener: any, useCapture: any ) { + const once = useCapture && useCapture.once; const patchedListener = (...args: any[]) => { const target = this; + if (once) { + plugin.removePatchedListener(this, type, listener); + } const span = plugin._createSpan(target, 'click'); if (span) { return plugin._tracer.withSpan(span, () => { diff --git a/plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts b/plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts index 3810c412a2..a5ef1535f7 100644 --- a/plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts +++ b/plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts @@ -123,7 +123,7 @@ describe('UserInteractionPlugin', () => { assert.strictEqual(called, false); }); - it('should only not double-register a listener', () => { + it('should not double-register a listener', () => { let callCount = 0; const listener = function () { callCount++; @@ -140,6 +140,22 @@ describe('UserInteractionPlugin', () => { assert.strictEqual(callCount, 0); }); + it('should handle once-only callbacks', () => { + let callCount = 0; + const listener = function () { + callCount++; + }; + // addEventListener semantics treat the second call as a no-op + document.body.addEventListener('bodyEvent', listener, { once: true }); + document.body.addEventListener('bodyEvent', listener); // considered a double-register + document.body.dispatchEvent(new Event('bodyEvent')); + assert.strictEqual(callCount, 1); + // now that it's been dispatched once, it's been removed; should be able to re-add + document.body.addEventListener('bodyEvent', listener); + document.body.dispatchEvent(new Event('bodyEvent')); + assert.strictEqual(callCount, 2); + }); + it('should handle task without async operation', () => { fakeInteraction(); assert.equal(exportSpy.args.length, 1, 'should export one span'); From 4e6d4982432c1d2373eebeed0acbb49a5647d79e Mon Sep 17 00:00:00 2001 From: John Bley Date: Thu, 23 Jul 2020 19:41:58 -0400 Subject: [PATCH 9/9] chore: deeper testing for once:true --- .../test/userInteraction.nozone.test.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts b/plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts index a5ef1535f7..65a1e07966 100644 --- a/plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts +++ b/plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts @@ -150,10 +150,15 @@ describe('UserInteractionPlugin', () => { document.body.addEventListener('bodyEvent', listener); // considered a double-register document.body.dispatchEvent(new Event('bodyEvent')); assert.strictEqual(callCount, 1); - // now that it's been dispatched once, it's been removed; should be able to re-add + // now that it's been dispatched once, it's been removed + document.body.dispatchEvent(new Event('bodyEvent')); + assert.strictEqual(callCount, 1); + // should be able to re-add document.body.addEventListener('bodyEvent', listener); document.body.dispatchEvent(new Event('bodyEvent')); assert.strictEqual(callCount, 2); + document.body.dispatchEvent(new Event('bodyEvent')); + assert.strictEqual(callCount, 3); }); it('should handle task without async operation', () => {