190088 - Memory leak in pre-freeze checkin of cookie patch

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[[e-mail]]

Checkin of the patch for bug 179798 introduced a memory leak. See
http://lxr.mozilla.org/seamonkey/source/extensions/cookie/nsCookies.cpp#1529.

We create an nsCookie* ptr, but never delete it when we're finished. nsCookie
owns its strings, so it copies data. We only use the nsCookie object in the
function call to Permission_Check - after that, we don't need it anymore.

Comment 1

[[e-mail]]

Attachment: Patch

Changes the nsCookie* to a stack-based nsCookie which autodestructs. No need to
use nsICookie either, since we're in the same library.

Comment 2

[[e-mail]]

Marking blocker?, requesting r/sr=darin.

Comment 3

[Darin Fisher]

Comment on attachment 112281 [details] [diff] [review]
Patch

sr=darin

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 112281 [details] [diff] [review]
Patch

extra points for moving up that comment above the function call.

Comment 5

[[e-mail]]

Actually, this patch (and the current code) is wrong. mvl did some testing on
the current code, and found that some weird garbage collection was taking place,
several seconds after the functions involved have terminated.

New (correct) patch is upcoming. Marking old patch obsolete.

Comment 6

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: use a nsCOMPtr

Updated patch. Created this as discussed with dwitte.

Comment 7

[[e-mail]]

Comment on attachment 112441 [details] [diff] [review]
use a nsCOMPtr

This is the correct way to do it (thanks for the help and testing, timeless and
mvl).

According to timeless: "you shouldn't send an object w/ 0 refcount across xpcom
interfaces". It turns out that's actually not happening here, because we happen
to throw the nsCookie object into an nsMutableArray (which addrefs) before it
goes to JS, but philosophically, I think this nsCOMPtr patch is the right thing
to do.

At least, it eliminates confusion, since it makes it obvious the nsCookie
doesn't persist forever - it has a lifespan indicated by the nsCOMPtr refcount.
And that's what alerted me to the potential leak in the first place.

Requesting r/sr. Darin, if you SR this, could you request a=?

Comment 8

[Darin Fisher]

Comment on attachment 112441 [details] [diff] [review]
use a nsCOMPtr

sr=darin (too bad this is necessary given that you are only passing the object
to a function within the same library)

Comment 9

[jag (Peter Annema)]

Comment on attachment 112441 [details] [diff] [review]
use a nsCOMPtr

r=jag

Comment 10

[[e-mail]]

Comment on attachment 112441 [details] [diff] [review]
use a nsCOMPtr

Requesting approval1.3b

This is a low-risk 1-line patch that changes a pointer to use nsCOMPtr, to make
the code clearer and avoid potential xpcom problems with 0-refcount objects.

Comment 11

[[e-mail]]

thanks to bz for checking in.

Marking resolved/fixed, removing blocking1.3b flag.