Closed
Bug 28712
Opened 25 years ago
Closed 24 years ago
MLK: nsSupportsHashtable has a problem
Categories
(Core :: XPCOM, defect, P1)
Core
XPCOM
Tracking
()
VERIFIED
INVALID
M18
People
(Reporter: bruce, Assigned: rayw)
Details
(Keywords: memory-leak)
This appears somewhat less than optimal:
If the entry is new, then it gets cloned when it is put into the hashtable in
nsHashtable::Put().
At any rate, this is leaking.
MLK: 2808 bytes leaked in 18 blocks
This memory was allocated from:
malloc [rtlib.o]
__bUiLtIn_nEw [libxpcom.so]
__builtin_new [rtlib.o]
nsStringKey::Clone()const [nsHashtable.cpp:282]
nsHashKey* nsStringKey::Clone() const
{
=> return new nsStringKey(mStr);
}
const nsString& nsStringKey::GetString() const
nsHashtable::Put(nsHashKey*,void*) [nsHashtable.cpp:156]
he->value = aData;
} else {
PL_HashTableRawAdd(hashtable, hep, hash,
=> (void *) aKey->Clone(), aData);
}
if (mLock) PR_Unlock(mLock);
nsSupportsHashtable::Put(nsHashKey*,void*) [nsHashtable.cpp:379]
{
nsISupports* element = NS_REINTERPRET_CAST(nsISupports*,
aData);
NS_IF_ADDREF(element);
=> return nsHashtable::Put(aKey, aData);
}
void*
nsPresState::SetStateProperty(const nsString&,const nsString&)
[nsPresState.cpp:94]
char* val = aValue.ToNewCString();
supportsStr->SetData(val);
nsAllocator::Free(val);
=> mPropertyTable->Put(&key, supportsStr);
return NS_OK;
}
nsGfxCheckboxControlFrame::SaveState(nsIPresContext*,nsIPresState**)
[nsGfxCheckboxControlFrame.cpp:526]
CaptureFrameStateFor(nsIPresContext*,nsIFrame*,nsILayoutHistoryState*)
[nsFrameManager.cpp:1402]
FrameManager::CaptureFrameState(nsIPresContext*,nsIFrame*,nsILayoutHistoryState*)
[nsFrameManager.cpp:1420]
FrameManager::CaptureFrameState(nsIPresContext*,nsIFrame*,nsILayoutHistoryState*)
[nsFrameManager.cpp:1429]
FrameManager::CaptureFrameState(nsIPresContext*,nsIFrame*,nsILayoutHistoryState*)
[nsFrameManager.cpp:1429]
FrameManager::CaptureFrameState(nsIPresContext*,nsIFrame*,nsILayoutHistoryState*)
[nsFrameManager.cpp:1429]
FrameManager::CaptureFrameState(nsIPresContext*,nsIFrame*,nsILayoutHistoryState*)
[nsFrameManager.cpp:1429]
FrameManager::CaptureFrameState(nsIPresContext*,nsIFrame*,nsILayoutHistoryState*)
[nsFrameManager.cpp:1429]
FrameManager::CaptureFrameState(nsIPresContext*,nsIFrame*,nsILayoutHistoryState*)
[nsFrameManager.cpp:1429]
PresShell::GetHistoryState(nsILayoutHistoryState**)
[nsPresShell.cpp:2459]
nsGenericHTMLElement::GetPrimaryPresState(nsIHTMLContent*,nsIStatefulFrame::StateType,nsIPresState**)
[nsGenericHTMLElement.cpp:1972]
nsHTMLInputElement::SetPresStateChecked(nsIHTMLContent*,nsIStatefulFrame::StateType,int)
[nsHTMLInputElement.cpp:560]
nsHTMLInputElement::SetChecked(int) [nsHTMLInputElement.cpp:631]
SetHTMLInputElementProperty(JSContext*,JSObject*,long,long*)
[nsJSHTMLInputElement.cpp:573]
js_SetProperty [jsobj.c:2050]
js_Interpret [jsinterp.c:2259]
js_Invoke [jsinterp.c:681]
js_Interpret [jsinterp.c:2292]
js_Invoke [jsinterp.c:681]
js_Interpret [jsinterp.c:2292]
js_Invoke [jsinterp.c:681]
js_InternalInvoke [jsinterp.c:754]
JS_CallFunctionValue [jsapi.c:2787]
Block of 156 bytes (18 times); last block at 0x1fcf3f8
Reporter | ||
Comment 1•25 years ago
|
||
a bit more investigation shows that I can't find where the keys in the
nsHashtable get freed after they are cloned. Here's to hoping I'm an idiot!
Comment 2•25 years ago
|
||
Scott ? I think this will more like beta2. Setting TFV to M16
Assignee: dp → scc
Target Milestone: M16
Reporter | ||
Comment 3•25 years ago
|
||
I really could use this being fixed by beta1. I'm working on a lot of leaks and
this one contributes a lot of noise to some of my tests. If someone can confirm
that it appears that keys in these hashtables are indeed not getting released,
I'll be happy to look into fixing it myself this week or weekend.
Comment 4•25 years ago
|
||
Ok sounds fair. Will look. Why are we calling clone ? Are we calling clone for
all hashtable. That would be suprising...
Comment 5•25 years ago
|
||
Wow no wonder you sound confused. Here is what I found this far.
nsStringKey::nsHashKey
nsHashKey has non pure virtual destructor and constructor. Hashtable deletes
the keys using (nsHashKey *) So the default destructor for nsHashKey is getting
called instead of nsStringKey::~nsStringKey
Of course, all this is theory. Scott would smack this in a minute.
Scott ?
Reporter | ||
Comment 6•25 years ago
|
||
Aha! I see now. I didn't know about the hash table callbacks until just now.
Very cool. Maybe the whole hashtable is leaking in my example below? I see
from beard's stuff that we're leaking the mServices hashtable as well.
Updated•25 years ago
|
Status: NEW → ASSIGNED
Comment 7•25 years ago
|
||
The fact that |nsHashTable| has a |virtual| destructor means the right one will
get called, however, no destructor will be called at all unless you are using an
|nsObjectHashtable| and have provided appropriate callbacks to get the
destructors called. See, for example,
<http://lxr.mozilla.org/seamonkey/source/xpcom/components/nsCategoryManager.cpp#
93>.
Perhaps this is a case where someone should have used an |nsObjectHashtable| but
picked an |nsHashtable| instead? Where is the actual hashtable in question
declared? For the future, I would love to provide a template-based hashtable
implementation that didn't need callbacks because it knew the static type of its
elements. I could do this without code-bloat by basing it on a hashtable of
|void*|s.
Updated•25 years ago
|
Summary: MLK: nsSupportsHashtable/nsHashtable → MLK: somebody is using the wrong hash table?
Updated•25 years ago
|
Priority: P3 → P1
Reporter | ||
Comment 8•25 years ago
|
||
The hashtable in question is defined at:
http://lxr.mozilla.org/seamonkey/source/layout/base/src/nsPresState.cpp#33
Comment 9•25 years ago
|
||
Hmmm. Well, that's the right hash table to use in this situation. Guess I'll
have to look deeper.
Summary: MLK: somebody is using the wrong hash table? → MLK: nsSupportsHashtable has a problem
Comment 10•25 years ago
|
||
Let's get Ray started with something fun. If you can't make any progress with
this, Ray, feel free to assign it back to me. Assigning it to you as we
discussed in person.
Assignee: scc → rayw
Status: ASSIGNED → NEW
Assignee | ||
Comment 11•25 years ago
|
||
I spent a while chasing lots of possibilities and all the code I looked at looks
tight. I cannot make further progress on this bug without information on how to
reproduce it. Specifically, I need to know the minimal set of how to invoke the
product, what feature(s) to exercise, and what tool was used to measure the
leak.
Using our own made-up configuration, ideas of what to measure, and tools, I do
not see this particular leak occurring. There is code in Hashtable that, if
recursively exercised, could cause a problem like this to occur, but I need to
be able to make it occur repeatably to study the case and resolve it.
Assignee | ||
Comment 12•25 years ago
|
||
I sent an email a few days ago to the original reporter of the bug,
bruce@cubik.org, requesting information on how to duplicate the leak. Thus far,
I have received no response.
Reporter | ||
Comment 13•25 years ago
|
||
Sorry ... I moved 1300 miles and have changed jobs, so I've been a bit busy. I
_think_ this one had to do with form elements on web pages. Try submitting a
form and seeing what happens. I don't have Purify any longer and lack the time
(or a recent build) to do any tests.
Assignee | ||
Updated•24 years ago
|
Status: NEW → ASSIGNED
Target Milestone: M16 → M18
Comment 14•24 years ago
|
||
In response to Bruce's comment of 2000-02-23 13:43, I just stuck in
MOZ_COUNT_(C|D)TOR macros into nsHashKey and nsHashtable in my tree and ran the
bloat tests, and it showed 5 (of 398) nsHashtable and 23 (of 83794) nsHashKey
leaked. Are these known leaks? Could they include the one above? Should those
macros be checked in, since these objects currently aren't listed in the
bloat/leak logs?
Assignee | ||
Comment 15•24 years ago
|
||
The bug described here is a leak on string keys. What you show is a leak on
keys, apparently caused by a leak on hashtables. The proportions you report are
such that it seems obvious to me that the leak on keys is caused by the leak on
hashtables. Hashtables not being released is the problem of whomever used the
hashtables, and has nothing to say about the hashtable implementation leaking
keys.
Comment 16•24 years ago
|
||
David: Yes, please check those macros in. Also put a note on tinderbox (star)
saying that leaks are going up because we're counting these now. Thanks.
Assignee | ||
Updated•24 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → INVALID
Assignee | ||
Comment 17•24 years ago
|
||
It seems quite obvious that where nsHashtables have not been properly released,
the keys inside the hashtables will also be held.
Closing this bug. Anyone who wants to open bugs on cases where modules fail to
release nsHashtables can do so.
You need to log in
before you can comment on or make changes to this bug.
Description
•