1262976 - browser.windows.onFocusChanged sometimes fires twice, even after the event listener is removed

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Bob Silverberg [:bsilverberg]]

On non-e10s the browser.windows.onFocusChanged behaves as expected. It fires once and if you remove the listener in the listener it does not fire again. On e10s however, the event fires twice, even if the listener is removed.

It has been suggested that bug 1248846 will fix this, so we can see if that is the case once that bug lands. I will attach a test to this bug which will only pass if the bug has been fixed.

Comment 1

[Bob Silverberg [:bsilverberg]]

Attachment: MozReview Request: Bug 1262976 - browser.windows.onFocusChanged sometimes fires twice, even after the event listener is removed, r?kmag

Review commit: https://reviewboard.mozilla.org/r/45103/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45103/

Comment 2

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8739191 [details]
MozReview Request: Bug 1262976 - browser.windows.onFocusChanged sometimes fires twice, even after the event listener is removed, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45103/diff/1-2/

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8739191 [details]
MozReview Request: Bug 1262976 - browser.windows.onFocusChanged sometimes fires twice, even after the event listener is removed, r?kmag

https://reviewboard.mozilla.org/r/45103/#review42107

There are two bugs here: One is that a second event sometimes fires even after the listener is removed. The other is that onFocusChange fires twice for the same focus change.

The test looks fine to me, as far as testing the basics of focus change events, but it doesn't test either of the other issues. The first will be tested elsewhere, but we should really test for the second.

Comment 4

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8739191 [details]
MozReview Request: Bug 1262976 - browser.windows.onFocusChanged sometimes fires twice, even after the event listener is removed, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45103/diff/2-3/

Comment 5

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/45103/#review42107

Ok, I have updated the test to verify that `onFocusChanged` only gets called once.

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8739191 [details]
MozReview Request: Bug 1262976 - browser.windows.onFocusChanged sometimes fires twice, even after the event listener is removed, r?kmag

https://reviewboard.mozilla.org/r/45103/#review43511

::: browser/components/extensions/test/browser/browser_ext_windows_events.js:21
(Diff revision 3)
>      });
>  
> +    browser.windows.onFocusChanged.addListener(function listener(windowId) {
> +      onFocusChangedCount++;
> +      browser.windows.onFocusChanged.removeListener(listener);
> +      browser.test.assertEq(1, onFocusChangedCount, "onFocusChanged only fired once");

This is back to testing the issue that's already tested by bug 1248846. What we need to test is that we only get one focus change event for the focus change, regardless of whether the listener was removed.

Comment 7

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8739191 [details]
MozReview Request: Bug 1262976 - browser.windows.onFocusChanged sometimes fires twice, even after the event listener is removed, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45103/diff/3-4/

Comment 8

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/45103/#review43511

> This is back to testing the issue that's already tested by bug 1248846. What we need to test is that we only get one focus change event for the focus change, regardless of whether the listener was removed.

Ok, that's now being checked. We end up with two events, one when the window is opened and one when it is focused, but I updated the test to deal with that. The test still currently fails because we are getting two events firing for each focus change. Is that being tracked in another bug? If so we should probably make this bug depend on that.

Comment 9

[Kris Maglione [:kmag]]

Can we just fix it in the same bug? It should be pretty trivial.

Comment 10

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8739191 [details]
MozReview Request: Bug 1262976 - browser.windows.onFocusChanged sometimes fires twice, even after the event listener is removed, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45103/diff/4-5/

Comment 11

[Bob Silverberg [:bsilverberg]]

Ok, I managed to get the test passing by making the event listeners for `focus` and `blur` capturing, but I'm not sure if I may have broken something else in the process.

Comment 12

[Kris Maglione [:kmag]]

Comment on attachment 8739191 [details]
MozReview Request: Bug 1262976 - browser.windows.onFocusChanged sometimes fires twice, even after the event listener is removed, r?kmag

https://reviewboard.mozilla.org/r/45103/#review43955

::: browser/components/extensions/test/browser/browser_ext_windows_events.js:20
(Diff revisions 4 - 5)
>      });
>  
>      browser.windows.onFocusChanged.addListener(function listener(windowId) {
>        onFocusChangedCount++;
> -      browser.test.assertTrue(onFocusChangedCount < 3,
> -                              `onFocusChanged fired no more than twice, actual number: ${onFocusChangedCount}`);
> +      browser.test.assertTrue(onFocusChangedCount < 5,
> +                              `onFocusChanged fired no more than four times, actual number: ${onFocusChangedCount}`);

Can we just check that it doesn't fire for the same window ID twice in a row?

::: browser/components/extensions/ext-utils.js:940
(Diff revision 5)
>  
>    addWindowListener(window, eventType, listener) {
> +    let useCapture = false;
> +    if (eventType === "focus" || eventType === "blur") {
> +      useCapture = true;
> +    }

`let useCapture = eventType === "focus" || eventType === "blur"`

We need to do the same check for removeEventListener.

Comment 13

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8739191 [details]
MozReview Request: Bug 1262976 - browser.windows.onFocusChanged sometimes fires twice, even after the event listener is removed, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45103/diff/5-6/

Comment 14

[Bob Silverberg [:bsilverberg]]

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

Comment 15

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8739191 [details]
MozReview Request: Bug 1262976 - browser.windows.onFocusChanged sometimes fires twice, even after the event listener is removed, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45103/diff/6-7/

Comment 16

[Bob Silverberg [:bsilverberg]]

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

Comment 17

[Bob Silverberg [:bsilverberg]]

Wow, there is a lot of orange on that try run, but none of it seems related to this patch.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7e649f9c72b0

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7e649f9c72b0