1493343 - tab left over after failed tab move between PB/nonPB

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement, P2)

Description

[Oriol Brufau [:Oriol]]

Run this code in an extension with the "tabs" permission:

async function createTab(params) {
  let tab = await browser.tabs.create(params);
  if (params.url != null && params.url !== "about:blank" && tab.url === "about:blank") {
    let url = new URL(params.url, location.href);
    await new Promise(resolve => {
      browser.tabs.onUpdated.addListener(function listener(changedId, changed) {
        if (tab.id === changedId && url.href === changed.url) {
          browser.tabs.onUpdated.removeListener(listener);
          resolve();
        }
      });
    });
  }
  return tab;
}
(async function() {
  let url = "http://example.com/";
  let {id: normalWindow} = await browser.windows.create({incognito: false});
  let {id: privateWindow} = await browser.windows.create({incognito: true});
  let {id: tabId} = await createTab({windowId: normalWindow, url, index: 1});
  await browser.tabs.move(tabId, {windowId: privateWindow, index: 1});
  console.log("Tab by index in old window", (await browser.tabs.query({windowId: normalWindow, index: 1}))[0]);
  console.log("Tab by index in new window", (await browser.tabs.query({windowId: privateWindow, index: 1}))[0]);
  console.log("Tab by ID", await browser.tabs.get(tabId));
  browser.windows.remove(normalWindow);
  browser.windows.remove(privateWindow);
})();

Result: tabs.move creates a tab in the private window, and attempts to make it adopt the tab from the normal window. And this fails.

So you end up with a new about:blank tab in the private window, the original tab remains in the normal window, both tabs have the same ID, and the lack of ID uniqueness breaks various assumptions which can make things go so wrong.

I think tabs.move should just reject if the specified tabId and windowId have different private modes.

Comment 1

[Shane Caraveo (:mixedpuppy)]

This is a bug in tabbrowser.  The actual tab is not moved between private and non-private browsers.  However, a new about:blank tab is created[1] to do the docswap, and is never cleaned up if docswap doesn't happen[2].


[1] https://searchfox.org/mozilla-central/rev/3c85ea2f8700ab17e38b82d77cd44644b4dae703/browser/base/content/tabbrowser.js#3700
[2] https://searchfox.org/mozilla-central/rev/3c85ea2f8700ab17e38b82d77cd44644b4dae703/browser/base/content/tabbrowser.js#3140

Comment 2

[Rob Wu [:robwu]]

We should not attempt to move tabs between windows with a different PBM state. That sounds like a bug in the extension implementation.

The leftover blank tab sounds like a bug, but then again I can see the argument that callers should not attempt to move tabs to a window with a different PBM state.

In any case, we should (at an extension API level) validate that a tab move is allowed before attempting to move it.
We already so so when the windows.create API is used to adopt an existing tab, we should also validate PBM state in tabs.move.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Adding additional validation in tabs.move is fine, but it's more expensive for some future consumer to spend time looking at this again than it is to address the problem in the base implementation.

Comment 4

[Dão Gottwald [:dao]]

FYI: Setting a priority when moving a bug to a different component means that it likely won't be triaged, and then nobody might work on it. Next time, please drop any previously set priority when moving a bug.

Comment 5

[Dão Gottwald [:dao]]

This doesn't seem to have anything to do with bug 1392352.

Comment 6

[Dão Gottwald [:dao]]

Attachment: Bug 1493343 - Make adoptTab remove the new tab if the original tab couldn't be adopted. r?mixedpuppy

Comment 7

[Dão Gottwald [:dao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=75989d16f0fad31dfd072acb3c4aa035556f46ef

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/190cbcbf2dd2
Make adoptTab remove the new tab if the original tab couldn't be adopted. r=mixedpuppy

Comment 9

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/190cbcbf2dd2