chrome.tabs.onActivated does not fire for new windows

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
WebExtensions: Frontend
P3
normal
RESOLVED FIXED
6 months ago
4 months ago

People

(Reporter: adam.schenker, Assigned: bsilverberg)

Tracking

51 Branch
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [triaged])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

6 months ago
Created attachment 8840581 [details]
Using the debugger, this extension can be used to test the behavior described

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_3) AppleWebKit/602.4.8 (KHTML, like Gecko) Version/10.0.3 Safari/602.4.8

Steps to reproduce:

The chrome.tabs.onActivated event does not fire when opening a new window. This can be checked with the following background code running and opening a new window:

chrome.tabs.onActivated.addListener(
	function(activeInfo) 
	{
		console.log("Event received");
	}
);


Actual results:

The event handler is not called when a new window is opened.


Expected results:

A new window being created also changes the active tab, since the tab in the new window will become active. Chrome always fires this event when opening a new window.

Updated

6 months ago
Component: Untriaged → WebExtensions: Frontend
Product: Firefox → Toolkit

Comment 1

6 months ago
There's one active tab per window, so opening a new window doesn't change the active tab. That tab is already active when it's created. Although I suppose I could make an argument for firing it anyway, since we do when we create a new tab in an existing window. But in that case the active tab is actually changing.

Updated

6 months ago
Priority: -- → P3
Whiteboard: [triaged]

Updated

5 months ago
webextensions: --- → ?

Updated

5 months ago
webextensions: ? → +

Updated

5 months ago
Assignee: nobody → bob.silverberg
Comment hidden (mozreview-request)

Comment 3

5 months ago
mozreview-review
Comment on attachment 8854549 [details]
Bug 1342207 - chrome.tabs.onActivated does not fire for new windows,

https://reviewboard.mozilla.org/r/126508/#review129040

::: browser/components/extensions/ext-tabs.js:108
(Diff revision 1)
> -          let tabId = tabTracker.getId(nativeTab);
> -          let windowId = windowTracker.getId(nativeTab.ownerGlobal);
> +            let tabId = tabTracker.getId(event.nativeTab);
> +            let windowId = windowTracker.getId(event.nativeTab.ownerGlobal);
> -          fire.async({tabId, windowId});
> +            fire.async({tabId, windowId});

let {id, windowId} = tabManager.wrapTab(event.nativeTab);
    fire.async({tabId: tab.id, windowId});

Or, preferably, just have the tab tracker emit the event with those properties.

Same for highlighted.

::: browser/components/extensions/ext-utils.js:314
(Diff revision 1)
>        };
>  
>        this.on("tab-detached", listener);
>      } else {
> +      // emitActivated to trigger tab.onActivated/tab.onHighlighted for a newly opened window.
> +      this.emitActivated(window.gBrowser.tabs[0]);

Should emit this *after* created. Please add tests for this.

::: browser/components/extensions/test/browser/browser_ext_tabs_onHighlighted.js:78
(Diff revision 1)
>      }
>  
>      /**
> +     * Opens a new window and asserts that the correct events are fired.
> +     *
> +     * @param {Array} urls

`@param {Array<string>} urls`

Please also add a description.

::: browser/components/extensions/test/browser/browser_ext_tabs_onHighlighted.js:84
(Diff revision 1)
> +     */
> +    async function openWindow(urls) {
> +      let window = await browser.windows.create({url: urls});
> +      browser.test.log(`Opened new window ${window.id}`);
> +
> +      for (let i = 0; i < urls.length; i++) {

for (let [i, url] of urls.entries()) {

::: browser/components/extensions/test/browser/browser_ext_tabs_onHighlighted.js:97
(Diff revision 1)
> +          ]);
> +        } else {
> +          browser.test.assertEq(undefined, events[tab.id],
> +                                "No events were fired for extra tabs in opened window.");
> +        }
> +      }

Should also check for the appropriate onCreated events, in the correct order.
(Assignee)

Updated

5 months ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment hidden (mozreview-request)
(Assignee)

Comment 5

5 months ago
mozreview-review-reply
Comment on attachment 8854549 [details]
Bug 1342207 - chrome.tabs.onActivated does not fire for new windows,

https://reviewboard.mozilla.org/r/126508/#review129040

> Should emit this *after* created. Please add tests for this.

I updated the test included in this patch to check for the onCreated event including that it is fired first. Is that sufficient, or do you think we need even more tests than that?
(Assignee)

Comment 6

5 months ago
Thanks for the review, Kris. 

I made all of the suggested changes, but when doing so I discovered that while the new code I added does fire onActivated after onCreated, as expected, the existing code for onActivated (that fires when a new tab is opened in an existing window) fires onActivated *before* onCreated. Is that expected in that scenario, or is that a bug that needs to be fixed?
Flags: needinfo?(kmaglione+bmo)

Comment 7

5 months ago
Please fix the existing code so onActivated always fires after onCreated.

Thanks.
Flags: needinfo?(kmaglione+bmo)

Comment 8

5 months ago
mozreview-review
Comment on attachment 8854549 [details]
Bug 1342207 - chrome.tabs.onActivated does not fire for new windows,

https://reviewboard.mozilla.org/r/126508/#review130052
Attachment #8854549 - Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 10

5 months ago
Thanks Kris.

