Last Comment Bug 305208 - mem leak in nsPersistentProperties::Enumerate
: mem leak in nsPersistentProperties::Enumerate
Status: RESOLVED FIXED
: fixed1.8.0.7, fixed1.8.1, mlk
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: x86 Linux
: -- critical (vote)
: ---
Assigned To: Doug Turner (:dougt)
:
: Nathan Froyd [:froydnj]
Mentors:
Depends on:
Blocks: mlk1.8
  Show dependency treegraph
 
Reported: 2005-08-19 08:17 PDT by Christian Persch (GNOME) (away; not receiving bug mail)
Modified: 2006-08-15 09:20 PDT (History)
4 users (show)
mtschrep: blocking1.8.1+
dveditz: blocking1.8.0.7+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed fix (2.08 KB, patch)
2005-08-19 08:17 PDT, Christian Persch (GNOME) (away; not receiving bug mail)
doug.turner: review-
Details | Diff | Splinter Review
updated patch (2.09 KB, patch)
2005-09-19 09:55 PDT, Christian Persch (GNOME) (away; not receiving bug mail)
doug.turner: review+
benjamin: superreview+
dveditz: approval1.8.0.7+
mtschrep: approval1.8.1+
Details | Diff | Splinter Review

Description Christian Persch (GNOME) (away; not receiving bug mail) 2005-08-19 08:17:12 PDT
nsPersistentProperties::Enumerate leaks the nsISupportsArray it creates, and
also the individual entries in that array:

==10797== 5943 (52 direct, 5891 indirect) bytes in 1 blocks are definitely lost
in loss record 307 of 472
==10797==    at 0x1B907710: operator new(unsigned) (vg_replace_malloc.c:132)
==10797==    by 0x1CAC1499: nsSupportsArray::Create(nsISupports*, nsID const&,
void**) (in /opt/firefox-1.8/lib/firefox-1.0+/libxpcom_core.so)
==10797==    by 0x1CAC1554: NS_NewISupportsArray(nsISupportsArray**) (in
/opt/firefox-1.8/lib/firefox-1.0+/libxpcom_core.so)
==10797==    by 0x1CABD6B7:
nsPersistentProperties::Enumerate(nsISimpleEnumerator**) (in
/opt/firefox-1.8/lib/firefox-1.0+/libxpcom_core.so)
==10797==    by 0x1E166B03: mozilla_decoders_init (in
/opt/firefox-1.8/lib/firefox-1.0+/components/libgfx_gtk.so)

==10797== 2160 bytes in 54 blocks are indirectly lost in loss record 280 of 472
==10797==    at 0x1B907710: operator new(unsigned) (vg_replace_malloc.c:132)
==10797==    by 0x1CABDFEF: AddElemToArray(PLDHashTable*, PLDHashEntryHdr*,
unsigned, void*) (in /opt/firefox-1.8/lib/firefox-1.0+/libxpcom_core.so)
==10797==    by 0x1CAB4E47: PL_DHashTableEnumerate (in
/opt/firefox-1.8/lib/firefox-1.0+/libxpcom_core.so)
==10797==    by 0x1CABD6D5:
nsPersistentProperties::Enumerate(nsISimpleEnumerator**) (in
/opt/firefox-1.8/lib/firefox-1.0+/libxpcom_core.so)
==10797==    by 0x1E166B03: mozilla_decoders_init (in
/opt/firefox-1.8/lib/firefox-1.0+/components/libgfx_gtk.so)
Comment 1 Christian Persch (GNOME) (away; not receiving bug mail) 2005-08-19 08:17:53 PDT
Created attachment 193161 [details] [diff] [review]
proposed fix
Comment 2 Radek 'sysKin' Czyz 2005-08-19 22:01:59 PDT
Stupid question: can't you put this element on stack rather than
allocating/deallocationg it on heap?

I'm mostly asking because I saw some other places where this could be done, and
it would be so much faster (no system calls, no heap fragmentation...)

I do realize I might be making fool of myself ;)
Comment 3 Radek 'sysKin' Czyz 2005-08-19 22:03:36 PDT
OK I see this can't be done *here*. Apologies for bugspam.
Comment 4 Christian Persch (GNOME) (away; not receiving bug mail) 2005-09-06 05:40:03 PDT
Comment on attachment 193161 [details] [diff] [review]
proposed fix

