Closed Bug 1128060 Opened 10 years ago Closed 9 years ago

Torn-off tabs cannot be session-restored

Categories

(Firefox :: Session Restore, defect)

x86
macOS
defect
Not set
normal
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 39
Iteration:
39.2 - 23 Mar
Tracking Status
e10s m6+ ---
firefox39 --- fixed

People

(Reporter: erik, Assigned: mossop)

References

Details

Attachments

(1 file)

I encountered this only after switching to e10s, so maybe it's related.

1. Open a page in a new tab.
2. Tear that tab off the window to make it its own window.
3. Close the new window.
4. Hit command-shift-N to restore the window.
5. Leap from your chair in surprise as a window from your deep past springs forth. The torn-off tab is gone forever, making this actually a data-loss bug.

In fact, the torn-off tab makes it into neither the Recently Closed Tabs nor the Recently Closed Windows submenus; it's falling through the cracks somewhere.

This is in the 2015-01-26 Nightly.
I saw this on Windows 8.1 as well, using the latest nightly.
tracking-e10s: --- → ?
Flags: firefox-backlog+
Assignee: nobody → mconley
Points: --- → 3
Flags: qe-verify-
Assignee: mconley → dtownsend
Iteration: --- → 39.1 - 9 Mar
Status: NEW → ASSIGNED
Attached patch patchSplinter Review
Switches to setting _permanentKey rather than permanentKey which overrides the property on the as yet unbound XBL. This also allows us to revert the change in bug 1072198 that made permanentKey a read/write property.
Attachment #8575322 - Flags: review?(wmccloskey)
Iteration: 39.1 - 9 Mar → 39.2 - 23 Mar
Comment on attachment 8575322 [details] [diff] [review]
patch

Review of attachment 8575322 [details] [diff] [review]:
-----------------------------------------------------------------

Great catch! (Just giving some test feedback.)

::: browser/base/content/test/general/browser_tab_detach_restore.js
@@ +1,1 @@
> +add_task(function*() {

Nit: "use strict";

@@ +15,5 @@
> +  yield new Promise(resolve => whenDelayedStartupFinished(win, resolve));
> +
> +  is(win.gBrowser.selectedBrowser.permanentKey, key, "Should have properly copied the permanentKey");
> +  win.close();
> +  yield promiseWaitForFocus(window);

yield promiseWindowClosed(win); ?

@@ +21,5 @@
> +  is(SessionStore.getClosedWindowCount(), 1, "Should have restore data for the closed window");
> +
> +  win = SessionStore.undoCloseWindow(0);
> +  yield BrowserTestUtils.waitForEvent(win, "load");
> +  yield BrowserTestUtils.browserLoaded(win.gBrowser.tabs[0].linkedBrowser);

I think we should rather wait for the initial tab's SSTabRestored event.

@@ +27,5 @@
> +  is(win.gBrowser.tabs.length, 1, "Should have restored one tab");
> +  is(win.gBrowser.selectedBrowser.currentURI.spec, uri, "Should have restored the right page");
> +
> +  win.close();
> +  yield promiseWaitForFocus(window);

yield promiseWindowClosed(win);
Attachment #8575322 - Flags: feedback+
Comment on attachment 8575322 [details] [diff] [review]
patch

Review of attachment 8575322 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser_tab_detach_restore.js
@@ +7,5 @@
> +
> +  let tab = gBrowser.addTab();
> +  let loadPromise = BrowserTestUtils.browserLoaded(tab.linkedBrowser);
> +  tab.linkedBrowser.loadURI(uri);
> +  yield loadPromise;

We really need to find the right pattern for tests to add a tab with a given URL and wait for it to load. The problem with this version, I think, is that if about:blank loads really quickly then you'll get a load event for that instead of for the intended URI. I'm not sure how likely this is, but it seems possible according to my understanding. I saw a similar problem in bug 1132566 too. Maybe you could talk to Steven MacLeod and figure out something to add to BrowserTestUtils?
Attachment #8575322 - Flags: review?(wmccloskey) → review+
Looks like this wasn't at fault: https://hg.mozilla.org/integration/fx-team/rev/5a79c295029a
Flags: needinfo?(dtownsend)
https://hg.mozilla.org/mozilla-central/rev/5a79c295029a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Depends on: 1143118
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: