305208 - mem leak in nsPersistentProperties::Enumerate

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Christian Persch (GNOME) (away; not receiving bug mail)]

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)]

Attachment: proposed fix

Comment 2

[Radek 'sysKin' Czyz]

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]

OK I see this can't be done *here*. Apologies for bugspam.

Comment 4

[Christian Persch (GNOME) (away; not receiving bug mail)]

Comment on attachment 193161 [details] [diff] [review]
proposed fix

Requesting r/sr ...

Comment 5

[Doug Turner (:dougt)]

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)]

(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)]

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)]

Attachment: updated patch

I removed the NS_ADDREF/RELEASE around the InsertElementAt call.

Comment 9

[Doug Turner (:dougt)]

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

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)]

(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]]

That's probably not enough of a win to take the qa hit for 1.8.0.1

Comment 13

[Mike Schroepfer]

Can we get this on the 1.8.1 branch?

Comment 14

[Doug Turner (:dougt)]

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

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)]

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

Comment on attachment 196665 [details] [diff] [review]
updated patch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 18

[Doug Turner (:dougt)]

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