1340739 - currentWindow does not match any tabs in query from sidebar

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P2)

Description

[Andy McKay]

Attachment: sidebar.zip

In a sidebar add-on I do tabs.query({currentWindow: true}), I notice that consistently the first tab is not included. Later tabs that are created are included.

Attached is a simple add-on that (I think) should detect 1 tabs in both cases. However it outputs when loaded in about:debugging as a temporary add-on:

Got 1 tabs from query.
Got 0 tabs from query with currentWindow.

Comment 1

[Andy McKay]

Ok, Kris, you were right :)

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1340739 sidebar window is not the browser window,

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

Comment 3

[Shane Caraveo (:mixedpuppy)]

This happens because the sidebar frame is contained in a xul frame [ie. chrome://browser/content/webext-panels.xul], thus context.currentWindow is the sidebar xul frame.  We need to grab the parent of that.

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8839277 [details]
Bug 1340739 sidebar window is not the browser window,

https://reviewboard.mozilla.org/r/113954/#review115672

::: toolkit/components/extensions/ExtensionParent.jsm:369
(Diff revision 1)
> +    if (this.viewType == "sidebar") {
> +      return this.xulBrowser.ownerGlobal.document.parentWindow;
> +    }
>      return this.xulBrowser.ownerGlobal;

Let's just always return the root window:

    win.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDocShell);
       .QueryInterface(nsIDocShellTreeItem).rootTreeItem
       .QueryInterface(nsIInterfaceRequestor).getInterface(nsIDOMWindow);

Comment 5

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8839277 [details]
Bug 1340739 sidebar window is not the browser window,

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

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8839277 [details]
Bug 1340739 sidebar window is not the browser window,

https://reviewboard.mozilla.org/r/113954/#review115718

::: browser/components/extensions/test/browser/browser_ext_sidebarAction.js:155
(Diff revision 2)
> +      `,
> +      "sidebar.js": function() {
> +        Promise.all([
> +          browser.tabs.query({}).then((tabs) => {
> +            browser.test.assertEq(1, tabs.length, "got tab without currentWindow");
> +            return true;

No need for either `return true`.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8839277 [details]
Bug 1340739 sidebar window is not the browser window,

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ad2f410d2e7
sidebar window is not the browser window, r=kmag

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3ad2f410d2e7