206528 - nsBaseHashtable should init its mLock variable

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Peter Van der Beken [:peterv]]

nsBaseHashtable doesn't init its mLock variable to null, so it accesses random
memory if the hashtable gets destroyed before it's been init'ed (destructor
tries to destroy mLock).

Comment 1

[Benjamin Smedberg]

Attachment: initialize mLock

Comment 2

[Alec Flett]

Comment on attachment 123874 [details] [diff] [review]
initialize mLock

doh! :) sr=alecf

Comment 3

[[e-mail] (gone - use [e-mail] instead)]

Idle thought - Alec, could this have been the cause of my weird 'we seem to be
leaking whole hashtables' leak bugs?  

Comment 4

[Benjamin Smedberg]

stephend: no, that's impossible, as nsBaseHashtable is currently not used
anywhere in the tree ;) Where were you leaking hashtables?

Comment 5

[[e-mail] (gone - use [e-mail] instead)]

Hmm...I'm having problems finding bug instances now, but in the past I've been
burned by filing memory leak bugs (all bogus, I've been told) that were in the
1000+ byte ranges and had hashtable code paths in the 2nd or 3rd in the call chain.

Comment 6

[[e-mail]]

stephend sent me one such leak report a while back. i investigated and decided
it was bogus.

alecf: any idea why purify would report this as a leak?

<quote>

Just loading http://home.netscape.com on a trunk win32 under Purify yielded:

   [W] MLK: Memory leak of 3072 bytes from 1 block allocated in PL_DHashAllocTable
       Distribution of leaked blocks
       Allocation location
       malloc         [dbgheap.c:129]
       PL_DHashAllocTable [pldhash.c:63]
       PR_IMPLEMENT(void *)
       PL_DHashAllocTable(PLDHashTable *table, PRUint32 nbytes)
       {
    =>     return malloc(nbytes);
       }
             PR_IMPLEMENT(void)
       PL_DHashTableInit [pldhash.c:202]
           table->generation = 0;
           nbytes = capacity * entrySize;
          =>     table->entryStore = ops->allocTable(table, nbytes);
           if (!table->entryStore)
               return PR_FALSE;
           memset(table->entryStore, 0, nbytes);
       nsHashtable::nsHashtable(UINT,int) [nsHashtable.cpp:148]
           MOZ_COUNT_CTOR(nsHashtable);
                 PRBool result = PL_DHashTableInit(&mHashtable, &hashtableOps,
nsnull,
    =>                                       sizeof(HTEntry), aInitSize);
                 NS_ASSERTION(result, "Hashtable failed to initialize");
         nsProxyObjectManager::nsProxyObjectManager(void)
[nsProxyObjectManager.cpp:118]
             nsProxyObjectManager::nsProxyObjectManager()
       {
    =>     mProxyClassMap = new nsHashtable(256, PR_TRUE);
           mProxyObjectMap = new nsHashtable(256, PR_TRUE);
                 mProxyCreationMonitor = PR_NewMonitor();
   nsProxyObjectManager::GetInstance(void) [nsProxyObjectManager.cpp:161]
       {
           if (! mInstance)
           {
    =>         mInstance = new nsProxyObjectManager();
           }
           return mInstance;
       }
   nsProxyEventObject::Release(void) [nsProxyEventObject.cpp:480]
           // This is to protect against shutdown issues where a proxy object could
           // be destroyed after (or while) the Proxy Manager is being destroyed...
           //
    =>     nsProxyObjectManager* manager = nsProxyObjectManager::GetInstance();
           nsAutoMonitor mon(manager ? manager->GetMonitor() : nsnull);
                 nsrefcnt count;
   nsCOMPtr<nsINetNotify>::~nsCOMPtr<nsINetNotify>(void) [nsCOMPtr.h:497]
   nsNetModRegEntry::~nsNetModRegEntry(void) [nsNetModRegEntry.cpp:214]
       {
           delete [] mTopic;
           nsAutoMonitor::DestroyMonitor(mMonitor);

Comment 7

[[e-mail]]

er, s/alecf/bsmedberg/ ;)

Comment 8

[Alec Flett]

that looks like a legit leak to me, but more of an OWNER leaking a hashtable
rather than the hashtable itself leaking memory (i.e. looks like we're leaking a
nsProxyObjectManager here, or that nsProxyObjectManager is leaking hashtables
itself)

Comment 9

[[e-mail]]

hmm, i recall putting printf's in the ctor method of nsProxyObjectManager.cpp
(where the hash is alloced) and in the dtor where it's deleted, and confirmed
that it was being cleaned up properly...

so unless nsHashtable doesn't clean itself up properly, i can't see a problem :/

Comment 10

[Alec Flett]

we should probably take this to another bug... but it may be runtime dependent -
i.e. it depends what tests stephend was running...

Comment 11

[[e-mail]]

hrm, the nsHashtable in question (used to) be new'ed in the nsProxyObjectManager
ctor and delete'd in the dtor... so the only way it could be runtime dependent
is if we're leaking the whole proxy thing. maybe stephend can clarify that if he
has time. (this code has subsequently been converted to use inline members for
the hashes, but afaict it was correct before that, so i don't see how that would
change anything...)
 
i just checked nsHashtable itself, and confirmed it's cleaning up its pldhash
properly as well. (i'm surprised how many of these things we create at startup!)
 
so i'm still miffed... if you think its worth opening a bug and digging around
with purify, let's do that ;)
 
(we've found a false positive with purify before, see bug 206195)