716080 - Restore Previous Session does not re-use blank tabs

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Ian Nartowicz]

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.

Comment 1

[Virgil Dicu [:virgil] [QA]]

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

Comment 2

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

(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.

Comment 3

[Ian Nartowicz]

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.

Comment 4

[u435773]

Attachment: 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.

Comment 5

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

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]

Comment 6

[Ian Nartowicz]

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.

Comment 7

[u435773]

Attachment: 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

Comment 8

[Ian Nartowicz]

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?

Comment 9

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

(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 10

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

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.

Comment 11

[u435773]

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!

Comment 12

[Ian Nartowicz]

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).

Comment 13

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

(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!

Comment 14

[u435773]

Attachment: 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

Comment 15

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

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).

Comment 16

[u435773]

Attachment: 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.

Comment 17

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

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!

Comment 18

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

Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/782a97166d7c

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/782a97166d7c