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

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
2 years ago
2 years 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

2 years ago
Posted file xpcshell script (obsolete) —
'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)

Updated

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

Updated

2 years ago
Depends on: 1332965
(Assignee)

Comment 9

2 years ago
Attachment #8829283 - Flags: review?(jaws)
(Assignee)

Updated

2 years ago
Assignee: nobody → florian
Status: NEW → ASSIGNED
(Assignee)

Comment 10

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

Comment 11

2 years ago
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

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

Comment 13

2 years ago
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+
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

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

2 years 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)
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...

Updated

2 years ago
See Also: → 1333386
(Assignee)

Comment 20

2 years 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.
(Assignee)

Updated

2 years ago
Depends on: 1334250

Updated

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