Closed Bug 225298 Opened 20 years ago Closed 20 years ago

clean up p3p hooks in cookies

Categories

(Core :: Networking: Cookies, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dwitte, Assigned: dwitte)

Details

(Keywords: perf)

Attachments

(1 file, 1 obsolete file)

the p3p hooks in nsCookieService are a bit messy... random switch/case
statements strewn about, and duplication of calls into the p3pservice. i also
don't think the cookieservice should be caring about the p3p pref string (the
8-letter string that defines the user's desired policy->status mapping) directly
- this would be better factored into the p3pservice.

patch coming up to clean up the hooks and move code out of the cookieservice and
into the p3pservice.
Attached patch patch v1 (obsolete) — Splinter Review
Comment on attachment 135234 [details] [diff] [review]
patch v1

pike, would you be ok with reviewing this?
Attachment #135234 - Flags: review?(axel)
oh, btw... there is a slight functional change in this patch. previously, we'd
get the site's p3p policy and store it with the cookie, regardless of whether
the user had his cookie prefs set to use p3p. so, this would mean the cookie
site policies for the current session of browsing would be visible in
cookiemanager, even if p3p wasn't being used.

however, storing these policies incurs a perf penalty, and if p3p isn't being
used i don't think there's any point in doing it. further, p3p policies aren't
stored in the cookie file, thus any advantage gained by unconditionally fetching
the policy would be lost in between sessions.
Comment on attachment 135234 [details] [diff] [review]
patch v1

pike said he knows not this code... so, i'll pick a random reviewer... mvl,
you're it ;)
Attachment #135234 - Flags: review?(axel) → review?(mvl)
Comment on attachment 135234 [details] [diff] [review]
patch v1

might as well ask for sr concurrently...

darin, i know p3p isn't really your cup of tea, but i don't think there's
anyone else that knows this code... mind taking a look? :/
Attachment #135234 - Flags: superreview?(darin)
fwiw, with this patch, if your cookie pref is set to something other than p3p,
we won't init the p3p service or load libp3p.so at all... so it's quite lazy :)
Comment on attachment 135234 [details] [diff] [review]
patch v1

>Index: netwerk/cookie/public/nsICookieConsent.idl

>+  nsCookiePolicy getPolicy(in nsIURI         uri,
>+                           in nsIHttpChannel httpChannel);
>+                 
>+  nsCookieStatus getConsent(in nsCookiePolicy policy,
>+                            in boolean        isForeign);

You could combine those into one method. does the cookie service are about the
policy?


>Index: netwerk/cookie/src/nsCookieService.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/netwerk/cookie/src/nsCookieService.cpp,v
>retrieving revision 1.15
>diff -u -6 -p -d -r1.15 nsCookieService.cpp
>--- netwerk/cookie/src/nsCookieService.cpp	11 Nov 2003 05:00:25 -0000	1.15
>+++ netwerk/cookie/src/nsCookieService.cpp	11 Nov 2003 08:45:12 -0000

>@@ -459,44 +446,26 @@ nsCookieService::Observe(nsISupports    
>+    nsCOMPtr<nsIPrefBranch> prefBranch = do_QueryInterface(aSubject);
>+    PrefChanged(prefBranch);

you should check if the QI succeeded. (here, or in PrefChanged())

>Index: extensions/p3p/src/nsP3PService.cpp
>===================================================================
>@@ -84,58 +124,100 @@ nsP3PService::ProcessResponseHeader(nsIH

>-    result = ProcessResponseHeader(aHttpChannel);
>-    NS_ENSURE_SUCCESS(result,result);
>+    rv = ProcessResponseHeader(aHttpChannel);
>+    if (NS_FAILED(rv)) return rv;

just use NS_ENSURE_SUCCESS. That seems to be the module style. (And i like it
better. It asserts when something goes wrong)

>Index: extensions/p3p/src/nsP3PService.h

>+   * each of the eight characters can be one of the following:
>+   *   'a': accept the cookie
>+   *   'd': accept the cookie but downgrade it to a session cookie
>+   *   'r': reject the cookie
>+   */

You can also have f: flagged
Attachment #135234 - Flags: review?(mvl) → review+
Attachment #135234 - Flags: superreview?(darin)
Attached patch patch v1.1Splinter Review
addresses comments. i merged the getPolicy and getConsent methods into one...
it was semantically nice to have them separate, but that doesn't really matter
:)

i also changed the handling of unknown nsCookieStatuses... getConsent will no
longer return nsICookie::STATUS_UNKNOWN, since that is ambiguous. if it finds
something it can't digest, it returns STATUS_REJECTED.
Attachment #135234 - Attachment is obsolete: true
Comment on attachment 135352 [details] [diff] [review]
patch v1.1

carrying over r+
Attachment #135352 - Flags: superreview?(darin)
Attachment #135352 - Flags: review+
>+  nsCAutoString uriSpec;
>+  nsresult rv = aURI->GetSpec(uriSpec);
>+  NS_ENSURE_SUCCESS(rv, rv);
> 
>   if (aHttpChannel) {
> 
> #ifdef NS_DEBUG
>     nsCOMPtr<nsIURI> uri;
>     aHttpChannel->GetURI(getter_AddRefs(uri));  
>     
>     if (uri) {
>-      nsCAutoString spec;
>-      uri->GetSpec(spec);
>+      nsCAutoString uriSpecFromChannel;
>+      uri->GetSpec(uriSpecFromChannel);
> 
>-      if (aURI) {
>-        NS_ASSERTION(spec.Equals(aURI), "URIs don't match");
>-      }
>+      NS_ASSERTION(uriSpec.Equals(uriSpecFromChannel), "URIs don't match");
>     }
> #endif

why not use nsIURI::equals here?  string comparisons like this are fragile
since some URI types allow case insensitive comparisons over certain parts
of the URI.  moreover, one of the URIs might have more or less escaped 
characters.  (i'm not saying that nsIURI::equals gets all of this right
either; i'm just saying that it theoretically could.)
Comment on attachment 135352 [details] [diff] [review]
patch v1.1

sr=darin
Attachment #135352 - Flags: superreview?(darin) → superreview+
nice point... fixed, and checked in. thanks for the fast reviews!
Status: NEW → RESOLVED
Closed: 20 years ago
Keywords: perf
Resolution: --- → FIXED
This fix broke some proprietary legacy code I'm working on by changing the P3P
default value for error conditions from UNKNOWN to REJECTED (comment 8). In
addition, having no HTTP channel used to be handled (still is, in GetConsent)
but this change makes that a REJECTED cookie.

I guess I could change my extension to make up a channel, but I'm sure this
affects others. Since the default cookie behavior is P3P (why, oh why?) a
default REJECTED status is overly harsh. If P3P is available the default policy
in the worst case is "FLAGGED".
You need to log in before you can comment on or make changes to this bug.