Closed Bug 1342207 Opened 7 years ago Closed 7 years ago

chrome.tabs.onActivated does not fire for new windows

Categories

(WebExtensions :: Frontend, defect, P3)

Product:
WebExtensions
Component:
Frontend
Version:
51 Branch
Type:
defect

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed
Blocking Flags:
webextensions +

People

(Reporter: adam.schenker, Assigned: bsilverberg)

Details

(Whiteboard: [triaged])

Attachments

(2 files)

Using the debugger, this extension can be used to test the behavior described
1.49 KB, application/zip
Details
Bug 1342207 - chrome.tabs.onActivated does not fire for new windows,
59 bytes, text/x-review-board-request
kmag
: review+
Details 
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.
Component: Untriaged → WebExtensions: Frontend
Product: Firefox → Toolkit
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.
Priority: -- → P3
Whiteboard: [triaged]
webextensions: --- → ?
webextensions: ? → +
Assignee: nobody → bob.silverberg
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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?
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)
Please fix the existing code so onActivated always fires after onCreated.

Thanks.
Flags: needinfo?(kmaglione+bmo)
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+
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 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 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+
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)
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)
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/32432ad097d8
chrome.tabs.onActivated does not fire for new windows, r=kmag
https://hg.mozilla.org/mozilla-central/rev/32432ad097d8
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: