Closed Bug 1310019 Opened 7 years ago Closed 7 years ago

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

Categories

(WebExtensions :: General, defect, P2)

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

Tracking

(firefox51 verified, firefox52 verified)

Status:
VERIFIED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox51 --- verified
firefox52 --- verified

People

(Reporter: me, Assigned: bsilverberg)

Details

(Keywords: regression, Whiteboard: triaged[browserAction])

Attachments

(2 files)

bug-ff.zip
981 bytes, application/zip
Details
Bug 1310019 - web extension API : chrome.tabs.query doesn’t get tabs title at first call,
58 bytes, text/x-review-board-request
kmag
: review+
gchang
: approval-mozilla-aurora+
Details
Attached file 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.
That behavior is new , that "bug" didn’t occurred in previous Firefox version (about 2~3 weeks ago).
Component: Untriaged → WebExtensions: General
Product: Firefox → Toolkit
Status: UNCONFIRMED → NEW
Ever confirmed: true
Please also note that I cannot reproduce the issue while debugging the extension (title is always filled)...
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Whiteboard: triaged
Whiteboard: triaged
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
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(kmaglione+bmo)
Priority: -- → P2
Whiteboard: triaged[browserAction]
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.
Attachment #8806451 - Flags: review?(kmaglione+bmo)
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!
Attachment #8806451 - Flags: review?(kmaglione+bmo) → review+
This looks good on try, requesting check in.
Keywords: checkin-needed
Autoland couldn't rebase this for landing.
Thanks Ryan. I have rebased against central. It should be good to land now.
Keywords: checkin-needed
Pushed by philringnalda@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5ec9dc4ea6e9
web extension API : chrome.tabs.query doesn’t get tabs title at first call, r=kmag
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5ec9dc4ea6e9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Bob, this was introduced in 51 by bug 1259093, so can you please request uplift?

Thanks
status-firefox51: --- → affected
Keywords: regression
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.
Attachment #8806451 - Flags: approval-mozilla-aurora?
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.
Attachment #8806451 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
Needs rebasing for Aurora.
Flags: needinfo?(bob.silverberg)
I rebased against Aurora and pushed a new patch to MozReview.
Clearing my needinfo.
Flags: needinfo?(bob.silverberg)
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
status-firefox51: fixed → verified
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.