Closed Bug 28712 Opened 25 years ago Closed 24 years ago

MLK: nsSupportsHashtable has a problem

Categories

(Core :: XPCOM, defect, P1)

defect

Tracking

()

VERIFIED INVALID

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
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!
Scott ? I think this will more like beta2. Setting TFV to M16
Assignee: dp → scc
Target Milestone: M16
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.
Ok sounds fair. Will look. Why are we calling clone ? Are we calling clone for all hashtable. That would be suprising...
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 ?
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.
Status: NEW → ASSIGNED
Keywords: mlk
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.
Summary: MLK: nsSupportsHashtable/nsHashtable → MLK: somebody is using the wrong hash table?
Priority: P3 → P1
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
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
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.
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.
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.
Status: NEW → ASSIGNED
Target Milestone: M16 → M18
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?
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.
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.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → INVALID
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.
Marking Verified as Invalid.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.