Closed Bug 837091 Opened 7 years ago Closed 7 years ago

cookie-changed notifications are not useful for private cookies

Categories

(Core :: Networking: Cookies, defect)

10 Branch
x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox20 + fixed
firefox21 --- fixed

People

(Reporter: mozilla, Assigned: jdm)

References

Details

(Keywords: addon-compat)

Attachments

(1 file)

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.
Component: Untriaged → Private Browsing
Status: UNCONFIRMED → NEW
Component: Private Browsing → Networking: Cookies
Ever confirmed: true
Product: Firefox → Core
Josh, do you wanna volunteer here?  ;-)
Blocks: PBnGen
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.
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.
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.
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.
I suggest sending a different notification for private cookies.  We can name them private-cookie-changed, etc.
Assignee: nobody → josh
Summary: cookie-changed notifications crossing (per-window) private browsing boundary → cookie-changed notifications are not useful for private cookies
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.
(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 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!  ;-)
Attachment #710115 - Flags: review?(ehsan) → review+
Keywords: addon-compat
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
Attachment #710115 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/6a097b7a80cf
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Attachment #710115 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I have filled the similar bug 1311707 for dom-storage2-changed, please take a look
You need to log in before you can comment on or make changes to this bug.