581937 - "Recently closed tabs" list keep blank tabs

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Alice0775 White]

User-Agent:       Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; rv:2.0b3pre) Gecko/20100726 Minefield/4.0b3pre ID:20100726040625
Build Identifier: Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; rv:2.0b3pre) Gecko/20100726 Minefield/4.0b3pre ID:20100726040625

Recently closed tabs list keep blank tabs

See http://forums.mozillazine.org/viewtopic.php?p=9676103#p9676103

Reproducible: Always

Steps to Reproduce:
1. Start Minefield
2. Open New Tab
3. Close the New Tab
4. History > Recently Closed Tabs

Actual Results:
 "Recently closed tabs" listed New Tab 

Expected Results:
 Should not

Regression Window:

Works:
http://hg.mozilla.org/mozilla-central/rev/8ec5010204bc
Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; rv:2.0b3pre) Gecko/20100723 Minefield/4.0b3pre ID:20100723155116

Fails:
http://hg.mozilla.org/mozilla-central/rev/6995adf228ed
Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; rv:2.0b3pre) Gecko/20100722 Minefield/4.0b3pre ID:20100723161433

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8ec5010204bc&tochange=6995adf228ed

Candidate bug:
Bug 580819 - Session history confusion after transient about:blank page

Comment 2

[Johnny Stenback (:jst)]

Sounds like a frontend problem...

Comment 3

[Dão Gottwald [:dao]]

Well, somebody should verify this. At the moment we only know it's a regression from bug 580819.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

I looked into this today. This is definitely a regression from bug 580819.

When we open a new tab, we currently always force a transient about:blank content viewer (we do so to ensure that the securityUI is set up; I'll file a bug on looking into that tomorrow). New tabs are furthermore, explicitly told to load about:blank, so we get a *second* about:blank page load (I haven't tested to be sure that we re-use the inner window and avoid some work here, but I'm pretty sure we do). bug 580819 made it so that we successfully insert the real about:blank into session history (note: this is not a "persistant" entry, so we don't end up with additional back entries when the tab is navigated).

The bug here is in part caused by the code that runs when a tab is closed: we look at session history and if its length is > 0, then we serialize all of the history entries, assuming that they are useful. However, in this case, there is exactly one about:blank entry, leading to the change in behavior.

The easiest fix for this would be to not count non-persistant shistory entries when looking at the history for closed tabs or to special-case the "one history entry and it is about:blank" case. Dao, does that sound reasonable to you? Or should I be looking into more docshell stuff?

Comment 5

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

(In reply to comment #4)
> The easiest fix for this would be to not count non-persistant shistory entries
> when looking at the history for closed tabs or to special-case the "one history
> entry and it is about:blank" case. Dao, does that sound reasonable to you? Or
> should I be looking into more docshell stuff?

This wouldn't be the first time we special case about:blank in session restore, so that seems like a reasonable thing to do here, even if it's not the most elegant. I would take either so long as we add a test.

Comment 6

[Dietrich Ayala (:dietrich)]

Blocking+ for regression, and potential data-loss since valid entries will get pushed out of the list.

Comment 11

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

Attachment: Patch v0.1

No test yet. I'm actually surprised we don't already have one for blank tabs...

Comment 13

[Dietrich Ayala (:dietrich)]

Comment on attachment 480688 [details] [diff] [review]
Patch v0.1


>-      // save the window if it has multiple tabs or a single tab with entries
>+      // save the window if it has multiple tabs or a single savable tab

nit: saveable

also, is this a behavior change? right now, i thought we saved/restored a window even if it had just an about:blank tab. it could have recently-closed-tab data, or extension data.

>+   * @param aTabState
>+   *        The current tab state
>+   * @returns true/false

nit: @returns boolean

>+   */
>+  _shouldSaveTabState: function sss__shouldSaveTabState(aTabState) {
>+    // If the tab has only the transient about:blank history entry and no
>+    // userTypedValue, then we don't actually want to store this tab's data.
>+    return aTabState.entries.length &&
>+           !(aTabState.entries.length == 1 &&
>+             aTabState.entries[0].url == "about:blank" &&
>+             !aTabState.userTypedValue);
>+  },

the comment should also mention that the tab has no history.

Comment 15

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

(In reply to comment #13)
> Comment on attachment 480688 [details] [diff] [review]
> Patch v0.1
> 
> 
> >-      // save the window if it has multiple tabs or a single tab with entries
> >+      // save the window if it has multiple tabs or a single savable tab
> 
> nit: saveable
> 
> also, is this a behavior change? right now, i thought we saved/restored a
> window even if it had just an about:blank tab. it could have
> recently-closed-tab data, or extension data.

It's a behavior change from the currently broken behavior. Behavior on 3.6 doesn't save the window unless there were more than 1 tab or a single tab with history:
>       if (winData.tabs.length > 1 ||
>-          (winData.tabs.length == 1 && winData.tabs[0].entries.length > 0)) {

Of course that logic there leads us to save a window with 2 about:blank tabs... Perhaps that should be cleaned up (I'm sure there's a bug like that somewhere)

> >+   */
> >+  _shouldSaveTabState: function sss__shouldSaveTabState(aTabState) {
> >+    // If the tab has only the transient about:blank history entry and no
> >+    // userTypedValue, then we don't actually want to store this tab's data.
> >+    return aTabState.entries.length &&
> >+           !(aTabState.entries.length == 1 &&
> >+             aTabState.entries[0].url == "about:blank" &&
> >+             !aTabState.userTypedValue);
> >+  },
> 
> the comment should also mention that the tab has no history.

I'll make that explicit (it was implied by "only has the transient about:blank history entry")

Comment 17

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Test!

Simple test for the patch.

Comment 18

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

Attachment: Patch v0.2

Updated patch to address those age old comments and actually get review.

Comment 19

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

Comment on attachment 507250 [details] [diff] [review]
Test!

test looks good. thanks for doing it.

Comment 20

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

Pushed http://hg.mozilla.org/mozilla-central/rev/8e99b3137026 and http://hg.mozilla.org/mozilla-central/rev/cf073a3eafdf

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b11pre) Gecko/20110130 Firefox/4.0b11pre