Closed Bug 47437 Opened 24 years ago Closed 23 years ago

nsCookie.cpp is duplicating URL parsing logic

Categories

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

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla0.9.6

People

(Reporter: jud, Assigned: morse)

References

Details

Attachments

(1 file, 14 obsolete files)

28.70 KB, patch
morse
: review+
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: 23 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: 23 years ago23 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: 23 years ago23 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: