221185 - [cookies] rework notifications

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[[e-mail]]

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 ;)

Comment 1

[[e-mail]]

Attachment: first-shot patch

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 :)

Comment 2

[[e-mail]]

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 :)

Comment 3

[[e-mail]]

just for laughs, i worked out the algorithmic complexity of the 'remove all'
function in cookiemanager.

it's O( n^3 log(n) ).

Comment 4

[[e-mail]]

Attachment: backend patch v1

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 ;)

Comment 5

[[e-mail]]

Attachment: backend patch v1.1

same as previous, updated to trunk and with a couple tweaks.

Comment 6

[Michiel van Leeuwen (email: mvl+moz@)]

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.

Comment 7

[Michiel van Leeuwen (email: mvl+moz@)]

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.

Comment 8

[[e-mail]]

>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?

Comment 9

[[e-mail]]

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!

Comment 10

[Michiel van Leeuwen (email: mvl+moz@)]

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.

Comment 11

[[e-mail]]

>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.

Comment 12

[[e-mail]]

Attachment: backend patch v1.2 [Checked in: Comment 15]

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!

Comment 13

[Michiel van Leeuwen (email: mvl+moz@)]

Changes look good to me.

Comment 14

[Boris Zbarsky [:bzbarsky]]

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.

Comment 15

[[e-mail]]

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...

Comment 16

[[e-mail]]

ui changes have long since been made; marking fixed.