366572 - [SessionStore] clearing private data doesn't clear sessionstore.js at exit

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Simon Bünzli]

Steps to reproduce:
1. Configure Firefox to open the "windows and tabs from last time" at startup and to clear private data at shutdown
2. Load any page (for a nice effect, log into your email account)
3. Close Firefox and tell it to clear all private data (at least history and cookies)
4. Launch Firefox

Expected result:
Firefox loads blank since there should be nothing to reload, i.e. sessionstore.js has been purged at shutdown.

Actual result:
All the opened pages are nicely restored, you even remain logged in.

Comment 1

[Simon Bünzli]

Attachment: simple fix: don't recreate sessionstore.js if there are no browser windows opened

The problem is that we force sessionstore.js to be recreated, even if there are no browser windows opened from which we could want to re-save some information (i.e. the currently still opened tabs). There should really be no reason to do this -- if private data is cleared, I wouldn't expect anything to be restored afterwards (with the exception of what I can still see before me).

Drivers: Low-risk patch which (further) improves privacy without trading any comfort.

Comment 2

[Simon Bünzli]

... and should this be far too late for Firefox 2.0.0.2, consider this a request for blocking1.8.1.3 and approval1.8.1.3.

Comment 3

[Dietrich Ayala (:dietrich)]

I think this patch is problematic because on the CPD dialog, session information is not a configurable option, so users don't know that session data will be cleared. I think this has not been a problem so far only because of this "bug" :)

If this patch is accepted, users who have this configuration (browser.startup.page=3 + CPD-at-shutdown), and currently expect their session to be restored, will suddenly lose their session data after updating.

I think that browser.startup.page=3 should trump CPD-at-shutdown unless session info is configurable via the CPD dialog.

Comment 4

[Daniel Veditz [:dveditz]]

What about a compromise, that session cookies/auth are cleared in this case but the list of opened tabs is retained? User will have to re-log-in to something like mail, but in general a large list of newsy/bloggy tabs won't get lost.

Comment 5

[Simon Bünzli]

(In reply to comment #4)
> What about a compromise, that session cookies/auth are cleared in this case but
> the list of opened tabs is retained?

The problem is that we currently only react to clearing the History (which is the only one to notify observers). Implementing this would require changes outside of SessionStore.

Let's consider the use case for startup.page=3 + CPD-at-shutdown though: at first view, this doesn't make sense at all - you ask Firefox to purge _all_ History and at the same time it should retain the latest bits of it. Such a user must not only be paranoid but schizophrenic as well. ;-)

There's mainly one legitimate use of that setting constellation I could see here: if you're generally "privacy concerned", but want to keep the option of resuming selected sessions. So you set Firefox to ask you at shutdown and there you either discard the _whole_ session, or you don't discard any of it.

One other exception we might want to make is to not clear the session when the browser is only restarting. However in that case, it would IMO make more sense to not do any clearing at all. If necessary, this exception could quite easily be implemented inside SessionStore itself though (by not purging sessionstore.js when resume_session_once is true).

Comment 6

[Dietrich Ayala (:dietrich)]

