367052 - [SessionStore] about:blank shows up in back arrow history of restored blank tabs

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Michael Kraft [:morac]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1

I emailed Zeniko and he said this may be a duplicate, but he couldn't find the original bug and since I recreated it in the latest Minefield nightly I decided to file it.  

If a session that contains blank tabs is restored then when the blank tabs are used to go to web sites, the user can use the back arrow on those tabs to go back to the original "about:blank" page.  In addition if the tabs that started out as blank are closed, they are not saved and cannot be restored.  

The "about:blank" does not show up under the History menu, only in the back arrow menu.

The not saving on close problem appears to be because SessionStore believes that the tab's URL is still "about:blank" even though it no longer is so it doesn't save the tab.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/sessionstore/src/nsSessionStore.js&rev=1.58&mark=554#541

I think the real problem is that the "about:blank" should not be being saved in the history.  This only occurs when a session is restored and only for restored tabs that are blank (about:blank).

The Session Manager extension can also be used to save and restore sessions manually and they have the same problem so the problem appears to be on the restore.  For some reason the restored blank tabs aren't being treated as such.

Reproducible: Always

Steps to Reproduce:
1. Open Firefox
2. Change browser.sessionstore.resume_session_once preference to true.
3. Close all tabs and open 3 blank tabs.
4. On one of the blank tabs go to a web site (eg: www.google.com) and leave other 2 blank.
5. Close Browser and open it again
6. Use the 2 blank tabs to surf to sites.
7. Close the two previously blank tabs.
Actual Results:  
After step 6 tabs had "about:blank" in their back arrow menu list.
Tabs not in recently closed tab list in history menu. 

Expected Results:  
After step 6 tabs should not have "about:blank" in their back arrow menu list.
Tabs should be in recently closed tab list in history menu. 

The problem does not occur if the Tab Mix Plus extension is installed.

Problem also occurred in Minefield version (Gecko/20070115 Minefield/3.0a2pre)

Comment 1

[Michael Kraft [:morac]]

Attachment: Pre-created sessionstore.js file, just drop in profile

A premade sessionstore.js file.  Just close your browser, drop this in your profile and open up and select to resume crashed session.  Surf with the blank tabs and closed them.

Comment 2

[Simon Bünzli]

The first half of this bug (restored and then used blank tabs can't be reopened) will be fixed in bug 369151.

I haven't figured out what the best way to solve the base issue (about:blank is saved/restored as history entry) would be though. My preference would be to drop a leading about:blank when we restore a tab; depending on where exactly it is dropped, several tacit assumptions along the code path would have to be corrected (e.g. we depend on any page being loaded in the tab for knowing when it's fully restored).

One solution might be to drop about:blank in sss_restoreHistory if it's not the only history entry - not forgetting to adjust the history index - and otherwise to purge it right before the SSTabRestored event is dispatched (that's about as late in the game as possible), not forgetting to consider that in the meantime another page might already have been loaded on top of it.

Comment 3

[Simon Bünzli]

Attachment: fix tabData.entries handling

This patch makes tabData.entries be an empty Array for new (blank) tabs so that it's correctly restored without history entries. It furthermore fixes another possible issue with history breakage when sessionHistory.gotoIndex is handed an invalid index.

Comment 4

[Dietrich Ayala (:dietrich)]

Comment on attachment 334167 [details] [diff] [review]
fix tabData.entries handling


>     // store closed-tab data for undo
>-    if (tabState.entries.length > 1 || tabState.entries[0].url != "about:blank") {
>+    if (tabState.entries.length > 0) {
>       this._windows[aWindow.__SSi]._closedTabs.unshift({
>         state: tabState,
>         title: aTab.getAttribute("label"),
>         image: aTab.getAttribute("image"),
>         pos: aTab._tPos
>       });
>       var length = this._windows[aWindow.__SSi]._closedTabs.length;
>       if (length > maxTabsUndo)

wouldn't this cause about:blank tabs w/ no history to show up in the menu?

>@@ -1486,17 +1492,17 @@ SessionStoreService.prototype = {
>           aWindow.setTimeout(restoreHistoryFunc, 100, this);
>           return;
>         }
>       }
>     }
>     
>     // mark the tabs as loading
>     for (t = 0; t < aTabs.length; t++) {
>-      if (!aTabs[t].entries || !aTabs[t].entries[0])
>+      if (!aTabs[t].entries || aTabs[t].entries.length == 0)
>         continue; // there won't be anything to load
>       

or !aTabs[t].entries.length

looks ok otherwise. though it does scare me that changes like these are not covered by automation yet. this codepath could be executed post-startup by using the API to restore serialized tab state. can you please see if that's a viable route for testing this?

Comment 5

[Simon Bünzli]

(In reply to comment #4)
> wouldn't this cause about:blank tabs w/ no history to show up in the menu?

No, as this patch causes about:blank tabs without history and content not to have .entries at all.

> >+      if (!aTabs[t].entries || aTabs[t].entries.length == 0)
> or !aTabs[t].entries.length

We never use .length as a boolean value in nsSessionStore and I'd prefer to keep it like that.

> though it does scare me that changes like these are not covered by
> automation yet. this codepath could be executed post-startup by
> using the API to restore serialized tab state.

I'll see if I can wipe up something with a resemblance of a test...

Comment 6

[Simon Bünzli]

Attachment: unbitrotted and with tests

Comment 7

[Dietrich Ayala (:dietrich)]

Comment on attachment 334529 [details] [diff] [review]
unbitrotted and with tests

r=me, thanks

Comment 8

[Reed Loden [:reed]]

Pushed as 17137:f468ae1633d4.

Comment 9

[Dave Camp (:dcamp)]

This patch was causing failures in chrome tests:

*** 18 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/browser/components/sessionstore/test/chrome/test_bug393716.xul | Got a valid state object?
*** 19 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/browser/components/sessionstore/test/chrome/test_bug393716.xul | Got the expected state object (test URL)?

See http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1219223597.1219228278.9278.gz&fulltext=1

The patch has been backed out, reopening.

Comment 10

[Simon Bünzli]

Attachment: fixed the faulty test

Right, one of our chrome tests makes the wrong assumptions here. That test should be updated to use non-blank tabs for testing, anyway (in a dependency of bug 426045), so marking it as TODO for now.

Comment 11

[Simon Bünzli]

Sorry for the test failure. I've filed bug 451366 about fixing our chrome tests.

Comment 12

[Simon Bünzli]

Attachment: fixed the test and guarded again against page load errors

Comment 13

[Dão Gottwald [:dao]]

patch doesn't apply

Comment 14

[Simon Bünzli]

Attachment: unbitrotted for check-in

Comment 15

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/172a08c1ce8e

Comment 16

[Michael Kohler [:mkohler]]

Verified with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090617 Minefield/3.6a1pre ID:20090617031528

Comment 17

[u279076]

in-litmus+
https://litmus.mozilla.org/show_test.cgi?id=7795

Comment 18

[Charles Evans]

will this make it back to 3.5? 3.0? Please?
still losing data...

Comment 19

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #18)
> will this make it back to 3.5? 3.0? Please?
> still losing data...

The fix for this was in Firefox 3.5. If you're seeing problems, please file a new bug with symptoms & steps to reproduce.