Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Correctly remove all event listeners #2725

Merged
merged 1 commit into from Jul 7, 2020
Merged

fix: Correctly remove all event listeners #2725

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 26 additions & 3 deletions packages/browser/src/integrations/trycatch.ts
View file
Open in desktop
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah so we have to call remove event listener twice. Makes sense.

Also I would appreciate a comment on why this is happening.

}
} 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);
};
Expand Down
11 changes: 1 addition & 10 deletions packages/browser/test/integration/common/init.js
View file
Open in desktop
Expand Up @@ -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 = [];
Expand Down
23 changes: 21 additions & 2 deletions packages/browser/test/integration/suites/builtins.js
View file
Open in desktop
Expand Up @@ -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"));
Expand All @@ -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() {
Expand Down