In various parts of the code we do something like: elt.addEventListener("someevent", function handle() { elt.removeEventListener("someevent", handle); // do something with "this". }.bind(this)); Drew pointed out correctly that Function.prototype.bind() creates a new function, so removeEventListener here is not removing the actual listener. We should fix the points where we do that, by using arrow functions let handle = () => { elt.removeEventListener("someevent", handle); }; elt.addEventListener("someevent", handle); Searching dxr/mxr for ".bind(this), false)" or "bind(this), true)" should return most points to audit for the mistake: https://dxr.mozilla.org/mozilla-central/search?q=regexp%3A\.bind\%28this\%29%2C\s%28false|true%29&redirect=true Unfortunately there is also ".bind(this))" that could be in use (the addEventListener capture argument is optional) but has too many false positives on mxr/dxr, for that I think a local multiline grep could be used to find the ones that generate from addEventListener. https://dxr.mozilla.org/mozilla-central/search?q=}.bind%28this%29%29%3B&redirect=true&offset=200

Hi, I would like to work on this bug but i have a question. Some event listeners don't have removeEventListener inside. Should i just leave it that way or i have to change it like this? code example: > window.addEventListener("ContentStart", (function(evt) { > let content = shell.contentBrowser.contentWindow; > content.addEventListener("mozContentEvent", this, false, true); > }).bind(this), false);

if they don't have a removeEventListener, they should keep not having one. The scope of this bug is just to fix the above broken case, let's try to not scope creep.

Sorry, I'm a bit late. Despite all my research, i find only a few that fit this case. Here's the patch.

Comment on attachment 8648507 [details] [diff] [review] bug1185893.patch Review of attachment 8648507 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for the patch! This is pretty close for the things that are in here. Did you run the tests? There's a few small mistakes here, so I'm a little surprised if this passed (and in that case, we should figure out why). I've just checked the ones in this patch, I haven't checked myself if I could find other instances. Note that Marco is on vacation (ish) so please feel free to request review from me for the next round. ::: browser/base/content/test/general/browser_fullscreen-window-open.js @@ +331,5 @@ > Services.wm.removeListener(this); > > let domwindow = aXULWindow.QueryInterface(Ci.nsIInterfaceRequestor) > .getInterface(Ci.nsIDOMWindow); > + let onLoad = () => { Nit: you need to keep aEvent here, so you want: let onLoad = aEvent => { instead. ::: browser/components/loop/modules/MozLoopService.jsm @@ +897,5 @@ > if (chatbox.contentWindow.navigator.mozLoop) { > return; > } > > + let loaded = () => { same here, this should be: event => { because we use the event argument inside the function.

Thanks for your review Gijs! I made a new patch and i hope there is no mistake in this one.

(Thanks for the patch! I'm busy over the weekend - I'll review this on Monday!)

Comment on attachment 8651276 [details] [diff] [review] bug1185893_2.patch Review of attachment 8651276 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. Let's do a try-push to make sure we're not breaking anything that inadvertently relied on some of the brokenness here: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab253bb73ae3

This patch is a good start, but it looks like we're still missing this one: https://dxr.mozilla.org/mozilla-central/source/browser/components/downloads/content/downloads.js#374 I didn't spot any other problematic cases in the DXR link from comment #0, and neither my mac nor my Windows machine seem to have pcregexp to do multiline searches.

(In reply to :Gijs Kruitbosch from comment #7) > Comment on attachment 8651276 [details] [diff] [review] > bug1185893_2.patch > > Review of attachment 8651276 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks good. Let's do a try-push to make sure we're not breaking > anything that inadvertently relied on some of the brokenness here: > > remote: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab253bb73ae3 Annnnd we have timeouts in the password manager tests. Excellent. :-\ I'll land the other changes, and then file a followup for that and the downloads button change.

Thanks for the help Gijs and Marco!