Restore Previous Session does not re-use blank tabs

RESOLVED FIXED in Firefox 13

Status

()

Firefox
Session Restore
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Ian Nartowicz, Assigned: Piers)

Tracking

Trunk
Firefox 13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=zpao][lang=js])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
User Agent: Mozilla/5.0 (X11; Linux i686; rv:8.0.1) Gecko/20100101 Firefox/8.0.1
Build ID: 20111120135848

Steps to reproduce:

1. Set "When Firefox starts" to "Show a blank page"
2. Open several tabs or windows
3. Close Firefox
4. Open Firefox (one blank tab)
5. Restore previous session


Actual results:

Previous session is restored by the initial blank tab is not re-used.


Expected results:

Tabs set to the configured home page should be re-used for restored tabs.  This works if the home page is an actual URL.
Mozilla/5.0 (X11; Linux x86_64; rv:12.0a1) Gecko/20120115 Firefox/12.0a1

This is the default behavior for both the home page and the blank. None of them are re-used after session restore.
Are you sure this worked for you with a URL as home page?
Can you reproduce that in Safe mode?
https://support.mozilla.com/en-US/kb/Safe+Mode
Component: Untriaged → Session Restore
QA Contact: untriaged → session.restore
(In reply to Virgil Dicu [QA] from comment #1)
> Mozilla/5.0 (X11; Linux x86_64; rv:12.0a1) Gecko/20120115 Firefox/12.0a1
> 
> This is the default behavior for both the home page and the blank. None of
> them are re-used after session restore.

That's not true.

Session restore will attempt to overwrite your home page(s) - if all open tabs when you press "restore previous session" match your home page prefs exactly, then we'll overwrite them. However we didn't handle the blank tab preference.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: x86 → All
Whiteboard: [good first bug][mentor=zpao][lang=js]
Version: 8 Branch → Trunk
(Reporter)

Comment 3

6 years ago
Indeed.  Session restore is quite brutal about overwriting pages that match the configured home page(s), I was quite surprised.  Yet it doesn't overwrite a blank page when the setting is "Show a blank page".  Reproducible in all the recent versions I've tried, even with new profiles or safe mode.
Assignee: nobody → piers
(Assignee)

Comment 4

5 years ago
Created attachment 598486 [details] [diff] [review]
Patch to nsSessionStore.js which removes tabs with a blank page when a session is restored.

The problem stems from the fact that the home page setting and a blank start page are two different things. If a user's start page preference is for a blank page (which has a URI of about:blank), this blank tab will never be removed on session restore as 'about:blank' will never match any of the home page URIs. This is the case even when no home page is specified because the home page URI will default to 'about:home'. The only exception is if you explicitly set the home page to 'about:blank', then the blank start tab will be removed.

In my patch I have added a condition to the comparison of open tabs and home pages which checks if the user's start page preference is for a blank page and if the current tab is a blank page. If this condition is true the tab is removed.
Attachment #598486 - Flags: review?(paul)
Comment on attachment 598486 [details] [diff] [review]
Patch to nsSessionStore.js which removes tabs with a blank page when a session is restored.

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

Hey Piers, thanks for taking this on!

I think you're on the right path, but I'd like to make it more specific. If the homepages are open when the startup pref is to show blank, then I would consider that a deliberate action, and we should keep them open. Only about:blank should go away then. So with that in mind...

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +1685,2 @@
>      let homePages = aWindow.gHomeButton.getHomePage().split("|");
> +    let startupPref = this._prefBranch.getIntPref("startup.page"); 

If we set homePages conditionally then we don't need to change anything in the loop below.
> if (startupPref = blank)
>   homePages = [about:blank]
> else if startupPref = homepage
>   homePages = [homepages]
Attachment #598486 - Flags: review?(paul) → review-
(Reporter)

Comment 6

5 years ago
Or to put it a different way, the homepages pref value is irrelevant when startup.page is set to blank.  It is only stored as a relic memory, but is not used as the startup page and should not be treated as a startup page in the session restore code.

I hadn't actually realised that it did this, overwriting the homepages URL even if the startup.page is set to be blank.  I would have considered that to also be a bug and well worth changing.
(Assignee)

Comment 7

5 years ago
Created attachment 599543 [details] [diff] [review]
Patch to nsSessionStore.js which removes tabs with a blank page when a session is restored.

Thanks for the clarification Paul and Ian. I have attached a new patch for this bug.

I was unsure about the relationship between home pages and the blank page startup option and thought they were two separate functions. Just to understand this a little further, how should the home page button behave if the startup option is set to a blank page? From my testing, the home page button (and its tooltip) will use the URIs specified in the home page text box (or about:home if blank) as opposed to using about:blank. Is this another bug?

Cheers
Attachment #598486 - Attachment is obsolete: true
Attachment #599543 - Flags: review?(paul)
(Reporter)

Comment 8

5 years ago
You are correct.  The home page button navigates to the configured URL even if "When Firefox starts" is set to blank.  I believe this is intentional.  There would be very little use in having a button that took you to the place you already were on startup or with a new window.

I think what Paul described is still correct.  If startup.page is blank but someone is on the configured home page then it is because they explicitly pressed a button and they want to be on that page.  So we should leave them there and open the restored session elsewhere.

Is this going to need addressing again for the new tab page?  Presumably that page should be overwritten.  Always?
(In reply to Ian Nartowicz from comment #8)
> You are correct.  The home page button navigates to the configured URL even
> if "When Firefox starts" is set to blank.  I believe this is intentional. 
> There would be very little use in having a button that took you to the place
> you already were on startup or with a new window.

Right.

> Is this going to need addressing again for the new tab page?  Presumably
> that page should be overwritten.  Always?

Yea, it will need to be. I didn't want to pile on & do it here though - it's certainly related but it didn't make sense to handle newtab in some sessionstore cases but not all. The fix for that will happen in bug 717044.
Comment on attachment 599543 [details] [diff] [review]
Patch to nsSessionStore.js which removes tabs with a blank page when a session is restored.

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

After thinking about this some more, I've changed my mind a little bit (sorry!). We should just always overwrite about:blank tabs. With this we'll ignore multiple about:blank tabs only if your startup pref == blank. But if your startup pref == homepages, then we'll never close about:blank. That inconsistency doesn't feel right.

So let's seed homePages with about:blank and then just add pref.homepages if appropriate. You may find Array#concat (https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Array/concat) to be helpful.

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +1687,5 @@
>      let tabbrowser = aWindow.gBrowser;
>      let normalTabsLen = tabbrowser.tabs.length - tabbrowser._numPinnedTabs;
> +    let startupPref = this._prefBranch.getIntPref("startup.page"); 
> +    if (startupPref == 0)
> +      homePages = ['about:blank']

Nit: Use double quotes for consistency with other string literals in the file.
Attachment #599543 - Flags: review?(paul) → review-
(Assignee)

Comment 11

5 years ago
Ok great, not a problem Paul.

> So let's seed homePages with about:blank and then just add pref.homepages if
> appropriate. You may find Array#concat

Sorry to drag this on however I just wanted to clarify what you meant by "if appropriate". You are referring to when the user's startup preference is to load their homepages? E.g. when their preference is to load their homepages the array will contain their homepages plus blank, otherwise just blank.

> Nit: Use double quotes for consistency with other string literals in the
> file.

Thanks for the pickup!
(Reporter)

Comment 12

5 years ago
That's right.  Always add "about:blank" and then add more if they are actually configured to load on startup (ie. startup.page NOT 0).
(In reply to Piers Biddlestone from comment #11)
> > So let's seed homePages with about:blank and then just add pref.homepages if
> > appropriate. You may find Array#concat
> 
> Sorry to drag this on however I just wanted to clarify what you meant by "if
> appropriate". You are referring to when the user's startup preference is to
> load their homepages? E.g. when their preference is to load their homepages
> the array will contain their homepages plus blank, otherwise just blank.

Correct.

Ian: you're totally on the ball. Thanks for being active here!
(Assignee)

Comment 14

5 years ago
Created attachment 600332 [details] [diff] [review]
Patch to nsSessionStore.js which removes tabs with a blank page when a session is restored.

Thanks again Paul and Ian for the clarification.

The code change is now rather minimal.

Cheers
Attachment #599543 - Attachment is obsolete: true
Attachment #600332 - Flags: review?(paul)
Comment on attachment 600332 [details] [diff] [review]
Patch to nsSessionStore.js which removes tabs with a blank page when a session is restored.

So close! Just one thing that I should have mentioned last time.

>+    if (startupPref != 0)
>+      homePages = homePages.concat(aWindow.gHomeButton.getHomePage().split("|"));

Sorry to be nitpicky, but I think we should be explicit here and only add the homepages if pref == 1. It's not only clearer of our intentions, but it will also help prevent issues if we ever add any options to startup.page in the future).
Attachment #600332 - Flags: review?(paul) → review-
(Assignee)

Comment 16

5 years ago
Created attachment 600626 [details] [diff] [review]
Patch to nsSessionStore.js which removes tabs with a blank page when a session is restored.

Thanks Paul, I have made this change and attached the new patch.
Attachment #600332 - Attachment is obsolete: true
Attachment #600626 - Flags: review?(paul)
Comment on attachment 600626 [details] [diff] [review]
Patch to nsSessionStore.js which removes tabs with a blank page when a session is restored.

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

Looks great. Thanks Piers!
Attachment #600626 - Flags: review?(paul) → review+
Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/782a97166d7c
Whiteboard: [good first bug][mentor=zpao][lang=js] → [good first bug][mentor=zpao][lang=js][fixed-in-fx-team]
Summary: Restore Previous Session with blank home page does not re-use blank tabs → Restore Previous Session does not re-use blank tabs
https://hg.mozilla.org/mozilla-central/rev/782a97166d7c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=zpao][lang=js][fixed-in-fx-team] → [good first bug][mentor=zpao][lang=js]
Target Milestone: --- → Firefox 13
You need to log in before you can comment on or make changes to this bug.