488706 - clear privacy data and clear cookies set

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P2)

Description

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090416 Minefield/3.6a1pre ID:20090416042608

in today's nightly clear privacy date is SET and also clear Coockies is SET leaving people w/o coockies they didn't want to get rid of.

I bet this is due to bug 462041

expected:
don't changes someones settings !

I assume branch has the same problem, this may require ? blocking-firefox3.5

Comment 1

[(no longer active)]

Peter, can you please provide some more information about this problem?  I couldn't tell exactly what you're mentioning by reading comment 0.

Comment 2

[Peter van der Woude [:Peter6]]

Ehsan, I can't reproduce the full case myself, that was done by someone else (on moz forum)

my repro:
install yesterdays build (in my case a .zip)
start a new profile
open tools->Options->Privacy
[V]Clear History when Minefield Exits
press [Settings]
[V] anything you like but Coockies
Close FF

delete FF and install todays build
open tools->Options->Privacy
press [Settings]
note that Coockies is now checked.

Comment 3

[Shawn Wilsher :sdwilsh]

This could also be due to bug 480169 I'm told.  They landed in the same window didn't they? :(

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

This was actually caused by bug 480169 as far as I understand things.

We changed the default value of privacy.item.cookies to true, but that's the preference that's used when clearing on close. The value that's used for a one-time use of Clear Recent History is actually privacy.cpd.cookies - and its default is set the first time it's launched, based on the current value of privacy.item.cookies.

I think the right approach here is to not change the default value of privacy.item.cookies for existing profiles.

Comment 5

[Peter van der Woude [:Peter6]]

(In reply to comment #3)
> This could also be due to bug 480169 I'm told.  They landed in the same window
> didn't they? :(

right,
obviously the patch of bug 480169

@@ -395,7 +395,7 @@ pref("privacy.item.history",     true);
 pref("privacy.item.formdata",    true);
 pref("privacy.item.passwords",   false);
 pref("privacy.item.downloads",   true);
-pref("privacy.item.cookies",     false);
+pref("privacy.item.cookies",     true);

Comment 6

[Mike Connor [:mconnor]]

I don't think the "expected results" here is correct.  We have always changed defaults, which means that existing users get the new behaviour by default.  Otherwise a lot of people would still get windows instead of tabs by default.

I think the change is sane as a default, and I'm not convinced that migrating users is the right answer for a majority of people.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Connor convinced me that we should relnote this for release and see if it causes a furor. Peter: can you tell the MZ folks that the change is coming to branch nightlies and betas as well? Likely a higher percentage of that audience who may be affected by this ...

Comment 8

[Peter van der Woude [:Peter6]]

Mike, sure.
I'm not convinced by Mike Connors argument though.
If we delete the cookies by default then people won't be able to login to e.g. forums or gmail automagically anymore.
I know you can change the pref, but who is going to explain this to Joe User who just switched from IE (any version) and expects everything to work like before and even better ?

Comment 9

[(no longer active)]

(In reply to comment #6)
> I don't think the "expected results" here is correct.  We have always changed
> defaults, which means that existing users get the new behaviour by default. 
> Otherwise a lot of people would still get windows instead of tabs by default.
> 
> I think the change is sane as a default, and I'm not convinced that migrating
> users is the right answer for a majority of people.

Has this change been made to modify the behavior of sanitize on shutdown?  That's kind of out of scope for bug 480169.  I think the intention was that the cookies checkbox be checked by default.  I have a patch which does that, without modifying the sanitize on shutdown behavior.  I'll attach it here for consideration.

Comment 10

[(no longer active)]

Attachment: Patch (v1)

Comment 11

[Mike Connor [:mconnor]]

Comment on attachment 373216 [details] [diff] [review]
Patch (v1)

I think this is the right default.  If we want to migrate users who are already using the shutdown pref, that's one thing, but this is not the right approach.

Comment 12

[Mike Connor [:mconnor]]

(In reply to comment #8)
> I know you can change the pref, but who is going to explain this to Joe User
> who just switched from IE (any version) and expects everything to work like
> before and even better ?

Joe User who just switched from IE won't have clear on shutdown set by default.

Clearing history, but not cookies, is a little strange, since it's basically a false sense of privacy due to mild obscurity.  If I'm okay with logging in automagically to sites, why am I not okay with it being in my history?  Because someone snooping would need to look at cookies first, then go to sites?  I don't think that's especially effective privacy.

Like I said, if users are already using clear on shutdown, I'm open to migrating in some cases.  But I think beta feedback is a better barometer than nightly users in this case.

Comment 13

[IceDogg]

I'll tell you why: I don't allow any cookies at all.. then I set exceptions to only certain sites. So when my history is cleared I'm good.. the sites I have exceptions for on my cookies I don't care if someone sees that I browse those sites. I know this is not a common setup, but I wanted to post why this will catch many people off guard. It did me, and I know many others who use a setup like mine. Having said that, this time it's not a huge deal, but personally I think devs should be careful forcing pref changes on people for ANY reason, if they have non default ones set already. This WILL continue to cause problems. JMHO


PS Mike Connor.. I'm only arguing a point here..no personal disrespect is intended to you or anyone else.

Comment 14

[(no longer active)]

Unassigning myself until a decision is made on the desired behavior here.

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #7)
> Connor convinced me that we should relnote this for release and see if it
> causes a furor.

Here's my personal anecdote:

I've watched this bug with interest since it was filed, but I didn't realize that I'm actually affected. Today I've updated to a recent trunk build with my main profile. Although I'm well-informed theoretically, it took me some time to figure out why I was suddenly logged out everywhere: I used to clear my download history on shutdown. Only my download history, nothing else. Now because of this bug, cookies have been added to my highly customized list.

I don't think there's a way to interpret this as the correct behavior. Fixing this also seems P1-worthy to me, as I don't see what kind of beta feedback could possibly make this bug less critical.

Comment 16

[Shawn Wilsher :sdwilsh]

(In reply to comment #15)
> I used to clear my download history on shutdown.
What you really want to do is set browser.download.manager.retention to 1 for that to happen.  Not saying this isn't a bug, but it enables your old behavior still.

Comment 17

[Dão Gottwald [:dao]]

Ironically, the one and only hidden pref documentation on kb.mozillazine.org refers to the UI that we're talking about here if you look for browser.download.manager.retention...

I've unchecked cookies after figuring out what's going on, so I've already got my old behavior back. The bad thing here isn't long-term harm (assuming you do figure it out), but the initial confusion and dataloss (for sites like bugzilla that use cookies for user data beyond login information).

Comment 18

[(no longer active)]

I'd also vote for fixing this.  I don't think the point in comment 12 is entirely correct, as demonstrated by Dão in comment 15.  The main point in comment 12 is:

> Clearing history, but not cookies, is a little strange, since it's basically a
> false sense of privacy due to mild obscurity.  If I'm okay with logging in
> automagically to sites, why am I not okay with it being in my history?  Because
> someone snooping would need to look at cookies first, then go to sites?  I
> don't think that's especially effective privacy.

That would be true _if_ privacy.item.cookies would _only_ be checked if privacy.item.history was true, but in reality if someone sets privacy.item.history and privacy.item.cookies to false, and updates to a nightly (or beta) containing this bug, they'd end up with privacy.item.cookies set to true and the first time they close Firefox all of their cookies will get wiped out (but not any history items).

That said, I think it's wrong to assume that cookies are always associated with logins.  There is nothing in theory which makes this assumption valid, and in reality web apps such as whoisi.com use just cookies to remember the settings of their users, so cleaning up all cookies without the user's consent may actually make them re-create their settings on some sites (which may be non-trivial).

I really think that we should either take the patch already attached to this bug, or at least a version of it which sets privacy.item.cookies to true iff privacy.sanitize.sanitizeOnShutdown is false (to make sure we don't affect the users which have chosen to clear some parts of their history at shutdown).

Beltzner, what do you think?

Comment 19

[u88484]

(In reply to comment #15)
> Although I'm well-informed theoretically, it took me some time to
> figure out why I was suddenly logged out everywhere: I used to clear my
> download history on shutdown. Only my download history, nothing else. Now
> because of this bug, cookies have been added to my highly customized list.
> I don't think there's a way to interpret this as the correct behavior. Fixing
> this also seems P1-worthy to me, as I don't see what kind of beta feedback
> could possibly make this bug less critical.

Ah ha, that is why I keep being logged out of most sites recently.

Comment 20

[Michael Lefevre]

Also, although it is the way the UI is being approached, given the comments here and on MozillaZine I'm not sure privacy is the only concept to think about.  Some people (although maybe not "average" users) clear download logs, web caches and form entry history for reasons other than wanting other people using the same computer not to find stuff.

Comment 21

[Johnathan Nightingale [:johnath]]

Another option here could be to just drop the privacy.item->privacy.cpd migration code entirely, and specify out all the privacy.cpd defaults.

That way we don't change the "clear silently on shutdown" behaviour for people who use it, but people who use the dialog have to potentially fix some checkboxes if they had different behaviour in the past. It's a simpler code change, and any dataloss risk is tied to people manually interacting with a (changed, probably-curiosity-provoking) dialog.

What do you think, mconnor?

Comment 22

[(no longer active)]

I can provide a patch for Johnathan's suggestion as well, FWIW.

Comment 23

[Mike Connor [:mconnor]]

Johnath's solution doesn't really work either, since it means we can't change the default for privacy.item.cookies, which I think we definitely want to do.

So, here's the new proposal, which keeps everything in mind that we want to do:

* don't use privacy.item.* for anything in 3.5, add privacy.clearOnShutdown for the shutdown prefs
* leave the defaults for privacy.item as they were in 3.0
* create defaults for privacy.cpd and privacy.clearOnShutdown to match the behaviour we want.
* on first run (can be somewhat async, we won't hit this in < 15 seconds from first startup of 3.5) we do a one-off migration pass to clone privacy.item's values to both .cpd and .clearOnShutdown, conditionally on one of the following two cases being true:
** anything in privacy.item has a user (non-default) value.
** the clearOnShutdown pref is enabled

Thoughts?

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

That seems fine to me. As someone pointed out, we probably need to create some saner way in the long run to deal with migrating prefs, but this works for the issues at hand.

Comment 25

[Johnathan Nightingale [:johnath]]

(In reply to comment #23)
> Johnath's solution doesn't really work either, since it means we can't change
> the default for privacy.item.cookies, which I think we definitely want to do.
> 
> So, here's the new proposal, which keeps everything in mind that we want to do:
> 
...
> * on first run (can be somewhat async, we won't hit this in < 15 seconds from
> first startup of 3.5) we do a one-off migration pass to clone privacy.item's
> values to both .cpd and .clearOnShutdown, conditionally on one of the following
> two cases being true:
> ** anything in privacy.item has a user (non-default) value.
> ** the clearOnShutdown pref is enabled
> 
> Thoughts?

Overall sounds fine - but why a startup timer instead of on first invocation of the sanitizer code?

Comment 26

[(no longer active)]

(In reply to comment #23)
> * don't use privacy.item.* for anything in 3.5, add privacy.clearOnShutdown for
> the shutdown prefs
> * leave the defaults for privacy.item as they were in 3.0
> * create defaults for privacy.cpd and privacy.clearOnShutdown to match the
> behaviour we want.
> * on first run (can be somewhat async, we won't hit this in < 15 seconds from
> first startup of 3.5) we do a one-off migration pass to clone privacy.item's
> values to both .cpd and .clearOnShutdown, conditionally on one of the following
> two cases being true:
> ** anything in privacy.item has a user (non-default) value.
> ** the clearOnShutdown pref is enabled

This should be sanitizeOnShutdown, right?

Comment 27

[(no longer active)]

So I take it that the suggestion in comment 23 is fine and we should move to a patch based on it, right?

Comment 29

[(no longer active)]

Ping?

Comment 30

[Mike Connor [:mconnor]]

Yes, we should do that.  Note on the timing of the migration: it doesn't matter to me when, it's just that it's easier than trying to migrate on first actual use, it's probably simpler to do that, even if it lives in the Sanitizer object.

Comment 31

[(no longer active)]

So, let me see if I have understood everything correctly:

(In reply to comment #23)
> * don't use privacy.item.* for anything in 3.5, add privacy.clearOnShutdown for
> the shutdown prefs

This should mean renaming the "privacy.item" prefDomain for Sanitizer objects to something new, such as "privacy.aspect", and also "sanitizeOnShutdown" to something like "clearOnShutdown", right?

> * leave the defaults for privacy.item as they were in 3.0

Which means undoing the change from bug 480169.

> * create defaults for privacy.cpd and privacy.clearOnShutdown to match the
> behaviour we want.
> * on first run (can be somewhat async, we won't hit this in < 15 seconds from
> first startup of 3.5) we do a one-off migration pass to clone privacy.item's
> values to both .cpd and .clearOnShutdown>

This also includes copying privacy.item.* to privacy.aspect.*, right?

> conditionally on one of the following
> two cases being true:
> ** anything in privacy.item has a user (non-default) value.
> ** the clearOnShutdown pref is enabled

Did you mean privacy.sanitizeOnShutdown here?

Comment 32

[(no longer active)]

(In reply to comment #23)
> * don't use privacy.item.* for anything in 3.5, add privacy.clearOnShutdown for
> the shutdown prefs

Or perhaps you were referring to privacy.clearOnShutdown.*?  Now that I think of it, this should be what you were proposing, right?

Comment 33

[Mike Connor [:mconnor]]

(In reply to comment #31)
> So, let me see if I have understood everything correctly:
> 
> (In reply to comment #23)
> > * don't use privacy.item.* for anything in 3.5, add privacy.clearOnShutdown for
> > the shutdown prefs
> 
> This should mean renaming the "privacy.item" prefDomain for Sanitizer objects
> to something new, such as "privacy.aspect", and also "sanitizeOnShutdown" to
> something like "clearOnShutdown", right?

I think ideally the consumer sets the correct prefDomain, and we don't default to anything in the Sanitizer object.  Not sure how backwards compat that is for other consumers.  Or if we should care.

> > * leave the defaults for privacy.item as they were in 3.0
> 
> Which means undoing the change from bug 480169.

Yes, technically.  But we'll only care about those prefs if we hit the conditions below.

> > * create defaults for privacy.cpd and privacy.clearOnShutdown to match the
> > behaviour we want.
> > * on first run (can be somewhat async, we won't hit this in < 15 seconds from
> > first startup of 3.5) we do a one-off migration pass to clone privacy.item's
> > values to both .cpd and .clearOnShutdown>
> 
> This also includes copying privacy.item.* to privacy.aspect.*, right?

What's privacy.aspect supposed to be?  We have privacy.item so we can usefully tell if there's non-default settings for anything on migration.  We have .cpd for Clear Recent History and .clearOnShutdown for the shutdown prefs.

> > conditionally on one of the following
> > two cases being true:
> > ** anything in privacy.item has a user (non-default) value.
> > ** the clearOnShutdown pref is enabled
> 
> Did you mean privacy.sanitizeOnShutdown here?

privacy.sanitize.sanitizeOnShutdown, yes.

Comment 34

[(no longer active)]

Attachment: Patch v2

This is a WIP patch which I have right now.  It's untested as of now (I had to compile a full build since my tree was so old and broken), but I thought I'd post it here while the compile goes through.

It implements all of mconnor's suggestion I think.  I'm not sure how to test it yet; I might end up exposing the migrating function and just call it from a test file in a synthetic environment, but I still need to figure out the details.

(In reply to comment #33)
> I think ideally the consumer sets the correct prefDomain, and we don't default
> to anything in the Sanitizer object.  Not sure how backwards compat that is for
> other consumers.  Or if we should care.

Hmm, let's not do that until after 3.5 is shipped because extensions may be relying on the Sanitizer object providing a default for prefDomain...  In this patch I just changed the default prefDomain to privacy.clearOnShutdown.

Comment 35

[Mike Beltzner [:beltzner, not reading bugmail]]

Ehsan, think we'll have a patch for review sometime today?

Comment 36

[(no longer active)]

(In reply to comment #35)
> Ehsan, think we'll have a patch for review sometime today?

Yes, definitely.

Comment 37

[(no longer active)]

Comment on attachment 378386 [details] [diff] [review]
Patch v2

I think this should be good for review.

Comment 38

[Mike Connor [:mconnor]]

Comment on attachment 378386 [details] [diff] [review]
Patch v2


>-pref("privacy.item.history",     true);
>-pref("privacy.item.formdata",    true);
>-pref("privacy.item.passwords",   false);
>-pref("privacy.item.downloads",   true);
>-pref("privacy.item.cookies",     true);
>-pref("privacy.item.cache",       true);
>-pref("privacy.item.sessions",    true);
>-pref("privacy.item.offlineApps", false);
>-pref("privacy.item.siteSettings", false);

The tricky part here is that I want to migrate the Fx3 settings, but default to the 3.5 settings we want.  Maybe just have .cookies, since that's the only one that's different?  That'd work with the migration code you, since then we'd have the default false value in the privacy.item branch, plus any user values.

>+pref("privacy.clearOnShutdown.history",     true);
>+pref("privacy.clearOnShutdown.formdata",    true);
>+pref("privacy.clearOnShutdown.passwords",   false);
>+pref("privacy.clearOnShutdown.downloads",   true);
>+pref("privacy.clearOnShutdown.cookies",     false);

This should default to true.  See comment above.

>+    if (!prefService.getBoolPref("privacy.sanitize.migrateFx3Prefs")) {
>+      var cpdBranch = prefService.getBranch("privacy.cpd.");
>+      var itemBranch = prefService.getBranch("privacy.item.");
>+      var clearOnShutdownBranch = prefService.getBranch("privacy.clearOnShutdown.");
>+      var itemCount = { value: 0 };
>+      var itemArray = clearOnShutdownBranch.getChildList("", itemCount);

We should just get the privacy.item branch here, since if we only have .cookies plus anything that has a non-default value we'll have fewer items to iterate.  That would also mean we can move the getBranch calls for cpd/clearOnShutdown into the last code block since they're unnecessary unless we get that far.

>diff --git a/browser/base/content/sanitize.js b/browser/base/content/sanitize.js
>--- a/browser/base/content/sanitize.js
>+++ b/browser/base/content/sanitize.js
>@@ -48,17 +48,17 @@ Sanitizer.prototype = {
>       this.items[aItemName].clear();
>   },
> 
>   canClearItem: function (aItemName)
>   {
>     return this.items[aItemName].canClear;
>   },
>   
>-  prefDomain: "privacy.item.",
>+  prefDomain: "privacy.clearOnShutdown.",

Hmm.  I think I'd rather default this to privacy.cpd. and have the shutdown case set the prefDomain.  Or better yet, default to nothing, and require consumers to set it explicitly.

Anyway, close, but not quite there.  I'll be up late if you want to yell at me on IRC... :D

Comment 39

[Mike Beltzner [:beltzner, not reading bugmail]]

Connor: with those issues addressed would he get an r+? I'm a little worried about cycle time here ...

Comment 40

[(no longer active)]

Attachment: Patch (v2.1)

New version based on the review comments and IRC conversations.

Comment 41

[Dão Gottwald [:dao]]

Comment on attachment 378816 [details] [diff] [review]
Patch (v2.1)

>+      // See if any privacy.item prefs are set
>+      var doMigrate = false;
>+      itemArray.forEach(function (name) {
>+        if (itemBranch.prefHasUserValue(name))
>+          doMigrate = true;
>+      });

Something like that (it's untested) should break once the condition is true:

> var doMigrate =
>   itemArray.some(function (name) itemBranch.prefHasUserValue(name));

Comment 42

[Dão Gottwald [:dao]]

<dao> ehsan: why are you doing itemBranch.clearUserPref(name);?
<ehsan> dao: just to remain neat, since those prefs won't be of any use once we
<ehsan> are finished with the migration
<dao> ehsan: if you just leave it, you'd support switching back / downgrading to fx3
<ehsan> dao: hmm, that's a valid point
<dao> ehsan: and won't cpdBranch.setBoolPref(name, itemBranch.getBoolPref(name)); throw if the pref doesn't have a user value on itemBranch?
<dao> "the pref" meaning "some of the old prefs except cookies"
<dao> since cookies still has a default value
<ehsan> dao: you're right
<ehsan> new patch on its way

Comment 43

[(no longer active)]

Attachment: Patch

Comment 44

[(no longer active)]

Attachment: Patch (v2.2)

Comment 45

[(no longer active)]

Attachment: Patch (v2.3)

Log the migration exception to the console instead of ignoring it...

Comment 46

[Mike Connor [:mconnor]]

Comment on attachment 378829 [details] [diff] [review]
Patch (v2.3)

r=me, thanks!

Comment 47

[Dão Gottwald [:dao]]

(In reply to comment #45)
> Log the migration exception to the console instead of ignoring it...

Are we afraid that exceptions will be thrown other than the expected ones?

Comment 48

[Dave Townsend [:mossop]]

Landed on trunk: http://hg.mozilla.org/mozilla-central/rev/d0ae0b099a40

Comment 49

[(no longer active)]

(In reply to comment #47)
> (In reply to comment #45)
> > Log the migration exception to the console instead of ignoring it...
> 
> Are we afraid that exceptions will be thrown other than the expected ones?

Yes, in fact I think the exceptions that you were worried about actually shouldn't be thrown because we're iterating on the item branch now...

Comment 50

[Dão Gottwald [:dao]]

Oh, right, I didn't notice that itemArray holds only those prefs that actually exist.

Comment 51

[Dave Townsend [:mossop]]

Pushed to branch: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/525e43509e79

FWIW I think we should avoid the (function() { })() style, I don't see a real need for it in this case and it is quite different to the general style in browser.js. I'd also question why we didn't use the migration path that already exists in nsBrowserGlue.js rather than rolling a new one. I guess the BrowserGlue is in the direct startup I don't think it should be causing enough of a hit to be concerned about.

Comment 52

[tabmix.onemen]

its also need to clear "privacy.item." that has user value

Comment 53

[Dão Gottwald [:dao]]

(In reply to comment #52)
> its also need to clear "privacy.item." that has user value

see comment 42

Comment 54

[Aakash Desai [:aakashd]]

verified FIXED on builds following the steps in comment #2:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090522 Minefield/3.6a1pre ID:20090522032716

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090522 Shiretoko/3.5pre ID:20090522030802

Comment 55

[Janos Wild]

I am using 3.5 beta 4 Hungarian (I mention this 'cause I had to translate the options). My History is saved for 21 days, and all cokies (incl. 3rd parties) saved till they expire. Other checkboxes are empty, so the browser does not start in privacy mode, does not store download history, neither does the form data. Delete stuff when FF closes is also inactive.

So everything worked fine, until I didn't try the privacy mode. I tried it, and switched back. Since then, everytime FF closes, it clears the cookies. So my cookies got deleted, although I do not have the history clearing setting active, I've just tried private browsing. History is saved, but cookies gone.

Comment 56

[Dão Gottwald [:dao]]

(In reply to comment #55)
Please test with a build from <http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-1.9.1/>, and if it still fails, file a new bug.

Comment 57

[Janos Wild]

Thanks, I've installed it: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090601 Shiretoko/3.5pre

But still, every cookie vanishes, when I close the browser. Even if I start the Mozilla Firefox\firefox.exe (3.0.10) istead of Mozilla Firefox 3.5 Beta 4\firefox.exe or Shiretoko\firefox.exe, it downgrades the verison, but cookies always got deleted at exit...

New bug filed: https://bugzilla.mozilla.org/show_bug.cgi?id=495792