Closed Bug 221185 Opened 16 years ago Closed 12 years ago

[cookies] rework notifications

Categories

(Core :: Networking: Cookies, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dwitte, Assigned: dwitte)

References

Details

(Keywords: perf)

Attachments

(4 obsolete files)

the current notifications in cookies/permissions are cumbersome. firing a
notification that just says "hey, something changed, somewhere", and forcing the
consumer to reload the entire cookie list (via getEnumerator), scales badly. the
cookie manager UI doesn't help things by having a surprisingly terrible removal
algorithm. the net result is that, with moderate sized cookie/permission lists
(~100 items), it can take minutes to execute a 'remove all' operation in the
cookie manager. see bug 210465 for an example.

and i won't get started on the ridiculousness of the p3p "cookieIcon" notification.

the proposal for fixing this, is to replace the current notification system in
cookies/permissions with two notifications:
1) "cookie-rejected", whenever a cookie gets rejected as a result of user prefs
(this includes default prefs, and the permission list, and prompting. it does
not include actual failures to set a cookie, such as it being over the 4kb size
limit). this is for bug 216743, so we can implement a statusbar cookie icon for
easily seeing when cookies have been rejected. it will also replace the
"cookieIcon" p3p-related notification. it will pass the cookie host URI as an
nsIURI* subject to its observers, so that they may easily whitelist that URI via
permmgr.

2) "cookie-changed"/"perm-changed", whenever the cookie/permlist changes. this
will have four possible "data" categories, with an |nsICookie2*
aCookie|/|nsIPermission* aPermission| subject as described:
"deleted" means a cookie was deleted. aCookie is the deleted cookie.
"added"   means a cookie was added. aCookie is the added cookie.
"changed" means a cookie was altered. aCookie is the new cookie.
"cleared" means the entire cookie list was cleared. aCookie is null.
this will allow for extremely fast UI operations (no list reloading, after the
initial load), and easy "cookie snooping" for things that need it (such as
cookie monitors, silly p3p icons etc).

this can be implemented in two parts: first, add backend support; then rewrite
the UI to take advantage of it. (so for a while, we will have a bit of compat
code to fire the old notifications). any suggestions welcome ;)
Attached patch first-shot patch (obsolete) — Splinter Review
this is the backend work... there's a tiny bit of compat code in cookies, to
deal with the old notifications (which can die after the UI is fixed). the
permissions patch is ugly because permissions doesn't store nsIPermission
objects natively, so we have to make them just to notify. we might want to
change that; it's up to mvl :)
oh; a benefit of this new system... it eliminates the need for the LazyNotify
stuff darin is adding in, so it will end up bloat-neutral :)
just for laughs, i worked out the algorithmic complexity of the 'remove all'
function in cookiemanager.

it's O( n^3 log(n) ).
Blocks: 206939
Blocks: 221370
Attached patch backend patch v1 (obsolete) — Splinter Review
same as previous, but with some meaty comments in nsICookieService.idl and
nsIPermissionManager.idl describing the new notifications (thanks for comments
mvl!). it also adds a new method to nsICookie2, GetRawHost() - same as
GetHost() but without the leading dot (in the case of domain cookies). this is
useful for the cookiemgr UI, which i'm fixing to use the new notifications now.


i think this is ready for review ;)
Attachment #132621 - Attachment is obsolete: true
Attached patch backend patch v1.1 (obsolete) — Splinter Review
same as previous, updated to trunk and with a couple tweaks.
Attachment #132994 - Attachment is obsolete: true
Attachment #133126 - Flags: review?(mvl)
Attachment #133126 - Flags: superreview?(darin)
Comment on attachment 133126 [details] [diff] [review]
backend patch v1.1

> nsresult
> nsPermissionManager::AddInternal(const nsAFlatCString &aHost,
>-                                 PRInt32 aTypeIndex,
>-                                 PRUint32 aPermission)
>+                                 PRInt32               aTypeIndex,
>+                                 PRUint32              aPermission,
>+                                 PRBool                aNotify)

Please add a comment why the notification can't be done in Add(), but must be
in AddInternal(). I just can't remember it :)

r=mvl with that.
Attachment #133126 - Flags: review?(mvl) → review+
Comment on attachment 133126 [details] [diff] [review]
backend patch v1.1

>@@ -897,35 +912,24 @@ nsCookieService::Remove(const nsACString
>-      mCookieChanged = PR_TRUE;
>-      // we might want to eventually push this Write() call into the UI,
>-      // to just write once on cookiemanager close.
>       Write();

But don't forget the LazyWrite trick you mentioned, to speed the UI up a little
bit.
>Please add a comment why the notification can't be done in Add(), but must be
>in AddInternal(). I just can't remember it :)

will do. (the reason is because we need to do the hash lookup before we know
whether it's a removal, addition, or change).

>But don't forget the LazyWrite trick you mentioned, to speed the UI up a little
>bit.

darin - i'll change the Write() in Remove() to a LazyWrite(). sound ok?
Blocks: 192176
Comment on attachment 133126 [details] [diff] [review]
backend patch v1.1

punting to bz, to take some load off darin.

bz: please see comment 6 on for the current review comments that apply to this
patch... thanks!
Attachment #133126 - Flags: superreview?(darin) → superreview?(bzbarsky)
Comment on attachment 133126 [details] [diff] [review]
backend patch v1.1

>@@ -1316,48 +1305,49 @@ nsCookieService::AddInternal(nsCookie   
>+    // replace previous cookie
>+    NotifyChanged(aCookie, NS_LITERAL_STRING("changed").get());
>+    mCookieList.ReplaceElementAt(aCookie, insertPosition);
>+    NS_ADDREF(aCookie);

You notify before you change the cookie. This means that if the observer still
uses the enumerator to get all the cookies (as it does currently) it will get
the old list. Same with the other notifications here.


> nsPermissionManager::AddInternal(const nsAFlatCString &aHost,

>+        // adding
>+        NotifyObserversWithPermission(aHost,
>+                                      mTypeArray[aTypeIndex],
>+                                      aPermission,
>+                                      NS_LITERAL_STRING("added").get());
>   entry->SetPermission(aTypeIndex, aPermission);

Same here, but for permissions. Maybe it isn't a huge problem for cookies, but
you use the same notification here as before, so the sites tab for cookies now
shows the situation before the last change.
>You notify before you change the cookie. This means that if the observer still
>uses the enumerator to get all the cookies (as it does currently) it will get
>the old list. Same with the other notifications here.

yeah i wondered if anyone would catch that :)
good spotting. i noticed that a few days ago and fixed in my local tree. you'll
notice it's fixed in the hashtable patch in bug 143939.
Attachment #133126 - Flags: superreview?(bzbarsky)
addresses all previous review comments. mvl: i'll carry over your r= on this,
but it would be super if you could doublecheck to make sure i have the order
correct now, and also to make sure you're happy with the comment in
nsPermissionManager:AddInternal()... thx!
Attachment #133126 - Attachment is obsolete: true
Attachment #133620 - Flags: superreview?(bzbarsky)
Attachment #133620 - Flags: review+
Changes look good to me.
Comment on attachment 133620 [details] [diff] [review]
backend patch v1.2 [Checked in: Comment 15]

sr=bzbarsky with the comment changes I pointed out on irc.
Attachment #133620 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 133620 [details] [diff] [review]
backend patch v1.2 [Checked in: Comment 15]

landed this one for alpha. i'll keep this bug open for the UI changes that go
with it...
Attachment #133620 - Attachment description: backend patch v1.2 → backend patch v1.2 (checked in)
No longer blocks: 216743
No longer blocks: 221370
Attachment #133620 - Attachment description: backend patch v1.2 (checked in) → backend patch v1.2 [Checked in: Comment 15]
Attachment #133620 - Attachment is obsolete: true
No longer blocks: 210465
Depends on: 226511
Blocks: 216413
ui changes have long since been made; marking fixed.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.