1198235 - abuse of bind(this) with add/removeEventListener in downloads.js

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect)

Description

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from bug 1185893 comment #8)
> It looks like we're still missing this one:
> 
> https://dxr.mozilla.org/mozilla-central/source/browser/components/downloads/
> content/downloads.js#374

Specifically:

  _attachEventListeners() {
    // Handle keydown to support accel-V.
    this.panel.addEventListener("keydown", this._onKeyDown.bind(this), false);
    // Handle keypress to be able to preventDefault() events before they reach
    // the richlistbox, for keyboard navigation.
    this.panel.addEventListener("keypress", this._onKeyPress.bind(this), false);
  },

  /**
   * Unattach event listeners that were added in _attachEventListeners. This
   * is called automatically on panel termination.
   */
  _unattachEventListeners() {
    this.panel.removeEventListener("keydown", this._onKeyDown.bind(this),
                                   false);
    this.panel.removeEventListener("keypress", this._onKeyPress.bind(this),
                                   false);
  },


This is clearly not going to work. A patch should ensure that we remove exactly what we added, not call bind several times.

A technique that I've seen used here by e.g. the devtools is to replace the handler with its bound version when init() or the constructor of the object is called, which means you can avoid calling bind() individually for adding/removing the listener, and stuff Just Works because you pass the same function reference all the time.

Nazim, do you want to pick this up as well?

Comment 1

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

Yes, i would like to work on this bug as well. Thanks for letting me know. But this bug seems a bit different from the previous one and i'm not sure i understand correctly. It looks removing the bind calls are enough because we are passing same function reference, did i get right?

Comment 2

[:Gijs (he/him)]

(In reply to Nazim Can Altinova [:Canova] from comment #1)
> Yes, i would like to work on this bug as well. Thanks for letting me know.

Great!

> But this bug seems a bit different from the previous one and i'm not sure i
> understand correctly. It looks removing the bind calls are enough because we
> are passing same function reference, did i get right?

Not quite, the bind() call is necessary to ensure that the |this| object remains the same. Have a look at the MDN documentation for bind() if you're not familiar with it:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/bind

Without bind, the event listener would be called with |this| set to the target element, or the window.

Comment 3

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

Oh thanks for the clarification about bind :) So like previous bug, i should change the _onKeyDown and _onKeyPress functions to arrow functions, shouldn't i?

Comment 4

[:Gijs (he/him)]

(In reply to Nazim Can Altinova [:Canova] from comment #3)
> Oh thanks for the clarification about bind :) So like previous bug, i should
> change the _onKeyDown and _onKeyPress functions to arrow functions,
> shouldn't i?

That by itself won't be enough here, because they will be bound to the "this" at the time of declaration, which will be the global scope of the file...

I think the simplest solution is the following:


1) change the add... and removeEventListener calls to pass |this|: ...addEventListener("keydown", this, false)
2) add a "handleEvent" method onto the DownloadsPanel that takes 1 parameter ("event").
3) the body of handleEvent should look something like this:

switch (event.type) {
  case "keydown":
    return this._onKeyDown(event);
  case "keypress":
    return this._onKeyPress(event);
}


What happens is that gecko will call handleEvent with the event, and it will do so with |this| set to DownloadsPanel, as we would like. Then when we ourselves forward that call to _onKeyDown/_onKeyPress, we use |this| and so inside those methods, |this| will be set correctly.


I hope that made sense - if not, please ask Marco to clarify further, as I will be away from tomorrow until the 7th of September (same for reviewing a patch once you have one :-) ).

Comment 5

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

Attachment: bug1198235.patch

Thank you for your help Gijs. And i'm sorry for the dummy questions. I'm still trying to getting used to Mozilla's source code.
So I uploaded a patch for this bug. Could you review the patch Marco?

Comment 6

[:Gijs (he/him)]

Comment on attachment 8654438 [details] [diff] [review]
bug1198235.patch

Surprise! Off to Paris in a bit, but this looks great. Pushed to try for you:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=851b6ca4f502

If that is green, feel free to set the checkin-needed keyword so someone lands this for you. If you're not sure about the results of the trypush, check with Marco (mak).

Thanks again for the patch! :-)

Comment 7

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

Thank you for the review again Gijs :) Try has some success tests but most of the tests seems failed. What should i do about it?

Comment 8

[:Paolo Amadini]

You should be able to reproduce the failures locally with this command:

./mach test browser/components/downloads

Looks like there is a syntax error in the patch, I guess it's the missing colon after the third case statement. If this fixes the tests locally for you, feel free to attach an updated patch.

Comment 9

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

Attachment: bug1198235_new.patch

Oh I'm sorry :( I thought I was tested before i send the patch, but clearly I had made a mistake at somewhere at testing. Now I tested and I'm pretty sure there is no error in this one :)

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8655052 [details] [diff] [review]
bug1198235_new.patch

Review of attachment 8655052 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!

I pushed this to Try, if it should succeed (or just have intermittent failures), feel free to add the checkin-needed keyword.
feel free to ask for any question.
Thank you!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2d8143986d2

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4eef365f3890

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4eef365f3890