Closed Bug 195908 Opened 22 years ago Closed 22 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: 22 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: