Closed Bug 1493343 Opened 11 months ago Closed 11 months ago

tab left over after failed tab move between PB/nonPB

Categories

(Firefox :: Tabbed Browser, enhancement, P2)

63 Branch
enhancement

Tracking

()

RESOLVED FIXED
Firefox 64
Tracking Status
firefox64 --- fixed

People

(Reporter: Oriol, Assigned: dao)

Details

Attachments

(1 file)

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.
Priority: -- → P1
Assignee: nobody → 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
Assignee: mixedpuppy → nobody
Blocks: 1392352
Component: Frontend → Tabbed Browser
Priority: P1 → P2
Product: WebExtensions → Firefox
Summary: tabs.move should refuse to move non-private tab to private window or viceversa → tab left over after failed tab move between PB/nonPB
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.
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.
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.
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
This doesn't seem to have anything to do with bug 1392352.
No longer blocks: 1392352
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/190cbcbf2dd2
Make adoptTab remove the new tab if the original tab couldn't be adopted. r=mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/190cbcbf2dd2
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
You need to log in before you can comment on or make changes to this bug.