1310019 - web extension API : chrome.tabs.query doesn’t get tabs title at first call

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P2)

Description

[Grégory Paul]

Attachment: bug-ff.zip

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20161013004015

Steps to reproduce:

I’m using Firefox Linue 51.0a2 (2016-10-13) (64-bit).

I’m building an extension which requires current tab title.
I’m using web extension API like that : 

document.addEventListener('DOMContentLoaded', () => {
    chrome.tabs.query({active: true, currentWindow: true}, (tabs) => {
        // do something with `tabs[0].title;`
    });
});



Actual results:

tabs[0].title is undefined the first time the extension is opened. 
The second time it works fine (it contains the tab title).



Expected results:

tabs[0].title should always contains the tab’s title.

I build a basic extension which illustrate the bug.
In Firefox 51, The first time the extension is opened, tabs[0].title is undefined so the textfield is empty, the second click opening again the extension, tabs[0].title is filled.

Comment 1

[Grégory Paul]

That behavior is new , that "bug" didn’t occurred in previous Firefox version (about 2~3 weeks ago).

Comment 2

[Grégory Paul]

Please also note that I cannot reproduce the issue while debugging the extension (title is always filled)...

Comment 3

[Bob Silverberg [:bsilverberg]]

I have looked into this and have figured out what is causing it, but I'm not sure what the fix is.

The problem is that when a user clicks on the browserAction icon with the mouse, the “mousedown” handler [1] fires, which then calls `getPopup` which creates the html page for the popup. If there is JavaScript attached to that page, it runs immediately, i.e., before `onViewShowing` has a chance to run, and it is in `onViewShowing` [2] that we call `this.tabManager.addActiveTabPermission(tab)` in order to add the permission to the active tab.

This means that when the script in the browserAction popup calls `chrome.tabs.query({active: true, currentWindow: true}`, the permission has not been added to the active tab yet, and therefore when the `convert` function is run on the tab, it does not assign a title (or a url or a favIconUrl for that matter) [3]. 

This does seem like something that should be fixed. Do you have any suggestions for an approach, Kris?

[1] http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-browserAction.js#190
[2] http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-browserAction.js#121
[3] http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-utils.js#650

Comment 4

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1310019 - web extension API : chrome.tabs.query doesn’t get tabs title at first call,

Add permission to the active tab on mousedown in ext-browserAction.js

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

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8806451 [details]
Bug 1310019 - web extension API : chrome.tabs.query doesn’t get tabs title at first call,

https://reviewboard.mozilla.org/r/89884/#review89468

::: browser/components/extensions/ext-browserAction.js:199
(Diff revision 1)
>            let tab = window.gBrowser.selectedTab;
>            let popupURL = this.getProperty(tab, "popup");
>  
> +          // Add permission for the active tab so it will exist for the popup.
> +          // Keep track of whether we need to revoke the permission during clearPopup.
> +          this.revokeTabPermissionOnClearPopup = !this.tabManager.hasActiveTabPermission(tab);

We should probably store the tab, just to be safe. And only do this if there isn't already a pending popup.

::: browser/components/extensions/ext-browserAction.js:200
(Diff revision 1)
>            let popupURL = this.getProperty(tab, "popup");
>  
> +          // Add permission for the active tab so it will exist for the popup.
> +          // Keep track of whether we need to revoke the permission during clearPopup.
> +          this.revokeTabPermissionOnClearPopup = !this.tabManager.hasActiveTabPermission(tab);
> +          if (this.revokeTabPermissionOnClearPopup) {

No need for this check.

::: browser/components/extensions/ext-browserAction.js:269
(Diff revision 1)
>     */
>    clearPopup() {
>      this.clearPopupTimeout();
>      if (this.pendingPopup) {
> +      if (this.revokeTabPermissionOnClearPopup) {
> +        this.tabManager.revokeActiveTabPermission();

We should pass a tab here.

::: browser/components/extensions/ext-utils.js:603
(Diff revision 1)
>        this.hasTabPermissionFor.set(tab, tab.linkedBrowser.innerWindowID);
>      }
>    },
>  
> +  revokeActiveTabPermission(tab = TabManager.activeTab) {
> +    if (this.hasTabPermissionFor.has(tab)) {

No need for this check.

::: browser/components/extensions/test/browser/browser_ext_browserAction_popup.js:309
(Diff revision 1)
> +    },
> +
> +    files: {
> +      "popup.html": scriptPage("popup.js"),
> +      "popup.js": function() {
> +        window.onload = () => {

No need to wait for the load event. In fact, it's better if we don't.

::: browser/components/extensions/test/browser/browser_ext_browserAction_popup.js:310
(Diff revision 1)
> +
> +    files: {
> +      "popup.html": scriptPage("popup.js"),
> +      "popup.js": function() {
> +        window.onload = () => {
> +          chrome.tabs.query({active: true, currentWindow: true}, (tabs) => {

s/chrome/browser/, and please use the promise-based API.

::: browser/components/extensions/test/browser/browser_ext_browserAction_popup.js:311
(Diff revision 1)
> +    files: {
> +      "popup.html": scriptPage("popup.js"),
> +      "popup.js": function() {
> +        window.onload = () => {
> +          chrome.tabs.query({active: true, currentWindow: true}, (tabs) => {
> +            browser.test.assertTrue(tabs[0].title, "Tab has a title on first click");

We should check that we get the correct title.

::: browser/components/extensions/test/browser/browser_ext_browserAction_popup.js:326
(Diff revision 1)
> +  yield BrowserTestUtils.browserLoaded(win.gBrowser.selectedBrowser);
> +
> +  yield extension.startup();
> +
> +  let widget = getBrowserActionWidget(extension).forWindow(win);
> +  EventUtils.synthesizeMouseAtCenter(widget.node, {}, win);

This is pretty racy. We should send only a mousedown and wait for the response before sending a mouseup.

It would probably be good to test that active tab permissions get revoked if the popup is canceled, too.

Comment 6

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8806451 [details]
Bug 1310019 - web extension API : chrome.tabs.query doesn’t get tabs title at first call,

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

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8806451 [details]
Bug 1310019 - web extension API : chrome.tabs.query doesn’t get tabs title at first call,

https://reviewboard.mozilla.org/r/89884/#review89928

Perfect. Thanks!

Comment 8

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8806451 [details]
Bug 1310019 - web extension API : chrome.tabs.query doesn’t get tabs title at first call,

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

Comment 9

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8806451 [details]
Bug 1310019 - web extension API : chrome.tabs.query doesn’t get tabs title at first call,

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

Comment 10

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8806451 [details]
Bug 1310019 - web extension API : chrome.tabs.query doesn’t get tabs title at first call,

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

Comment 11

[Bob Silverberg [:bsilverberg]]

This looks good on try, requesting check in.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Autoland couldn't rebase this for landing.

Comment 13

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8806451 [details]
Bug 1310019 - web extension API : chrome.tabs.query doesn’t get tabs title at first call,

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

Comment 14

[Bob Silverberg [:bsilverberg]]

Thanks Ryan. I have rebased against central. It should be good to land now.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ec9dc4ea6e9
web extension API : chrome.tabs.query doesn’t get tabs title at first call, r=kmag

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5ec9dc4ea6e9

Comment 17

[Kris Maglione [:kmag]]

Bob, this was introduced in 51 by bug 1259093, so can you please request uplift?

Thanks

Comment 18

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8806451 [details]
Bug 1310019 - web extension API : chrome.tabs.query doesn’t get tabs title at first call,

Approval Request Comment
[Feature/regressing bug #]: Bug 1259093
[User impact if declined]: This will cause extension popups which run code to query the tabs to not retrieve the title or the favIcon the first time they are clicked.
[Describe test coverage new/current, TreeHerder]:The related features are
covered by extensive tests, and this patch adds coverage for this issue.
[Risks and why]: Low risk. This code only affects the mousedown behaviour of WebExtensions browserActions and is covered well by tests.
[String/UUID change made/needed]: None.

Comment 19

[Gerry Chang [:gchang]]

Comment on attachment 8806451 [details]
Bug 1310019 - web extension API : chrome.tabs.query doesn’t get tabs title at first call,

Fix an issue related to webextension API. Take it in 51 aurora.

Comment 20

[CosminB]

I can reproduce this issue on Firefox 51.0a2 (20161013004015) under Windows 7 64-bit and Ubuntu 12.04 LTS 64-bit , the title of the tab is not displayed when the extension is opened for the first time here is a video: http://screencast.com/t/tOBBgDNcmrY 


This issue is verified as fixed on Firefox 52.0a1 (20161107030203) under Windows 7 64-bit and Ubuntu 12.04 LTS 64-bit, the title of the tab is displayed when the extension is opened for the first time here is a video: http://screencast.com/t/j2zsWhLTED

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Aurora.

Comment 22

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8806451 [details]
Bug 1310019 - web extension API : chrome.tabs.query doesn’t get tabs title at first call,

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

Comment 23

[Bob Silverberg [:bsilverberg]]

I rebased against Aurora and pushed a new patch to MozReview.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1227a6641452

Comment 25

[Bob Silverberg [:bsilverberg]]

Clearing my needinfo.

Comment 26

[CosminB]

This issue is verified as fixed on Firefox 51.0a2 (20161114004005) under Windows 7 64-bit and Ubuntu 16.04 LTS 32-bit, the title of the tab is displayed when the extension is opened for the first time. 
Here is a video: http://screencast.com/t/wEhTf87t0sW