Requesting r/sr ...
Comment 5 Doug Turner (:dougt) 2005-09-06 08:04:08 PDT
Comment on attachment 193161 [details] [diff] [review]
proposed fix

Why do we need to NS_ADDREF before the InsertElementAt?
Comment 6 Christian Persch (GNOME) (away; not receiving bug mail) 2005-09-06 09:46:48 PDT
(In reply to comment #5)
> (From update of attachment 193161 [details] [diff] [review] [edit])
> Why do we need to NS_ADDREF before the InsertElementAt?
> 

I don't know. CVS history tells me the NS_ADDREF before InsertElementAt has been
there since the initial rev 1.1.
I've checked that nsSupportsArray::InsertElementAt does addref the element [see
http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsSupportsArray.cpp#390], so I
think I can remove the addref/release pair. I've done that in my tree; I'll wait
for further comments from reviewers before attaching an updated patch.
Comment 7 Doug Turner (:dougt) 2005-09-19 09:25:54 PDT
Comment on attachment 193161 [details] [diff] [review]
proposed fix

let get a new patch together.  I think that is the only problem I had with it
-- the add_ref+release around the insertElementAt.
Comment 8 Christian Persch (GNOME) (away; not receiving bug mail) 2005-09-19 09:55:22 PDT
Created attachment 196665 [details] [diff] [review]
updated patch

I removed the NS_ADDREF/RELEASE around the InsertElementAt call.
Comment 9 Doug Turner (:dougt) 2005-11-16 13:28:12 PST
Checking in nsPersistentProperties.cpp;
/cvsroot/mozilla/xpcom/ds/nsPersistentProperties.cpp,v  <--  nsPersistentProperties.cpp
new revision: 1.46; previous revision: 1.45
done

thanks christian.
Comment 10 Daniel Veditz [:dveditz] 2006-01-04 11:53:44 PST
How big a leak is this? Is the win big enough to be worth taking the risk on the 1.8.0.1 branch?
Comment 11 Christian Persch (GNOME) (away; not receiving bug mail) 2006-01-04 12:11:24 PST
(In reply to comment #10)
> How big a leak is this?

Taking the numbers from comment 0 which were from running under valgrind on my computer:

> ==10797== 5943 (52 direct, 5891 indirect) bytes in 1 blocks are definitely lost
> in loss record 307 of 472
> ==10797== 2160 bytes in 54 blocks are indirectly lost in loss record 280 of 472

= about 8k.
Comment 12 Daniel Veditz [:dveditz] 2006-01-04 21:51:28 PST
That's probably not enough of a win to take the qa hit for 1.8.0.1
Comment 13 Mike Schroepfer 2006-06-26 11:45:47 PDT
Can we get this on the 1.8.1 branch?
Comment 14 Doug Turner (:dougt) 2006-07-07 10:52:54 PDT
Comment on attachment 196665 [details] [diff] [review]
updated patch

this is safe and has baked on the trunk for some time.
Comment 15 Daniel Veditz [:dveditz] 2006-07-07 10:55:04 PDT
Doug says it's 8K per enum, which would be a whole different ball of wax.

Comment 12 was not meant to discourage any 1.8.0.x check-in, just that it was late in 1.8.0.1, too late for a mere "nice to have" fix.
Comment 16 Doug Turner (:dougt) 2006-07-13 11:04:01 PDT
Fixed checked into the MOZILLA_1_8_BRANCH today:

Checking in nsPersistentProperties.cpp;
/cvsroot/mozilla/xpcom/ds/nsPersistentProperties.cpp,v  <--  nsPersistentProperties.cpp
new revision: 1.45.28.1; previous revision: 1.45
done
Comment 17 Daniel Veditz [:dveditz] 2006-08-09 15:06:39 PDT
Comment on attachment 196665 [details] [diff] [review]
updated patch

approved for 1.8.0 branch, a=dveditz for drivers
Comment 18 Doug Turner (:dougt) 2006-08-15 09:20:15 PDT
Checking in xpcom/ds/nsPersistentProperties.cpp;
/cvsroot/mozilla/xpcom/ds/nsPersistentProperties.cpp,v  <--  nsPersistentProperties.cpp
new revision: 1.45.36.1; previous revision: 1.45
done

Note You need to log in before you can comment on or make changes to this bug.