622379 - Current session data is lost upon browser triggered restart (e.g. upgrade) if the option to clear browsing history upon shutdown is set (again)

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect, P2)

Description

[Michael Kraft [:morac]]

I reporting the same exact problem as was reported in bug 487219.  Basically the problem is that if the option to "Clear History when <browser> closes" is checked and then "Browsing History" is also checked, then if the browser is restarted, the current session is lost.

This was fixed for Firefox 3.6 and still works as of Firefox 3.6.13, but is now broken again in Firefox 4.0.b8 and the latest 4.0 nightly load.  I've seen this for myself under Windows and have had it reported to me from someone using OS X.

I looked at the code and the fix for bug 487219 is still in the code, but something must have been changed to cause it to no longer work correctly.  I haven't had a chance to look into it that deeply.

For more details on the original problem see bug 487219.

Comment 1

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

That seems unintentional but it also sounds a lot like bug 589441. I'll look into it.

Comment 2

[Michael Kraft [:morac]]

It is similar to bug 589441, but slightly different.  While bug 589441 was rejected because privacy settings should take preference when shutting down Firefox, from a user standpoint restarting is basically a continuation of the current session. There shouldn't be a privacy issue since the user hasn't chose to shut down the browser, but rather restart it and continue working. 

For example installing or upgrading  an add-on and clicking restart shouldn't dump the current session regardless of what the clear on shutdown settings are set to as that would be bad.  The fix for bug 487219 fixed this (you can read through that for the reasoning behind it), but for some reason that's no longer working. You can see that the _clearingOnShutdown flag is still set to false on a restart. 

Personally I would expect crashed sessions to be restored as well since technically the user never shut down the browser so I wouldn't think the browsing history shouldn't be cleared on startup, but since there's no guarantee the user will rerun the browser immediately after the crash I guess I understand the reasoning.  Restarting always reruns the browser though so that shouldn't follow the same rules as shutting down or crashing.

Comment 3

[Luke Iliffe (Harlequin99)]

Nightly regression range on win 7 right back during July 2010:
works	2010-07-01-04 	http://hg.mozilla.org/mozilla-central/rev/82edf5bd1abe
broken	2010-07-02-04	http://hg.mozilla.org/mozilla-central/rev/c173731c9d90

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=82edf5bd1abe&tochange=c173731c9d90

I can't guess what checkin caused this, hopefully others can.

Comment 4

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Looks like dupe of bug #589441 / #577652

Comment 5

[Michael Kraft [:morac]]

