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.
Created attachment 112281 [details] [diff] [review] Patch Changes the nsCookie* to a stack-based nsCookie which autodestructs. No need to use nsICookie either, since we're in the same library.
Marking blocker?, requesting r/sr=darin.
Comment on attachment 112281 [details] [diff] [review] Patch sr=darin
Comment on attachment 112281 [details] [diff] [review] Patch extra points for moving up that comment above the function call.
Attachment #112281 - Flags: review?(dougt) → review+
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.
Attachment #112281 - Attachment is obsolete: true
Created attachment 112441 [details] [diff] [review] use a nsCOMPtr Updated patch. Created this as discussed with dwitte.
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 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)
Attachment #112441 - Flags: superreview?(darin) → superreview+
Comment on attachment 112441 [details] [diff] [review] use a nsCOMPtr r=jag
Attachment #112441 - Flags: review?(timeless) → review+
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.
Attachment #112441 - Flags: approval1.3b?
Attachment #112441 - Flags: approval1.3b? → approval1.3b+
Flags: blocking1.3b? → blocking1.3b+
thanks to bz for checking in. Marking resolved/fixed, removing blocking1.3b flag.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.