Michael Kraft [:morac] Reporter
	Description • 16 years ago

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3) Gecko/20090305 Firefox/3.1b3 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090315 Minefield/3.2a1pre (.NET CLR 3.5.30729)

Starting with Firefox 3.1b3, whether or not SessionStore saves the current browser state at shutdown when the user sets Firefox to clear the visited pages at browser shutdown, depends on how shutdown is triggered.  

If shutdown is triggered via the File->Exit menu, than the current browser state will be saved.

If shutdown is triggered by closing the last browser window, than the current browser state will be cleared in the sessionstore.js file.

I'm not sure what the desired action should be if the user sets the browser to clear the visited pages on shutdown, but it should not be dependent on how the browser is shut down.

The problem also occurs on the nightly trunk.



Reproducible: Always

Steps to Reproduce:
1. On the General tab, set the startup option to "Show my Windows and Tabs From Last Time".
2. On the privacy tab, check the "Always clear my private data when I close ..." checkbox and make sure the "Visited Pages" box is checked in the Clear Private Data settings.
3. Open Firefox and load a page.  Click on File->Exit
4. Open Firefox again and once page loads, close it by clicking the "X" in the top right corner (Windows version).

Actual Results:  

When the browser is exited using the File->Exit command, the current browser state is saved in sessionstore.js and then restored the next time the browser is run.  
When the browser is exited by closing the last browser window, the state is set to ({"windows":[],"selectedWindow":0,"session":{"state":"stopped","lastUpdate":1237180299539}}) in sessionstore.js so it is not restored the next time the browser is run.

Expected Results:  

Either the state should be cleared on all instances of shutdown or it should be saved on all instances of shutdown.


Calling the SessionStore API's getBrowserState() during the quit-application notification state results in cleared out session data when when closing the last browser window.  When using File->Exit, the actual current state is return.

This most likely has to do with the order that the observer notifications are coming in when closing the last window as opposed to when using the File->Exit shutdown method.

	Dão Gottwald [:dao]
	Comment 1 • 16 years ago

Another method would be ctrl+w, which is bug 481560.

	(mostly gone) XtC4UaLL [:xtc4uall]
	Comment 2 • 16 years ago

addition: clicking top right "X" = using alt+f4 resp. top left "programm icon"/"Close" = double-click top left "programm icon".

Dão, is a regressionrange still wanted?

	Dão Gottwald [:dao]
	Comment 3 • 16 years ago

Yes.

	(mostly gone) XtC4UaLL [:xtc4uall]
	Comment 4 • 16 years ago

ok,

fails both ways (file/exit resp. right top "X"):
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090106 Minefield/3.2a1pre ID:20090106034601
http://hg.mozilla.org/mozilla-central/rev/f54a6c9a0316

works on one part (file/exit) like reported in comment 0:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090107 Minefield/3.2a1pre ID:20090107033209
http://hg.mozilla.org/mozilla-central/rev/670a3b50dfe0

=> range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f54a6c9a0316&tochange=670a3b50dfe0

=> likely bug 470348

	Marco Bonardo [:mak]
	Comment 5 • 16 years ago

the related change is probably that the session is saved on quit-application-granted rather than on quit-application

	Michael Kraft [:morac] Reporter
	Comment 6 • 16 years ago

I'm still not sure what the desired action should be since the user is choosing to clear all "visited pages" at shut down, but also restore the windows and tabs next time.  The former triggers SessionStore to wipe it's data and the later triggers SessionStore to save the data for next time.  Basically the two options are mutually exclusive and probably should not be permitted to both be set.

In any case, the problem appears to the order of the notifications when shutting down.  In the case of closing the last window, "browser:purge-session-history" fire right before "quit-application", while on File->Exit it fires before "domwindowclosed".  Also for some reason the "quit-application-granted" is triggered twice on File->Exit.


The order of the notifications for file->exit, as reported by sessionstore.js, is:

quit-application-requested
quit-application-granted
browser:purge-session-history
domwindowclosed
quit-application-granted
quit-application


The order for closing the last window is:

quit-application-requested
domwindowclosed
quit-application-granted
browser:purge-session-history
quit-application

	Ian
	Comment 7 • 16 years ago

https://bugzilla.mozilla.org/show_bug.cgi?id=480466 [OSX only?]
"Shutdown dispatches quit-application-granted twice"
If they act on https://bugzilla.mozilla.org/show_bug.cgi?id=480466#c5 then it may change what you need to do. 

My bugzilla-fu has failed yet again as I am pretty sure there is (at least was) a reason for it.  I think it was  related to clearing private data on exit

	Mike Connor [:mconnor] Assignee
	Comment 8 • 16 years ago

See bug 398817 for discussion of expected behaviour here.

based on comment 6, the bug is fairly obvious, since we're calling purge-session-history before we close the last window, and on closing that window, we save that window state.

Will fix this tomorrow.

	Marco Bonardo [:mak]
	Comment 9 • 16 years ago

please pay attention to not move down clear history code in the notifications changes, nothing should try to use Places after quit-application-granted atm.

	Michael Kraft [:morac] Reporter
	Comment 10 • 16 years ago

Is the first quit-application-granted notification correct? (bug 480466)

If it was removed, then the browser:purge-session-history notification could be moved after the domwindowclosed notification?

	Ruokola Pasi
	Comment 11 • 16 years ago

I can reproduce this bug with Firefox 3.0.8 too. I think this bug has been there for quite some time now.

	Simon Bünzli
	Comment 12 • 16 years ago

(In reply to comment #11)
You're actually seeing bug 366572 which was fixed for Firefox 3.1 Beta 1 - this regression only has the same symptoms, but a different cause (a code change in bug 470348).

	Mike Connor [:mconnor] Assignee
	Comment 13 • 16 years ago

So, basically, if we get a purge-history notification while quitting, we will not restore the session automatically, ever.  This was intentional, but not enforced in Session Store.  Now we're enforcing this.

	Dietrich Ayala (:dietrich)
	Comment 14 • 16 years ago

Comment on attachment 370701 [details] [diff] [review]
never save session when clearing history on shutdown

looks good, r=me with a test.

	Mike Connor [:mconnor] Assignee
	Comment 15 • 16 years ago

We can't automate testing of shutdown/startup yet.

Need a litmus test instead.

	Ruokola Pasi
	Comment 16 • 16 years ago

So little case with patch 370701... User has selected to clear history on exit, he install an extention or update, then he restarts firefox after he is prompted and he even gets a nice message from firefox telling him that the session will be restored and when firefox starts up the session is gone. Not good.

Please reconsider the expected behaviour here.

	Michael Kraft [:morac] Reporter
	Comment 17 • 16 years ago

I would agree with comment #16.  Restarting probably should not clear out the history since the user really isn't shutting down.  It would definitely not be the expected behavior on a restart.  That might be considered a separate bug though.

	Mike Connor [:mconnor] Assignee
	Comment 18 • 16 years ago

It's really a separate bug, IMO.

	Dão Gottwald [:dao]
	Comment 19 • 16 years ago

http://hg.mozilla.org/mozilla-central/rev/2b33ad84d384

	Dão Gottwald [:dao]
	Comment 20 • 16 years ago

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ac7e72e97238

	Michael Kraft [:morac] Reporter
	Comment 21 • 16 years ago

Based on comment 18, I filed Bug 487219 documenting the issue from comment 16 caused by this fix.

	u279076
	Comment 22 • 15 years ago

in-litmus+
https://litmus.mozilla.org/show_test.cgi?id=7802