497656 - No UI for "privacy.cpd.passwords/offlineApps" pref (if set, clears all saved passwords and offline apps data on 'clear recent history')

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Open about:config
 2. Flip "privacy.cpd.passwords" to 'true'
 3. Visit URL (or any login page)
 4. Type in username and password (at URL, username='foo' password='bar')
 5. Click "Remember" in notification-bar, to save password
 6. Ctrl-Shift-Delete to bring up "Clear Recent History"
 7. Uncheck all boxes -- so, nothing should be cleared.
 8. Press "Clear Now"
 9. Inspect saved passwords in Prefs / Security / Saved Passwords. (and/or restart firefox and visit login page again)

ACTUAL RESULTS:
 No saved passwords are stored.

I'm not sure what this pref is supposed to be for, but it was somehow set to "true" in my testing profile, which caused me to run into this bug (and lose all my saved passwords) when I cleared my cache.  I'm sure I never manually set it, and I can't find any UI that would set/clear it.

Comment 1

[Daniel Holbert [:dholbert]]

Adding dependency to bug 488706, because that's when this pref was created (/ renamed)

Comment 2

[Nochum Sossonko [:Natch]]

Regression from bug 488706, blocks bug 488706.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: screenshot of "clear recent history" UI

If nothing else, this is perhaps a UI bug (principle of least surprise).

See the attached screenshot of the "clear recent history" dialog -- it *appears* to give the user fine-grained control over what will & won't be cleared, but in actuality, it may end up clearing items beyond those that are listed.

Comment 4

[Daniel Holbert [:dholbert]]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090611 Minefield/3.6a1pre

Comment 5

[Johnathan Nightingale [:johnath]]

This is basically a dup of bug 490702 - we want to delete that code entirely, now that it's not UI-exposed.

Comment 6

[Daniel Holbert [:dholbert]]

Setting this bug to depend on that one -- this seems marginally more appropriate than duping, since that bug is on a category of cleanup to be done, whereas this bug describes a real-world dataloss symptom from a particular use-case.

Moving 488706 back into 'depends' so as not to create a circular dependency.

Comment 7

[Justin Dolske [:Dolske]]

bug 490702 suggests deleting the code in comment 0, but then reverts that decision in comment 2.

Comment 8

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Can this affect any user that didn't run a beta and set that pref before the UI disappeared?  Not clear to me why this was nominated, since there's no comment from the nominator!

Comment 9

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

Connor and Eshan might know more here, but I think this is the sort of thing that can only be set if you moved from beta back down to 3.0 with the same profile at a strange time in development.

Comment 10

[Mike Connor [:mconnor]]

No... with the migration from privacy.item, if you have this enabled in Fx3 it'll still be there in fx3.5... we should just kill that code, IMO.

Comment 11

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

Honestly, this is a ridiculous thing for someone to have set in the first place, and not something that I think affects real users. Doesn't block release. I agree that we should fix it.

Comment 12

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

Oh, and by "fix it" I mean "no longer observe that ridiculous preference".

Comment 13

[Nochum Sossonko [:Natch]]

