Closed
Bug 66867
Opened 24 years ago
Closed 23 years ago
nsMsgNewsFolder::SetCachedNewsrcLine leaks
Categories
(MailNews Core :: Networking: NNTP, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla0.9.2
People
(Reporter: dbaron, Assigned: hwaara)
References
Details
(Keywords: memory-leak)
Attachments
(6 files)
628 bytes,
patch
|
Details | Diff | Splinter Review | |
631 bytes,
patch
|
Details | Diff | Splinter Review | |
577 bytes,
patch
|
Details | Diff | Splinter Review | |
574 bytes,
patch
|
Details | Diff | Splinter Review | |
555 bytes,
patch
|
Details | Diff | Splinter Review | |
807 bytes,
patch
|
Details | Diff | Splinter Review |
When starting mozilla, starting mailnews, reading a news message, and exiting, the Boehm GC showed the call to nsCRT::strdup as being leaked: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/news/src/nsNewsFolder.cpp&rev=1.156&mark=1589#1579 I assume the leak was because nsMsgNewsFolder::SetCachedNewsrcLine was called multiple times. I think you should do one of: * fail silently if it's called multiple times, without a strdup * assert if it's called multiple times, and then figure out why * free the old value if it's non-null
Comment 1•24 years ago
|
||
Comment 2•24 years ago
|
||
Following dbaron's third step, this should fix the leak. It might not be the correct way to fix it, but it definatly gets rid of the leak. When I load mailnews, 5 strings are leaked.
Comment 3•24 years ago
|
||
Comment 4•24 years ago
|
||
Ignore the first patch. Can someone take a look at this?
Reporter | ||
Comment 5•24 years ago
|
||
For memory allocated with nsCRT::strdup, you should free with nsCRT::free. I'd also prefer the style (although the module owner of this code may prefer braces): if (mCcahedNewsrcLine) nsCRT::free(mCachedNewsrcLine) (There's no need to set to null because you're assigning again right below, and the "nsnull !=" is unnecessary.)
Comment 6•24 years ago
|
||
Comment 7•24 years ago
|
||
Thank you David for your comments. The above patch addresses your comments. In other parts of the file, delete [] is used instead of nsCRT::free, so maybe I should just leave it at that to keep it consistent. I'll attach one using delete[] so the module owner could choose which one they prefer.
Comment 8•24 years ago
|
||
Comment 9•24 years ago
|
||
So, choose between patch #24319 and #24320.
Reporter | ||
Comment 10•24 years ago
|
||
Comments on the existing uses of delete[] in that file: UpdateSummaryFromNNTPInfo - incorrect. Memory was allocated by nsMemory::Alloc and should thus be freed with nsMemory::Free. This should probably be documented in nsMsgKeySet.h HandleNewsrcLine - This is correct because nsMsgGroupRecord::GetFullName allocates using new[].
Assignee | ||
Comment 11•23 years ago
|
||
I guess this patch needs r= and sr=
Assignee | ||
Comment 12•23 years ago
|
||
actually, r=hwaara on latest patch. if HandleNewsrcLine was allocated with new [] it should be deallocated with delete [].
QA Contact: esther → stephend
Assignee | ||
Comment 13•23 years ago
|
||
bienvenu, can you sr= this patch? I can update it to the trunk and check it in if needed.
Assignee | ||
Comment 14•23 years ago
|
||
*** Bug 82524 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 15•23 years ago
|
||
I find it ridicolous that this patch after merely 4 months still hasn't got sr=. It's just a one-liner...
Assignee | ||
Comment 16•23 years ago
|
||
Taking to drive the patch in. No one seems to care. Now, to hunt down someone for sr=.
Assignee: sspitzer → hwaara
Target Milestone: --- → mozilla0.9.2
Comment 17•23 years ago
|
||
give me a chance after 0.9.1 and I'll review it.
Assignee | ||
Comment 18•23 years ago
|
||
Seth, can you please sr= this little fix?
Comment 19•23 years ago
|
||
mCachedNewsrcLine is always allocated with nsCRT::strdup(), so you should use nsCRT::free to free it (although the dtor uses PR_FREEIF()). Even with your patch, the if (mCachedNewsrcLine) is redundant; |delete| and | delete[]| are guaranteed safe with null pointers.
Assignee | ||
Comment 20•23 years ago
|
||
Comment 21•23 years ago
|
||
sr=blizzard
Comment 22•23 years ago
|
||
a= asa@mozilla.org for checkin to the trunk. (on behalf of drivers)
Blocks: 83989
Assignee | ||
Comment 23•23 years ago
|
||
fix checked in yesterday.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 24•23 years ago
|
||
Comment 25•23 years ago
|
||
supplimental patch. thanks to sfraser for pointing out the dtor problem.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 26•23 years ago
|
||
Seth, can you explain the patch please.
Comment 27•23 years ago
|
||
1) the CRTFREEFIF will prevent a crash if mCachedNewsrcLine is null. 2) as sfmr wrote: "mCachedNewsrcLine is always allocated with nsCRT::strdup(), so you should use nsCRT::free to free it (although the dtor uses PR_FREEIF())." smfr pointed out that we had a mis matched memory free in the dtor, and this patch fixes that.
Assignee | ||
Comment 28•23 years ago
|
||
Sounds like the right thing to do. r=hwaara
Comment 29•23 years ago
|
||
this has rs=mscott.
Reporter | ||
Comment 30•23 years ago
|
||
a=dbaron for trunk checkin (on behalf of drivers)
Comment 31•23 years ago
|
||
fixed again.
Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
sending leak/opt bugs over to my other bugzilla account
QA Contact: stephend → stephen.donner
Verified FIXED, this didn't show up in my Purify logs for news lately.
Status: RESOLVED → VERIFIED
Updated•20 years ago
|
Product: MailNews → Core
Updated•16 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•