1281354 - Add test that current window is correct after tab is detached

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[Hans Meyer]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.84 Safari/537.36

Steps to reproduce:

1) Add this code to an extension's options.html:

setInterval(function() {
    browser.windows.getCurrent(function(win) {
        console.log('current window id: ', win.id);
    });
}, 1000);

2) Ensure manifest includes:

"options_ui": {
    "page": "options.html",
    "open_in_tab": true
  }

3) Load extension and open Browser Toolbox

4) Open extension's options page from about:addons by clicking on extension's Options button

5) Observe "current window id" logs in Browser Toolbox console

6) Detach options page tab into a new or alternate window

7) Observe "current window id" logs in Browser Toolbox console for changes...


Actual results:

In step 7 above, the logged "current window id" DOES NOT CHANGE


Expected results:

In step 7 above, the logged "current window id" should have updated to reflect the window id of the new/alternate window the tab was attached to.

The reported current window should be the window that contains the code that is currently executing. See: https://developer.chrome.com/extensions/windows#current-window

See also related Issue 1202479

Note that this affects the browser.tabs API as well. Eg, browser.tabs.query({ currentWindow: true }, function(tabs) {})

Comment 1

[Kris Maglione [:kmag]]

This should be pretty easy to fix. We just need to look up the current parent window directly here[1], rather than caching it here[2]. We should also add some tests for this to the existing browser_tabs_move_window*.js tests.

[1]: https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-tabs.js#98
[2]: https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-tabs.js#66

Comment 2

[Atique Ahmed Ziad [:atiqueahmedziad]]

Hello,

can I work on this bug?

Comment 3

[Mike Hoye [:mhoye]]

Yes, absolutely! You can learn how to get set up to to work on it here:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

and please let us know in this bug (or you can email me directly) if you've got any questions.

Comment 4

[Anagh]

Hi ,
Is the bug is still unresolved if yes can i work on this bug ?

Comment 5

[Mike Hoye [:mhoye]]

I think that Atique might be working on this one already. Atique, is that the case? 

Anagh, if you'd like to email me - mhoye at mozilla.com - I can help you find a bug to work on.

Comment 6

[Kris Maglione [:kmag]]

(In reply to Anagh from comment #4)
> Hi ,
> Is the bug is still unresolved if yes can i work on this bug ?

It might be a good idea to start with the instructions that Mike left in comment 3, anyway. If Atique is indeed still working on this, I'm sure we can find something else for you to work on once you're up and running.

Comment 7

[Atique Ahmed Ziad [:atiqueahmedziad]]

Thanks Mike for the instructions.
Yes, I am trying to build firefox locally. I will update you within next week.

Comment 8

[Tomislav Jovanovic :zombie]

(In reply to Atique Ahmed Ziad [:atiqueahmedziad] from comment #7)
> Yes, I am trying to build firefox locally. I will update you within next
> week.

Hey Atique, are you still planning to work on this? Do you need some help?

Comment 9

[Tomislav Jovanovic :zombie]

It looks like this got fixed by bug 1285493, but a test could still be useful here.

Comment 10

[Tomislav Jovanovic :zombie]

Attachment: bug 1281354 - test current window correct after moving tab,

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

Comment 11

[Tomislav Jovanovic :zombie]

note that this test depends on windows.create() waiting for browser-delayed-startup-finished before resolving, which is fixed by my patch in bug 1273146.

Comment 12

[Andrew Swan [:aswan]]

Comment on attachment 8799526 [details]
bug 1281354 - test current window correct after moving tab,

https://reviewboard.mozilla.org/r/84670/#review83818

looks good, with a couple little nitpicks.  thanks!

::: browser/components/extensions/test/browser/browser_ext_tabs_move_window.js:66
(Diff revision 1)
> +    const url = browser.extension.getURL("current.html");
> +    browser.tabs.create({url}).then(tab => {
> +      tabId = tab.id;
> +    });
> +    browser.test.onMessage.addListener(msg => {
> +      if (msg === "create") {

can you call this "move" just to be more clear

::: browser/components/extensions/test/browser/browser_ext_tabs_move_window.js:92
(Diff revision 1)
> +  yield extension.awaitMessage("moved");
> +
> +  extension.sendMessage("current");
> +  const second = yield extension.awaitMessage("id");
> +
> +  ok(first !== second, "current window id is different after moving the tab");

you can just do `isnot(first, second, "...")`

Comment 13

[Tomislav Jovanovic :zombie]

Comment on attachment 8799526 [details]
bug 1281354 - test current window correct after moving tab,

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

Comment 14

[Tomislav Jovanovic :zombie]

Comment on attachment 8799526 [details]
bug 1281354 - test current window correct after moving tab,

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/db47a06d14eb
test current window correct after moving tab, r=aswan

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/db47a06d14eb