Closed
Bug 305208
Opened 19 years ago
Closed 19 years ago
mem leak in nsPersistentProperties::Enumerate
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: chpe, Assigned: dougt)
References
Details
(Keywords: fixed1.8.0.7, fixed1.8.1, memory-leak)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
dougt
:
review+
benjamin
:
superreview+
dveditz
:
approval1.8.0.7+
mtschrep
:
approval1.8.1+
|
Details | Diff | Splinter Review |
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)
Reporter | ||
Comment 1•19 years ago
|
||
Updated•19 years ago
|
Severity: normal → critical
Comment 2•19 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•19 years ago
|
||
OK I see this can't be done *here*. Apologies for bugspam.
Updated•19 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter | ||
Comment 4•19 years ago
|
||
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•19 years ago
|
||
Comment on attachment 193161 [details] [diff] [review]
proposed fix
Why do we need to NS_ADDREF before the InsertElementAt?
Reporter | ||
Comment 6•19 years ago
|
||
(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•19 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-
Reporter | ||
Comment 8•19 years ago
|
||
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•19 years ago
|
Attachment #196665 -
Flags: superreview?(benjamin)
Attachment #196665 -
Flags: superreview?(alecf)
Attachment #196665 -
Flags: review?(dougt)
Attachment #196665 -
Flags: review+
Updated•19 years ago
|
Attachment #196665 -
Flags: superreview?(benjamin) → superreview+
Assignee | ||
Comment 9•19 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
Closed: 19 years ago
Resolution: --- → FIXED
Updated•19 years ago
|
Updated•19 years ago
|
Attachment #196665 -
Flags: approval1.8.1?
Attachment #196665 -
Flags: approval1.8.0.1?
Comment 10•19 years ago
|
||
How big a leak is this? Is the win big enough to be worth taking the risk on the 1.8.0.1 branch?
Reporter | ||
Comment 11•19 years ago
|
||
(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.
Updated•19 years ago
|
Attachment #196665 -
Flags: approval1.8.1?
Attachment #196665 -
Flags: approval1.8.0.1?
Comment 12•19 years ago
|
||
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?
Assignee | ||
Comment 14•19 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?
Comment 15•19 years ago
|
||
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?
Updated•19 years ago
|
Attachment #196665 -
Flags: approval1.8.0.6?
Updated•19 years ago
|
Flags: blocking1.8.1? → blocking1.8.1+
Updated•19 years ago
|
Attachment #196665 -
Flags: approval1.8.1? → approval1.8.1+
Assignee | ||
Comment 16•19 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•19 years ago
|
Keywords: fixed1.8.1
Updated•18 years ago
|
Flags: blocking1.8.0.7? → blocking1.8.0.7+
Comment 17•18 years ago
|
||
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•18 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.
Description
•