142072 - nsThread frees nsILogModule, which it doesn't own

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Peter A Jonsson]

How to reproduce: Start mozilla and quit with ^q.

==5515== 10 bytes in 1 blocks are definitely lost in loss record 20 of 131
==5515==    at 0x400428DC: malloc (vg_clientfuncs.c:100)
==5515==    by 0x4095324A: __strdup (strdup.c:45)
==5515==    by 0x405FFF66: PR_NewLogModule (prlog.c:335)
==5515==    by 0x40518FC1: nsThread::nsThread(void) (nsThread.cpp:77)
==5515==    by 0x40519DBF: nsIThread::GetIThread(PRThread *, nsIThread **)
(nsThread.cpp:382)
==5515==    by 0x40519CE2: nsIThread::GetCurrent(nsIThread **) (nsThread.cpp:363)
==5515==    by 0x40519EA9: nsIThread::SetMainThread(void) (nsThread.cpp:404)
==5515==    by 0x404A8A64: NS_InitXPCOM2 (nsXPComInit.cpp:328)
==5515==    by 0x80614F0: main (nsAppRunner.cpp:1787)
==5515==    by 0x408F93BA: __libc_start_main (../sysdeps/generic/libc-start.c:129)

On line 102 of nsThread.cpp there is a PR_DELETE( nsIThreadLog );, shouldn't
this also free nsIThreadLog->name?
(http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/src/io/prlog.c#335)

Comment 1

[Doug Turner (:dougt)]

to neeti.

Comment 2

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

Peter, thanks for filing this.  Please remember to mark leak bugs with 'mlk' in
the keyword field.  Thanks.

Comment 3

[neeti]

Attachment: patch to fix leak

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 82928 [details] [diff] [review]
patch to fix leak

Why do you have free that?!    The problem probably is that the thread is not
being cleaned up prior to exit.  take a look at the nspr code.	This field is
freed during _PR_LogCleanup.

Comment 5

[Peter A Jonsson]

This patch does make the leak go away. 

Comment 6

[Peter A Jonsson]

Following the instructions on how to reproduce with a build from today gives no
leak for some reason. I will check again in a few days if it still is gone and
then close the bug if that is the case.

Comment 7

[Peter A Jonsson]

Ignore previous comment, managed to reproduce it by pasting
http://www.mozilla.org  and then clicking on "New checkins today".

Comment 8

[neeti]

Based on my email conversation with wtc, calling PR_DELETE( nsIThreadLog) is 
wrong. According to him we could just let the PRLOGModuleInfo structures leak( 
they are static objects) or call PR_Cleanup at the end of the main function. 

Comment 9

[Bradley Baetz (:bbaetz)]

No, the real problem is that we shouldn't be freeing _any_ of it - nspr frees it
on shutdown, assuming that we haven't freed the data structure for it...

patch coming

Comment 10

[Bradley Baetz (:bbaetz)]

Attachment: don't free someone else's data structure

Comment 11

[Bradley Baetz (:bbaetz)]

Note that this patch won't actually fix the leak, and will in fact make the leak
a few bytes bigger. nspr would do that if we called PR_Cleanup(), but we don't
(see bug 54189 and bug 54050 for why that is - basically, we'd have to unload
all our libraries first, and thats not gong to happen anytime soon)

I still think that my patch should go in, because freeing something which we're
not meant to is generally a bad idea. It will probably also cause a crash if we
ever do call PR_Cleanup, since nspr's list of log moduiles will have a pointer
to the freed memory in it.

(Interestingly, this patch does cause valgrind to no longer complain about the
leaked string. I have no idea why that is, though - printfs show that the log
cleanup stuff isn't being done)

Comment 12

[Wan-Teh Chang]

Comment on attachment 96536 [details] [diff] [review]
don't free someone else's data structure

r=wtc.

Although memory leak is bad, it is worse to
free someone else's memory.

Perhaps the best fix is to add a new
PR_DestroyLogModule function.  I will look
into that.  Once created, all the log
modules are owned by NSPR (NSPR maintains
a linked list of them).  Unless there is a
PR_DestroyLogModule function, NSPR clients
should not free a log module.

It would be a good idea to replace the code
you deleted by a comment that says the
nsIThreadLog log module could be freed at
that point if the appropriate NSPR function
existed.

Comment 13

[Brian Ryner (not reading)]

Comment on attachment 96536 [details] [diff] [review]
don't free someone else's data structure

sr=bryner

Comment 14

[Bradley Baetz (:bbaetz)]

Checked in with a comment pointing to this bug. Making the summary reflect the
fact that this patch doesn't fix the leak.

Marking as FIXED; we have other bugs to track the fact that mozilla doesn't call
PR_Cleanup().