(In reply to comment #11)
> Honestly, this is a ridiculous thing for someone to have set in the first
> place, and not something that I think affects real users. Doesn't block
> release. I agree that we should fix it.

Hmm? That's pref has been around _in_the_ui_ for quite a while now:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla%2Fbrowser%2Fbase%2Fcontent%2Fsanitize.xul&rev=&cvsroot=%2Fcvsroot&mark=155#134

Seems like Aviary 1.5, it's a standard delete cookie pref, what's so unusual about it? Most browsers have a way to clear cookies, Mozilla persists the user's choice for it. I'm not saying it should block necessarily, but it's a pretty standard idea...

Comment 14

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

I'm not going to debate whether or not it's rational to tell Firefox to save a password, then have it delete that password when you exit. We can get into that sort of productive discussion outside of the bug.

Yes, the UI has been around forever. We have better and more sensible UI for controlling one's privacy now. Users are welcome to go and fiddle all of those new and wonderful knobs. What I am saying, and what mconnor is suggesting in comment 10, and indeed what I believe is the point of bug 490702, is that we should stop listening to those antiquated preferences, the way that we have all stopped listening to 8-track tapes and people warning us that the death of the CISC chip is imminent or that Y2K will be a destructive computer apocalypse.

Comment 15

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

This is about passwords, not cookies.  The user explicitly controls whether a password is remembered, which is why it's not really a very useful control.

Comment 16

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

With a heavy heart, I restore the ?s

Comment 17

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

Sorry, I think I suffered bugzilla fail, ftr:

 - not blocking 3.5
 - yes, blocks 3.6
 - maybe take it on a point release

Comment 18

[Nochum Sossonko [:Natch]]

Right, s/cookies/passwords, everything still applies. I'm *not* debating
whether or not to include this option. That decision has been made already. The
problem here is that userA sets his clear password option to true in 3.0.x then
upgrades to 3.5 and can't undo it in the ui! This isn't a shutdown pref, it's a
persistence pref for the new sanitize dialog.

The password deletion pref could be useful once in a while, so userA sets it to
true when he changed all his passwords (or for whatever reason) while on
Firefox 3.0. Then he upgrades. Now _every_time_ he uses the "Clear Recent
History", which does not provide an option to delete passwords, all his
passwords will unwittingly be deleted.

I'm not trying to start a debate here, and I'll discontinue my rants. I'm just
trying to make sure everything is understood here. Firefox ui will lock the
pref to delete all the passwords with no option to unset it. Peace.

Comment 19

[Drew Willcoxon :adw]

Beyond whether or not the passwords pref should be set, Daniel's comment 3 is right.  CRH hands off to the sanitize code, which simply looks at which sanitization prefs are set to true and clears those.  We don't expose passwords or offlineApps in the CRH UI, but if they're true they'll be cleared on CRH.

Comment 20

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

Please stop messing with the flags. The blocking decisions are final, all is understood. Thanks.

Comment 21

[Daniel Holbert [:dholbert]]

(In reply to comment #18)
> The password deletion pref could be useful once in a while, so userA sets it to
> true when he changed all his passwords (or for whatever reason) while on
> Firefox 3.0.

Natch: If this pref were actually settable via UI in 3.0, I'd be lobbying for this to be a 3.5-blocker as well, but I don't think the UI actually controlled the pref in Firefox 3.0.

I just tried tweaking the checkboxes in the "Clear Private Data" dialog in Firefox 3.0.10, and this didn't change my "privacy.item.*" prefs at all, nor did these checkbox-settings persist in subsequent openings of the same dialog.  Those checkboxes only seem to have become persistent *after* Firefox 3.0.x.

I'm guessing my privacy.*.passwords pref was inadvertantly flipped on in some intermediate development build, but assuming our users are running release (or even probably beta) Firefox builds, they shouldn't run into this issue.

Comment 22

[Nochum Sossonko [:Natch]]

Tools->Options->Privacy->Private Data->Settings is where this gets set in 3.0.x.

Comment 23

[Mike Connor [:mconnor]]

Correct.  Still definitely not a blocker, IMO.

Comment 24

[Johnathan Nightingale [:johnath]]

Attachment: We could just do this...

Arguably easiest, rather than teaching the sanitizer when it should and shouldn't do these deletions, is to just not migrate the now-confusing prefs.

No tests here, yet.  It would be easy enough to automate setting some prefs, calling initializeSanitizer() and then checking pref values, but I wonder if that's too narrow - migration things could happen in other places and screw us up, in theory.  This might be better as a litmus test, where we can just "Create a 3.0 profile.  Flip these prefs.  Now open that pref in 3.5, and make sure they weren't migrated."  That part is hard to automate.

Drew, what do you think?

Comment 25

[Johnathan Nightingale [:johnath]]

(Let's all pretend I used !== instead of !=...)

Comment 26

[Dão Gottwald [:dao]]

(In reply to comment #25)
> !== instead of !=

What for?

Comment 27

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

My name is Mike Beltzner, and I approve of this solution. Not sure I give it a191, but it would need to at least pass unit tests and get reviewed before we get to that point.

Comment 28

[Drew Willcoxon :adw]

(In reply to comment #24)
> Drew, what do you think?

Since you asked -- I'm not able to hold the different pref branches, their uses, and migration paths in my head all at once.  Are cpd.passwords and cpd.offlineApps revealed anywhere in the UI except the CRH dialog?  If so, then that won't work.  If not, then OK by me.  The corresponding preference elements would need to be removed from sanitize.xul also, which is the point of bug 490702.

Comment 29

[Drew Willcoxon :adw]

(In reply to comment #28)
> uses, and migration paths in my head all at once.  Are cpd.passwords and
> cpd.offlineApps revealed anywhere in the UI except the CRH dialog?  If so, then
> that won't work.

Er, they default to false as you've said, so nevermind.

Comment 30

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 382845 [details] [diff] [review]
We could just do this...

Looks good to me. Looking at http://hg.mozilla.org/mozilla-central/rev/9498df7e5ecd , we removed the checkboxes for offlineApps and passwords, so there's no need to migrate these over, and the defaults are false which is fine.

Comment 31

[Johnathan Nightingale [:johnath]]

Also - try server unit test builds have all come up green

Comment 32

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

Comment on attachment 382845 [details] [diff] [review]
We could just do this...

a191=beltzner, please land immediately

Comment 33

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/releases/mozilla-1.9.1/rev/bdc37d522cfe

Comment 34

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/ddad1c32b194

Comment 35

[(no longer active)]

Removing relnote and wanted1.9.1.x since this landed on trunk and branch.

Comment 36

[Johnathan Nightingale [:johnath]]

Flagging for litmus per comment 24

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The patch on this bug fixes the problem only halfway. We still suffer from the dataloss when users are calling the CRH dialog directly via the tools menu. The CRH on shutdown part works fine instead. I have filed bug 499670 to cover the remaining issue.

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, sorry for the noise in comment 37. The fix on that bug works as expected.

Verified fixed on trunk and 1.9.1 with builds like:

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

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1) Gecko/20090616 Firefox/3.5 ID:20090616212246

(In reply to comment #24)
> up, in theory.  This might be better as a litmus test, where we can just
> "Create a 3.0 profile.  Flip these prefs.  Now open that pref in 3.5, and make
> sure they weren't migrated."  That part is hard to automate.

Juan or Al, could you add this test to the major upgrade path?

Comment 39

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

Mass change: adding fixed1.9.2 keyword

(This bug was identified as a mozilla1.9.2 blocker which was fixed before the mozilla-1.9.2 repository was branched (August 13th, 2009) as per this query: http://is.gd/2ydcb - if this bug is not actually fixed on mozilla1.9.2, please remove the keyword. Apologies for the bugspam)