442970 - Home Page Opens Along With Tabs Of Previous Session

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect, P1)

Description

[WildcatRay]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.1pre) Gecko/2008070106 GranParadiso/3.0.1pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.1pre) Gecko/2008070106 GranParadiso/3.0.1pre

Startign with today's Gran Paradiso Nightly, when the browser starts or restarts, the home page opens to the right of the tabs from the previous session. Options > Main > Startup is set to "Show windows and tabs from last time".

Reproducible: Always

Steps to Reproduce:
1. With new profile, set up one or more tabs other than the listed home page.
2. Set Options > Main > Startup to "Show windows and tabs from last time".
3. Close the browser.
4. Open the browser.
Actual Results:  
Along with the tabs from the previous session, the home page opens in the right-most tab

Expected Results:  
Only the tabs from the previous session should open.

Will add regression range once I determine it. So far it is between the Gran Paradiso nighties of 2008063009 (OK) and 2008070106 (Not OK).

Other information here: http://forums.mozillazine.org/viewtopic.php?f=23&t=712845

Comment 1

[Hasham]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.1pre) Gecko/2008070106 GranParadiso/3.0.1pre

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.1pre) Gecko/2008070106 GranParadiso/3.0.1pre

Confirmed on XP and Vista. Windows only problem. I tried OS-X Leopard and it worked fine.

Comment 2

[WildcatRay]

The regression range:

OK: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.1pre) Gecko/2008063023 GranParadiso/3.0.1pre

Not OK: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.1pre) Gecko/2008070100 GranParadiso/3.0.1pre

Whatever was checked in in the 2008070100 build appears to have adversely affected the session saving and restoring.

Comment 3

[Simon Bünzli]

Gavin: This regression seems to have happened due to bug 441120.

Comment 4

[Hasham]

Whoops. Sorry about that. My Gran Paradiso was not updated on OS-X. It is a problem on all OS.

Comment 5

[Samuel Sidler (old account; do not CC)]

This should block both 1.8.1.16 and 1.9.0.1. 

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch?

The problem is that the session store code relies on being able to detect default parameters so that it can strip them out and do it's own thing:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/sessionstore/src/nsSessionStore.js&rev=1.100#1992

This check is now broken, because due to bug 441120 we no longer pass in defaultArgs directly, instead putting them into an array and passing that.

I thought of a few options:
1) Put a flag in the array that we're passing in to indicate "default parameters"

Not ideal since the object we pass needs to go through XPCOM (so we can't just pass a JS object with a property), and adding an entry to the nsISupportsArray we pass in means adding code in browser.js to deal with that flag and avoid loading it as a URI.

2) Have nsSessionStore deconstruct the nsISupportsArray to compare it to defaultArgs

Ugly and and not really reliable...

3) Pass another argument to browser.xul

I explicitly avoided doing this in bug 441120 to avoid potential conflicts with extensions using that slot in window.arguments (not sure how common this is).

4) Set a JS property on the window from the command line code, and have the sessionstore code use that.

This is what I went with... I was a bit afraid that xpconnect wrappers would get in the way, but it seems to work. It's hacky, but I don't see a better way that doesn't involve reworking how command-line/session store/browser window code interact.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Attachment: better patch?

Just thought of this after attaching the last patch. This one just reverts to using the old way of opening tabs for the defaultArgs cases, instead of having them also use the new array technique.

I avoided this in the original patch, because it means that callers of openWindow need to indicate which technique they want to use, which is potentially problematic if someone copy-pastes an openWindow call without realizing what that parameter means, but that's probably avoidable with a good comment. This has the advantage of not needing any sessionstore changes, and reverting to a known-working state (the one from before bug 441120).

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Attachment: with comments

Added some comments and made the callers use a more obvious constant rather than just passing a "true". I ran through most of my tests with & without the "restore my tabs" settings. 

(Was confused at first because session store code strips arguments in two different places, one in nsSessionStartup and one in nsSesssionStore, and my dump()s to make sure that the right code was being triggered weren't being hit.)

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 327757 [details] [diff] [review]
with comments

r=dveditz

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 327757 [details] [diff] [review]
with comments

Approved for 1.8.1.16, a=dveditz for release-drivers

Comment 11

[Samuel Sidler (old account; do not CC)]

Consider this approval for 1.9.0.1. a=ss.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Landed on CVS trunk and branch:
mozilla/browser/components/nsBrowserContentHandler.js 	1.12.2.25
mozilla/browser/components/nsBrowserContentHandler.js 	1.49

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Thanks Ray and Hasham for finding/filing/confirming this so quickly, and thanks for finding the cause of the regression Simon!

Comment 14

[WildcatRay]

Your welcome, Gavin.

Comment 15

[WildcatRay]

Just updated to Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.1pre) Gecko/2008070206 GranParadiso/3.0.1pre ID:2008070206 and the home page did not open on restarting.

Comment 16

[Marcia Knous [:marcia]]

verified fixed on 1.9.0.1 using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.1) Gecko/2008070206 Firefox/3.0.1. I verified using the STR in Comment 0.

Comment 17

[Hasham]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.16) Gecko/20080702 Firefox/2.0.0.16

Verified on 1.8.1.16 based on STR in comment 0.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

This fix was included in the patch for bug 441120 that I landed on mozilla-central.