fix: Correctly remove all event listeners (#2725)

kamilogorek · web-flow · commit 95edc3c68c1b · 2020-07-07T14:29:13.000+02:00
diff --git a/packages/browser/src/integrations/trycatch.ts b/packages/browser/src/integrations/trycatch.ts
@@ -174,11 +174,34 @@ export class TryCatch implements Integration {
         fn: EventListenerObject,
         options?: boolean | EventListenerOptions,
       ): () => void {
-        let callback = (fn as any) as WrappedFunction;
+        /**
+         * There are 2 possible scenarios here:
+         *
+         * 1. Someone passes a callback, which was attached prior to Sentry initialization, or by using unmodified
+         * method, eg. `document.addEventListener.call(el, name, handler). In this case, we treat this function
+         * as a pass-through, and call original `removeEventListener` with it.
+         *
+         * 2. Someone passes a callback, which was attached after Sentry was initialized, which means that it was using
+         * our wrapped version of `addEventListener`, which internally calls `wrap` helper.
+         * This helper "wraps" whole callback inside a try/catch statement, and attached appropriate metadata to it,
+         * in order for us to make a distinction between wrapped/non-wrapped functions possible.
+         * If a function has `__sentry__` property, it means that it was wrapped, and it has additional property
+         * of `__sentry__original__`, holding the handler. And this original handler, has a reversed link,
+         * with `__sentry_wrapped__` property, which holds the wrapped version.
+         *
+         * When someone adds a handler prior to initialization, and then do it again, but after,
+         * then we have to detach both of them. Otherwise, if we'd detach only wrapped one, it'd be impossible
+         * to get rid of the initial handler and it'd stick there forever.
+         * In case of second scenario, `__sentry_original__` refers to initial handler, and passed function
+         * is a wrapped version.
+         */
+        const callback = (fn as any) as WrappedFunction;
         try {
-          callback = callback && (callback.__sentry_wrapped__ || callback);
+          if (callback && callback.__sentry__) {
+            original.call(this, eventName, callback.__sentry_original__, options);
+          }
         } catch (e) {
-          // ignore, accessing __sentry_wrapped__ will throw in some Selenium environments
+          // ignore, accessing __sentry__ will throw in some Selenium environments
         }
         return original.call(this, eventName, callback, options);
       };
diff --git a/packages/browser/test/integration/common/init.js b/packages/browser/test/integration/common/init.js
@@ -3,16 +3,7 @@
 // - make assertions re: wrapped functions
 var originalBuiltIns = {
   setTimeout: setTimeout,
-  setInterval: setInterval,
-  requestAnimationFrame: requestAnimationFrame,
-  xhrProtoOpen: XMLHttpRequest.prototype.open,
-  headAddEventListener: document.head.addEventListener, // use <head> 'cause body isn't closed yet
-  headRemoveEventListener: document.head.removeEventListener,
-  consoleDebug: console.debug,
-  consoleInfo: console.info,
-  consoleWarn: console.warn,
-  consoleError: console.error,
-  consoleLog: console.log,
+  addEventListener: document.addEventListener,
 };
 
 var events = [];
diff --git a/packages/browser/test/integration/suites/builtins.js b/packages/browser/test/integration/suites/builtins.js
@@ -27,14 +27,13 @@ describe("wrapped built-ins", function() {
     return runInSandbox(sandbox, function() {
       var div = document.createElement("div");
       document.body.appendChild(div);
-      var click = new MouseEvent("click");
       var fooFn = function() {
         foo();
       };
       var barFn = function() {
         bar();
       };
-      div.addEventListener("click", fooFn, false);
+      div.addEventListener("click", fooFn);
       div.addEventListener("click", barFn);
       div.removeEventListener("click", barFn);
       div.dispatchEvent(new MouseEvent("click"));
@@ -43,6 +42,26 @@ describe("wrapped built-ins", function() {
     });
   });
 
+  it("should remove the original callback if it was registered before Sentry initialized (w. original method)", function() {
+    return runInSandbox(sandbox, function() {
+      var div = document.createElement("div");
+      document.body.appendChild(div);
+      window.capturedCall = false;
+      var captureFn = function() {
+        window.capturedCall = true;
+      };
+      // Use original addEventListener to simulate non-wrapped behavior (callback is attached without __sentry_wrapped__)
+      window.originalBuiltIns.addEventListener.call(div, "click", captureFn);
+      // Then attach the same callback again, but with already wrapped method
+      div.addEventListener("click", captureFn);
+      div.removeEventListener("click", captureFn);
+      div.dispatchEvent(new MouseEvent("click"));
+    }).then(function(summary) {
+      assert.equal(summary.window.capturedCall, false);
+      delete summary.window.capturedCalls;
+    });
+  });
+
   it("should capture exceptions inside setTimeout", function() {
     return runInSandbox(sandbox, function() {
       setTimeout(function() {
