Closed Bug 195908 Opened 20 years ago Closed 20 years ago

Cookie rewrite phase 2

Categories

(Core :: Networking: Cookies, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.4alpha

People

(Reporter: dwitte, Assigned: dwitte)

References

Details

Attachments

(9 files, 5 obsolete files)

17.99 KB, patch
Details | Diff | Splinter Review
15.91 KB, patch
Details | Diff | Splinter Review
165.68 KB, patch
dwitte
: review+
dwitte
: superreview+
Details | Diff | Splinter Review
12.36 KB, patch
Details | Diff | Splinter Review
14.34 KB, patch
Details | Diff | Splinter Review
10.54 KB, patch
Details | Diff | Splinter Review
5.49 KB, patch
dbradley
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
1.31 KB, patch
mvl
: review+
Details | Diff | Splinter Review
6.59 KB, patch
Details | Diff | Splinter Review
Ok, here's phase 2 of the cookie rewrite. I met up with darin a few days ago to
go over it, so hopefully it's in pretty good shape. Ready for review.

A brief summary, more details on request:

Objectives
----------
to begin improving/optimizing/sanitizing the code, in preparation for merging
everything into nsCookieService. This is the last patch that'll have the files
separate, hopefully. Made things a whole lot more readable, created a new
prefobserver (ditched the old callback system), fixed cookies to be written out
in order (bug 188850), optimized some codepaths and refactored things...

Questions (mainly for darin)
---------
a) can you check my usage of mCookiesP3PString in
nsCookiePrefObserver::ReadPrefs and nsCookiePrefObserver::~nsCookiePrefObserver
(both in nsCookies.cpp)? Some free'ing and stuff going on there, just wanna be sure.
b) I whacked IsInDomain a bit more, and added comments to that effect. Just a
heads-up to let you know this changed from Friday.
c) can you check the mailnews blocking thing in cookie_CheckPrefs? I'm wondering
if we have it too strict. maybe we should allow if (aFirstURI is null, but
currentURI isn't from mailnews)?

Upcoming improvements (after cookieservice merge)
---------------------
1) new cookie storage class -> much better mem efficiency
2) binary cookielist sorting/searching (or hashtables) -> 30x improvement in
list traversal speed (GetCookie/SetCookie)
3) more API & general code cleanup
Attached patch phase 2 patch (obsolete) — Splinter Review
a couple extra things:

1) the changes to TestCookie.cpp are to fix up the tests - now that we made the
mailnews checking stricter (so it fails on firstURI null && 'block from
mailnews' pref), the tests died. this fixes to pass in non-null firstURI.

2) the patch involves a lot of code movement, and is unfortunately difficult to
read. posting another (hopefully more readable) patch with the functions in the
same order they used to be in. might be easier to review (but not compile,
missing prototypes...).
Attached patch more readable patch (obsolete) — Splinter Review
darin, one more note:

I created a helper class COOKIE_ChangeFormat to do the cookiestruct->nsICookie
(with expirytime) conversion. Can you check its usage (especially the
addreffing) in nsCookies.cpp/COOKIE_SetCookie and nsCookieManager.cpp/GetNext?
I'm not sure about the xpcom fu there...
Attachment #116201 - Flags: superreview?(darin)
Attachment #116201 - Flags: review?(alecf)
Blocks: 188850
Target Milestone: --- → mozilla1.4alpha
QA Contact: tever → cookieqa
Comment on attachment 116201 [details] [diff] [review]
phase 2 patch

so are cookie_CookieStruct and nsICookie going to merge at some point? Because
if so, you can use nsCOMArray<nsICookie> to hold them, and then you'll get
enumerators for free.

but in any case I'm finally starting to look at the patch! :)
>so are cookie_CookieStruct and nsICookie going to merge at some point? Because
>if so, you can use nsCOMArray<nsICookie> to hold them, and then you'll get
>enumerators for free.

yes, they'll merge in the next patch. named... phase 3 :)
I didn't know about nsCOMArray, but it sounds neat. so it supports
nsISimpleEnumerator and allows us to kill that from the cookiemanager? that'd be
nice. and I take it nsCOMArray inherits from nsVoidArray (or the other way
around? i remember poking around those classes a while back) so our current list
code will be okay.

note, however, that a serious revving of our list traversal fu might take place
soon (maybe during 1.4b). linear traversals are just bad - either b-search or
hashtables would be much nicer. using these may invalidate nsCOMArray usage if
we move to a single-node-per-hostname list structure, but we can think about
such things later.

>but in any case I'm finally starting to look at the patch! :)

neat, thx :)
Attachment #116201 - Attachment is obsolete: true
Attachment #116201 - Flags: superreview?(darin)
Attachment #116201 - Flags: review?(alecf)
Attached patch updated patch (obsolete) — Splinter Review
this patch obsoletes previous. I made a few changes, as follows:

a) optimized ChangeFormat() a little, by making it return an
already_AddRefed<nsICookie>. makes it more efficient and it looks a little
nicer. (this change touched several places: the ChangeFormat() call and the
ChangeFormat() impl in nsCookies.cpp, the call in nsCookieManager.cpp, and the
prototype in nsCookies.h). Also fixed the enumerator function GetNext() in
nsCookieManager.cpp, to properly set the outparam ptr to null if it errors out.


b) fixed some broken recursion and cookie max-size testing in SetCookie
(nsCookies.cpp). a recent bug on the trunk pointed this one out. I've split
SetCookie into two portions: the first does prefchecks common to all cookies
present in the header string (including parsing the server time string, which
was previously done in GetExpiry); then it loops over each cookie, calling
SetCookieInternal to process each one. this is a little more optimal and
logical, I hope.

c) fixed a return value in nsCookieManager::Add - we now return NS_OK
unconditionally, since although the callee COOKIE_Add can return a failed
nsresult, that doesn't indicate a failure (it's just used for logging purposes)


d) fixed the cookie nsIFile caching in nsCookieService.cpp - forgot to update
the cached nsIFile location on profile switch. thanks mvl!

e) made a minor change to the cookie parser, cookie_GetTokenValue. since necko
removes CR | LF's (except as a delimiter between multiple set-cookie headers),
there's no point in pretending to allow them inside quoted-strings (as was
previously the case). this would show up as a bug if we had a malformed
quoted-string (missing the terminating quote) _and_ multiple cookies
(->newlines), since we'd chew up more than we should. so it makes the parser a
little more robust.

okay, so that list wasn't so short, sorry about that ;)
thanks guys!
Attachment #116202 - Attachment is obsolete: true
Attachment #116757 - Flags: superreview?(darin)
Attachment #116757 - Flags: review?(alecf)
Attached patch updated patch: more readable (obsolete) — Splinter Review
same as above, but with functions in preserved order in nsCookies.cpp
even better, thanks to jkeiser's neat diffpatch: a diffdiff showing the stuff I
mentioned above.

for your reviewing pleasure :)
Comment on attachment 116757 [details] [diff] [review]
updated patch

>Index: extensions/cookie/nsCookieManager.cpp

> NS_IMETHODIMP nsCookieManager::Add(const nsACString &aDomain,
...
>+  nsresult rv = COOKIE_Add(cookie, currentTime, nsnull, "(added via cookiemanager interface)");

looks like this text is only used with logging.  can you use a #define
and make it nsnull when !defined(PR_LOGGING)?


>Index: extensions/cookie/nsCookieService.cpp

>+    gCookieIconVisible = (!nsCRT::strcmp(aData, NS_LITERAL_STRING("on").get()));

      gCookieIconVisible = (aData[0] == 'o' && aData[1] == 'n' && aData[2] ==
'\0');

NS_LITERAL_STRING("on") => NS_ConvertASCIItoUSC2("on") under linux and other
platforms where wchar_t is not UCS-2.  yes, we suck for not having methods in
nsCRT which make comparisons like this easier and still lightweight.


overall looks really good.  thanks dan!  r/sr=darin (leave it to alecf to
give final sr=).
Attachment #116757 - Flags: superreview?(darin)
Attachment #116757 - Flags: superreview?(alecf)
Attachment #116757 - Flags: review?(alecf)
Attachment #116757 - Flags: review+
dwitte: also, now that P3P is fully open source, please verify P3P is still
happy with these changes.  thx!
sure thing, I'll see about testing out the p3p stuff when I get the chance. The
only change the p3p code will notice, is that we use the nsCookieStatus "type"
as a returnvalue from CheckPrefs. So it can be STATUS_REJECTED or _ACCEPTED in a
non-p3p situation.. . _REJECTED doesn't matter since SetCookie just terminates,
but _ACCEPTED will get written to the cookie status field. I'll check whether
p3p cares about this.

cc'ing harishd in case he has a 'quick answer'?

I'll post a new patch once alecf has had a look.
Attached patch v3 complete patch (obsolete) — Splinter Review
Hmm, on second thought I should post the update before alecf looks at it.

most changes are self-explanatory (incl fixing darin's comments). some
noteworthy things, for your feedback:

a) is the .get() appendage to COOKIE_ChangeFormat (in nsCookieManager.cpp)
correct? need to cast from an already_AddRefed<nsICookie> to a simple ptr.

b) my GetCharPref() fu for mCookiesP3PString was wrong. Threw away the free's
and went with an XPIDLCString. Note that we test for a non-sane return value
from GetCharPref, and need to override the string with a default (an
NS_NAMED_LITERAL_CSTRING) - the XPIDLCString will free its previously held
buffer on reassignment, right?

