66867 - nsMsgNewsFolder::SetCachedNewsrcLine leaks

Status: VERIFIED FIXED

(MailNews Core :: Networking: NNTP, defect)

Description

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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

[Kevin Higgins]

Attachment: patch to fix the leak

Comment 2

[Kevin Higgins]

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

[Kevin Higgins]

Attachment: real patch this time.

Comment 4

[Kevin Higgins]

Ignore the first patch.  Can someone take a look at this?

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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

[Kevin Higgins]

Attachment: new patch

Comment 7

[Kevin Higgins]

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

[Kevin Higgins]

Attachment: patch using delete [] to keep it consistent

Comment 9

[Kevin Higgins]

So, choose between patch #24319 and #24320.

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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[].

Comment 11

[Håkan Waara]

I guess this patch needs r= and sr=

Comment 12

[Håkan Waara]

actually, r=hwaara on latest patch. if HandleNewsrcLine was allocated with new
[] it should be deallocated with delete [].

Comment 13

[Håkan Waara]

bienvenu, can you sr= this patch?  I can update it to the trunk and check it in
if needed.

Comment 14

[Håkan Waara]

*** Bug 82524 has been marked as a duplicate of this bug. ***

Comment 15

[Håkan Waara]

I find it ridicolous that this patch after merely 4 months still hasn't got sr=.
It's just a one-liner...

Comment 16

[Håkan Waara]

Taking to drive the patch in. No one seems to care.

Now, to hunt down someone for sr=.

Comment 17

[(not reading, please use [e-mail] instead)]

give me a chance after 0.9.1 and I'll review it.

Comment 18

[Håkan Waara]

Seth, can you please sr= this little fix?

Comment 19

[Simon Fraser [no longer active]]

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.

Comment 20

[Håkan Waara]

Attachment: new fix

Comment 21

[Christopher Blizzard (:blizzard)]

sr=blizzard

Comment 22

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the trunk.
(on behalf of drivers)

Comment 23

[Håkan Waara]

fix checked in yesterday.

Comment 24

[(not reading, please use [e-mail] instead)]

Attachment: supplimental patch to fix dtor and potential crasher.

Comment 25

[(not reading, please use [e-mail] instead)]

supplimental patch.

thanks to sfraser for pointing out the dtor problem.

Comment 26

[Håkan Waara]

Seth, can you explain the patch please.

Comment 27

[(not reading, please use [e-mail] instead)]

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.

Comment 28

[Håkan Waara]

Sounds like the right thing to do. r=hwaara

Comment 29

[(not reading, please use [e-mail] instead)]

this has rs=mscott.

Comment 30

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

a=dbaron for trunk checkin (on behalf of drivers)

Comment 31

[(not reading, please use [e-mail] instead)]

fixed again.

Comment 32

[[e-mail] (gone - use [e-mail] instead)]

sending leak/opt bugs over to my other bugzilla account

Comment 33

[[e-mail] (gone - use [e-mail] instead)]

Verified FIXED, this didn't show up in my Purify logs for news lately.