Merge pull request #158 from johnbley/master

fix: patch removeEventListener to properly remove patched callbacks

plugins/web/opentelemetry-plugin-user-interaction/src/userInteraction.ts

@@ -48,6 +48,11 @@ export class UserInteractionPlugin extends BasePlugin<unknown> {
   moduleName = this.component;
   private _spansData = new WeakMap<api.Span, SpanData>();
   private _zonePatched = false;
+  // for addEventListener/removeEventListener state
+  private _wrappedListeners = new WeakMap<
+    Function,
+    Map<string, Map<HTMLElement, Function>>
+  >();
 
   constructor() {
     super('@opentelemetry/plugin-user-interaction', VERSION);
@@ -165,6 +170,61 @@ export class UserInteractionPlugin extends BasePlugin<unknown> {
     }
   }
 
+  /**
+   * 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();
+      this._wrappedListeners.set(listener, listener2Type);
+    }
+    let element2patched = listener2Type.get(type);
+    if (!element2patched) {
+      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,
+    listener: Function
+  ): Function | undefined {
+    const listener2Type = this._wrappedListeners.get(listener);
+    if (!listener2Type) {
+      return undefined;
+    }
+    const 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
@@ -179,8 +239,12 @@ export class UserInteractionPlugin extends BasePlugin<unknown> {
         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, () => {
@@ -193,7 +257,37 @@ export class UserInteractionPlugin extends BasePlugin<unknown> {
             return listener.apply(target, args);
           }
         };
-        return original.call(this, type, patchedListener, useCapture);
+        if (plugin.addPatchedListener(this, type, 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.removePatchedListener(
+          this,
+          type,
+          listener
+        );
+        if (wrappedListener) {
+          return original.call(this, type, wrappedListener, useCapture);
+        } else {
+          return original.call(this, type, listener, useCapture);
+        }
       };
     };
   }
@@ -434,11 +528,22 @@ export class UserInteractionPlugin extends BasePlugin<unknown> {
           '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();

plugins/web/opentelemetry-plugin-user-interaction/test/userInteraction.nozone.test.ts

@@ -89,6 +89,78 @@ describe('UserInteractionPlugin', () => {
       context.disable();
     });
 
+    it('should not break removeEventListener', () => {
+      let called = false;
+      const listener = function () {
+        called = true;
+      };
+      // 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;
+      // 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);
+    });
+
+    it('should 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 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
+      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', () => {
       fakeInteraction();
       assert.equal(exportSpy.args.length, 1, 'should export one span');