1331599 - Avoid removeEventListener calls when {once: true} can be used instead

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Florian Quèze [:florian]]

Attachment: xpcshell script

'once' has been supported since Gecko 50 / bug 1287706, I think it's time to use it more consistently in our code. I'm attaching an xcpshell script that does this change automatically.

Comment 1

[Florian Quèze [:florian]]

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

Comment 2

[Florian Quèze [:florian]]

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

Comment 3

[Florian Quèze [:florian]]

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

Comment 4

[Florian Quèze [:florian]]

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

Comment 5

[Florian Quèze [:florian]]

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

Comment 6

[Florian Quèze [:florian]]

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

Comment 7

[Florian Quèze [:florian]]

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

Comment 8

[Florian Quèze [:florian]]

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

Comment 9

[Florian Quèze [:florian]]

Attachment: script-generated patch

Comment 10

[Florian Quèze [:florian]]

Attachment: xpcshell script

Version of the xpcshell script used to generate attachment 8829283 [details] [diff] [review].

Comment 11

[Florian Quèze [:florian]]

Attachment: tests fixed by hand

browser/base/content/test/general/browser_identity_UI.js
This test had a race between the load event transferred from the child process by the add-ons shim, and BrowserTestUtils.openNewForegroundTab which transfers the event from the child process using browserLoaded / the "browser-test-utils:loadEvent" message. I touched the add-ons shim a bit in bug 1332965, which is enough to make this test fail half the time without this fix.
The test failure was:
 TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_identity_UI.js | location matches for test #0 (normal domain) - Got about:blank, expected http://test1.example.org/
 (see try push from comment 7)
 It's fixed with this change, see green try push in comment 8.

gfx/layers/apz/test/mochitest/apz_test_native_event_utils.js
Couldn't be processed automatically by the script, due to the 'useCapture' variable, so I handled that file by hand.

Note: The script also can't handle http://searchfox.org/mozilla-central/source/devtools/client/webconsole/test/test-own-console.html#15 where removing the removeEventListener call would result in the listener doing nothing. I'm not sure of what this code is trying to do, so I left it unmodified.

Other files modified in this patch are only tests that hardcoded line numbers, which change slightly without the removeEventListener calls.

Comment 12

[Florian Quèze [:florian]]

Attachment: fix the balanced-listeners custom eslint rule

- handle addEventListener's options parameter,
- add tests.

Comment 13

[Florian Quèze [:florian]]

Attachment: new eslint rule

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8829283 [details] [diff] [review]
script-generated patch

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

rs=me, I skimmed the patch and didn't see anything odd. I also looked at the xpcshell script you used to generate the patch.

::: addon-sdk/source/test/addons/e10s-content/lib/test-content-script.js
@@ +656,5 @@
>        assert(input.onclick === onclick, "on* attributes are equal to original function set");
>  
>        let addEventListenerCalled = false;
>        let expandoCalled = false;
>        input.addEventListener("click", function onclick(event) {

The function name wasn't removed here but it could have been. Line 673 uses input.onclick property (the same name), but otherwise this function doesn't reference anything named onclick.

Comment 15

[Jan Henning [:JanH]]

I see that this seems to have missed the mobile session store (presumably because until now, the call to removeEventListener is not the very first call in the listener function). Note that bug 1332863 is in the process of landing, so if you want to manually include this fix, you'll need to base it on top of that patch (https://reviewboard.mozilla.org/r/106320/diff/1#index_header) - since I've swapped the order of function calls around, maybe the script should pick it up as well now if you rerun it with that patch included?

Comment 16

[Jan Henning [:JanH]]

Sorry spreading this across three separate comments but I forgot - if you want to import bug 1332863 before it's landed on central, you'll also need to apply the patch for bug 1330422 first (https://reviewboard.mozilla.org/r/106316/diff/1#index_header).

Comment 17

[Florian Quèze [:florian]]

(In reply to Jan Henning [:JanH] from comment #15)
> I see that this seems to have missed the mobile session store (presumably
> because until now, the call to removeEventListener is not the very first
> call in the listener function). Note that bug 1332863 is in the process of
> landing, so if you want to manually include this fix, you'll need to base it
> on top of that patch
> (https://reviewboard.mozilla.org/r/106320/diff/1#index_header) - since I've
> swapped the order of function calls around, maybe the script should pick it
> up as well now if you rerun it with that patch included?

The script didn't pick it up before because removeEventListener wasn't the first instruction, and it won't pick it up after because you used an onLoad variable outside the scope of the listener.

I mean you did:
        let onLoad = /* listener */
        window.addEventListener("load", onLoad);

rather than:
        window.addEventListener("load", function onLoad() {
          /* listener */
        });

Don't hesitate to use {once: true} in any new code you are writing :-).

Comment 18

[Jan Henning [:JanH]]

Right, although doing so would conflict with the usage of "this" inside the listener.

> Don't hesitate to use {once: true} in any new code you are writing :-).
Unfortunately I saw the blog post about this only today, otherwise I would have already included it when turning on strict mode, since I had to touch that function anyway.

If you don't change it within this bug, I'll pick it up later...

Comment 19

[Florian Quèze [:florian]]

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

Comment 20

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec0f3678bb31c8e5273d72c27aed48edc899e71c
Bug 1331599 - script-generated patch to replace removeEventListener calls with the once option when possible, r=jaws.

https://hg.mozilla.org/integration/mozilla-inbound/rev/59db8fffcf9a3418541e146bed60c3bb3a5e12fc
Bug 1331599 - tests fixed by hand after making addEventListener use the once option when possible, r=jaws.

https://hg.mozilla.org/integration/mozilla-inbound/rev/37161ea3d91bcf16576bf72e6604d7dc1e369ded
Bug 1331599 - fix the balanced-listeners custom eslint rule to handle addEventListener's options parameter, r=jaws.

https://hg.mozilla.org/integration/mozilla-inbound/rev/337f6e703a5ba9b08fee4fca422d243c1e71c5a5
Bug 1331599 - add an eslint rule to reject removeEventListener calls when the once option can be used instead, r=jaws.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ec0f3678bb31
https://hg.mozilla.org/mozilla-central/rev/59db8fffcf9a
https://hg.mozilla.org/mozilla-central/rev/37161ea3d91b
https://hg.mozilla.org/mozilla-central/rev/337f6e703a5b