837091 - cookie-changed notifications are not useful for private cookies

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Ian Nartowicz]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:10.0.10) Gecko/20100101 Firefox/10.0.10
Build ID: 20121024073834

Steps to reproduce:

1. Start a normal browsing window.
2. Start a private browsing window.
3. Add a "cookie-changed" observer in each window.
4. Browse


Actual results:

1. cookie-changed notifications are received relating to cookies that effectively don't exist for that window.


Expected results:

Notifications should only be received relating to changes to the cookies database for that window?  Would seem to break the whole concept of observer notifications.

Or different notifications are required for private cookie database and the regular one?  Each window decides what it needs to listen to.

Or the notification indicates in some way whether it relates to a cookie in a public or private window.

Doing this from a javascript code module should also be possible.  I guess the last two options would work, but not the first.

Comment 1

[(no longer active)]

Josh, do you wanna volunteer here?  ;-)

Comment 2

[Josh Matthews [:jdm]]

I'm not certain what we want to do here. I don't foresee us creating a private browsing cookie management interface for FF 20, so maybe we just want to not send notifications for private cookies? Alternatively we could send a different notification.

Comment 3

[Josh Matthews [:jdm]]

I suppose the other option is to add an isPrivate attribute to nsICookie2, since the notification passes one with most notifications. It wouldn't work for clearing the list, of course.

Comment 4

[Ian Nartowicz]

The notification is more or less useless without any interface to the private browsing cookies themselves.  It would be better not to have the notification at all than to get it for "nonexistent" cookies.  Would private browsing windows still get notifications about regular cookies?

Bug 831197 (now subsumed in bug 823941) addresses the need for an interface to private browsing cookies.

Comment 5

[Josh Matthews [:jdm]]

Yes, there is no way to limit which observers receive a given notification, so conflating the concepts of observers and windows doesn't do any good.

Comment 6

[(no longer active)]

I suggest sending a different notification for private cookies.  We can name them private-cookie-changed, etc.

Comment 7

[Ian Nartowicz]

A separate notification for private cookies would work well.  After all, a window opens (will open) as private or not, and will never change.  Possibly the second notification doesn't even need to work until the related bugs are fixed.

Just one wrinkle I can think of.  There is a stated desire for Firefox to support, although not actually to offer, per-tab private browsing.  Two notifications could still claim to offer that support if anyone wanted to go to the trouble.

Comment 8

[(no longer active)]

(In reply to comment #7)
> Just one wrinkle I can think of.  There is a stated desire for Firefox to
> support, although not actually to offer, per-tab private browsing.  Two
> notifications could still claim to offer that support if anyone wanted to go to
> the trouble.

If and when that desire actually turns into an implementation plan, we'd probably want a whole new way of mapping these notifications to tabs.  For now, we have no plans on supporting per-tab PB.

Comment 9

[Josh Matthews [:jdm]]

Attachment: Rename cookie-changed notification for private cookies.

Comment 10

[(no longer active)]

Comment on attachment 710115 [details] [diff] [review]
Rename cookie-changed notification for private cookies.

Review of attachment 710115 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/cookie/nsCookieService.cpp
@@ +1669,5 @@
>  nsCookieService::NotifyChanged(nsISupports     *aSubject,
>                                 const PRUnichar *aData)
>  {
> +  const char* topic = mDBState == mPrivateDBState ?
> +      "private-cookie-changed" : "cookie-changed";

Weird indentation!  ;-)

Comment 11

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a097b7a80cf

Comment 12

[Josh Matthews [:jdm]]

Comment on attachment 710115 [details] [diff] [review]
Rename cookie-changed notification for private cookies.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 722850
User impact if declined: Addons observing cookie changes could receive confusing notifications.
Testing completed (on m-c, etc.): m-c, automated test
Risk to taking this patch (and alternatives if risky): No risk. 
String or UUID changes made by this patch: None

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a097b7a80cf

Comment 14

[Josh Matthews [:jdm]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c70f78759651

Comment 15

[JustOff]

I have filled the similar bug 1311707 for dom-storage2-changed, please take a look