c) changed the default value for the "disable cookies in mailnews" pref, from
OFF to ON. this only matters if the prefservice fails. i figure it's better to
be on by default, so...

d) decided to switch from using string.ToInteger() to PR_sscanf, since the
latter is 64-bit compatible, and we need that. Added an expiry test to
COOKIE_Read() so we don't read in cookies that have already expired. Various
other little optimizations/fixes.

I'm presuming I can carry over r/sr=darin here, contingent on alecf reviewing
these changes. let me know if this is wrong :)
Attachment #116757 - Attachment is obsolete: true
Attachment #116758 - Attachment is obsolete: true
shows changes between previous and latest versions.
Attachment #116757 - Flags: superreview?(alecf)
Attachment #116873 - Flags: superreview?(alecf)
Attachment #116873 - Flags: review+
Blocks: 184419
Blocks: 187254
Comment on attachment 116873 [details] [diff] [review]
v3 complete patch

Ok, I'm finally reviewing this. No more spacin'!

>Index: extensions/cookie/nsCookieService.cpp
>===================================================================
>+// sadly, we need this multiple definition until everything is in one file
>+static NS_NAMED_LITERAL_CSTRING(kCookieFileName2, "cookies.txt");

Sadly, you can't have static class instances. for now use 
static const char kCookieFileName2[] = "cookies.txt";

and then use NS_LITERAL_CSTRING where appropriate

> nsresult nsCookieService::Init()
> {
>-  COOKIE_RegisterPrefCallbacks();
>+  // install the main cookie pref observer (defined in nsCookieService.h)
>+  // this will be integrated into nsCookieService when nsCookies is removed.
>+  gCookiePrefObserver = new nsCookiePrefObserver();
>+  // create sCookieList

Do we really need a whole other observer, or can we just use "this" as an
observer?

>+  sCookieList = new nsVoidArray();
>+  // if we can't create them, return an error so do_GetService() fails

can sCookieList just become mCookieList and be a non-pointer member of
nsCookieService?

>   } else if (!nsCRT::strcmp(aTopic, "cookieIcon")) {
>-    gCookieIconVisible = (!nsCRT::strcmp(someData, NS_LITERAL_STRING("on").get()));
>+    gCookieIconVisible = (aData[0] == 'o' && aData[1] == 'n' && aData[2] == '\0');

Just add a comment here so I don't have to parse this every time I encounter
the code :)