(In reply to comment #4)
> Looks like dupe of bug #589441 / #577652

See comment #2

Comment 6

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Ahhh, thx for clarification, didn't read deep enough ;p
But still I want this bug to be fixed.
How about nominating this as blocker for Fx4, because of data loss in this case ?

Comment 7

[Luke Iliffe (Harlequin99)]

Added the dataloss keyword and nominating for blocking as a user can unexpectedly lose their entire session when installing or updating a simple extension or any other action that requires a restart.

The previous identical (but fixed) bug 487219 for the 3.x branch had blocking 3.5 status.

Comment 8

[Michael Kraft [:morac]]

Looking into this I can see what's going wrong.  Here's the order the observer notifications fire in nsSessionStore.js under Firefox 3.6.13:

domwindowopened
quit-application-requested
quit-application-granted
browser:purge-session-history
domwindowclosed
timer-callback
domwindowclosed
quit-application

Here's the order under Firefox 4.0b9pre (nightly):

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


As you see "browser:purge-session-history" runs last in Firefox 4.0.  As part of the processing that nsSessionStore does in observe() for "browser:purge-session-history", it calls "this._clearDisk()" which deletes the sessionstore.js file.

So while the "sessionstore.resume_session_once" preference is still set to true, the sessionstore.js file is gone so there's nothing to restore.

Also the shutdown processing has changed so the "this._clearingOnShutdown" flag really isn't even checked on shutdown so it's not doing anything setting it to false in the quit-application notification.

It looks like the easiest fix for this is to create a new flag that gets set in the quit-application notification when aData is "restart" and then check that before clearing the disk.

I'll post a very simply patch which fixes this problem shortly.

Comment 9

[Michael Kraft [:morac]]

Attachment: Don't delete sessionstore.js file when restarting

This simply prevents the sessionstore.js file from ever being deleted when the browser is restarted.  The file will still be deleted on normal shut downs if the browser history is cleared.

There's no way of automated a test for this so it will require a Litmus test.

Paul you seem to be the current owner of SessionStore based on the patch log, so I'm asking you for a review.  If you're not the right person, please tell me.

Comment 10

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

Michael, first off thanks for catching & investigating this.

It looks like bug 529821 is what regressed this (browser:purge-session-history doesn't happen until during places-shutdown now, which is after quit-application).

For the patch (I'm going to look more closely now), I'm wondering if it might be better to also use places-shutdown instead of quit-application. Not sure what other implications that would have. Also, I'm not sure that adding a new variable which basically has the same meaning as _clearingOnShutdown is the best way to go. I'll look a bit more closely at how it's fitting together.

Comment 11

[Michael Kraft [:morac]]

Whatever you think is best.  Take not though that the way the "_clearingOnShutdown" variable worked previously under Firefox 3.6.13 had nothing to do with the browser:purge-session-history.  The browser:purge-session-history would set the _clearingOnShutdown flag to true and clear the disk as well as the session data in memory.  Then the quit-application-requested notification would come in and and repopulate the session data in memory and finally the quit-application notification would come in and call _uninit() which would call the _doResumeSession() to determine if the session data should be saved to disk or if it should call _clearDisk() to delete the sessionstore.js file.  If _clearingOnShutdown was true, then _doResumeSession() would return false and sessionstore.js would be deleted.  The _clearingOnShutdown flag got set to false on a restart prior to uninit() being called to prevent this.

Under Firefox 4.0, the _uninit() function no longer calls either _doResumeSession() or _clearDisk(), so the _clearingOnShutdown really isn't used under Firefox 4.0 at all.  It's still read in the _doResumeSession(), but as it's only ever set to true on the browser:purge-session-history notification when the browser is quitting and browser:purge-session-history comes last, the _clearingOnShutdown flag will always be false when _doResumeSession() is called.  Basically it's not used.

So basically under Firefox 3.6.13, _uninit() deleted sessionstore.js.  Under Firefox 4.0 the browser:purge-session-history observer does it.

You need to find a way to keep the "clearDisk()" call from executing in the browser:purge-session-history, if the notification comes after a quit-application notification with the aData set to "restart".  It doesn't matter how it's done.  Actually to make things simple, it might be easier to simply unregister the "quit-application notification" observer in the quit-application when aData is set to restart since at that point we don't need to do any of the processing in the "quit-application notification" observer and it's basically just adding extra processing to shut down.

So basically replace the line under "quit-application" observer:

this._clearingOnShutdown = false;

with:

Services.obs.removeObserver(this, "browser:purge-session-history");


Then remove all other occurrences of _clearingOnShutdown since it's not used.

Comment 12

[Michael Kraft [:morac]]

Argh, typo in the "You need..." paragraph.

s/unregister the "quit-application notification"/unregister the "browser:purge-session-history"/

Comment 13

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

(In reply to comment #11)
> Under Firefox 4.0, the _uninit() function no longer calls either
> _doResumeSession() or _clearDisk(), so the _clearingOnShutdown really isn't
> used under Firefox 4.0 at all.  It's still read in the _doResumeSession(), but
> as it's only ever set to true on the browser:purge-session-history notification
> when the browser is quitting and browser:purge-session-history comes last, the
> _clearingOnShutdown flag will always be false when _doResumeSession() is
> called.  Basically it's not used.

Ah yes. Looks like I got rid of the check in uninit with http://hg.mozilla.org/mozilla-central/rev/d8502fae9678. So yea, it's really not being used at all now.

> So basically replace the line under "quit-application" observer:
> 
> this._clearingOnShutdown = false;
> 
> with:
> 
> Services.obs.removeObserver(this, "browser:purge-session-history");
> 
> 
> Then remove all other occurrences of _clearingOnShutdown since it's not used.

I like this idea. There's potential for a minimal perf hit (in comparison to 3.6) for people clearing on shutdown, but it's not going to be any different from what is currently in the tree (we're going to update current state and then say we don't need it and delete it). Would you be willing to run with that?

While I'm looking at this code... We could probably improve the browser:purge-session-history case in observe by calling clearDisk first, followed by a return if STATE_QUITTING (because we're not really quitting then, we've already quit). Not quite part of this bug though...

Comment 14

[Michael Kraft [:morac]]

I'll throw something together tomorrow when I get the chance.  It should be fairly straightforward.  I can even throw in the last part since like you said, there's no point doing processing clearing out memory objects when the browser is about to exit anyway and the memory's already going to get dumped.

I'm fairly certain this can't be automatically tested since it requires a restart and the original bug was litmus tested, so I won't write a test for it.

Comment 15

[Michael Kraft [:morac]]

Attachment: Patch 2.0 - implements changes mentioned in comments 11 and 13

This patch removes the unused _clearingonshutdown flag from the code and unregisters the "browser:purge-session-history" notification on a browser restart.  That fixes the bug, at least it works for me when I tested it under the latest nightly Windows Minefield load.

It also optimizes things as suggested in comment #13 by not doing any of the memory cleanup processing if in shut down since there's no reason to do so.  

I'll mention that I originally put the _clearDisk() call inside the "... == STATE_QUITTING" if statement at the top of the "browser:purge-session-history" by mistake.  When I tested this, it appeared to work as expected, which makes sense since the saveState() call below that will overwrite the sessionstore.js file.  When I double checked my code I saw this "error" and changed it back to the way it has always worked which is if the user manually clears the browsing history, the sessionstore.js file will be deleted and recreated, rather than simply overwritten.  That doesn't hurt anything, but it's not particularly efficient.

Comment 16

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

Comment on attachment 501697 [details] [diff] [review]
Patch 2.0 - implements changes mentioned in comments 11 and 13

>     case "quit-application":
>       if (aData == "restart") {
>         this._prefBranch.setBoolPref("sessionstore.resume_session_once", true);
>-        this._clearingOnShutdown = false;
>+        // Do not clear session data on disk on a restart.  There's no need to 
>+        // do other sanitization processing as the browser is about to exit anyway.

I know it gets mentioned lower, but you could make it clear here that purge-session-history occurs after quit-application? I'll gladly take redundant comments since that's really not something obvious.

>     case "browser:purge-session-history": // catch sanitization 
>+      this._clearDisk();
>+      // If the browser is shutting down, simply return after clearing the 
>+      // session data on disk as this notification fires after the 
>+      // quit-application notification so the browser is about to exit.
>+      if (this._loadState == STATE_QUITTING) {
>+        return;
>+      }

I'd prefer this without braces, but with a new line following it and a new line above the comment. There are almost no newlines in that whole switch block & it's getting quite unreadable. One of these days we'll move each of these cases into functions.

(In reply to comment #15)
> I'll mention that I originally put the _clearDisk() call inside the "... ==
> STATE_QUITTING" if statement at the top of the "browser:purge-session-history"
> by mistake.  When I tested this, it appeared to work as expected, which makes
> sense since the saveState() call below that will overwrite the sessionstore.js
> file.  When I double checked my code I saw this "error" and changed it back to
> the way it has always worked which is if the user manually clears the browsing
> history, the sessionstore.js file will be deleted and recreated, rather than
> simply overwritten.  That doesn't hurt anything, but it's not particularly
> efficient.

I agree that it's not the most efficient thing, but it also won't be a bad hit (and it's already the case so we're not regressing). _clearDisk also clears out sessionstore.bak so we should continue to do that.

Comment 17

[Michael Kraft [:morac]]

(In reply to comment #16)
> I know it gets mentioned lower, but you could make it clear here that
> purge-session-history occurs after quit-application? I'll gladly take redundant
> comments since that's really not something obvious.

It's sort of implied as otherwise removing the purge-session-history observer wouldn't do anything, but I'll explicitly add a comment about this.

> I'd prefer this without braces, but with a new line following it and a new line
> above the comment. There are almost no newlines in that whole switch block &
> it's getting quite unreadable. One of these days we'll move each of these cases
> into functions.
> 

I actually didn't mean to do that.  The braces are left over from when I accidentally put the _clearDisk call in the braces.  I simply forgot to remove them, which I'll do now.

I'll upload the changes shortly.

Comment 18

[Michael Kraft [:morac]]

Attachment: Patch 2.1 with review fixes

I also found and removed trailing white space from my 2.0 patch which is always a plus.  :)

Comment 19

[Michael Kraft [:morac]]

Oh and I'm assuming that this will need to be flagged for a litmus test since the old bug 487219 was.  I'm assuming copying the original litmus would work, though adding that the current session is cleared on a normal quit should probably be added.

https://litmus.mozilla.org/show_test.cgi?id=7738

Comment 21

[Michael Kraft [:morac]]

Based on the dupe, a litmus test either already exists or was just added. It is obviously failing since the patch hasn't been checked in yet.

https://litmus.mozilla.org/show_test.cgi?id=11311

Comment 23

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/9e1ab3fb7831

Comment 24

[u279076]

Verified using Minefield nightly from 2011-01-22.