32185 - [MLK] Leaking Services during XPCOM Shutdown

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Patrick C. Beard]

If a client of PLHashTable calls PL_HashTableEnumerateEntries(), and the 
PLHashEnumerator function calls PL_HashTableRawLookup, then not all of the 
entries in the PLHashTable will be successfully visited (see bug #32184). Because 
of this, if a service calls nsServiceManager::ReleaseService() during 
nsServiceManager::ShutdownGlobalServiceManager() (which is currently being done 
by nsGlobalHistory::~nsGlobalHistory()), there's a chance that the PLHashEntry* 
and hash key object will leak, or worse that hashed service itself will leak.

We could fix this problem by enumerating the contents of the hashtable into a 
vector (nsISupportsArray, etc.) before releasing all of the services. Or perhaps 
it's a bug for a service to be holding a strong reference to another service in 
the first place, and we should require all objects that are to be used as 
services be capable of providing nsIWeakReferences to themselves.

Comment 1

[Suresh Duddi (gone)]

Weren't the NSPR folks fixing this in the hash table. srinivas ? wtc ?

Should we fix it regardless in the servicemanager shutdow ?

Comment 2

[Wan-Teh Chang]

We checked in a fix in the plhash code (see bug #32184).

I'd like to find out why you need to call PL_HashTableRawLookup
from the enumerator function.  I'm having trouble finding out
where these PL_HashTableEnumerateEntries and PL_HashTableRawLookup
calls are in the source tree.  Can you point me to the files (preferrably
with line numbers too)?

Comment 3

[Patrick C. Beard]

The file is mozilla/xpcom/ds/nsHashtable.cpp, and here are the line numbers:

nsHashtable.cpp line 135
     PLHashEntry **hep = PL_HashTableRawLookup(hashtable, hash, (void *) aKey);

nsHashtable.cpp line 149
     PLHashEntry **hep = PL_HashTableRawLookup(hashtable, hash, (void *) aKey);

nsHashtable.cpp line 168
     void *ret = PL_HashTableLookup(hashtable, (void *) aKey);

nsHashtable.cpp line 181
     PLHashEntry **hep = PL_HashTableRawLookup(hashtable, hash, (void *) aKey);

Comment 4

[Wan-Teh Chang]

Sorry I wasn't clear.  I'd like to know where this
calling sequence is in the source tree:
... ==> PL_HashTableEnumerateEntries ==> PLHashEnumerator function ==> ... ==> 
PL_HashTableRawLookup

Comment 5

[Scott Collins]

Patrick, is there anything further I need to do to resolve this bug?

Comment 6

[Patrick C. Beard]

32184 isn't adequately fixed in my opinion. The existing API is broken, because 
of the reasons I stated before. Currently, if in the destructor of one service, 
we release another service, this bug will occur. One such service destructor I've 
encountered is nsGlobalHistory::~nsGlobalHistory(). It calls ReleaseService() 
with NS_RDFSERVICE_CID. This will cause a lookup in the service manager's 
mServices hash table. We could fix this by using wtc's new NSPR API, 
PL_HashTableRawLookupConst(), instead of PL_HashTableRawLookup(), but there may 
still be clients of NSPR out in the world that can be bitten by this bug. 
Therefore, I think that PL_HashTableRawLookup() should be fixed NOT to reorder 
the hash table during enumeration.

Comment 7

[Scott Collins]

Patrick, you have great knowledge of this bug ... does that mean you want to fix 
it?

Comment 8

[Patrick C. Beard]

The fix is either to take my patches for PLHashtable, or to use the newer non-
modifying calls provided by NSPR.

Comment 9

[Wan-Teh Chang]

I've checked in the newer non-modifying plhash lookup calls.

Comment 10

[Patrick C. Beard]

Moving to M17.

Comment 11

[Patrick C. Beard]

Here's a possible solution, change nsHashtable to protect itself under 
enumeration (something I'd prefer PLHashTable would do instead). I've added a 
protected Lookup() method that is aware that of enumerations, and uses 
PL_HashTableRawLookupConst() when that is the case. Then, in nsHashtable.cpp, all 
calls to PL_HashTableRawLookup() are replaced with calls to Lookup(), which 
should do the correct thing.

Index: mozilla/xpcom/ds/nsHashtable.h
===================================================================
RCS file: /cvsroot/mozilla/xpcom/ds/nsHashtable.h,v
retrieving revision 1.26
diff -c -2 -r1.26 nsHashtable.h
*** nsHashtable.h	2000/06/14 06:28:13	1.26
--- nsHashtable.h	2000/06/21 18:04:37
***************
*** 58,61 ****
--- 58,62 ----
    PLHashTable *hashtable;
    PRLock *mLock;
+   PRBool mEnumerating;
  
  public:
***************
*** 72,75 ****
--- 73,83 ----
    void Reset();
    void Reset(nsHashtableEnumFunc destroyFunc, void* closure = NULL);
+ 
+ protected:
+   PLHashEntry **Lookup(PLHashNumber aHash, nsHashKey *aKey) {
+     return mEnumerating ?
+       PL_HashTableRawLookupConst(hashtable, aHash, (void *) aKey) :
+       PL_HashTableRawLookup(hashtable, aHash, (void *) aKey);
+   }
  };

Index: mozilla/xpcom/ds/nsHashtable.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpcom/ds/nsHashtable.cpp,v
retrieving revision 3.30
diff -c -2 -r3.30 nsHashtable.cpp
*** nsHashtable.cpp	2000/06/15 02:20:30	3.30
--- nsHashtable.cpp	2000/06/21 18:08:32
***************
*** 119,123 ****
  
  nsHashtable::nsHashtable(PRUint32 aInitSize, PRBool threadSafe)
!   : mLock(NULL)
  {
    MOZ_COUNT_CTOR(nsHashtable);
--- 119,123 ----
  
  nsHashtable::nsHashtable(PRUint32 aInitSize, PRBool threadSafe)
!   : mLock(NULL), mEnumerating(PR_FALSE)
  {
    MOZ_COUNT_CTOR(nsHashtable);
***************
*** 152,156 ****
    if (mLock) PR_Lock(mLock);
  
!   PLHashEntry **hep = PL_HashTableRawLookup(hashtable, hash, (void *) aKey);
  
    if (mLock) PR_Unlock(mLock);
--- 152,156 ----
    if (mLock) PR_Lock(mLock);
  
!   PLHashEntry **hep = Lookup(hash, aKey);
  
    if (mLock) PR_Unlock(mLock);
***************
*** 166,170 ****
    if (mLock) PR_Lock(mLock);
  
!   PLHashEntry **hep = PL_HashTableRawLookup(hashtable, hash, (void *) aKey);
  
    if ((he = *hep) != NULL) {
--- 166,170 ----
    if (mLock) PR_Lock(mLock);
  
!   PLHashEntry **hep = Lookup(hash, aKey);
  
    if ((he = *hep) != NULL) {
***************
*** 172,175 ****
--- 172,177 ----
      he->value = aData;
    } else {
+     // shouldn't be adding an item during enumeration.
+     PR_ASSERT(!mEnumerating);
      PL_HashTableRawAdd(hashtable, hep, hash,
                         (void *) aKey->Clone(), aData);
***************
*** 182,193 ****
  
  void *nsHashtable::Get(nsHashKey *aKey) {
  
    if (mLock) PR_Lock(mLock);
  
!   void *ret = PL_HashTableLookup(hashtable, (void *) aKey);
  
    if (mLock) PR_Unlock(mLock);
  
!   return ret;
  }
  
--- 184,200 ----
  
  void *nsHashtable::Get(nsHashKey *aKey) {
+   void *res =  NULL;
+   PLHashNumber hash = aKey->HashValue();
+   PLHashEntry *he;
  
    if (mLock) PR_Lock(mLock);
  
!   PLHashEntry **hep = Lookup(hash, aKey);
!   if ((he = *hep) != NULL)
!     res = he->value;
  
    if (mLock) PR_Unlock(mLock);
  
!   return res;
  }
  
***************
*** 198,206 ****
    if (mLock) PR_Lock(mLock);
  
!   PLHashEntry **hep = PL_HashTableRawLookup(hashtable, hash, (void *) aKey);
    void *res = NULL;
    
    if ((he = *hep) != NULL) {
      res = he->value;
      PL_HashTableRawRemove(hashtable, hep, he);
    }
--- 205,214 ----
    if (mLock) PR_Lock(mLock);
  
!   PLHashEntry **hep = Lookup(hash, aKey);
    void *res = NULL;
    
    if ((he = *hep) != NULL) {
      res = he->value;
+     PR_ASSERT(!mEnumerating);
      PL_HashTableRawRemove(hashtable, hep, he);
    }
***************
*** 232,239 ****
--- 240,250 ----
  
  void nsHashtable::Enumerate(nsHashtableEnumFunc aEnumFunc, void* closure) {
+   PRBool wasEnumerating = mEnumerating;
+   mEnumerating = PR_TRUE;
    _HashEnumerateArgs thunk;
    thunk.fn = aEnumFunc;
    thunk.arg = closure;
    PL_HashTableEnumerateEntries(hashtable, _hashEnumerate, &thunk);
+   mEnumerating = wasEnumerating;
  }
  

Comment 12

[Patrick C. Beard]

The problem with this solution is that PL_HashTableRawLookupConst() returns 
PLHashEntry *const *, which must be cast away by Lookup() to provide the required 
services. Therefore, PL_HashTableRawLookupConst() is really overly strict, 
because we still may want to modify the table, but what we really don't want to 
do is mess up a current enumeration. My original solution was the least invasive 
because it simply provided a way to keep the enumeration stable under lookup.

Comment 13

[Patrick C. Beard]

This version of the fix simply asserts before attempting a destructive 
modification of the nsHashtable during enumeration. It provides a Lookup() 
operation that doesn't have the const cast away problem.

Index: mozilla/xpcom/ds/nsHashtable.h
===================================================================
RCS file: /cvsroot/mozilla/xpcom/ds/nsHashtable.h,v
retrieving revision 1.26
diff -c -2 -r1.26 nsHashtable.h
*** nsHashtable.h	2000/06/14 06:28:13	1.26
--- nsHashtable.h	2000/06/21 18:24:23
***************
*** 58,61 ****
--- 58,62 ----
    PLHashTable *hashtable;
    PRLock *mLock;
+   PRBool mEnumerating;
  
  public:
***************
*** 72,75 ****
--- 73,83 ----
    void Reset();
    void Reset(nsHashtableEnumFunc destroyFunc, void* closure = NULL);
+ 
+ protected:
+   void *Lookup(nsHashKey *aKey) {
+     return mEnumerating ?
+       PL_HashTableLookupConst(hashtable, (void *) aKey) :
+       PL_HashTableLookup(hashtable, (void *) aKey);
+   }
  };
  
Index: mozilla/xpcom/ds/nsHashtable.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpcom/ds/nsHashtable.cpp,v
retrieving revision 3.30
diff -c -2 -r3.30 nsHashtable.cpp
*** nsHashtable.cpp	2000/06/15 02:20:30	3.30
--- nsHashtable.cpp	2000/06/21 18:25:55
***************
*** 119,123 ****
  
  nsHashtable::nsHashtable(PRUint32 aInitSize, PRBool threadSafe)
!   : mLock(NULL)
  {
    MOZ_COUNT_CTOR(nsHashtable);
--- 119,123 ----
  
  nsHashtable::nsHashtable(PRUint32 aInitSize, PRBool threadSafe)
!   : mLock(NULL), mEnumerating(PR_FALSE)
  {
    MOZ_COUNT_CTOR(nsHashtable);
***************
*** 152,160 ****
    if (mLock) PR_Lock(mLock);
  
!   PLHashEntry **hep = PL_HashTableRawLookup(hashtable, hash, (void *) aKey);
  
    if (mLock) PR_Unlock(mLock);
  
!   return *hep != NULL;
  }
  
--- 152,160 ----
    if (mLock) PR_Lock(mLock);
  
!   void* value = Lookup(aKey);
  
    if (mLock) PR_Unlock(mLock);
  
!   return value != NULL;
  }
  
***************
*** 166,169 ****
--- 166,171 ----
    if (mLock) PR_Lock(mLock);
  
+   // shouldn't be adding an item during enumeration.
+   PR_ASSERT(!mEnumerating);
    PLHashEntry **hep = PL_HashTableRawLookup(hashtable, hash, (void *) aKey);
  
***************
*** 182,193 ****
  
  void *nsHashtable::Get(nsHashKey *aKey) {
  
    if (mLock) PR_Lock(mLock);
  
!   void *ret = PL_HashTableLookup(hashtable, (void *) aKey);
  
    if (mLock) PR_Unlock(mLock);
  
!   return ret;
  }
  
--- 184,196 ----
  
  void *nsHashtable::Get(nsHashKey *aKey) {
+   PLHashNumber hash = aKey->HashValue();
  
    if (mLock) PR_Lock(mLock);
  
!   void* res = Lookup(aKey);
  
    if (mLock) PR_Unlock(mLock);
  
!   return res;
  }
  
***************
*** 198,201 ****
--- 201,206 ----
    if (mLock) PR_Lock(mLock);
  
+   // shouldn't be removing items while enumerating.
+   PR_ASSERT(!mEnumerating);
    PLHashEntry **hep = PL_HashTableRawLookup(hashtable, hash, (void *) aKey);
    void *res = NULL;
***************
*** 232,239 ****
--- 237,247 ----
  
  void nsHashtable::Enumerate(nsHashtableEnumFunc aEnumFunc, void* closure) {
+   PRBool wasEnumerating = mEnumerating;
+   mEnumerating = PR_TRUE;
    _HashEnumerateArgs thunk;
    thunk.fn = aEnumFunc;
    thunk.arg = closure;
    PL_HashTableEnumerateEntries(hashtable, _hashEnumerate, &thunk);
+   mEnumerating = wasEnumerating;
  }
  

Comment 14

[Wan-Teh Chang]

Hmm... the fact that PL_HashTableRawLookupConst returns
a different type from PL_HashTableRawLookup means you
can't really use these two functions interchangeably
without casting away the 'const' as in the first version
of your new Lookup() method.

If your solution doesn't need PL_HashTableRawLookupConst,
I'm going to remove this function.  You just need
PL_HashTableLookupConst, right?
  

Comment 15

[Wan-Teh Chang]

Attachment: Proposed patch for nsHashtable.h and nsHashtable.cpp.

Comment 16

[Wan-Teh Chang]

The second version of Patrick's patch has a problem:
the Exists() method uses the Lookup() method and
hence cannot distinguish between the case that the
table entry doesn't exist and the case that the value
of the entry is NULL.  To distinguish between these
two cases, the Exists() method needs to do a raw
lookup.  The patch I attached (id=10502) does this.
I also removed the Lookup() method and moved that
logic into the Get() method.

Comment 17

[Patrick C. Beard]

Only leaking on shutdown, so I'd say this is nsbeta3-. I could be persuaded 
otherwise.

Comment 18

[Warren Harris]

Reassigning to Jud because this is needed for embedding.

Comment 19

[Judson Valeski]

fix is in

Comment 20

[leger]

valeski...you'll need to mark verified if all is well please.

Comment 21

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old