Closed Bug 581937 Opened 9 years ago Closed 9 years ago

"Recently closed tabs" list keep blank tabs

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 4.0b11
Tracking Status
blocking2.0 --- final+

People

(Reporter: alice0775, Assigned: zpao)

References

Details

(Keywords: regression, Whiteboard: [softblocker])

Attachments

(2 files, 1 obsolete file)

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
blocking2.0: --- → ?
Duplicate of this bug: 582917
Flags: in-testsuite?
Sounds like a frontend problem...
Component: Document Navigation → Tabbed Browser
Product: Core → Firefox
QA Contact: docshell → tabbed.browser
Well, somebody should verify this. At the moment we only know it's a regression from bug 580819.
Component: Tabbed Browser → Session Restore
QA Contact: tabbed.browser → session.restore
OS: Windows 7 → All
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?
(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.
Blocking+ for regression, and potential data-loss since valid entries will get pushed out of the list.
blocking2.0: ? → final+
Duplicate of this bug: 584645
Duplicate of this bug: 585367
Duplicate of this bug: 590076
Duplicate of this bug: 592272
Attached patch Patch v0.1 (obsolete) — Splinter Review
No test yet. I'm actually surprised we don't already have one for blank tabs...
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #480688 - Flags: feedback?(dietrich)
Hardware: x86 → All
Whiteboard: [has patch][needs test][needs feedback 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.
Attachment #480688 - Flags: feedback?(dietrich) → feedback+
Whiteboard: [has patch][needs test][needs feedback dietrich] → [has patch][needs test]
Duplicate of this bug: 607337
(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")
Duplicate of this bug: 624689
Whiteboard: [has patch][needs test] → [has patch][needs test][softblocker]
Attached patch Test!Splinter Review
Simple test for the patch.
Attachment #507250 - Flags: review?(paul)
Attached patch Patch v0.2Splinter Review
Updated patch to address those age old comments and actually get review.
Attachment #480688 - Attachment is obsolete: true
Attachment #507252 - Flags: review?(dietrich)
Whiteboard: [has patch][needs test][softblocker] → [softblocker][needs review dietrich]
Comment on attachment 507250 [details] [diff] [review]
Test!

test looks good. thanks for doing it.
Attachment #507250 - Flags: review?(paul) → review+
Attachment #507252 - Flags: review?(dietrich) → review+
Pushed http://hg.mozilla.org/mozilla-central/rev/8e99b3137026 and http://hg.mozilla.org/mozilla-central/rev/cf073a3eafdf
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Whiteboard: [softblocker][needs review dietrich] → [softblocker]
Target Milestone: --- → Firefox 4.0b11
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b11pre) Gecko/20110130 Firefox/4.0b11pre
Status: RESOLVED → VERIFIED
Duplicate of this bug: 628134
Blocks: 639836
Depends on: 884585
See Also: → 1449956
You need to log in before you can comment on or make changes to this bug.