Closed Bug 177698 Opened 22 years ago Closed 22 years ago

Cookie is not stored if its VALUE contains "max-age"

Categories

(Core :: Networking: Cookies, defect, P2)

Product:
Core
Component:
Networking: Cookies
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.4alpha

People

(Reporter: harunaga, Assigned: dwitte)

References

Details

Attachments

(3 files, 14 obsolete files)

review comments
(deleted), text/plain
Details
reply to darin's comments
(deleted), text/plain
Details
to mollify gcc...
(deleted), patch
alecf
: review+
dwitte
: superreview+
Details | Diff | Splinter Review
Cookie is not stored if it's VALUE contains "max-age". I believe VALUE can contain a string "max-age". testcase: <html> <head> <meta http-equiv="Set-Cookie" content="test=max-age"> <title>test</title> </head> <body>test</body> </html> See: http://lxr.mozilla.org/seamonkey/source/extensions/cookie/nsCookies.cpp#1802
Summary: Cookie is not stored if it's VALUE contains "max-age" → Cookie is not stored if its VALUE contains "max-age"
Status: UNCONFIRMED → NEW
Ever confirmed: true
Status: NEW → ASSIGNED
Priority: -- → P2
Attachment #104812 - Flags: review+
Comment on attachment 104812 [details] [diff] [review] skip ahead to attributes before testing for max-age or expires r=harishd One question though. If "max-age" attribute overrides expires then shouldn't you be checking for max-age first before checking for "expires"? That's if "max-age" was present then we don't have to check for "expires" - minor optimization....I believe. Am I making sense? :-/
Yes, what you are saying makes sense. It's not directly related to this bug, however, so why don't you open a separate bug report requesting that optimization. Thanks for catching that.
Comment on attachment 104812 [details] [diff] [review] skip ahead to attributes before testing for max-age or expires The Cookie attribute code is pretty ugly, looking for attributes as it needs them and thus scanning the string multiple times. Inefficient, but worse several bits of the code could potentially find its key as part of the value in a completely different token. For example, what if there's "path=/something/imax-agent/something"? You'll drop the cookie in that case too. How hard would it be to parse the header/string in order, making sure you're only looking for tokens where tokens ought to be? Or in the short term, make sure each token you think you found is preceeded by a semi-colon (and optionally whitespace if the spec allows that).
Attachment #104812 - Flags: needs-work+
Attached patch complete rewrite of cookie parsing code (obsolete) (deleted) — — Splinter Review
Attachment #104812 - Attachment is obsolete: true
Attachment #106428 - Flags: superreview?(dveditz)
Comment on attachment 106429 [details] [diff] [review] Same patch but with whitespace diffs removed -- might be easier to review >+ nsCAutoString path(pathAttribute); >+ path.SetLength(pathAttributeLength); SetLength call is redundant. no? You have done the same in other places too. >+ if (*domainAttribute != '.' && !cookie_IsIPAddress(cur_host.get())) { > // if host is not an IP address, force domain name to start with a dot > domain = '.'; >+ domainAttributeLength++; optional: Replace domain = '.' with domain.Assign('.'); >+ // extract remaining attributes >+ while(*ptr) { >+ if(*ptr == '\n' || *ptr == '\r') { >+ *ptr = '\0'; >+ break; Should we worry about \t\b etc?
From Harish's review (comment ) + nsCAutoString path(pathAttribute); + path.SetLength(pathAttributeLength); SetLength call is redundant. no? You have done the same in other places too. No, it's not redundant. The pathAttribute pointer points to the beginning of the attribute in the set-cookie header. That header continues well on past the atribute as there could be other attributes following. In other words, there is no null after the specific attribute in question. That is why pathAttributeLength was calculated when the attribute value was obtained, and is now being used to delimit the autostring to just that value. optional: Replace domain = '.' with domain.Assign('.'); Consider it done. + // extract remaining attributes + while(*ptr) { + if(*ptr == '\n' || *ptr == '\r') { + *ptr = '\0'; + break; Should we worry about \t\b etc? This code was just to make sure we don't go to far in case there is an embedded cr-lf in the set-cookie header. Embedded spaces and tabs are fine and, in fact, are filtered out explicitlyh as we search for each attribute.
>That header continues well on past the >atribute as there could be other attributes following. In other words, there is >no null after the specific attribute in question. But, that adds to overhead. That's you're creating a bigger string ( => more allocation - though, I belive, the auto string heap allocates only if the length is greater than 128 ) and then forcing a truncation by calling SetLength.
You should probably do this: Replace nsCAutoString path(pathAttribute); path.SetLength(pathAttributeLength); with const nsACString& path = Substring(pathAttribute, pathAttribute + pathAttributeLength);
Attached patch address the issues in comment 10 (obsolete) (deleted) — — Splinter Review
Attachment #106428 - Attachment is obsolete: true
Attachment #106429 - Attachment is obsolete: true
Attached patch same patch but with whitespace diffs removed (obsolete) (deleted) — — Splinter Review
Attachment #107106 - Flags: superreview?(dveditz)
Attachment #107106 - Flags: review?(harishd)
Comment on attachment 107107 [details] [diff] [review] same patch but with whitespace diffs removed >+ while (isspace(c)) { // skip over spaces at end of attribute name >+ c = *(++ptr); >+ } Is it possible that you could end up dereferencing a null ptr? > for(ptr=date; *ptr != '\0'; ptr++) { optional: Add whitespace around '='. That is, for (ptr = date.. Other than that r=harishd
Attachment #107107 - Flags: review+
harishd asked: Is it possible that you could end up dereferencing a null ptr? No. ptr is initialized to setCookieHeaderInternal before that routine is called.
Attachment #107107 - Flags: superreview?(jst)
Comment on attachment 107107 [details] [diff] [review] same patch but with whitespace diffs removed Rick kindly agreed to look sr this patch :-) Over to him.
Attachment #107107 - Flags: superreview?(jst) → superreview?(rpotts)
Attachment #106428 - Flags: superreview?(dveditz)
Comment on attachment 107107 [details] [diff] [review] same patch but with whitespace diffs removed -w patches can be helpful, but they aren't the real thing and shouldn't be the version approved.
Attachment #107107 - Flags: superreview?(rpotts) → superreview-
Comment on attachment 107106 [details] [diff] [review] address the issues in comment 10 > PRIVATE void >-cookie_SetCookieString(nsIURI * curURL, nsIPrompt *aPrompter, const char * setCookieHeader, >- time_t timeToExpire, nsIHttpChannel* aHttpChannel, nsCookieStatus status) { >+cookie_SetCookieString(nsIURI* curURL, nsIPrompt* aPrompter, char* setCookieHeader, >+ char* nameAttribute, char* valueAttribute, char* pathAttribute, >+ char* domainAttribute, char* secureAttribute, time_t timeToExpire, >+ PRInt32 nameAttributeLength, PRInt32 valueAttributeLength, >+ PRInt32 pathAttributeLength, PRInt32 domainAttributeLength, >+ nsIHttpChannel* aHttpChannel, nsCookieStatus status) { This is one of the ugliest param lists I've seen, find some way to simplify. If the args were nsAFlatStrings you wouldn't need to pass length for each attribute. A better choice would be a cookie-attribute structure with the items you need as members. You've already got cookie_CookieStruct -- you might be able to construct something that would work for both this use and what you eventually use that for, or just come up with something new. If you're using char* and length because these are all pointers into a common string and you're trying to avoid unnecessary copies (good thinking) then nsDependentSubstring might be a useful form of nsAString to look into. Why did setCookieHeader change from const char* to plain char*? It looks like it's only used in logging calls and never touched. >+ // process secure attribute >+ isSecure = secureAttribute ? PR_TRUE : PR_FALSE; Seems like this decision could have been made in cookie_ParseAttributes >+ // process path attribute >+ if (pathAttribute) { >+ const nsACString& path = Substring(pathAttribute, pathAttribute + pathAttributeLength); >+ CKutil_StrAllocCopy(path_from_header, PromiseFlatCString(path).get()); >+ } This chunk is trapped between two string worlds and ends up looking something like a Star Trek transporter accident. If you're going to keep living in the char* world then you should beef up your CKutil_StrAllocCopy() routines to take a length terminated form -- or better yet use PL_strndup() and don't reinvent the wheel. Even better would be to make this some useful nsACString form and eliminate the need for all the manual freeing that's going on here, especially if the arg is passed in as some nsACString or nsACString member of a struct. Same applies to the rest of the transporter accidents following. >+ int cur_host_length = PL_strlen(cur_host.get()); One of the nice things about moz strings is that the length is always known, recalculating it is wasted time: cur_host.Length() >+ nsresult rv; >+ nsCOMPtr<nsIPref> prefs(do_GetService(NS_PREF_CONTRACTID, &rv)); >+ if (!prefs || NS_FAILED(prefs->GetBoolPref(cookie_strictDomainsPref, &pref_scd))) { >+ pref_scd = PR_FALSE; >+ } >+ if ( pref_scd == PR_TRUE ) { >+ cur_host.SetCharAt(cur_host_length-domain_length, '\0'); >+ dot = (char *)strchr(cur_host.get(), '.'); >+ cur_host.SetCharAt(cur_host_length-domain_length, '.'); >+ if (dot) { It's got to be faster to scan the string than to call up the pref service, and you do this on every cookie. If you scan for dots first then in the normal compliant case you won't even have to check the pref, taking that time only for malformed cookies. (Or you could cache the pref value and add a pref-change callback... didn't think so ;-) ) The string searching is again of two worlds -- use cur_host.FindChar(). The standard nsAString::FindChar() only takes a starting point so if you use that you could check the return value and make sure it's bigger than the value you're looking for. But since , but since you know you have specifically an nsCAutoString you could take advantage of the third count arg to limit the search. You don't ever use domain_length except here, might be more readable if you renamed it and calculated the value you really want just once. int non_domain_length = cur_host.Length - PL_strlen(domain_from_header); dot = cur_host.FindChar('.', 0, non_domain_length); if ( dot != kNotFound) // check pref to see if this is OK >+ /* all tests passed, copy in domain to hostname field */ >+ CKutil_StrAllocCopy(host_from_header, domain_from_header); >+ isDomain = PR_TRUE; >+ PR_Free(domain_from_header); You can assign host_from_header = domain_from_header and avoid the extra allocate and free I'm going to deal with cookie_ParseAttributes and cookie_GetAttributeValue in reverse order because I'm more concerned with the logical flow: >+PRIVATE void >+cookie_ParseAttributes( >+ char* setCookieHeaderInternal, char*& nameAttribute, char*& valueAttribute, >+ char*& pathAttribute, char*& domainAttribute, char*& secureAttribute, >+ char*& expiresAttribute, char*& maxageAttribute, >+ PRInt32& nameAttributeLength, PRInt32& valueAttributeLength, >+ PRInt32& pathAttributeLength, PRInt32& domainAttributeLength) { as in the cookie_SetCookieString case this prototype is pretty ugly. Whatever you decide to do there (some sort of moz strings or a structure) you should do here. A structure might be nicest -- pass it in here to get filled in, then pass it on. You pass the cookie header in here as char* instead of const char*. You do potentially modify it, but only to stomp a null over a \n or \r. I guess that separates one cookie from another, but any such modified cookie string would never get used outside this parse routine. The calling routine (COOKIE_SetCookieStringFromHttp) already handles \n so if \r is also important you should add that at the outer level (use PL_strpbrk instead of PL_strchr) and leave the string mods out of here. Once you do that you don't need to duplicate the header just for this; one less string you have to worry about leaking or freeing at the end. >+ static const char PATH[] = "path"; style nit: Mozilla coding guidelines would lead people to look for #defines in a header if they see all caps like this. consts like this are encouraged to have names like "kPath" (initial 'k'). >+ static const PATH_LEN = sizeof(PATH) - 1; I think what you really want here is a bunch of NS_NAMED_LITERAL_CSTRING NS_NAMED_LITERAL_CSTRING(kPath, "path"); ... etc ... kPath is then a true nsACString and you can take advantage of .Length() and .Equals() when you need it >+ char c; >+ char* ptr; >+ >+ // extract cookie name >+ ptr = setCookieHeaderInternal; >+ while (isspace(c)) { // skip over spaces at start of cookie name >+ c = *(++ptr); >+ } c is undefined the first call to isspace(). It's not really helping the clarity here anyway, why not drop it in favor of while (isspace(*ptr)) ++ptr; Do you really need to check for all possible spaces? might run faster if you check only for the ones that are actually valid/likely, like ' ' and \t. If you don't want to write that out all the time you could make a COOKIE_SPACE() macro for it. >+ nameAttribute = ptr; >+ while ((c = *ptr) && !isspace(c) && c != '=' && c != ';') { >+ ptr++; >+ } This isn't quite the set of characters the RFC says is valid for names, do we care? Ditto for the attribute names later. Rather than do a slow check against all the individual possibilities like this (even more if you follow the RFC) you could create a 256-char table and use *ptr as the index. >+ valueAttribute = ptr; >+ while ((c = *ptr) && !isspace(c) && c != ';') { >+ ptr++; >+ } >+ valueAttributeLength = ptr - valueAttribute; The RFC talks about quoted values and allowing backslash escapes in quoted strings. Are values never quoted in practice? >+ while(*ptr) { So at this point we should either be on the null terminator, a space after the cookie value (there can't be spaces inside a cookie value? No, there can, Bugzilla uses them for column lists for example), or we're on the ';' separator. If you're on whitespace you really only want to eat whitepspace, but I think this falls into your default else clause that eats anything up to a null or semi-colon. >+ if(*ptr == '\n' || *ptr == '\r') { >+ *ptr = '\0'; >+ break; This is what I referred to earlier -- if COOKIE_SetCookieStringFromHttp caught all these correctly these chars would never be found here. >+ } else if (*ptr == ';') { Here we have attributes. Looks like you handle unknown attributes (either the spec'd "comment=" or experimental new ones) by ignoring them up to the next ';'. That might lead to trouble with quoted values that contain a ; which appears legal from the spec. The series of PL_strncasecmps will match invalid attributes which start with a valid prefix, which means you then have to catch that. (oddly you do it elsewhere which means you have to pass the matching part and its length around also). If you parsed the whole token--using the correct set of characters via a charmap table as mentioned before--you could grab the token in a Substring and then use Equals() to compare. That'd be a fast compare because it would quickly bail if the lengths didn't match, and you'd also know ptr was now at the end of a token and wouldn't have to worry about being in the middle. Since you want a case-insensitive compare you'd have to use the two-arg form of Equals() which might be a bit uglier than one would like. if (attr.Equals(kPath, nsCaseInsensitiveCStringComparator)) // whatever else if (attr.Equals(kDomain, nsCaseInsensitiveCStringComparator)) etc >+cookie_GetAttributeValue(char*& ptr, char*& attribValue, PRInt32 len, PRBool equalsRequired) { Doesn't seem to handle quoted values or backslash escapes. >+ // note: previously we were truncating the cookie string. That was bad because we might >+ // have chopped an attribute in half, such as the expire string. It's probably better to >+ // just reject such large cookies. The spec does not say how this is to be handled. In fact the spec does say: you can reject a cookie that's too big, but chopping it off is not allowed. >+ char *setCookieHeaderInternal = PL_strdup(setCookieHeader); I don't think you need to duplicate this string here since you can eliminate the need for cookie_ParseAttributes to modify the string. possibly you should have duped it earlier, though, since this routine modifies the supposedly const setCookieHeader arg. It gets unmodified, but there may be threading issues if necko is also using this header (unlikely). >+ Recycle(setCookieHeaderInternal); The old code used nsCRT::strdup() instead of PL_strdup(). If you make this change you need to change the Recycle() to PL_strfree().
Attachment #107106 - Flags: superreview?(dveditz) → superreview-
Some cleanup prior to addressing comment 17. I noticed that the changes in the preceding patch are affecting cookie logging in a bad way so I fixed that up. Also the uninitialized variable mentioned in comment 17 was causing the cookie code not to work, so I fixed that as well. Attaching a patch with those changes. Will address the remainder of comment 17 next.
Attachment #107106 - Attachment is obsolete: true
Attachment #107107 - Attachment is obsolete: true
I recommend changing the summary to indicate that this is incidentally including rewriting the cookie code to use mozilla strings instead of char*s internally.
I've been working on the parser code as part of a cookie rewrite I've been doing. I'm not entirely sure where I stand with this, since cookies is apparently now unowned, but if possible I'd like to completely rewrite our cookie code (perhaps in time for 1.4a, including reviews). Any comments or advice on this would be welcome. I have the parser code more or less finished at the moment. It's a rewrite of what Steve's done in this patch, and addresses dveditz's comments. I'll post something shortly (although it won't be diffed, since I've rewritten the calling functions extensively, and that patch would be too large to post in this bug).
Blocks: 187304
harish, dveditz: either of you willing to own this bug/patch? thx!
Attached patch dwitte's parser patch (not diffed) (obsolete) (deleted) — — Splinter Review
Initial code snippet for the new parser (not diffed; the code context should be clear from the calling code snippet commented at the top). BNF grammar is included, which details our impl and how/why it differs from RFC spec. dveditz: could you take a look at this and let me know if it addresses your comments above (or if you have more comments)? I'd like to fold those messy prototypes into a struct, but I need to check some string stuff with scc or someone first. (pending enhancement)
Attachment #111155 - Flags: review?(dveditz)
Attachment #108985 - Attachment is obsolete: true
Attachment #111155 - Attachment is obsolete: true
Attachment #111155 - Flags: review?(dveditz)
Attached patch Parser patch + moz string conversion (obsolete) (deleted) — — Splinter Review
This patch is complete and diffed, and does two things: integrates the new parser (previous patch) into nsCookies, and also converts pretty much all of cookies to using Moz strings instead of char*. So, I consider this a portion of the ongoing cookie rewrite. It'd be nice for this to go in 1.3b. Alec, can you let me know if you're able to review this? Some notes: 1) I chose not to convert nsCookieService.cpp to moz strings yet, since this is a big enough refactoring as it is. 2) The parser has been tested and debugged, but the rest of the changes have been less thoroughly tested. 3) There is a little bit of messiness in the char*->moz string conversion evident in nsCookieManager.cpp, but ignore that. Another patch (bug 179798), which will hopefully be checked in very shortly, fixes ownership issues for nsICookie and makes this cruft obsolete. I'll remove it as soon as the other patch gets checked in.
Attachment #112048 - Flags: superreview?(darin)
Attachment #112048 - Flags: review?(alecf)
Attachment #112048 - Attachment is obsolete: true
Attachment #112048 - Flags: superreview?(darin)
Attachment #112048 - Flags: review?(alecf)
Attached patch update of previous... fixes some nits (obsolete) (deleted) — — Splinter Review
Sorry guys, previous patch had some compile/runtime nits. Tested and fixed now. An extra note from previous comment: 1) The string conversion is not optimal in all places, because it's not complete yet. There's more extensive rewrite work that has to go on before all the string fu is nice. This patch brings it to a mostly-converted, reasonably efficient, working state.
Attachment #112086 - Flags: superreview?(darinf)
Attachment #112086 - Flags: review?(alecf)
Attachment #112086 - Flags: superreview?(darinf) → superreview?(darin)
Attachment #112086 - Attachment is obsolete: true
Attachment #112086 - Flags: superreview?(darin)
Attachment #112086 - Flags: review?(alecf)
Attached patch Update from timeless' comments (obsolete) (deleted) — — Splinter Review
Some updates from timeless' comments on IRC (thanks timeless). Adds a few more optimizations and removes the cookie_FixQuoted() function completely - nice string functions make it redundant. Ready for r/sr :)
Attachment #112143 - Flags: superreview?(darin)
Attachment #112143 - Flags: review?(alecf)
Comment on attachment 112143 [details] [diff] [review] Update from timeless' comments So I'm seeing some ugliness with the ToNewCString() stuff - are you aware the ToNewCString allocates a new string, which must be freed? I see you passing these newly allocated strings to nsCookie() I'm guessing that what you really want to do is use .get() to get at the string buffers that are contained in nsCAutoString name; and so forth. also, since this is such an extensive change, can you use the -p switch on your "diff"? it will put the current function name in the @@ lines. (so at least I know the name of the function being changed in each hunk)
Attachment #112143 - Flags: review?(alecf) → review-
Actually, ToNewCString() was what I intended, though if I'm wrong please correct me. When I submitted this patch, the ownership model of nsCookie was whacked, because nsCookie doesn't strdup the char* strings you pass to it (but it frees them in the dtor). The nsCAutoStrings will go out of scope and destruct when the instancing method (GetNext) terminates, so if I used .get() then the char* ptrs in the nsCookie object would point to freed memory, right? As I mentioned above, the patch for bug 179798 (checked in recently - before the 1.3b freeze) reworked the ownership model of nsCookie. It now uses nsCStrings. So this portion of my patch can be redone. Actually - is it possible to instance an abstract string class in GetNext? I'm not quite sure if it'd work, but I think the principle makes sense. Here's what I mean: COOKIE_Enumerate finds a cookieStruct object in memory, which has CString members. It then passes it to FixQuoted, which takes the ACString& and returns another ACString& which has the quote chars backwhacked. COOKIE_Enumerate then returns this ACString& as an outparam. So far, no copying, just references. What I want to do is replace the CAutoString concrete strings in GetNext, with ACString& references. So the outparam strings of COOKIE_Enumerate are still just references, instead of being copied into a concrete class. Then I can pass these references to |new nsCookie()|, whose CString members will perform the copy. So it avoids the additional copy involved in using CAutoString members. Can I do this?
Attached patch Update of previous (obsolete) (deleted) — — Splinter Review
alecf: here's an updated patch, which removes some of the ToNewCString() messiness, as a result of the checkin for bug 179798. Also, I used diff -up as requested. My previous question still applies to this patch; namely, avoiding that extra copy. Let me know what you think... I know the diff is hard to read in places, mostly because of the messiness involved in removing all that old code and replacing it with new stuff... so if I can make your job easier (by uploading the cpp's or something), please let me know :) requesting r=alecf, sr=darin, target is 1.4a.
Attachment #112143 - Attachment is obsolete: true
Attachment #112755 - Flags: superreview?(darin)
Attachment #112755 - Flags: review?(alecf)
Reassigning to self, targeting for 1.4a
Assignee: morse → dwitte
Status: ASSIGNED → NEW
Target Milestone: --- → mozilla1.4alpha
Attachment #107106 - Flags: review?(harishd)
Attachment #112143 - Flags: superreview?(darin)
Comment on attachment 112755 [details] [diff] [review] Update of previous overall, the code looks ok, its really hard to tell if this mimics the exact behavior of the code in question. I guess the key is to pound the heck out of it in 1.4alpha. The two concerns I have: 1) that the cookie_CookieStruct is now bigger than it was before (it increased by 48 bytes, due to the conversion from char* to nsCString) so I sure hope that this structure isn't used much. 2) that this introduces some kind of regression that is security-sensitive. cookies is a very dangerous area to be fiddling with and I would REALLY like to see someone make up some test cases that address previous security bugs that we have fixed w.r.t. cookies. I think a really useful thing to do would be to do a CVS Blame (see http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/extensions/cookie/nsCookies .h for an example, or look directly at the logs in http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/extensions/cookie/nsCookies.h ) of each of the cookies files, and look for security-related comments. From there, we can hopefully recreate test cases that we can test this code against. I can help get these test cases up on www.mozilla.org if necessary (we can also check them into the source tree, if we don't need them served up via HTTP) So hopefully darin will review this soon, so we can get this in as soon as the tree opens for 1.4alpha
Attachment #112755 - Flags: review?(alecf) → review+
Wow, you kinda snuck up on me with that r+, thanks alecf :) your concerns: 1) cookieStruct is the main cookie storage medium, so it's used for every cookie. So yeah, this part has become a little worse. We have three options, I leave the choice to you: a) go back to char*, and throw in some dependentCString ugliness to wrap them, every time we access the struct. eww. b) you can trust me that I'll be fixing this in "part 3" of the rewrite. This patch is part 1. Part 2 is almost finished, and cleans up most of the functionality-related mess in cookies. part 3 will just be shifting everything into nsCookieService and killing nsCookies.cpp, which will allow some nice refactoring - specifically the switch to using a proper, efficient nsCookie storage class. This will happen pretty soon, if the reviews go ok. c) you can ask me to incorporate the new storage class now, which might drag this patch out even longer. 2) I agree. There are lots of things which will be changing in "part 2" which will need a thorough thumping. I've checked out a lot of them as I go, by thinking "why the hell do we do this?" and then running to CVS blame - but some good QA will be needed. I should probably go chat to tever and see if we can thrash out a test suite of sorts. For this reason, "part 2" will probably be a meaty patch to review, so if you're not keen to review it please let me know. Over to darin. Thanks again alecf!
adding alecf as cc. I posted a comment above, regarding your r+...
yeah, I'll go with b) that you'll be improving this in the rewrite... as for getting those test cases, I guess we should start compiling a list of bugs to look at. I'm really glad to hear you're looking back over CVS Blame!
Attached patch Yet another update (obsolete) (deleted) — — Splinter Review
Forgot to change a couple lines dealing with the isSecure attribute, fixed now. (just needed s/isSecure/cookie->isSecure/ in 2 places). Carrying over r=alecf, requesting sr=darin yet again ;)
Attachment #112755 - Attachment is obsolete: true
Attachment #112755 - Flags: superreview?(darin)
Attachment #114366 - Flags: superreview?(darin)
Attachment #114366 - Flags: review+
Attached file review comments (deleted) —
my comments on attachment 114366 [details] [diff] [review].
> size increased by 3x24 bytes er.. i meant, size increased by 3x(4x4)... sizeof(nsCString) == 16. don't worry too much about the size increase for now. it is minor, especially since the cookie list is never very large. sizeof(cookie_CookieStruct) == 88. max number of cookies in the list is 300, so max overhead due to cookie struct (not counting strings) is ~25k. previously it was 11k. it could be made as low as 5k by combining all the flags into one 32 bit integer. this savings might be worthwhile, but save it for another bug ;-)
Attachment #114366 - Flags: superreview?(darin) → superreview-
Attached patch one more patch (obsolete) (deleted) — — Splinter Review
addresses review comments.
Attachment #114366 - Attachment is obsolete: true
Attachment #114847 - Flags: superreview?(darin)
Attached file reply to darin's comments (deleted) —
my reply to darin's sr- comments. fixed everything apart from 2 nits, which I'd like to leave for the next patch, if that's ok :) thanks darin!
Attachment #114847 - Flags: superreview?(darin) → superreview+
Comment on attachment 114847 [details] [diff] [review] one more patch PR_STATIC_CALLBACK
Attachment #114847 - Flags: review-
Attached patch to mollify timeless (obsolete) (deleted) — — Splinter Review
okay. carrying over sr=darin. requesting a quick review from alecf (or timeless?)
Attachment #114847 - Attachment is obsolete: true
Attachment #114902 - Flags: superreview+
Attachment #114902 - Flags: review?(alecf)
Attached patch to mollify gcc... (deleted) — — Splinter Review
sigh... carrying sr=darin again, r=alecf?
Attachment #114902 - Attachment is obsolete: true
Attachment #114902 - Flags: review?(alecf)
Attachment #114909 - Flags: superreview+
Attachment #114909 - Flags: review?(alecf)
Drivers will gladly line up a carpool into the pre-open-for-any-checkin 1.4alpha trunk, if that would help. Let us know soon, we're lining up carpool landings. /be
Comment on attachment 114909 [details] [diff] [review] to mollify gcc... The one thing I'm concerned about is: + if (domain.Equals(NS_LITERAL_CSTRING(".") + host, nsCaseInsensitiveCStringComparator())) { I think technically the NS_LITERAL_CSTRING will go out of scope after the nsConcatenatedCString is created.. I think you need to make this an NS_NAMED_LITERAL_CSTRING() fix that and sr=alecf
Attachment #114909 - Flags: review?(alecf) → review+
if that doesn't work, then this wouldn't work either: nsCString a; a = NS_LITERAL_CSTRING("foo") + NS_LITERAL_CSTRING("bar"); if that doesn't work, then we have a lot of broken code in the tree. it's got to be ok ;-) i'm just going to leave this code as is.
The code alecf quoted is fine. The temporary doesn't get destroyed until the statement is completed.
fixed-on-trunk
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
I hate to say it, but this bumped our codesize by 4k - thats no small change. The diff from tinderbox is below.. I really feel like the next stage of the rewrite needs to focus on reducing codesize (by reducing complexity of course) The good news is that cookie_SetCookieString dropped from 3.8k to 2k, but you can see there are new methods where some of that logic went (maybe cookie_ParseAttributes?) libcookie.so Total: +4260 (+14768/-10508) Code: +4004 (+12948/-8944) Data: +256 (+1820/-1564) +4004 (+12948/-8944) T (CODE) +4004 (+12948/-8944) UNDEF:libcookie.so:T +2020 cookie_SetCookieString(nsIURI *, nsIPrompt *, nsAFlatCString const &, _cookie_CookieStruct *, long, nsIHttpChannel *, int) +1856 cookie_ParseAttributes(nsDependentCString &, _cookie_CookieStruct *, nsACString &, nsACString &) +1268 COOKIE_GetCookie(nsIURI *) +1172 COOKIE_Remove(nsACString const &, nsACString const &, nsACString const &, int) +1044 COOKIE_AddCookie(nsACString const &, nsACString const &, nsACString const &, nsACString const &, int, int, long, int, int) +768 cookie_IsInDomain(nsAFlatCString const &, nsAFlatCString const &) +688 cookie_GetTokenValue(char const *&, char const *&, nsDependentSingleFragmentCSubstring &, nsDependentSingleFragmentCSubstring &) +684 cookie_LogSuccess(int, nsIURI *, nsAFlatCString const &, _cookie_CookieStruct *) +520 COOKIE_SetCookieStringFromHttp(nsIURI *, nsIURI *, nsIPrompt *, char const *, char *, nsIHttpChannel *) +516 cookie_CheckForPrevCookie(nsACString const &, nsACString const &, nsACString const &) +444 cookie_CheckForMaxCookiesFromHost(nsACString const &) +292 cookie_SameDomain(nsAFlatCString const &, nsAFlatCString const &) +260 COOKIE_Enumerate(int, nsACString &, nsACString &, int &, nsACString &, nsACString &, int &, unsigned long long &, int &, int &) +192 cookie_Count(nsAFlatCString const &) +168 cookie_IsFromHost(_cookie_CookieStruct *, nsAFlatCString const &) +136 cookie_ParseDate(nsAFlatCString const &, long &) +116 cookie_FixQuoted(nsACString const &, nsACString &) +76 nsDependentSingleFragmentCSubstring::~nsDependentSingleFragmentCSubstring(void) +72 __tcf_0 +72 __tcf_1 +72 __tcf_2 +72 __tcf_3 +72 __tcf_4 +68 COOKIE_RemoveSessionCookies(void) +64 cookie_IsIPAddress(nsAFlatCString const &) +64 cookie_RemoveExpiredCookies(void) +64 cookie_RemoveOldestCookie(void) +52 atexit +28 nsIURL::GetIID(void) +16 COOKIE_Read(void) +12 nsDependentSingleFragmentCSubstring::GetBufferHandle(void) const -4 cookie_isForeign(nsIURI *, nsIURI *) -8 nsCookiePromptService::CookieDialog(nsIDOMWindow *, nsICookie *, nsACString const &, int, int, int *, int *) -48 deleteCookie(void *, void *) -56 cookie_IsIPAddress(char const *) -56 COOKIE_Write(void) -96 cookie_IsFromHost(_cookie_CookieStruct *, char *) -128 cookie_ParseDate(char *, long &) -176 cookie_IsInDomain(char *, char *) -180 cookie_FixQuoted(char *) -180 cookie_Count(char *) -188 cookie_SameDomain(char *, char *) -268 cookie_CheckForPrevCookie(char const *, char const *, char const *) -272 cookie_CheckForMaxCookiesFromHost(char const *) -284 nsCookieManager::Remove(nsACString const &, nsACString const &, nsACString const &, int) -412 COOKIE_Enumerate(int, nsACString &, nsACString &, int *, nsACString &, nsACString &, int *, unsigned long long *, int *, int *) -476 COOKIE_Remove(char const *, char const *, char const *, int) -536 nsCookieManager::Add(nsACString const &, nsACString const &, nsACString const &, nsACString const &, int, int) -672 cookie_LogSuccess(int, nsIURI *, char const *, _cookie_CookieStruct *) -1084 COOKIE_AddCookie(char *, char *, char *, char *, int, int, long, int, int) -3820 cookie_SetCookieString(nsIURI *, nsIPrompt *, char const *, long, nsIHttpChannel *, int) +128 (+132/-4) D (DATA) +128 (+132/-4) UNDEF:libcookie.so:D +128 nsDependentSingleFragmentCSubstring virtual table +4 force_to_data -4 kWhitespace +80 (+80/+0) B (DATA) +80 (+80/+0) UNDEF:libcookie.so:B +12 kDomain.2150 +12 kExpires.2155 +12 kMaxage.2160 +12 kPath.2145 +12 kSecure.2165 +4 _.tmp_0.2146 +4 _.tmp_1.2151 +4 _.tmp_2.2156 +4 _.tmp_3.2161 +4 _.tmp_4.2166 +48 (+1608/-1560) R (DATA) +48 (+1608/-1560) UNDEF:libcookie.so:R +1592 sPrefChangedTopic +16 _Q26nsIURL18GetIID__6nsIURL..0.iid -1560 secureToken.2034
4k seems pretty small to me, especially given what I would expect are significant improvements in correctness (from making the logic of the code match the logic of the requirements of the code, i.e., proper parsing). I wouldn't be surprised if some of this increase were related to the string APIs, though. (Is it possible that we could focus on more than one aspect of code quality at once rather than sequentially focusing on a sequence of important things without any simultaneous consideration for the others, and then realizing what we've missed and switching focus? Nevertheless, I hope that this patch is the beginning of a cleanup of the cookie module that would lead to its becoming considerably smaller.)
Sorry for not being around over the weekend to respond to this stuff... and thanks for the compiling/checkin darin! In response to alecf/dbaron: the _only_ functional change (i.e. rewritten code) this patch introduced, was the new parser. This consists of two functions, cookie_ParseAttributes and cookie_GetTokenValue; which replace code that formerly lived in cookie_SetCookieString and (to a much smaller extent) COOKIE_SetCookieStringFromHttp. I expect this change to have a small impact on codesize (probably a reduction, since a bunch of unrolled code was rolled up into a loop). The entire rest of the patch converted the module to use ns*Strings instead of char*. So the brunt of the 4k increase was probably due to that - so although I don't mean to absolve responsibility for the increase, there's not too much I can do about it (unless you want me to reverse the string conversion parts). alecf is much more qualified to comment on this than myself. dbaron: I try to consider the effects of changes as I go. There are some regressions I knew this patch would introduce (datasize- and perf-wise), which I'll be fixing in the next few patches. In a way, this patch just sets the stage for later reductions. But this 4k increase from a string conversion (if that's correct) surprises me... Any comments?
4k doesn't seem like a lot sure.. but what did we gain? we fixed a bug dealing with max-age. That wasn't 4k worth of value, in my mind. And like I said I realize we're rewriting this module, but let's not blindly bloat in the name of.. whatever we want to call this :) What will stage 2 look like? another 4k? How many 4k chunks do we keep adding before we're at the point of no return? Is the cookie module collapsing or expanding? :) my main issue is this (and it sounds like we're mostly on the same page here, but I want to be clear) If we're rewriting cookies with only minor increases in functionality, it simply shouldn't be bigger. The current code is a mess of minor functions with wierd codepaths that has inherent bloat in its own obscurity. A rewritten module should be consise and the design should try to incorporate edge cases and so forth. You can't say "there's not too much I can do about it" because there is something you can do about it: make judgements about where using things like the string classes is appropriate, where maximizing code reuse is appropriate and so forth.. you are not at the mercy of the compiler. Anyway, carry on.. I look forward to the future cleanup.
>4k doesn't seem like a lot sure.. but what did we gain? we fixed a bug dealing >with max-age. Yeah, unfortunately that's all that can be said for this patch. It comes down to the way I chose to divide the patches. When the rewrite is finished, the division (hopefully) doesn't matter, but that doesn't mean that each portion shouldn't be scrutinized - and yes, I sure hope the cookie module gets smaller ;) >A rewritten module >should be consise and the design should try to incorporate edge cases and so >forth. You can't say "there's not too much I can do about it" because there is >something you can do about it: make judgements about where using things like the >string classes is appropriate, where maximizing code reuse is appropriate and so >forth.. you are not at the mercy of the compiler. I agree. The second example I hope I've followed... the first, I could use some advice on. The only advice I'm aware of so far is "use strings, they're good". We've already spoken about optimizing the cookie storage class to use arena allocation w/o string classes internally; but as far as the cookie code at large goes, I have no idea where using strings would be inappropriate. So if you think my conjecture is correct (that the string conversion gave us most of the 4k increase), and you think that's a "bad thing" overall, then maybe we should talk about it. >Anyway, carry on.. I look forward to the future cleanup. As do I ;)
dwitte asked me on irc if I could work out what was happening. Random suggestions involve code inlining and the like, or overhead due to virual function calls However, tinderbox shows 824 bytes, not 4K: http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1045963140.15713.gz&fulltext=1 cookie Total: +824 (+6068/-5244) Code: +438 (+5543/-5105) Data: +386 (+525/-139) Some of the data is changed string text, but the majority is +282 UNDEF:cookie:idata$6 I have no clue what that is, but since member vars were changed from char* to nsCString, theres extra constructor code (which was either inlined, or had stack pushes arround a call), and similarly extra destuctor code. Ditto for the .bss size increase. On the code size, 438 bytes seems reasonable to me. If your goal is to reduce size as much as possible, then use MSVC's equiv to -Os. Just don't expect perf to get better (althogh of course it may, due to cache issues or the like) Not to mention that even 4K is only 0.049% of the mZ numbers from beast, and only 0.028% of the Z. UPDATE: OK, so the 4K was from comet. This is larger, possibly because its without -O2[*], but we're still only taking about 0.032% and 0.021% size increase. Yes, I know, lots of little changes make big changes, and I'm not claiming that size isn't important but... Looking at comet, I see that the large DATA increase I couldn't explain above was for the virtual table for nsDependentSingleFragmentCSubstring. You can't really get away without that. (If libcookie wasn't in its own .so, then the linker would combine it with others) If you really want to go into this more, look at an asm diff between the two versions and see what you find. [*] Which is not to claim that -O2 is automatically smaller than -O. Theres that size/speed tradeoff again...
In addition to what bbaetz said: After looking at alecf's results (for the 4k version, not the 800b version), it seems that almost all the increase was due to the string conversion portion of the patch, of which the worst offenders are: +1172/ -476 COOKIE_Remove +768/ -176 cookie_IsInDomain +516/ -268 cookie_CheckForPrevCookie +444/ -272 cookie_CheckForMaxCookiesFromHost +292/ -188 cookie_SameDomain +168/ -96 cookie_IsFromHost Take cookie_IsInDomain for example - the only changes are some calls to .Equals(), Substring(), and .Length(). And this bloated the function by 600 bytes, according to the data... So I have three questions. 1) Which one should we care about most - the 4k or 800b version? 2) From purely random correlation with bbaetz's musings, I'm guessing the increase in cookie_IsInDomain increase is related to Substring(), which invokes nsDependentSingleFragmentCSubstrings in this case. If so, what can we do to fix it? 3) I'd really like to know what |+282 UNDEF:cookie:idata$6| means... If it'll help, poking around the asm might be possible. alecf, let me know what you think?
1) percentagewise, they're roughly equal, so either, I suspect. 2) Some of this is because stuff is defined in the headers, and so we get one copy per .so. IOW, the 600 bytes is not the use of substring _here_, but rather the use of any anywhere. If you were to add Substring several hunderd more times, hopefully it wouldn't bve 600 bytes each type. OTOH, since this is counted as part of the function, it may be that much, because of inlining 3) From the linux results, thats the vtable I mentioned. Thats definately once per .so.
A while ago, waterson worked on the static build, and IIRC with gcc/gld patches done by rfg@monkeys.com that combine identical vtables and other readonly data. Did those changes make it back into the toolchain sources? Does this static build joy address (3) for some embedders? Maybe we should combine libcookie with another .so. /be
folks: please note that this "cookies rewrite" is a work in progress. we already accepted a max bloat increase of 14k with _this_ patch (due to storing nsCString objects in the cookie database). see comment #37. we know how to eliminate that bloat, and in the end, the dynamic footprint of the cookies database will actually go down by roughly 6k. so, one step at a time. moreover, this patch is a great improvement to the cookies code. the new code from dwitte is far more manageable that what we had before. because of this, we will have a much easier time optimizing cookies. i think that more than justifies a little codesize increase in the short term. brendan: the plan is to combine the cookies backend with libnecko. that will probably happen during the 1.4 cycle.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: