clear private data doesn't clear site-specific settings

VERIFIED FIXED in Firefox 3.6a1

Status

()

Firefox
General
P1
normal
VERIFIED FIXED
11 years ago
3 years ago

People

(Reporter: myk, Assigned: johnath)

Tracking

({late-l10n, privacy, verified1.9.1})

Trunk
Firefox 3.6a1
late-l10n, privacy, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 -
blocking-firefox3.5 +
wanted-firefox3 +
blocking1.9.0.1 -
wanted1.9.0.x +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

Updated

11 years ago
Keywords: privacy

Comment 1

10 years ago
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

10 years ago
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.
Flags: blocking-firefox3?

Updated

10 years ago
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
Keywords: uiwanted
Priority: -- → P4
Target Milestone: --- → Firefox 3 M11
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?
(Reporter)

Updated

10 years ago
Depends on: 407910
(Reporter)

Comment 4

10 years ago
(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.
Flags: blocking1.9.0.1?
Flags: blocking-firefox3.1?

Updated

10 years ago
Duplicate of this bug: 440710
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?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.1?
Flags: blocking1.9.0.1+
Flags: blocking-firefox3.1?
Flags: blocking-firefox3.1+
Flags: blocking1.9.0.1+ → blocking1.9.0.1-

Updated

9 years ago
Duplicate of this bug: 450851

Updated

9 years ago
Duplicate of this bug: 466832
Johnath, can you take a stab at this, since you've now touched it last?
Assignee: nobody → johnath
(Assignee)

Comment 10

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

Comment 11

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

Comment 12

9 years ago
It would be awfully nice to get bug 407910 as well so that we could clear those settings implicitly as part of this action.
Status: NEW → ASSIGNED
I agree, can we lump that clearing action into this item, rather than the patch on that bug?
(Assignee)

Comment 14

9 years ago
(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?
(Reporter)

Comment 15

9 years ago
(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.

Updated

9 years ago
Attachment #352368 - Flags: review?(mconnor)

Updated

9 years ago
Whiteboard: [needs new patch]
(Assignee)

Comment 16

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

Updated

9 years ago
Attachment #352368 - Attachment is obsolete: true
(Assignee)

Comment 17

9 years ago
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.
Whiteboard: [needs new patch]
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.
Priority: P4 → P2

Updated

9 years ago
Whiteboard: [has patch][needs review mconnor]
Attachment #352787 - Flags: review?(mconnor) → review+
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.
Blocks: 407910
No longer depends on: 407910
Whiteboard: [has patch][needs review mconnor] → [has patch]

Updated

9 years ago
Whiteboard: [has patch] → [needs new patch][has reviews]
Keywords: late-l10n

Comment 20

9 years ago
Why does this block 3.1 when it, according to my reading of the history, didn't block 3.0?
Whiteboard: [needs new patch][has reviews] → [needs new patch][has reviews][missed string freeze]
(Assignee)

Updated

9 years ago
Blocks: 474431
(Assignee)

Comment 21

9 years ago
Created attachment 357798 [details] [diff] [review]
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+
Attachment #352787 - Attachment is obsolete: true
Attachment #357798 - Flags: review+
(Assignee)

Updated

9 years ago
Whiteboard: [needs new patch][has reviews][missed string freeze] → [missed string freeze]
(Assignee)

Comment 22

9 years ago
Landed on mozilla-central:

http://hg.mozilla.org/mozilla-central/rev/7b60ede5399d
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
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.
Flags: in-litmus?
Marcia's comment implies REOPENED.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

9 years ago
Depends on: 474792
(Assignee)

Comment 25

9 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
No longer depends on: 474792
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Depends on: 474792
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.
Status: RESOLVED → VERIFIED

Updated

9 years ago
Target Milestone: Firefox 3 beta3 → Firefox 3.2a1

Updated

9 years ago
Keywords: uiwanted
Whiteboard: [missed string freeze] → [needs 1.9.1 landing after b3]
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!
--> 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.
Priority: P2 → P1
(Assignee)

Comment 29

9 years ago
Pushed to 1.9.1

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/63840aa81ceb
Keywords: fixed1.9.1
Whiteboard: [needs 1.9.1 landing after b3]
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..
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
Keywords: fixed1.9.1 → verified1.9.1

Updated

9 years ago
Duplicate of this bug: 491867
https://litmus.mozilla.org/show_test.cgi?id=7716 added to Litmus.
Flags: in-litmus? → in-litmus+

Updated

8 years ago
Depends on: 506446

Updated

4 years ago
See Also: → bug 771630
Depends on: 771630
Duplicate of this bug: 460342
You need to log in before you can comment on or make changes to this bug.