>Index: extensions/cookie/nsCookieService.h
>===================================================================
+#ifdef MOZ_PHOENIX
+    // unfortunately, we require this #ifdef for now, since Phoenix uses
different
+    // (more optimized) prefs to Mozilla. This will be fixed shortly.
+    // the following variables are Phoenix hacks to reduce ifdefs in the code.
+    PRBool			 mCookiesEnabled_temp,		     // These
two prefs are collapsed
+				 mCookiesForDomainOnly_temp,	     // into
mCookiesPermissions.
+				 mCookiesDisabledForMailNews;	     // Disable
cookies in mailnews
+#else
+    PRBool			 mCookiesDisabledForMailNews;	     // Disable
cookies in mailnews
+    PRInt32			 mCookiesLifetimeSec;		     //
Lifetime limit specified in seconds
+#endif
+    PERMISSION_BehaviorEnum	 mCookiesPermissions;	// PERMISSION_{Accept,
DontAcceptForeign, DontUse, P3P}
+    PRBool			 mCookiesAskPermission, // Ask user permission
before storing cookie
+				 mCookiesLifetimeEnabled,	     // Cookie
lifetime limit enabled
+				 mCookiesLifetimeCurrentSession;     // Limit
cookie lifetime to current session
+    nsXPIDLCString		 mCookiesP3PString;		     // P3P
settings
+    PRBool			 mCookiesStrictDomains; // Optional pref to
apply stricter domain checks

can you consolidate the PRBools and turn them into PRPackedBool (and yes, that
may make the pref observers not quite as clean, but I see a lot of PRBools
here)

+
+  private:
+    nsCOMPtr<nsIPrefBranch> mPrefBranch;

Does this create a bad ownership loop?

>Index: extensions/cookie/nsCookies.cpp
>===================================================================
>+NS_NAMED_LITERAL_CSTRING(kCookiesP3PString_Default, "drdraaaa");

Sorry, yet another one of these - can't declare strings globally like this, use
const char[]

>+  PRExplodedTime explodedTime;
>+  PR_ExplodeTime(PR_Now(), PR_GMTParameters, &explodedTime);

These can actually be kind of expensive - can you #ifdef the code around this
so that this only happens in logging-enabled builds?

>+
>+// processes domain attribute, and returns PR_TRUE if host has permission to set for this domain.
>+PRIVATE inline PRBool
>+cookie_CheckDomain(cookie_CookieStruct *aCookie,
>+                   nsIURI              *aHostURI)

Stopped here, will continue in just a bit.
Comment on attachment 116873 [details] [diff] [review]
v3 complete patch


>+    // strip down everything after the last slash to get the path,
>+    // ignoring slashes in the query string part.
>+    // if we can QI to nsIURL, that'll take care of the query string portion.
>+    // otherwise, it's not an nsIURL and can't have a query string, so just find the last slash.
>+    nsCOMPtr<nsIURL> hostURL = do_QueryInterface(aHostURI);
>+    if (hostURL) {
>+      hostURL->GetDirectory(aCookie->path);
>+    } else {
>+      PRInt32 slash = aCookie->path.RFindChar('/');
>+      if (slash != kNotFound) {
>+        aCookie->path.Truncate(slash + 1);
>+      }

is a simple rfind safe here? I mean, can query strings also have '/'? Does this
case ever even happen?
>+    nsInt64 expires;
>+    PRTime temp;
> 
>-    if (server_date && *server_date) {
>-      cookie_ParseDate(nsDependentCString(server_date), sDate);
>+    // parse expiry time
>+    if (PR_ParseTimeString(expiresAttribute.get(), PR_TRUE, &temp) == PR_SUCCESS) {
>+      expires = nsTime(temp) / PR_USEC_PER_SEC;

"temp"? Never name a variable "temp" - temp what? :)
>+  nsInt64 currentTime = nsTime() / PR_USEC_PER_SEC, lastAccessedCounter = currentTime;

declare currentTime and lastAccessedCounter seperately - this is confusing.
is nsTime() just going to call PR_Now() and go away? Should we just call PR_Now
directly?

>+  while (isMore && NS_SUCCEEDED(lineInputStream->ReadLine(bufferUnicode, &isMore))) {
>+    // downconvert to ASCII. eventually, we want to fix nsILineInputStream
>+    // to operate on a CString buffer...
>+    CopyUCS2toASCII(bufferUnicode, buffer);

I wonder if nsManifestLineReader() might help us here... something to think
about for the future..

>+    // todo: use iterators?
>+    if ((isDomainIndex = buffer.FindChar('\t', hostIndex)     + 1) == 0 ||
>+        (pathIndex     = buffer.FindChar('\t', isDomainIndex) + 1) == 0 ||
>+        (secureIndex   = buffer.FindChar('\t', pathIndex)     + 1) == 0 ||
>+        (expiresIndex  = buffer.FindChar('\t', secureIndex)   + 1) == 0 ||
>+        (nameIndex     = buffer.FindChar('\t', expiresIndex)  + 1) == 0 ||
>+        (cookieIndex   = buffer.FindChar('\t', nameIndex)     + 1) == 0) {
>+      continue;

ugh! I'm not a huge fan, mostly because it leaves lots of these variables
uninitialized... (and even if they are initialized to zero, is that what we
want?) the system of boolean logic is also kind of confusing, at the very least
put a comment saying "a cheap way of parsing a tab-delimited line into..."

ok, those are my comments as far as this patch goes - lots of the questions
don't even require a specific answer, I just want to make sure that we're doing
the right thing and make sure you're thinking about these things. For instance,
there may be a call to release the prefbranch for the prefobserver, but I
missed it.
Comment on attachment 116873 [details] [diff] [review]
v3 complete patch

thinking about this, I'm going to give an sr=alecf on the assumption that the
above issues are addressed.

(another diffdiff would be cool)
Attachment #116873 - Flags: superreview?(alecf) → superreview+
Attached patch patch v4Splinter Review
ready for checkin!
Attachment #116873 - Attachment is obsolete: true
Comment on attachment 118051 [details] [diff] [review]
patch v4

carrying over r/sr
Attachment #118051 - Flags: superreview+
Attachment #118051 - Flags: review+
thanks for the sr alecf!

comments to follow.
everything i've not responded to has been fixed up (see diffdiff). response to
alecf's review comments:

>Do we really need a whole other observer, or can we just use "this" as an
>observer?

not really... I can merge them if you'd like. phase 3 would be a good place
to do this, since everything will fold into nsCookieService. would you like them
merged? (the only reason I see for keeping them separate, is readability,
but that's a tenuous reason ;)

>can sCookieList just become mCookieList and be a non-pointer member of
>nsCookieService?

definitely; phase 3 would be a great place to do this, not too much point in
messing with it now (it was like that to start with, so don't blame me ;)

>+  private:
>+    nsCOMPtr<nsIPrefBranch> mPrefBranch;
>
>Does this create a bad ownership loop?

i don't believe so - all our prefobservers hold weak refs, but you'd be the
person to give an answer on this. could you double-check all the AddObservers
to make sure, please?

>>+  PRExplodedTime explodedTime;
>>+  PR_ExplodeTime(PR_Now(), PR_GMTParameters, &explodedTime);
>
>These can actually be kind of expensive - can you #ifdef the code around this
>so that this only happens in logging-enabled builds?

hmm, the logging functions only get called if MOZ_LOGGING or PR_LOGGING is
#defined. i'm guessing that in release builds, we enable MOZ_LOGGING at the
compiler level, and then:

PRIVATE PRLogModuleInfo *gCookieLog = PR_NewLogModule("cookie");

will be null if the env variable isn't set at runtime. i'm just guessing though;
it may be that we suck, and the PR_LOG(...) macro itself checks the env vars.
in which case, all this code will be alive in release builds. if so,
this will be something to fix in the near future. thanks for bringing it up ;)

>is a simple rfind safe here? I mean, can query strings also have '/'? Does this
>case ever even happen?

i believe so; that's what darin told me, at least. anything that doesn't QI
to nsIURL shouldn't support query strings. that's the reason for the comment
block just above that code.

>is nsTime() just going to call PR_Now() and go away? Should we just call PR_Now
>directly?

yes, this call just serves as a wrapper for PR_Now. we need it because PR_Now
returns PRInt64 which can't be dealt with nicely (LL_* macros). nsTime()
returns a nicely sanitized nsInt64-compatible type, which has overloaded
operators defined, for sexier-looking code.

>I wonder if nsManifestLineReader() might help us here... something to think
>about for the future..

tell me more please ;)
i've filed a bug to make nsILineInputStream not suck, i'll deal to that
at some point hopefully.
cookie.dll increased by 1168 on win32.
dwitte said he will address code size issue in next couple of patches (other
bugs, I assume :-) )
so we busted the OSF1 port with this checkin, which brought to attention that
the nsTime/nsInt64 fu is wrong...

filed bug 198694 to request improvements to those convenience classes (not very
convenient as they stand!), and in the meantime, here's a patch so we deal with
int64's correctly.

this will work fine until we can get those classes looking nicer, which will
remove the need for the two ugly macros added...
Attachment #118123 - Flags: superreview+
Comment on attachment 118123 [details] [diff] [review]
patch for nsTime bustage

r=dbradley
Attachment #118123 - Flags: review+
alecf: so you had a really good point about the expensiveness of logging. turns
out that PR_LOG() will be a no-op if logging isn't enabled thru the env vars (in
a release build), so we still end up executing the entire logging function...

easy to fix; just add a PR_LOG_TEST() at the beginning of the logging function.
this will give us a perf improvement, not sure how much, but... ;)
This seems to have added a few "might be used uninitialized" warnings (see also
bug 59652) to Brad TBox:

+extensions/cookie/nsCookieService.cpp:257
+ `nsresult rv' might be used uninitialized in this function
+
+extensions/cookie/nsCookies.cpp:610
+ `PRInt32 oldestPositionFromHost' might be used uninitialized in this function
+
+extensions/cookie/nsPermissionManager.cpp:205
+ `permission_HostStruct*hostStruct' might be used uninitialized in this function

At least some of those indicate real problems. For example, the first one comes
from the following code:

257                 nsresult rv;
258               
259                 if (aHttpChannel) {
260                   nsCOMPtr<nsIHttpChannelInternal> httpInternal =
do_QueryInterface(aHttpChannel);
261                   if (!httpInternal) {
262                     COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, aCookieHeader,
"unable to QueryInterface httpInternal");
263                     return rv;

Obviously, if do_QueryInterface fails, the function would return a random nsresult!
I landed the nTime bustage at 10:13 PM PST.
update on nsTime bustage: all fixed up, after a couple of iterations. what ended
up in the tree was slightly different to the bustage patch posted here, if
anyone cares, i can upload what actually made it in...

Aleksey: thanks, good catch on the 'rv uninitialized' thing. the other two i'm
aware of and are bogus.

tiny upcoming patch to fix the return fu in nsCookieService::SetCookieString.
not really sure we need two distinct log messages for related failure
conditions, so i just merged them. since all this logging stuff goes into
release builds (which i previously wasn't explicitly aware of), every little
bit helps ;)
Comment on attachment 118130 [details] [diff] [review]
patch for un-inited variable

rs=alecf?
Attachment #118130 - Flags: superreview?(alecf)
Attachment #118130 - Flags: review?(alecf)
I just checked this patch in to fix MOZ_PHOENIX bustage.  Could you review it
to make sure it's correct?
dbaron: looks perfect... thanks very much for fixing it up; sorry for not
catching the bustage on the phoenix tbox... will pay close attention to them in
future.
Comment on attachment 118130 [details] [diff] [review]
patch for un-inited variable

sr=alecf
Attachment #118130 - Flags: superreview?(alecf) → superreview+
Comment on attachment 118130 [details] [diff] [review]
patch for un-inited variable

Ok, r=mvl.
The pre-phase 2 didn't return a proper return value either. You might consider
fixing it in the future.
Attachment #118130 - Flags: review?(alecf) → review+
Re: comment #25

The first warning went away, but the othert two are still there.
the second warning is just the compiler not being smart enough - there's no
problem there, so I don't intend to fix it (although I won't confirm that until
i've figured out bug 199056...)

the third warning i have no idea about; mvl?
update:

so we've had a whole bunch of bugs filed on this rewrite and the permissions
rewrite (bug 191380) over the last few days. it's really amazed me just how
effective and responsive user-testing is... thanks a lot to mvl who diagnosed
and tested bugs/fixes, darin & alecf for the many & fast reviews, and timeless
for the checkin favors. all the bugs to date have been taken care of; now it's
up to people to thrash 1.4a and hope there aren't any more ;)

about the various codesize increases we've seen - after both rewrites landed, we
removed some old files from the build and dropped codesize by ~4k. so iirc, that
puts us back around neutral again. hopefully, codesize & perf will get better
from this point on - the crunchy work has been done, now it's time for
optimizations and other improvements for beta.

closing this one out - thanks again guys!
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.