Closed Bug 47437 Opened 25 years ago Closed 23 years ago

nsCookie.cpp is duplicating URL parsing logic

Categories

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

Product:
Core
Component:
Networking: Cookies
Platform:
x86
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla0.9.6

People

(Reporter: jud, Assigned: morse)

References

Details

Attachments

(1 file, 14 obsolete files)

no longer using public nsCOMPtr ...
28.70 KB, patch
morse
: review+
alecf
: superreview+
Details | Diff | Splinter Review
The following link points to a URL parsing function. We need to remove this and change over to nsIURL parsing for consistencies sake and to remove code duplication. http://lxr.mozilla.org/seamonkey/source/extensions/cookie/nsCookie.cpp#2631
*** Bug 46758 has been marked as a duplicate of this bug. ***
-> morse
Assignee: valeski → morse
Status: NEW → ASSIGNED
Whiteboard: [y]
Netscape nav triage team: based on Steve Morse's pre-triage recommendation, this is not a beta stopper.
Keywords: nsbeta1-
Whiteboard: [y]
Target Milestone: --- → mozilla1.2
Is this bug still valid ... nsCookie.cpp was heavily changed in the meantime. Has the mentioned code been removed or moved to another file?
There's not enough information given in this bug report to know if it's still valid or not, because the code it references has been moved and there's no way to find out what the code that was referenced was. So I'm marking this as invalid because certainly the report is invalid as written. Jud, if you can be more specific and tell us what code it is that you are referring to, then reopen the report. Thanks.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → INVALID
re-opening. see the cookie url parsing routines here http://lxr.mozilla.org/seamonkey/source/extensions/cookie/nsUtils.h#39 this is bad. on top of that nsCookies.cpp includes duplicate domain searching/parsing code, as well as host searching/comparing code.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Just so we don't lose the reference when line numbers again change or modules get rearranged, the code Jud is referring to is the routine CKutil_ParseURL from the file extensions/cookie/nsUtils.cpp
Status: REOPENED → ASSIGNED
I think CKutil_ParseURL can be easily replaced with a call to nsIOService::ExtractUrlPart(const char *urlString, PRInt16 flag, PRUint32 *startPos, PRUint32 *endPos, char **urlPart) Currently CKutil_ParseURL is only called with PATH or HOST. If it stays this way and not more then one component is asked for at once, it should work. But then again ExtractUrlPart looks at some parts really bogus to me. That stuff needs more serious investigation.
Depends on: 97983
Okay, here is a first cut to remove the urlparser-logic from the cookie extension. There maybe some issues here: - cookie_isForeign returns a boolean. What shall happen if no IOService is available? Currently I answer for cookie_isForeign with PR_TRUE. - There was a check for the hostlength inside the old urlparser-logic in nsUtils. This is now gone. Is it really necessary to have this check? - Since the new function returns the host always without it's port it made sense to remove the code that cut it from the host in the old world. Maybe this code can be even more simplified.
- isForeign... we need to cache the IOService in a static var somewhere. these routines are called a lot and retrieving the IOService so much is going to be expensive. let's cache it (probably init it in COOKIE_Read()) instead. - I don't recall on the host len piece. steve? - removing the port makes sense in this context. what's our port behavior supposed to be though Steve. I *think* different ports can't share cookies.
The host length was important because the cookies need to be sorted by reverse host-length. It has to do with the testing of a host being in a domain. Search for comment /* shorter strings always come last so there can be no ambiquity */ in nsCookies.cpp to see where this is made use of. I'm not sure about the port issue either. Can someone check the cookie spec on this?
here's the spec http://www.netscape.com/newsref/std/cookie_spec.html first and foremost, we can't break existing functionality.
I agree that we should cache the ioservice. Any hint on where to look how that is done best? This hostlength-check was a truncation at a fixed length to avoid a crash, at least that's what the comment said. My question is if this check is still necessary and if yes in which context. It looked to me like the old code returned host+port and then the port was cut each time. I will take a look at the cookiespec to see what I can get from it.
> This hostlength-check was a truncation at a fixed length to avoid a crash, at > least that's what the comment said Oops, sorry, I was talking about the use of hostlength in nsCookies.cpp because I noticed that was involved in your patch. I realize now that you are talking about its use in nsUtils.cpp. Therefore please ignore my comment above. I have no recollection of why that code was added. But it appears to be to prevent overflowing a fixed-length buffer that was used in the old implementation.
The above patch contains a first try on caching the ioService. Can't say I like it and something must be wrong because I crash at shutdown. I have a private pointer to an IOService in nsCookies.cpp and a function that get's it from there or fills the variable and then returns it (on first call). Judson is this the kind of caching you thought of?
The patch is really bogus, I crash in getter_AddRefs when setting a Cookie.
close. getIOService() is still more expensive that needed. your private declaration is fine and init to null is good. now we just need to init it and clean it up (set it to '0'). a good init place would be in COOKIE_Read(), I'm not sure where the best clean up place is. maybe everytime we "write" cookies? If there's a better cleanup/shutdown call, we should use that instead; steve?
COOKIE_RemoveAll might be a good place to do the cleanup.
Okay, if I understand it correctly you want to init the private variable on COOKIE_read and than simply use the variable (without check!). On COOKIE_removeAll it should be set to 0 again.
Attached patch updated patch (obsolete) — — Splinter Review
I have updated the patch, but it is still crashing on shutdown. Something must still be wrong, but gdb does not give any useful output. I'm on vacation until wednesday next week (diving in Egypt ... again), hope this latest patch is a basis to work on.
I made a new diff, no longer crashing on shutdown. Don't ask me why ... Maybe someone (Steve?) can apply the patch and verify that all is working, better than I can do it ...
Let's readdress the issue of port numbers. I think I understand it a little better now than I did when I made my meaningless comments above. In COOKIE_GetCookie (nsCookies.cpp) there was code that calculated the length of the host/domain name for the current server exclusive of the port number, and then we tested to see if that name was indeed in the same domain as the cookie in the cookie list. That's confusing, so let me clarify by example. Suppose we are visiting "www.xyz.com:80" and we previously stored a domain cookie for ".xyz.com". Prior to this patch we would determine if we were in the domain by calling cookie_IsInDomain(".xyz.com", "www.xyz.com:80", 11); What that routine would do is take the first 11 characters of the second argument, namely "www.xyz.com" and see if that ends with the first argument, namely ".xyz.com". And in this case the test is successful. In your patch you are removing the stripping of the :80 when computing the length. So you are going to call the cookie_IsInDomain routine with a 14 instead of 11. So the routine will take the first 14 characters of the second argument, namely "www.xyz.com:80" and see if that ends with the first argument, namely ".xyz.com". And now the test will fail. So I believe that your patch has introduced an error in that www.xyz.com:80 will no longer appear to be in the .xyz.com domain.
No, I don't think so. A real difference between the old urlparser stuff in nsUtils.cpp and ExtractUrlPart from nsIOService is that the new stuff always returns the host *without* the port. Having code in there that removes a port that is never there seems a waste of space and cycles to me ...
OK, I didn't read that far. If you are removing the port numbers, are you sure that you will not be reintroducing bug 99311?
Okay, you got me. There must be a place where the host:port information is stored as site info, not the host/domain alone. Back on the drawing board ...
I added the url_Port mask to nsIIOService and also code to query for it and for url_Host | url_Port which returns host:port. The colon stuff is back in, I verified that the issue in bug 99311 works with this patch.
Looks good now. r=morse cc'ing alecf for sr.
Target Milestone: mozilla1.2 → mozilla0.9.5
Comment on attachment 51449 [details] [diff] [review] updated nsIOService.cpp to deal with jags ToNewCString changes r=morse
Attachment #51449 - Flags: review+
Comment on attachment 51449 [details] [diff] [review] updated nsIOService.cpp to deal with jags ToNewCString changes nit: this line (in two places) is not needed: + portstr = 0;
Attachment #51449 - Flags: superreview+
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Comment on attachment 51529 [details] [diff] [review] complete patch incorporating dougs comments excellent! sr=alecf
Attachment #51529 - Flags: superreview+
Attachment #51529 - Flags: review+
I think we should wait until after 0.9.5 is branched to check this in.
sounds good to me. it's all reviewed up and ready to go once that happens.
OK, I'll hold off checking it in.
Attached patch updated patch (obsolete) — — Splinter Review
I had to initialize the port in ExtractUrlPart and also found one more place to add url_Port.
Attached patch another update (obsolete) — — Splinter Review
... and I had to use some brackets in ExtractUrlPart to make sure host|port was correctly executed.
r=morse on the latest patch. alecf, can you please redo your sr on this? Thanks.
Comment on attachment 52440 [details] [diff] [review] another update This is looking great. I love seeing all that duplicate code being removed :-). couple things tough... - Please assert (NS_ASSERTION) IOService before using it. I know we're initing it in the right place, but, it's better to be safe by asserting that it's valid before using it. - in nsPermissions and nsImage, we should use the static (PRIVATE) IOService caching mechanism as well so we don't lookup the service each time through. That code should look just like the IOService declaration and init code in cookies.
Attachment #52440 - Flags: needs-work+
Integrated assertions, also used cached IOService in nsPermissions.cpp. I can't find anything useful in nsImages.cpp, how can the cached IOService work there?
Comment on attachment 52462 [details] [diff] [review] updated patch using Judson's comments Permissions looks good. Regarding images... nsImages.cpp is just a utility cpp file. It looks like the real driver of those functions is the nsImgManager. You should cache the IOService as a member variable of that, and pass it into the nsImages.cpp routine(s) as a parameter (http://lxr.mozilla.org/seamonkey/source/extensions/cookie/nsImgManager.cpp#130).
Attachment #52462 - Flags: needs-work+
Comment on attachment 52575 [details] [diff] [review] caching IOService in nsPermissionManager the same way as in nsImgManager r=morse if the crash is gone
Attachment #52575 - Flags: review+
Attached patch unified patch (obsolete) — — Splinter Review
The crash is gone again in the unified patch, but I would really like to see this being tested on more than one machine. I can't say I have a good feeling about this crash at shutdown showing up and than vanishing again.
can someone mark the patches that are no longer relevant as "obsolete"? I'm getting lost figuring out which patches are current...
Attachment #52575 - Attachment is obsolete: true
Attachment #52563 - Attachment is obsolete: true
Attachment #52462 - Attachment is obsolete: true
Attachment #52440 - Attachment is obsolete: true
Attachment #52433 - Attachment is obsolete: true
Attachment #51529 - Attachment is obsolete: true
Attachment #51449 - Attachment is obsolete: true
Attachment #51289 - Attachment is obsolete: true
Attachment #51206 - Attachment is obsolete: true
Attachment #49570 - Attachment is obsolete: true
Attachment #49460 - Attachment is obsolete: true
Attachment #49450 - Attachment is obsolete: true
Attachment #49324 - Attachment is obsolete: true
Comment on attachment 52576 [details] [diff] [review] unified patch r=morse
Attachment #52576 - Flags: review+
Comment on attachment 52576 [details] [diff] [review] unified patch sr=alecf
Attachment #52576 - Flags: superreview+
This bug uses a static nsCOMPtr to the IO service. Static nsCOMPtrs are bad because they don't get freed until after XPCOM shutdown and should never be used. This is causing increased leaks on some tinderboxes and crashes on other tinderboxes. (Also, rv = nsServiceManager::GetService(kIOServiceCID, NS_GET_IID(nsIIOService), getter_AddRefs(mIOService)); is much better written as mIOService = do_GetService(kIOServiceCID, &rv); .)
Patch was checked in before dbaron's comment was added.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
ack! sorry I missed that in the reviews.. you know "caching" actually threw a red flag up in my mind, but I forgot to check the patch for static nsCOMPtrs. we need a better solution here or we need to back this out..
Patch backed out due to memory leaks and crashes.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
To make matters worse, this patch caused crashes on the ports page. Namely: monkeypox Linux/ppc 2.2.15pre3 Depend nsPermissionManager.cpp: In method `nsresult nsPermissionManager::Add(const char *, int, int)': nsPermissions.h:59: too few arguments to function `void PERMISSION_Add(const char *, int, int, nsIIOService *)' nsPermissionManager.cpp:135: at this point in file Also nebiros SunOS/sparc 5.7 Depend nsImages.h", line 46: Error: nsIIOService is not defined. The second one looked like it could be a mid-checkin problem except that I checked in nsIIOservice before I checked in any of the cookie files.
Status: REOPENED → ASSIGNED
> To make matters worse, this patch caused crashes on the ports page Correction: I meant to say "redness", not "crashes".
Sorry it was to late for me to be present when this was checked in. The redness on the ports page was midcheckin. No problem there. That leaves the memory leak and the doGet_Service stuff. Anybody has a better idea where to put the cached IOService for nsCookies.cpp?
eek. I was the one who suggested the static nsCOMPtr usage. I should have known better; sorry. The way to avoid this would be to cache the ptr down in the component itself (something that lives and dies by the component manager), then pass that ptr down through the raw C functions (like we did w/ nsImage.cpp).
Yes, I was on that track too, but it is not that simple with nsCookies.cpp because of the way it is structured. I don't see it will work this way. Is there a way to remove (destroy) a nsCOMPtr manually from memory? We could do that easily in the place where we remove the cookielist.
Why can't you make the ptr an attribute of nsCookieService, and have all the COOKIE_* methods take the IOService as a param (at least those that need it)? Then the attr would automatically get cleaned up when nsCookieService's destructor was called.
With nsPermissions and nsImages it was easy because the functions in question were only called from nsImgManager and nsPermissionsManager. With nsCookies it is different. Most of its functions look like typical c-like public functions, can I even guarantee that there is an CookieService instance before the first of the functions in question is called?
those functions should not be called anywhere else, if they are, we have a bug. we should make all of those functions private I think, then you're sure to be fine.
Attachment #52576 - Attachment is obsolete: true
Really ugly having to pass the ioservice parameter to all those routines. But I guess there's no way to avoid it. cc'ing harishd because I know that he has a patch that also adds a parameter to many of these same routines and so he's going to get a merge conflict when he updates his patch. So at least now he has a heads-up.
Comment on attachment 54116 [details] [diff] [review] no longer using public nsCOMPtr ... r=morse
Attachment #54116 - Flags: review+
alecf, it's your turn again. Thanks.
Comment on attachment 54116 [details] [diff] [review] no longer using public nsCOMPtr ... seems ok to me.. I feel dumb for missing this before :) sr=alecf
Attachment #54116 - Flags: superreview+
Fix checked in. Hope it stays in this time.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago → 23 years ago
Resolution: --- → FIXED
v/fixed.
QA Contact: tever → benc
Summary: nsCookie.cpp is duplicating URL parsing logic. → nsCookie.cpp is duplicating URL parsing logic
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: