Closed
Bug 1493343
Opened 6 years ago
Closed 6 years ago
tab left over after failed tab move between PB/nonPB
Categories
(Firefox :: Tabbed Browser, enhancement, P2)
Tracking
()
RESOLVED
FIXED
Firefox 64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: Oriol, Assigned: dao)
Details
Attachments
(1 file)
Bug 1493343 - Make adoptTab remove the new tab if the original tab couldn't be adopted. r?mixedpuppy
46 bytes,
text/x-phabricator-request
|
Details | Review |
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.
Updated•6 years ago
|
Priority: -- → P1
Updated•6 years ago
|
Assignee: nobody → mixedpuppy
Comment 1•6 years ago
|
||
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
Comment 2•6 years ago
|
||
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•6 years ago
|
||
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.
Assignee | ||
Comment 4•6 years ago
|
||
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 | ||
Updated•6 years ago
|
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•6 years ago
|
||
This doesn't seem to have anything to do with bug 1392352.
No longer blocks: 1392352
Assignee | ||
Comment 6•6 years ago
|
||
Assignee | ||
Comment 7•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=75989d16f0fad31dfd072acb3c4aa035556f46ef
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
Comment 9•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/190cbcbf2dd2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
You need to log in
before you can comment on or make changes to this bug.
Description
•