I wanted to submit that code so you could see what I changed, but there's a problem with it because it introduced a weird failure in my test, which I cannot figure out.

With my latest patch, my test passes consistently for the main process, but the test run that uses oop extensions fails consistently, and in a strange manner. The test does the following:

1. Open 3 new tabs, checking that the expected events fire, in the expected order.
2. Select each of the 3 tabs, checking that onActivated and onHighlighted fire for each tab.
3. Open 3 new windows, with varying numbers of tabs, checking that the expected tab events fire, in the expected order.

Parts 1 and 3 pass in all cases, but in part 2, the test for the very first tab to be selected fails because only the onActivated event is recorded, not the onHighlighted event. Part 2 does pass for the second and third tab selected, though. This failure is consistent and reproducible, but I have no idea what is causing it. It doesn't seem like it's possible for onActivated to fire without onHighlighted firing as well, and it's particularly strange to me that it always fails for the first tab, but not the second or third.

Do you have any idea what's causing this strange failure in the test when run with oop extensions?
Flags: needinfo?(kmaglione+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

4 months ago
Comment on attachment 8854549 [details]
Bug 1342207 - chrome.tabs.onActivated does not fire for new windows,

Kris, I made a change to the test to address those failures we were seeing on try. Could you please take a look at it to see if it seems okay? Everything is passing on try with this change.
Flags: needinfo?(kmaglione+bmo)
Attachment #8854549 - Flags: review+ → review?(kmaglione+bmo)

Comment 15

4 months ago
mozreview-review
Comment on attachment 8854549 [details]
Bug 1342207 - chrome.tabs.onActivated does not fire for new windows,

https://reviewboard.mozilla.org/r/126508/#review132138

::: browser/components/extensions/test/browser/browser_ext_tabs_onHighlighted.js:56
(Diff revision 6)
> +      while (!((events[tabId] !== undefined &&
> +                events[tabId].length === expectedEvents.length) ||
> +                ticks === 5)) {

This logic makes my brain hurt...

Please make it something like:

    for (let i = 0; i < 5 && (!events[tabId] || events[tabId].length < expectedEvents.length); i++)

::: browser/components/extensions/test/browser/browser_ext_tabs_onHighlighted.js:151
(Diff revision 6)
>        highlightTab(tabIds[0]),
>        highlightTab(tabIds[1]),
>        highlightTab(tabIds[2]),
>      ]);
>  
> +    await Promise.all([

Please assert that there are no recorded events left between tests.
Attachment #8854549 - Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request)

Comment 17

4 months ago
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7b1f88583896
chrome.tabs.onActivated does not fire for new windows, r=kmag
Backed out for frequently failing browser_ext_tabs_onHighlighted.js:

https://hg.mozilla.org/integration/autoland/rev/ee155f18c098981d9101c685cf8f87a8a2023c8f

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7b1f8858389682cb0104603b2a52bdd31490acf4&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=91332720&repo=autoland

[task 2017-04-13T15:09:14.163496Z] 15:09:14     INFO - TEST-PASS | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_tabs_onHighlighted.js | Tab 496 highlighted - Expected: 496, Actual: 496 - 
[task 2017-04-13T15:09:14.168798Z] 15:09:14     INFO - Expecting events: onActivated, onHighlighted
[task 2017-04-13T15:09:14.170580Z] 15:09:14     INFO - Buffered messages finished
[task 2017-04-13T15:09:14.173319Z] 15:09:14     INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_tabs_onHighlighted.js | Got expected number of events for 496 - Expected: 2, Actual: 1 - 
[task 2017-04-13T15:09:14.175197Z] 15:09:14     INFO - Stack trace:
[task 2017-04-13T15:09:14.177022Z] 15:09:14     INFO -     expectEvents@moz-extension://733f1f64-c407-4613-b52d-fb2e4f2e55db/%7B558b712f-8c56-42c2-a046-c483e0bfc7d3%7D.js:54:7
[task 2017-04-13T15:09:14.178988Z] 15:09:14     INFO -     promise callback*highlightTab@moz-extension://733f1f64-c407-4613-b52d-fb2e4f2e55db/%7B558b712f-8c56-42c2-a046-c483e0bfc7d3%7D.js:127:13
[task 2017-04-13T15:09:14.180814Z] 15:09:14     INFO -     promise callback*background@moz-extension://733f1f64-c407-4613-b52d-fb2e4f2e55db/%7B558b712f-8c56-42c2-a046-c483e0bfc7d3%7D.js:146:7
[task 2017-04-13T15:09:14.182612Z] 15:09:14     INFO -     promise callback*@moz-extension://733f1f64-c407-4613-b52d-fb2e4f2e55db/%7B558b712f-8c56-42c2-a046-c483e0bfc7d3%7D.js:1:17
[task 2017-04-13T15:09:14.184728Z] 15:09:14     INFO -
Flags: needinfo?(bob.silverberg)
Comment hidden (mozreview-request)
(Assignee)

Comment 20

4 months ago
I bumped up the iterations that are allowed for waiting for the expected number of messages to be received, and everything looks fine on try (although it also looked fine on try before the last push), so let's try landing this again.
Flags: needinfo?(bob.silverberg)

Comment 21

4 months ago
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/32432ad097d8
chrome.tabs.onActivated does not fire for new windows, r=kmag

Comment 22

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/32432ad097d8
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.