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

RESOLVED FIXED in Firefox 3.1b1

Status

()

Firefox
Session Restore
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: Simon Bünzli, Assigned: Simon Bünzli)

Tracking

({privacy})

Trunk
Firefox 3.1b1
privacy
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

12 years ago
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.
(Assignee)

Comment 1

12 years ago
Created attachment 251082 [details] [diff] [review]
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.
Attachment #251082 - Flags: review?(dietrich)
Attachment #251082 - Flags: approval1.8.1.2?
(Assignee)

Comment 2

12 years ago
... 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.
Flags: blocking1.8.1.2?
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.
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.
(Assignee)

Comment 5

12 years ago
(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).
(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.
(Assignee)

Comment 7

12 years ago
(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).
Severity: major → normal
Flags: blocking1.8.1.2?
Whiteboard: [wontfix?]
(Assignee)

Updated

12 years ago
Attachment #251082 - Flags: review?(dietrich)
Attachment #251082 - Flags: approval1.8.1.2?
Component: General → Session Restore
QA Contact: general → session.restore

Comment 8

11 years ago
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
(Assignee)

Comment 9

11 years ago
(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

11 years ago
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.
(Assignee)

Comment 11

11 years ago
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.
(Assignee)

Updated

11 years ago
Assignee: zeniko → nobody
(Assignee)

Comment 12

11 years ago
This was effectively WONTFIXED in comment #6.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → WONTFIX
Whiteboard: [wontfix?]
(Assignee)

Comment 13

11 years ago
Reopening per bug 398817 comment #5.
Status: RESOLVED → REOPENED
Depends on: 398817
Resolution: WONTFIX → ---
(Assignee)

Comment 14

11 years ago
Created attachment 289845 [details] [diff] [review]
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.
Assignee: nobody → zeniko
Attachment #251082 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Attachment #289845 - Flags: review?(mconnor)
(Assignee)

Updated

11 years ago
Attachment #289845 - Flags: review?(mconnor) → review?(gavin.sharp)
(Assignee)

Comment 15

11 years ago
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.
Attachment #289845 - Attachment is obsolete: true
Attachment #289845 - Flags: review?(gavin.sharp)
(Assignee)

Comment 16

11 years ago
Closing as per bug 398817 comment#1: WORKSFORME.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → WORKSFORME
Version: unspecified → Trunk
(Assignee)

Comment 17

10 years ago
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.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
(Assignee)

Comment 18

10 years ago
Created attachment 336241 [details] [diff] [review]
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.
Attachment #336241 - Flags: review?(dietrich)
(Assignee)

Updated

10 years ago
Status: REOPENED → ASSIGNED
(Assignee)

Comment 19

10 years ago
Created attachment 336258 [details] [diff] [review]
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.
Attachment #336241 - Attachment is obsolete: true
Attachment #336258 - Flags: review?(dietrich)
Attachment #336241 - Flags: review?(dietrich)
Comment on attachment 336258 [details] [diff] [review]
more thorough patch

r=me, this seems a fair balance
Attachment #336258 - Flags: review?(dietrich) → review+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
(Assignee)

Updated

10 years ago
Duplicate of this bug: 443389
(Assignee)

Updated

10 years ago
Duplicate of this bug: 431155
(Assignee)

Comment 23

10 years ago
Created attachment 337043 [details] [diff] [review]
unbitrotted
Attachment #336258 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/775443aac5e3
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.1b1
(Assignee)

Updated

9 years ago
Blocks: 483566
You need to log in before you can comment on or make changes to this bug.