Memory leak in pre-freeze checkin of cookie patch

RESOLVED FIXED in mozilla1.3beta

Status

()

Core
Networking: Cookies
--
critical
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: dwitte@gmail.com, Assigned: Michiel van Leeuwen (email: mvl+moz@))

Tracking

Trunk
mozilla1.3beta
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

699 bytes, patch
jag (Peter Annema)
: review+
Darin Fisher
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

16 years ago
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.
(Reporter)

Comment 1

16 years ago
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.
(Reporter)

Comment 2

16 years ago
Marking blocker?, requesting r/sr=darin.
Flags: blocking1.3b?
(Reporter)

Updated

16 years ago
Attachment #112281 - Flags: superreview?(darin)
Attachment #112281 - Flags: review?(darin)
(Reporter)

Updated

16 years ago
Target Milestone: --- → mozilla1.3beta
(Reporter)

Updated

16 years ago
Severity: blocker → critical

Comment 3

16 years ago
Comment on attachment 112281 [details] [diff] [review]
Patch

sr=darin
Attachment #112281 - Flags: superreview?(darin)
Attachment #112281 - Flags: superreview+
Attachment #112281 - Flags: review?(dougt)
Attachment #112281 - Flags: review?(darin)

Comment 4

16 years ago
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+
(Reporter)

Comment 5

16 years ago
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.
(Reporter)

Updated

16 years ago
Attachment #112281 - Attachment is obsolete: true
(Assignee)

Comment 6

16 years ago
Created attachment 112441 [details] [diff] [review]
use a nsCOMPtr

Updated patch. Created this as discussed with dwitte.
(Reporter)

Comment 7

16 years ago
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=?
Attachment #112441 - Flags: superreview?(darin)
Attachment #112441 - Flags: review?(timeless)

Comment 8

16 years ago
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 9

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

r=jag
Attachment #112441 - Flags: review?(timeless) → review+
(Reporter)

Comment 10

16 years ago
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+
(Reporter)

Comment 11

16 years ago
thanks to bz for checking in.

Marking resolved/fixed, removing blocking1.3b flag.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Flags: blocking1.3b+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.