nsMsgNewsFolder::SetCachedNewsrcLine leaks

VERIFIED FIXED in mozilla0.9.2

Status

VERIFIED FIXED
18 years ago
10 years ago

People

(Reporter: dbaron, Assigned: hwaara)

Tracking

({memory-leak})

Trunk
mozilla0.9.2
x86
All
memory-leak

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

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

18 years ago
Created attachment 24297 [details] [diff] [review]
patch to fix the leak

Comment 2

18 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

18 years ago
Created attachment 24300 [details] [diff] [review]
real patch this time.

Comment 4

18 years ago
Ignore the first patch.  Can someone take a look at this?
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 7

18 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

18 years ago
Created attachment 24320 [details] [diff] [review]
patch using delete [] to keep it consistent

Comment 9

18 years ago
So, choose between patch #24319 and #24320.
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

18 years ago
I guess this patch needs r= and sr=
Keywords: patch, review
OS: Linux → All
(Assignee)

Comment 12

18 years ago
actually, r=hwaara on latest patch. if HandleNewsrcLine was allocated with new
[] it should be deallocated with delete [].
Keywords: review → approval
QA Contact: esther → stephend
(Assignee)

Comment 13

18 years ago
bienvenu, can you sr= this patch?  I can update it to the trunk and check it in
if needed.
(Assignee)

Comment 14

18 years ago
*** Bug 82524 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 15

18 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

18 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
give me a chance after 0.9.1 and I'll review it.
(Assignee)

Comment 18

18 years ago
Seth, can you please sr= this little fix?

Comment 19

18 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

18 years ago
Created attachment 38154 [details] [diff] [review]
new fix
sr=blizzard
a= asa@mozilla.org for checkin to the trunk.
(on behalf of drivers)
Blocks: 83989
(Assignee)

Comment 23

18 years ago
fix checked in yesterday.
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
Created attachment 38618 [details] [diff] [review]
supplimental patch to fix dtor and potential crasher.
supplimental patch.

thanks to sfraser for pointing out the dtor problem.



Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 26

18 years ago
Seth, can you explain the patch please.
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

18 years ago
Sounds like the right thing to do. r=hwaara
fixed again.
Status: REOPENED → RESOLVED
Last Resolved: 18 years ago18 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
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.