Closed
Bug 837091
Opened 12 years ago
Closed 12 years ago
cookie-changed notifications are not useful for private cookies
Categories
(Core :: Networking: Cookies, defect)
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: mozilla, Assigned: jdm)
References
Details
(Keywords: addon-compat)
Attachments
(1 file)
(deleted),
patch
|
ehsan.akhgari
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Updated•12 years ago
|
Component: Untriaged → Private Browsing
Assignee | ||
Updated•12 years ago
|
Status: UNCONFIRMED → NEW
Component: Private Browsing → Networking: Cookies
Ever confirmed: true
Product: Firefox → Core
Comment 1•12 years ago
|
||
Josh, do you wanna volunteer here? ;-)
Assignee | ||
Comment 2•12 years ago
|
||
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.
Assignee | ||
Comment 3•12 years ago
|
||
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.
Reporter | ||
Comment 4•12 years ago
|
||
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.
Assignee | ||
Comment 5•12 years ago
|
||
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•12 years ago
|
||
I suggest sending a different notification for private cookies. We can name them private-cookie-changed, etc.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → josh
Summary: cookie-changed notifications crossing (per-window) private browsing boundary → cookie-changed notifications are not useful for private cookies
Reporter | ||
Comment 7•12 years ago
|
||
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•12 years ago
|
||
(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.
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #710115 -
Flags: review?(ehsan)
Comment 10•12 years ago
|
||
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+
Assignee | ||
Updated•12 years ago
|
tracking-firefox20:
--- → ?
Assignee | ||
Comment 11•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Keywords: addon-compat
Assignee | ||
Comment 12•12 years ago
|
||
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?
Comment 13•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Updated•12 years ago
|
Attachment #710115 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
status-firefox20:
--- → affected
Assignee | ||
Comment 14•12 years ago
|
||
Updated•12 years ago
|
status-firefox21:
--- → fixed
Comment 15•8 years ago
|
||
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.
Description
•