1547581 - Wrong 'this' sent to event handlers rewritten from on* event attributes to addEventListener()

Status: RESOLVED FIXED

(Thunderbird :: Preferences, defect)

Description

[:aceman]

In the Account Settings > Synchronisation and Storage > Advanced dialog when you click Cancel, you get:
TypeError: this._rollbackMap is undefined msgSelectOfflineFolders.js:100:33

Similarly, in Tools->Options->Composition->General->Keywords, when clicking OK you get:
TypeError: this.keywordListBox is undefined in attachmentReminder.js:70:21

It it seems the handler functions that get called on these events expect a different 'this' as being passed to them from addEventListener().

Previously, 'this' was the object in which these functions (like onOK, on Cancel) were defined (e.g. gSelectOffline). Now it is seems to be 'document' or the element on which addEventListener is called.

There are some docs on this in https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener how to pass proper 'this' (e.g. using bind()), which would probably be better and more readable than rewriting the functions to name the containing global object directly.

It seems all addEventListener calls added in https://hg.mozilla.org/comm-central/rev/ea892780482c need to be audited for this problem and also maybe patches to other attributes (aside from ondialogaccept and ondialogcancel).

Comment 1

[:aceman]

Maybe we should just add the .bind(object) unconditionally to every addEventListener() where we call object.method() to future proof the methods, as even if now the method() does not use this, it may start to use it in the future, as it is placed in the 'object' and any other method() in the object uses it so coders will naturally use it and assume it will work. We should prevent those future errors going unnoticed.

Comment 2

[:aceman]

Attachment: 1552851.patch

Add the .bind() calls where we call Object.function() from addEventListener.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=da8e40a0819ddfb05572b58c7523425dec174fdb

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9066069 [details] [diff] [review]
1552851.patch

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

I think this is in the wrong bug.

Comment 4

[:aceman]

Attachment: 1547581.patch

Right, wrong patch, sorry.

Comment 5

[Geoff Lankow (:darktrojan)]

Instead of gFoo.bar.bind(gFoo), you could use () => gFoo.bar(), which I think would be cleaner, since some of these variable names are quite long.

Comment 6

[:aceman]

How will that know to bind the right gFoo? Also, it may be shorter but I wonder if everybody would understand the point of it and not remove the '() =>' as redundant.

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 9066162 [details] [diff] [review]
1547581.patch

I'll leave the review to the JS geeks.

Comment 8

[:aceman]

Attachment: 1547581.patch v2

So if you like this one better. It seems to work but I do not know why.
I know '=>' functions have some differences in handling 'this' but how can it guess the right object?

Comment 9

[Geoff Lankow (:darktrojan)]

I still don't understand the question. Where is there more than one object with the same name? There is only one gWhatever in each of these situations.

Using the arrow function makes no difference either. It's no different from function() { gFoo.bar(); } except that it's shorter, because there's no reference to this inside the function.

Comment 10

[Geoff Lankow (:darktrojan)]

Comment on attachment 9066553 [details] [diff] [review]
1547581.patch v2

I like this one better.

Comment 11

[:aceman]

(In reply to Geoff Lankow (:darktrojan) from comment #9)

I still don't understand the question. Where is there more than one object with the same name? There is only one gWhatever in each of these situations.

No, I do't talk about there being more than one gWhatever.

Using the arrow function makes no difference either. It's no different from function() { gFoo.bar(); } except that it's shorter, because there's no reference to this inside the function.

Inside of gFoo.bar() there is a call to 'this' and we want 'this' to be gFoo. With a plain 'document.addEventListener("event", gFoo.bar())', 'this' ended up being 'document' or something, not gFoo. Can you explain how thanks to '() =>' or 'function() { }' suddenly 'this' becomes 'gFoo' inside of gFoo.bar() ?

Comment 12

[Magnus Melin [:mkmelin]]

Please see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions#No_separate_this

In essence: for arrow functions, the this value of the enclosing lexical scope is used.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/423c3668781b
use arrow functions when calling methods from addEventListener to use 'this' from the enclosing lexical scope. r=darktrojan

Comment 14

[Jorg K (CEST = GMT+2)]

I hope you're happy with the adjusted commit message.

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 9066553 [details] [diff] [review]
1547581.patch v2

More beta material, right?

Comment 16

[Jorg K (CEST = GMT+2)]

TB 68 beta:
https://hg.mozilla.org/releases/comm-beta/rev/9a0eb44a1ef5f97f5f9c8e5dfb6156a83c3ce845

Comment 17

[:aceman]

(In reply to Magnus Melin [:mkmelin] from comment #12)

Please see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions#No_separate_this

In essence: for arrow functions, the this value of the enclosing lexical scope is used.

I have read that page before, but I do not see how it applies here. How is "enclosing lexical scope" determined?

(In reply to Jorg K (GMT+2) from comment #15)

More beta material, right?

Yes!

Comment 18

[Jorg K (CEST = GMT+2)]

Looks like https://hg.mozilla.org/releases/comm-beta/rev/9a0eb44a1ef5f97f5f9c8e5dfb6156a83c3ce845#l2.12 caused bug 1558775 :-(