(In reply to comment #5)
> (In reply to comment #4)
> Let's consider the use case for startup.page=3 + CPD-at-shutdown though: at
> first view, this doesn't make sense at all - you ask Firefox to purge _all_
> History and at the same time it should retain the latest bits of it. Such a
> user must not only be paranoid but schizophrenic as well. ;-)

Dveditz's idea seems like the right direction. I talked to Beltzner and thunder about this, who noted that if the user chose both prefs, we should also clear the history for each tab, only restoring the most-recent entry.

> There's mainly one legitimate use of that setting constellation I could see
> here: if you're generally "privacy concerned", but want to keep the option of
> resuming selected sessions. So you set Firefox to ask you at shutdown and there
> you either discard the _whole_ session, or you don't discard any of it.

The problem with doing that is that the CPD dialog does not give any indication that session data will be cleared. Also, the option above doesn't require such an extreme decision.

> One other exception we might want to make is to not clear the session when the
> browser is only restarting. However in that case, it would IMO make more sense
> to not do any clearing at all. If necessary, this exception could quite easily
> be implemented inside SessionStore itself though (by not purging
> sessionstore.js when resume_session_once is true).
> 

Yep, shouldn't clear at all for restarts.

Comment 7

[Simon Bünzli]

(In reply to comment #6)
> if the user chose both prefs, we should also clear
> the history for each tab, only restoring the most-recent entry.

Might also make sense.

> the CPD dialog does not give any indication that session data will be cleared.

And IMO rightly so. That'd just be one more option users would have to care about. It makes much more sense to just clear those parts of the stored session the user is clearing anyway.

Still, this bug is about clearing everything together. So it's probably WONTFIX then. Feel free to file a bug for implementing one of the alternative suggestions -- or maybe better for doing a privacy audit of SessionStore first.

> Yep, shouldn't clear at all for restarts.

That's for another bug as well (either just for SessionStore or for CPD as a whole).

Comment 8

[Chris Hecker]

Came visiting to file a bug about the fact that CPD doesn't clear the session store in some situations and found this but, so I guess I'll just comment here.  My use-case was if firefox crashes or hangs around in the background, and then you use it normally a number of times, hitting CPD occasionally, then you can get it in a state where the next time you run, it prompts you if you want to restore the session, and if yes, it restores the session from long before the last CPD, popping up windows that are not in the history or that there should be any trace of.  This is surprising, since hitting CPD implies that it clears all the state associated with the user's browsing session (and is casually advertised as such for browsing privacy).  But, the sessionstore.js is another place where there is private data that is unclearable by CPD, which seems broken.

Related: https://bugzilla.mozilla.org/show_bug.cgi?id=162599

Thanks,
Chris

Comment 9

[Simon Bünzli]

(In reply to comment #8)
> since hitting CPD implies that it clears
> all the state associated with the user's browsing session

This bug is only about sessionstore.js not being cleared _at exit_, otherwise that file should be deleted immediately when you clear your browsing history. If it isn't, that'd be worth a different bug. Before you file one, please make sure that the behavior you describe isn't due to a session managing extension, though.

One possible reason for what you observe might be bug 351551 (possibly in combination with a different bug reverting the symptoms of bug 351551 over time). AFAICT those files aren't deleted through CPD although they probably should be as well.

Comment 10

[Greg Huntoon]

FireFox 2.0.0.4. About this bug, FireFox was using 50% CPU after loading any page either in normal or safe mode.  What I discovered was the following - SessionStore files named like sessionstore-0000.js  0kb in size to sessionstore-9999.js all 10,000 files were 0kb in size.  Once I deleted the files my CPU issues are gone.  Not sure what the issue is, maybe based on the naming convention of the files no more could be created or maybe 10,000 0kb files was causing strain. Could be why people are reporting corrupt profiles or stating that creating a new profile fixed the issue, ie. nothing was fixed they just were using a clean profile.  From what I hearing on the Web about FireFox being a hog etc., fixing this issue could be huge.

Comment 11

[Simon Bünzli]

Greg: That's definitively bug 351551. Make sure your virus/malware scanner and your desktop search engine (should you have any) don't scan/index sessionstore.js.

Comment 12

[Simon Bünzli]

This was effectively WONTFIXED in comment #6.

Comment 13

[Simon Bünzli]

Reopening per bug 398817 comment #5.

Comment 14

[Simon Bünzli]

Attachment: don't recreate sessionstore.js if there are no browser windows opened

This three-liner makes sure that the WONTFIX'd bug 398817 doesn't accidentally regress.

Comment 15

[Simon Bünzli]

Comment on attachment 289845 [details] [diff] [review]
don't recreate sessionstore.js if there are no browser windows opened

This patch is indeed quite useless. It seems that we simply don't get the purge-session-history notification on branch while we _do_ get it on Trunk, be it because we are now registered up to a later point or because the sanitizer kicks in at an earlier moment in the shutdown queue.

Comment 16

[Simon Bünzli]

Closing as per bug 398817 comment#1: WORKSFORME.

Comment 17

[Simon Bünzli]

Reopening, as per bug 398817 comment #19, sessionstore.js is only cleared when closing the last browser window through Alt+F4 or clicking on the window's [X], not however when exiting through File -> Exit.

Comment 18

[Simon Bünzli]

Attachment: don't recreate sessionstore.js while quitting

The problem is that when we're quitting, we keep window data around that we don't explicitly delete during purging. In the case of Alt+F4 or clicking the [X], the last window is closed before we start quitting, though, whereas File -> Exit sets quitting mode first before closing the window (and thus discarding its data).

Simply not recreating sessionstore.js when we're not running finally fixes this issue for good.

Comment 19

[Simon Bünzli]

Attachment: more thorough patch

Not recreating sessionstore.js might not be enough in case sss_saveState gets called again before finally exiting, so we really should clean out the states of all closed windows as well.

Comment 20

[Dietrich Ayala (:dietrich)]

Comment on attachment 336258 [details] [diff] [review]
more thorough patch

r=me, this seems a fair balance

Comment 23

[Simon Bünzli]

Attachment: unbitrotted

Comment 24

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/775443aac5e3