The default bug view has changed. See this FAQ.

mem leak in nsPersistentProperties::Enumerate

RESOLVED FIXED

Status

()

Core
XPCOM
--
critical
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Christian Persch (GNOME) (away; not receiving bug mail), Assigned: dougt)

Tracking

({fixed1.8.0.7, fixed1.8.1, mlk})

Trunk
x86
Linux
fixed1.8.0.7, fixed1.8.1, mlk
Points:
---
Bug Flags:
blocking1.8.1 +
blocking1.8.0.7 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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)
Created attachment 193161 [details] [diff] [review]
proposed fix

Updated

12 years ago
Severity: normal → critical

Comment 2

12 years ago
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

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

Updated

12 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 193161 [details] [diff] [review]
proposed fix

Requesting r/sr ...
Attachment #193161 - Flags: superreview?(alecf)
Attachment #193161 - Flags: review?(dougt)
(Assignee)

Comment 5

12 years ago
Comment on attachment 193161 [details] [diff] [review]
proposed fix

Why do we need to NS_ADDREF before the InsertElementAt?
(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.
(Assignee)

Comment 7

12 years ago
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.
Attachment #193161 - Flags: superreview?(alecf)
Attachment #193161 - Flags: review?(dougt)
Attachment #193161 - Flags: review-
Created attachment 196665 [details] [diff] [review]
updated patch

I removed the NS_ADDREF/RELEASE around the InsertElementAt call.
Attachment #193161 - Attachment is obsolete: true
Attachment #196665 - Flags: superreview?(alecf)
Attachment #196665 - Flags: review?(dougt)
(Assignee)

Updated

12 years ago
Attachment #196665 - Flags: superreview?(benjamin)
Attachment #196665 - Flags: superreview?(alecf)
Attachment #196665 - Flags: review?(dougt)
Attachment #196665 - Flags: review+
Attachment #196665 - Flags: superreview?(benjamin) → superreview+
(Assignee)

Comment 9

12 years ago
Checking in nsPersistentProperties.cpp;
/cvsroot/mozilla/xpcom/ds/nsPersistentProperties.cpp,v  <--  nsPersistentProperties.cpp
new revision: 1.46; previous revision: 1.45
done

thanks christian.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Blocks: 320915
Flags: blocking1.8.1?
Flags: blocking1.8.0.1?
Attachment #196665 - Flags: approval1.8.1?
Attachment #196665 - Flags: approval1.8.0.1?
How big a leak is this? Is the win big enough to be worth taking the risk on the 1.8.0.1 branch?
(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.
Attachment #196665 - Flags: approval1.8.1?
Attachment #196665 - Flags: approval1.8.0.1?
That's probably not enough of a win to take the qa hit for 1.8.0.1
Flags: blocking1.8.1?
Flags: blocking1.8.0.1?

Comment 13

11 years ago
Can we get this on the 1.8.1 branch?
Flags: blocking1.8.1?
(Assignee)

Comment 14

11 years ago
Comment on attachment 196665 [details] [diff] [review]
updated patch

this is safe and has baked on the trunk for some time.
Attachment #196665 - Flags: approval1.8.1?
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.
Flags: blocking1.8.0.6?
Attachment #196665 - Flags: approval1.8.0.6?

Updated

11 years ago
Flags: blocking1.8.1? → blocking1.8.1+

Updated

11 years ago
Attachment #196665 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Comment 16

11 years ago
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
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.1
Flags: blocking1.8.0.7? → blocking1.8.0.7+
Comment on attachment 196665 [details] [diff] [review]
updated patch

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #196665 - Flags: approval1.8.0.7? → approval1.8.0.7+
(Assignee)

Comment 18

11 years ago
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
Keywords: fixed1.8.0.7
You need to log in before you can comment on or make changes to this bug.