380852 - clear private data doesn't clear site-specific settings

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Myk Melez [:myk] [@mykmelez]]

The "clear private data" feature doesn't clear the site-specific preferences in the exceptions lists for the "block pop-up windows", "load images automatically", "accept cookies from sites", "remember passwords for sites", and "warn me when sites try to install add-ons" settings.  And once the site-specific preferences service lands, it won't clear those site-specific preferences either.

Shouldn't it clear all these preferences?  They do, after all, include private information about which sites the user has visited (since users generally don't set these site-specific preferences for sites they haven't visited).

Comment 1

[Arthur]

This is even more important for things where the user doesn't actively set a site preference, like page zoom. Those hosts are stored in the content-prefs.sqlite db and stay there without that the user ever would have taken an action telling Firefox to remember anything (unlike "don't block pop-ups from ..." which at least let's the user assume that it *could* be safed for later sessions).

Comment 2

[Arthur]

Asking for blocking FF3. Especially implicit per-site settings like page-zoom will lead to a large list of sites left in the profile even when supposedly clearing all private data.

Comment 3

[(not reading, please use [e-mail] instead)]

about the privacy issue with site specific prefs, I'm worried about the scenario where:

1)  I go to http://www.replaytv.com/
2)  I zoom in because I'm old and blind
3)  I clear my browsing history ("Tools | Clear Private Data... | Browsing History")

Later, when my TiVo uses my browser and goes to http://www.replaytv.com/ to see if I've been playing around, all hell breaks loose because the page loads and I'm zoomed in.

Myk, could we / should we spin off the per-site settings to another bug?

Comment 4

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #3)
> Myk, could we / should we spin off the per-site settings to another bug?

Yup, sounds like a good idea.  I've filed bug 407910 on the issue.

Comment 6

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

Needs attention and a patch, not going to block the first branch release on this. I'm unsure about the relationship between this and bug 407910, though. Is this requesting an entry for site-specific settings whereas bug 407910 is requesting that per-site settings vanish with history clearing?

Comment 9

[Mike Connor [:mconnor]]

Johnath, can you take a stab at this, since you've now touched it last?

Comment 10

[Johnathan Nightingale [:johnath]]

