1185893 - abuse of bind(this) with add/removeEventListener

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Marco Bonardo [:mak]]

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

Comment 1

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

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);

Comment 2

[Marco Bonardo [:mak]]

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.

Comment 3

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: bug1185893.patch

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

Comment 4

[:Gijs (he/him)]

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.

Comment 5

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: bug1185893_2.patch

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

Comment 6

[:Gijs (he/him)]

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

Comment 7

[:Gijs (he/him)]

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

Comment 8

[:Gijs (he/him)]

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.

Comment 9

[:Gijs (he/him)]

(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.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2c55957b5538

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2c55957b5538

Comment 12

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Thanks for the help Gijs and Marco!