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

RESOLVED FIXED in Firefox 54

Status

()

Firefox
General
RESOLVED FIXED
7 months ago
5 months ago

People

(Reporter: florian, Assigned: florian)

Tracking

(Depends on: 1 bug)

53 Branch
Firefox 54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 affected, firefox54 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

7 months ago
Created attachment 8827408 [details]
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.
(Assignee)

Comment 1

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5cb10a84739
(Assignee)

Updated

7 months ago
Attachment #8827408 - Attachment mime type: application/binary → text/plain
(Assignee)

Comment 2

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9fe85a1451a
(Assignee)

Comment 3

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3128213c8497
(Assignee)

Comment 4

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=32cac9c74a07
(Assignee)

Comment 5

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4e1b532e668
(Assignee)

Comment 6

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cea677bef5a
(Assignee)

Comment 7

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=01f0a0a80d3b
(Assignee)

Comment 8

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=67f044dd5b77
(Assignee)

Updated

7 months ago
Depends on: 1332965
(Assignee)

Comment 9

7 months ago
Created attachment 8829283 [details] [diff] [review]
script-generated patch
Attachment #8829283 - Flags: review?(jaws)
(Assignee)

Updated

7 months ago
Assignee: nobody → florian
Status: NEW → ASSIGNED
(Assignee)

Comment 10

7 months ago
Created attachment 8829284 [details]
xpcshell script

Version of the xpcshell script used to generate attachment 8829283 [details] [diff] [review].
Attachment #8827408 - Attachment is obsolete: true
(Assignee)

Comment 11

7 months ago
Created attachment 8829286 [details] [diff] [review]
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.
Attachment #8829286 - Flags: review?(jaws)
(Assignee)

Comment 12

7 months ago
Created attachment 8829287 [details] [diff] [review]
fix the balanced-listeners custom eslint rule

- handle addEventListener's options parameter,
- add tests.
Attachment #8829287 - Flags: review?(jaws)
(Assignee)

Comment 13

7 months ago
Created attachment 8829288 [details] [diff] [review]
new eslint rule
Attachment #8829288 - Flags: review?(jaws)
Attachment #8829286 - Flags: review?(jaws) → review+
Attachment #8829287 - Flags: review?(jaws) → review+
Attachment #8829288 - Flags: review?(jaws) → review+
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.
Attachment #8829283 - Flags: review?(jaws) → review+

Comment 15

7 months ago
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?

Updated

7 months ago
Flags: needinfo?(florian)

Comment 16

7 months ago
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).
(Assignee)

Comment 17

7 months ago
(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 :-).
Flags: needinfo?(florian)

Comment 18

7 months ago
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...
(Assignee)

Comment 19

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

Updated

7 months ago
See Also: → bug 1333386
(Assignee)

Comment 20

7 months ago
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

7 months ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
(Assignee)

Updated

7 months ago
Depends on: 1334250

Updated

5 months ago
Depends on: 1348940
You need to log in before you can comment on or make changes to this bug.