(In reply to comment #6)
> Needs attention and a patch, not going to block the first branch release on
> this. I'm unsure about the relationship between this and bug 407910, though. Is
> this requesting an entry for site-specific settings whereas bug 407910 is
> requesting that per-site settings vanish with history clearing?

This is requesting an entry for site permissions like "can open popups" or "can install addons" - things the user has explicitly authorized and which, implementation wise, are managed by the nsIPermissionManager.  Bug 407910 is about implicitly clearing per-site memory for things like zoom level whenever browser history is cleared.

It would be nice to have both things.

(In reply to comment #9)
> Johnath, can you take a stab at this, since you've now touched it last?

I'll take a look.  Given that permissions are explicitly set by giving consent to something in a notification bar or equivalent, I'm treating them as data, like saved passwords. That is a good thing too, since they are not flagged with creation/access times.

Comment 11

[Johnathan Nightingale [:johnath]]

Attachment: Add "Site-specific Settings" to Data section

- Adds site-specific settings checkbox to the "Data" section
- Adds associated pref, sanitizer code, and strings
- Adds test of sanitizer function
- Removes 2 references to junk privacy.item.siteprefs preference that isn't used anywhere, and was added by Ben at the dawn of time, as part of a patch that, even then, didn't use it.

Comment 12

[Johnathan Nightingale [:johnath]]

It would be awfully nice to get bug 407910 as well so that we could clear those settings implicitly as part of this action.

Comment 13

[Mike Connor [:mconnor]]

I agree, can we lump that clearing action into this item, rather than the patch on that bug?

Comment 14

[Johnathan Nightingale [:johnath]]

(In reply to comment #13)
> I agree, can we lump that clearing action into this item, rather than the patch
> on that bug?

Actually that's probably the right thing to do - Myk, what do you think?

Comment 15

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #14)
> (In reply to comment #13)
> > I agree, can we lump that clearing action into this item, rather than the patch
> > on that bug?
> 
> Actually that's probably the right thing to do - Myk, what do you think?

I agree, since both sets of settings are the same from the user's perspective, even though they have different underlying datastores.

Comment 16

[Johnathan Nightingale [:johnath]]

Attachment: Combined with Myk's patch

Basically a straight merge of Myk's patch, but with the sanitize code moved to siteSettings instead of being part of history.  I also removed the try-catch around the clearing code in Myk's, because the sanitize() function expects exceptions, and has a mechanism for reporting them back to its callers after finishing up the other sanitize tasks.

Comment 17

[Johnathan Nightingale [:johnath]]

mconnor - I forgot to clear the whiteboard when I attached the new patch, so I imagine you've been overlooking this one, but this is the real patch for review.

Comment 18

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

This is a P2, assuming it comes with tests and can be checked for correctness. If we get it for Beta 3, though, more's the better.

Comment 19

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

Comment on attachment 352787 [details] [diff] [review]
Combined with Myk's patch

Got here by way of bug 407910, which is r?me, so I'll steal this...

>diff --git a/browser/base/content/sanitize.js b/browser/base/content/sanitize.js

>+      get canClear()

This should be fairly easy to implement correctly (already possible with nsIPermissionManager, and nsIContentPrefService could expose it pretty easily) - followup bug?

>diff --git a/browser/base/content/test/browser_sanitize-sitepermissions.js b/browser/base/content/test/browser_sanitize-sitepermissions.js

Would be nice to have a test to ensure that this clears nsIContentPrefService prefs as well, though for the moment that requires checking the DB directly (at least until that followup is fixed!)

>diff --git a/browser/locales/en-US/chrome/browser/sanitize.dtd b/browser/locales/en-US/chrome/browser/sanitize.dtd

>+<!ENTITY itemSiteSettings.label       "Site-specific Settings">
>+<!ENTITY itemSiteSettings.accesskey   "S">

Hmm, too bad this missed the b3 string freeze! :( One string+accesskey shouldn't be too bad, though you probably might need to coordinate with Axel? I suppose we'll be having another string freeze for RCs.

>diff --git a/toolkit/components/contentprefs/public/nsIContentPrefService.idl b/toolkit/components/contentprefs/public/nsIContentPrefService.idl

>-[scriptable, uuid(746c7a02-f6c1-4869-b434-7c8b86e60e61)]
>+[scriptable, uuid(9c72a562-98c7-428c-ad21-1e04de1a14f8)]
> interface nsIContentPrefObserver : nsISupports

Noticed this in the other bug, this is changing the wrong IID (nsIContentPrefObserver instead of nsIContentPrefService).

r=me with that addressed.

Comment 20

[Axel Hecht [:Pike]]

Why does this block 3.1 when it, according to my reading of the history, didn't block 3.0?

Comment 21

[Johnathan Nightingale [:johnath]]

Attachment: Updated patch for checkin

(In reply to comment #19)
> (From update of attachment 352787 [details] [diff] [review])
> >diff --git a/browser/base/content/sanitize.js b/browser/base/content/sanitize.js
> 
> >+      get canClear()
> 
> This should be fairly easy to implement correctly (already possible with
> nsIPermissionManager, and nsIContentPrefService could expose it pretty easily)
> - followup bug?

Yeah, I agree.  bug 474431 filed.

> >diff --git a/browser/base/content/test/browser_sanitize-sitepermissions.js b/browser/base/content/test/browser_sanitize-sitepermissions.js
> 
> Would be nice to have a test to ensure that this clears nsIContentPrefService
> prefs as well, though for the moment that requires checking the DB directly (at
> least until that followup is fixed!)

I've made note of that in the followup as well, unless you'd rather that we test it here using the DB prodding and then change it once that bug is fixed?

> >diff --git a/toolkit/components/contentprefs/public/nsIContentPrefService.idl b/toolkit/components/contentprefs/public/nsIContentPrefService.idl
> 
> >-[scriptable, uuid(746c7a02-f6c1-4869-b434-7c8b86e60e61)]
> >+[scriptable, uuid(9c72a562-98c7-428c-ad21-1e04de1a14f8)]
> > interface nsIContentPrefObserver : nsISupports
> 
> Noticed this in the other bug, this is changing the wrong IID
> (nsIContentPrefObserver instead of nsIContentPrefService).

Nice catch. Fixed.

(In reply to comment #20)
> Why does this block 3.1 when it, according to my reading of the history, didn't
> block 3.0?

We're giving this dialog more prominence in 3.1 by featuring it on the Private Browsing page, which we expect to be a high-visibility feature.  I also think that reasoning from prior releases can get us into an unhealthy loop, that this is something we arguably should have fixed when we first introduced site specific prefs, and the fact that it's lasted this long shouldn't be a good reason to make it last longer.  :)

On the other hand, though, we are past string freeze. What is our strategy here, Axel? 

In any event, I'll land this on trunk as soon as I finish a clean browser-chrome test pass, while we figure out the branch logistics.  Carrying forward gavin's r+

Comment 22

[Johnathan Nightingale [:johnath]]

Landed on mozilla-central:

http://hg.mozilla.org/mozilla-central/rev/7b60ede5399d

Comment 23

[Marcia Knous [:marcia]]

Running  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090121 Minefield/3.2a1pre, I am able to clear the site specific setting for all the categories myk mentions in Comment 0 except the password exceptions. If I set an exception and then clear using the site specific checkbox, the password exception still remains.

Comment 24

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

Marcia's comment implies REOPENED.

Comment 25

[Johnathan Nightingale [:johnath]]

Yeah, I consider this a follow-up bug. We have support with this bug for clearing site-specific settings in general, but "never save this password" is a special case that we're missing because it doesn't use nsIPermissionManager or nsIContentPrefService, instead using its own table in the pwmgr database.  I've filed 474792 and nominated it to block final release (I don't think it requires beta exposure).

Thanks to Marcia for finding it, though. I didn't even realize that pwmgr was special in this way.

Comment 26

[Marcia Knous [:marcia]]

Verifying fixed on the trunk based on Comment 23 using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090121 Minefield/3.2a1pre. Bug 474792 is on file for the Password manager issue.

Comment 27

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

The mozilla-1.9.1 tree is open for string changes again. Please make sure the bug stays marked as latel10n, and check in when the tree is green!

Comment 28

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

--> P1, as this bug will require the wider feedback of a beta release or is of sufficient complexity that we should be looking at it sooner, not later.

Comment 29

[Johnathan Nightingale [:johnath]]

Pushed to 1.9.1

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/63840aa81ceb

Comment 30

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Er... why did removeGroupedPrefs get added to the middle of this interface?  It should have been added to the end (either way, with or without the iid change), and there would have been no need to rev the iid..

Comment 31

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090406 Shiretoko/3.5b4pre

Comment 33

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=7716 added to Litmus.