Closed Bug 468700 Opened 16 years ago Closed 15 years ago

Having "Ask me Everytime" enabled for Cookies during Private Browsing is unusable

Categories

(Core :: Networking: Cookies, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: R.Kelley.Cook, Assigned: ehsan.akhgari)

References

Details

(Keywords: verified1.9.1)

Attachments

(2 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2) Gecko/20081201 Firefox/3.1b2 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2) Gecko/20081201 Firefox/3.1b2 (.NET CLR 3.5.30729)

If one enters Private Browsing mode with the "Ask me Everytime" enabled for cookies, and then one goes to a new website where it presents cookies.  There is the checkbox for "always use this option" is greyed out and it leads to a useabilty nightmare.

Reproducible: Always

Steps to Reproduce:
1. Either clear all your cookies, or choose a known site such as Amazon.com and clear all those.
2. Tools->Option->Privacy Select Accept Cookies and "Ask me everytime"
3. Enter Private Browsing Mode.
4. Go to www.amazon.com (in the previous example).

Actual Results:  
Prepare to get spammed with Accept cookie? notice  No way to say yes or no for the site as a whole.

Expected Results:  
Like in normal mode, allow you to choose always accept/reject from this site (until private browsing mode is exited).
Unfortunately the reproduciblity steps are wrong as Amazon.com seems to work for some reason.  Instead Try http://www.flexbeta.com (which is a hijacked search URL for flexbeta.net) but does do the cookie spam thing.  There is no way to accept/deny all for the entirety of the Private Browsing session.
Well, you can always change your setting. Although maybe there should be an option on the dialog to accept cookies temporarily for this site during the private browsing mode. Or maybe there should be only one way in private browsing mode: the session cookie.
(In reply to comment #2)
> Well, you can always change your setting. Although maybe there should be an
> option on the dialog to accept cookies temporarily for this site during the
> private browsing mode. Or maybe there should be only one way in private
> browsing mode: the session cookie.

There should at least also be "Deny". "Accept" could be greyed-out on the dialog maybe?
(In reply to comment #3)

AFAIK the problem is with the prefs, not the cookies, i.e. firefox accepts cookies in private browsing, albeit only for the private session itself, however it doesn't allow writing prefs to disk during private browsing. So Accept and Deny in this case are the same. IMHO this should be considered an explicit OK to write to disk from the user and should be allowed, then again maybe not everyone understands the implications, and that this could result in the session becoming "unprivate".
(In reply to comment #4)
> [...] So Accept and Deny in this case are the same. [...]

That's quite counterintuitive. If write-to-disk is disabled, I would expect "Accept" and "Accept for session" to be the same, with "Deny" being something else.
I meant when when "always remember this option" is ticked then accept and deny are the same (as in both choices write to disk, being that firefox has to remember the choice), if a special private browsing pref manager is made, keeping prefs only for the private session, then implementing something like this would be possible, or, like i said before, exceptions can be made for this explicit write to disk request. Similarly it is possible to make bookmarks in private browsing mode even though that essentially writes to disk.
This was done explicitly in bug 461625.  The reasoning is that enabling the check box can lead to exposure of the web site you've been visiting in the private browsing mode.

I think this should be resolved as INVALID, since this is by design.  CCing mconnor to see if he agrees.
OS: Windows XP → All
Hardware: PC → All
Whiteboard: [invalid?]
Version: unspecified → Trunk
But it makes browsing unusable, so simply change the wording and meaning of the checkbox to only last for the private browsing session.  

Note that what I am requesting is seems to be the same as Mike's comment #2 in the above referenced bug 461625.  A change to "Remember for Session".  

Please try using a browser without a way to answer "There's no need to ask me again for this website".  Without it, it makes Private Browsing unusable for the users using an otherwise very useful Firefox feature (The only store cookies I want (Amazon, etc.) while ignoring all the DoubleClick ****).

Another though less useful fix would be to turn off "Ask me everytime" feature when going to Private Browsing since the cookies will be, by the very nature of private browsing, end-up being session cookies.  Though this easy solution, of course, eliminates the option to deny the cookie.
I guess we could store the list of sites which the action is supposed to be remembered in a memory table of some sort, instead of storing them as permissions, during the private browsing mode.

I'm not sure if this is worth doing, but this needs a UX decision anyway.  CCing Alex and Beltzner to see what they think.
>I'm not sure if this is worth doing, but this needs a UX decision anyway. 
>CCing Alex and Beltzner to see what they think.

My initial reaction is that accepting or denying cookies on a case by case basis is a .0001% usage feature, and should simply be moved to an extension.  Also, this feature exposes the underlying implementation of the Web directly in the user interface, which is pretty much the definition of bad UI design.  Removing the interface entirely would obviously making it no longer an issue when the user is in private browsing mode.

Regardless of that change however, I think we should simply ignore the pref when in private browsing mode, since the cookies are going to be cleared either way.  Also, displaying the dialog is confusing since "accept" really means "just for now."
I've actually never had this pref flipped on before, but after encountering 8 modal dialogs going to amazon.com, and 15 going to ebay, I've filed bug 469260.  I really believe cookie micromanagement is far too niche to ship as a core feature.
Hm.
Don't know about you, m'boy, but if I can't both deny (not accept for session but outright deny) cookies from ad.doubleclick.net and accept (if only temporarily) cookies from bugzilla.mozilla.org, then I think I'll avoid that so-called "safe browsing mode" as the plague.
(In reply to comment #10)
> Regardless of that change however, I think we should simply ignore the pref
> when in private browsing mode, since the cookies are going to be cleared either
> way.  Also, displaying the dialog is confusing since "accept" really means
> "just for now."

Taking this over then.
Assignee: nobody → ehsan.akhgari
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Whiteboard: [invalid?]
Component: General → Networking: Cookies
Product: Firefox → Core
QA Contact: general → networking.cookies
Attached patch Patch (v1) (obsolete) — Splinter Review
Patch with a unit test.

This removes the cookie accept dialog prompt inside the private browsing mode.  All the user's stored prefs will still be honored, and whenever the user's decision needs to be asked via a prompt, the "accept" action is assumed.
Attachment #352761 - Flags: superreview?(mconnor)
Attachment #352761 - Flags: review?(mconnor)
Comment on attachment 352761 [details] [diff] [review]
Patch (v1)

>diff --git a/extensions/cookie/nsCookiePermission.cpp b/extensions/cookie/nsCookiePermission.cpp
>--- a/extensions/cookie/nsCookiePermission.cpp
>+++ b/extensions/cookie/nsCookiePermission.cpp
>+PRBool
>+nsCookiePermission::InPrivateBrowsing() const
>+{
>+  PRBool inPrivateBrowsingMode = PR_FALSE;
>+  nsCOMPtr<nsIPrivateBrowsingService> pbService =
>+    do_GetService(NS_PRIVATE_BROWSING_SERVICE_CONTRACTID);

please cache the pbService as a member on nsCookiePermission (inited lazily, like you do here). with that, r=me.
Attachment #352761 - Flags: review?(mconnor) → review+
Attached patch Patch (v1.1)Splinter Review
(In reply to comment #15)
> please cache the pbService as a member on nsCookiePermission (inited lazily,
> like you do here). with that, r=me.

Done.
Attachment #352761 - Attachment is obsolete: true
Attachment #357557 - Flags: superreview?(bzbarsky)
Attachment #352761 - Flags: superreview?(mconnor)
Comment on attachment 357557 [details] [diff] [review]
Patch (v1.1)

Looks ok; I assume you made sure this can't form reference cycles and leak, right?
Attachment #357557 - Flags: superreview?(bzbarsky) → superreview+
(In reply to comment #17)
> (From update of attachment 357557 [details] [diff] [review])
> Looks ok; I assume you made sure this can't form reference cycles and leak,
> right?

Yes, the private browsing service doesn't store (or even use) any reference to the cookie permissions component.
http://hg.mozilla.org/mozilla-central/rev/78a74a156436
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Attachment #357557 - Flags: approval1.9.1?
(In reply to comment #16)
> Created an attachment (id=357557) [details]
> Patch (v1.1)

The test in this patch breaks with applications that don't have a private browsing service. With Thunderbird TUnit build with mozilla-central I'm seeing:

TypeError: Cc['@mozilla.org/privatebrowsing;1'] is undefined

(http://tinderbox.mozilla.org/Thunderbird/)
Attached patch Test fixSplinter Review
(In reply to comment #20)
> The test in this patch breaks with applications that don't have a private
> browsing service. With Thunderbird TUnit build with mozilla-central I'm seeing:
> 
> TypeError: Cc['@mozilla.org/privatebrowsing;1'] is undefined

Oops, sorry about that.  :/
Attachment #357982 - Flags: superreview?(bzbarsky)
Attachment #357982 - Flags: review?(bzbarsky)
(In reply to comment #21)
> Oops, sorry about that.  :/

No problem. I was trying to get round to sorting it today but didn't succeed.

There are still two errors with Thunderbird running this test however I believe they are more likely to do with Thunderbird's cookie handling rather than some issue with the test - I'll deal with those in a follow-up bug once I've investigated them a bit more.
(In reply to comment #22)
> There are still two errors with Thunderbird running this test however I believe
> they are more likely to do with Thunderbird's cookie handling rather than some
> issue with the test - I'll deal with those in a follow-up bug once I've
> investigated them a bit more.

TB has its own nsICookiePermission impl (in case you don't know, but you probably do)... if i can help just cc me.
Comment on attachment 357982 [details] [diff] [review]
Test fix

I assume you tested this, right?
Attachment #357982 - Flags: superreview?(bzbarsky)
Attachment #357982 - Flags: superreview+
Attachment #357982 - Flags: review?(bzbarsky)
Attachment #357982 - Flags: review+
I tested this and made sure that it makes the test pass if the private browsing service does not exist.  This of course doesn't work around any cookie permissions stuff specific to Thunderbird though.

http://hg.mozilla.org/mozilla-central/rev/0dec4e077be2
Depends on: 478522
Attachment #357982 - Flags: approval1.9.1?
Verified fixed on OS X and Windows with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090301 Minefield/3.2a1pre
Status: RESOLVED → VERIFIED
Flags: blocking1.9.1?
Blocking, as this makes a certain feature unusable.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Target Milestone: mozilla1.9.2a1 → mozilla1.9.1
Attachment #357557 - Flags: approval1.9.1?
Attachment #357982 - Flags: approval1.9.1?
It's already fixed on trunk. So we should leave the TM as it was to avoid confusion.
Target Milestone: mozilla1.9.1 → mozilla1.9.2a1
I landed both patches as a single changeset on 1.9.1:

<http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8bd9ba6e43f2>
Keywords: fixed1.9.1
Verified fixed on the 1.9.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090311 Shiretoko/3.1b4pre and Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b4pre) Gecko/20090311 Shiretoko/3.1b4pre. Adding keyword.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: