Bug 577950 - Use a hashtable to speed up nsPrefBranch::RemoveObserver. r=bzbarsky, a2.0=jst
☠☠ backed out by a7a0c3423b72 ☠ ☠
authorJustin Lebar <justin.lebar@gmail.com>
Fri, 16 Jul 2010 14:10:01 -0700
changeset 50829 63bb61d1416e49cc1000298eee7650d78d090c1c
parent 50826 7c19fb706708693cf9b0b762e9f3500eb05dd220
child 50830 878049d882e62a3dae4ae673b9a0cbba92f49d08
child 50837 a7a0c3423b72692e7ce951b0f0b2f663bb8e6e42
push idunknown
push userunknown
push dateunknown
reviewersbzbarsky
bugs577950
milestone2.0b5pre
Bug 577950 - Use a hashtable to speed up nsPrefBranch::RemoveObserver. r=bzbarsky, a2.0=jst
modules/libpref/src/nsPrefBranch.cpp
modules/libpref/src/nsPrefBranch.h
modules/libpref/test/unit/test_bug577950.js
xpcom/glue/nsClassHashtable.h
--- a/modules/libpref/src/nsPrefBranch.cpp
+++ b/modules/libpref/src/nsPrefBranch.cpp
@@ -64,31 +64,20 @@
 #include "prefapi_private_data.h"
 
 // Definitions
 struct EnumerateData {
   const char  *parent;
   nsTArray<nsCString> *pref_list;
 };
 
-struct PrefCallbackData {
-  nsPrefBranch     *pBranch;
-  nsISupports      *pCanonical;
-  nsIObserver      *pObserver;
-  nsIWeakReference *pWeakRef;
-  char pDomain[1];
-};
-
-
 // Prototypes
 static PLDHashOperator
   pref_enumChild(PLDHashTable *table, PLDHashEntryHdr *heh,
                  PRUint32 i, void *arg);
-static nsresult
-  NotifyObserver(const char *newpref, void *data);
 
 #ifdef MOZ_IPC
 using mozilla::dom::ContentChild;
 
 static ContentChild*
 GetContentChild()
 {
   if (XRE_GetProcessType() == GeckoProcessType_Content) {
@@ -102,21 +91,22 @@ GetContentChild()
 }
 #endif  // MOZ_IPC
 
 /*
  * Constructor/Destructor
  */
 
 nsPrefBranch::nsPrefBranch(const char *aPrefRoot, PRBool aDefaultBranch)
-  : mObservers(nsnull)
 {
   mPrefRoot = aPrefRoot;
   mPrefRootLength = mPrefRoot.Length();
   mIsDefault = aDefaultBranch;
+  mFreeingObserverList = PR_FALSE;
+  mObservers.Init();
 
   nsCOMPtr<nsIObserverService> observerService =
     mozilla::services::GetObserverService();
   if (observerService) {
     ++mRefCnt;    // Our refcnt must be > 0 when we call this, or we'll get deleted!
     // add weak so we don't have to clean up at shutdown
     observerService->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, PR_TRUE);
     --mRefCnt;
@@ -697,233 +687,191 @@ NS_IMETHODIMP nsPrefBranch::GetChildList
 
 
 /*
  *  nsIPrefBranch2 methods
  */
 
 NS_IMETHODIMP nsPrefBranch::AddObserver(const char *aDomain, nsIObserver *aObserver, PRBool aHoldWeak)
 {
-  PrefCallbackData *pCallback;
+  PrefCallback *pCallback;
   const char *pref;
 
   NS_ENSURE_ARG_POINTER(aDomain);
   NS_ENSURE_ARG_POINTER(aObserver);
 
 #ifdef MOZ_IPC
   if (ContentChild* cpc = GetContentChild()) {
     return cpc->AddRemotePrefObserver(nsDependentCString(aDomain), mPrefRoot, aObserver, aHoldWeak);
   }
 #endif
 
-  if (!mObservers) {
-    mObservers = new nsAutoVoidArray();
-    if (nsnull == mObservers)
-      return NS_ERROR_OUT_OF_MEMORY;
-  }
-
-  pCallback = (PrefCallbackData *)NS_Alloc(sizeof(PrefCallbackData) + strlen(aDomain));
-  if (nsnull == pCallback)
-    return NS_ERROR_OUT_OF_MEMORY;
-
-  pCallback->pBranch = this;
-  pCallback->pObserver = aObserver;
-
   // hold a weak reference to the observer if so requested
   if (aHoldWeak) {
     nsCOMPtr<nsISupportsWeakReference> weakRefFactory = do_QueryInterface(aObserver);
     if (!weakRefFactory) {
       // the caller didn't give us a object that supports weak reference... tell them
-      nsMemory::Free(pCallback);
       return NS_ERROR_INVALID_ARG;
     }
-    nsCOMPtr<nsIWeakReference> tmp = do_GetWeakReference(weakRefFactory);
-    NS_ADDREF(pCallback->pWeakRef = tmp);
+
+    // Construct a PrefCallback with a weak reference to the observer.
+    pCallback = new PrefCallback(aDomain, weakRefFactory, this);
+
   } else {
-    pCallback->pWeakRef = nsnull;
-    NS_ADDREF(pCallback->pObserver);
+    // Construct a PrefCallback with a strong reference to the observer.
+    pCallback = new PrefCallback(aDomain, aObserver, this);
   }
 
-  // In RemoveObserver, we need a canonical pointer to compare the removed
-  // observer to. In order to avoid having to call QI every time through the
-  // loop, we call QI here. There are two cases:
-  // 1. We hold a strong reference to the nsIObserver. This must hold
-  //    pCanonical alive.
-  // 2. We hold a weak reference: either the object stays alive and eventually
-  //    we QI the nsIObserver to nsISupports and compare successfully. Or the
-  //    object dies and we have a dangling (but unused) pointer to the
-  //    canonical supports pointer until we remove this entry.
-  CallQueryInterface(aObserver, &pCallback->pCanonical);
-  pCallback->pCanonical->Release();
+  if (mObservers.Get(pCallback)) {
+    NS_WARNING("Ignoring duplicate observer.");
+    delete pCallback;
+    return NS_OK;
+  }
 
-  strcpy(pCallback->pDomain, aDomain);
-  mObservers->AppendElement(pCallback);
+  PRBool putSucceeded = mObservers.Put(pCallback, pCallback);
+
+  if (!putSucceeded) {
+    delete pCallback;
+    return NS_ERROR_FAILURE;
+  }
 
   // We must pass a fully qualified preference name to the callback
-  pref = getPrefName(aDomain); // aDomain == nsnull only possible failure, trapped above
+  // aDomain == nsnull is the only possible failure, and we trapped it with
+  // NS_ENSURE_ARG_POINTER above.
+  pref = getPrefName(aDomain);
   PREF_RegisterCallback(pref, NotifyObserver, pCallback);
   return NS_OK;
 }
 
 NS_IMETHODIMP nsPrefBranch::RemoveObserver(const char *aDomain, nsIObserver *aObserver)
 {
   NS_ENSURE_ARG_POINTER(aDomain);
   NS_ENSURE_ARG_POINTER(aObserver);
 
+  nsresult rv = NS_OK;
+
 #ifdef MOZ_IPC
   if (XRE_GetProcessType() == GeckoProcessType_Content) {
-    nsresult rv = NS_OK;
     ContentChild *cpc = ContentChild::GetSingleton();
     // In case cpc doesn't exist here, we're silently returning (instead of
     // asserting), because the child process is likely to be null
     // when this is called during xpcom-shutdown.
     if (cpc)
       rv = cpc->RemoveRemotePrefObserver(nsDependentCString(aDomain), 
                                          mPrefRoot, 
                                          aObserver);
     return rv;
   }
 #endif
 
-  if (!mObservers)
+  // If we're in the middle of a call to freeObserverList, don't process this
+  // RemoveObserver call -- the observer in question will be removed soon, if
+  // it hasn't been already.
+  //
+  // It's important that we don't touch mObservers in any way -- even a Get()
+  // which retuns null might cause the hashtable to resize itself, which will
+  // break the Enumerator in freeObserverList.
+  if (mFreeingObserverList)
     return NS_OK;
 
-  nsCOMPtr<nsISupports> canonical(do_QueryInterface(aObserver));
-  return RemoveObserverFromList(aDomain, canonical);
+  // Remove the relevant PrefCallback from mObservers and get an owning
+  // pointer to it.  Unregister the callback first, and then let the owning
+  // pointer go out of scope and destroy the callback.
+  PrefCallback key(aDomain, aObserver, this);
+  nsAutoPtr<PrefCallback> pCallback;
+  mObservers.RemoveAndForget(&key, pCallback);
+  if (pCallback) {
+    // aDomain == nsnull is the only possible failure, trapped above
+    const char *pref = getPrefName(aDomain);
+    rv = PREF_UnregisterCallback(pref, NotifyObserver, pCallback);
+  }
+
+  return rv;
 }
 
 NS_IMETHODIMP nsPrefBranch::Observe(nsISupports *aSubject, const char *aTopic, const PRUnichar *someData)
 {
   // watch for xpcom shutdown and free our observers to eliminate any cyclic references
   if (!nsCRT::strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID)) {
     freeObserverList();
   }
   return NS_OK;
 }
 
-static nsresult NotifyObserver(const char *newpref, void *data)
+/* static */
+nsresult nsPrefBranch::NotifyObserver(const char *newpref, void *data)
 {
 #ifdef MOZ_IPC
   if (GetContentChild()) {
-    // We shouldn't ever get here, since we never register NotifyObserver in the 
+    // We shouldn't ever get here, since we never register NotifyObserver in the
     // content process
     NS_NOTREACHED("Remote prefs observation should be done from the \
                   chrome process!");
     return NS_OK;
   }
 #endif
 
-  PrefCallbackData *pData = (PrefCallbackData *)data;
+  PrefCallback *pCallback = (PrefCallback *)data;
+
+  if (pCallback->IsExpired())
+  {
+    // Remove the expired weak reference from the pref branch's observers list.
+    pCallback->GetPrefBranch()->RemoveExpiredCallback(pCallback);
+    return NS_OK;
+  }
+
+  nsCOMPtr<nsIObserver> observer(pCallback->GetObserver());
 
   // remove any root this string may contain so as to not confuse the observer
   // by passing them something other than what they passed us as a topic
-  PRUint32 len = pData->pBranch->GetRootLength();
-  nsCAutoString suffix(newpref + len);  
+  PRUint32 len = pCallback->GetPrefBranch()->GetRootLength();
+  nsCAutoString suffix(newpref + len);
 
-  nsCOMPtr<nsIObserver> observer;
-  if (pData->pWeakRef) {
-    observer = do_QueryReferent(pData->pWeakRef);
-    if (!observer) {
-      // this weak referenced observer went away, remove them from the list
-      pData->pBranch->RemoveObserverFromList(pData->pDomain, pData->pCanonical);
-      return NS_OK;
-    }
-  } else {
-    observer = pData->pObserver;
-  }
-
-  observer->Observe(static_cast<nsIPrefBranch *>(pData->pBranch),
+  observer->Observe(static_cast<nsIPrefBranch *>(pCallback->GetPrefBranch()),
                     NS_PREFBRANCH_PREFCHANGE_TOPIC_ID,
                     NS_ConvertASCIItoUTF16(suffix).get());
   return NS_OK;
 }
 
+PLDHashOperator
+FreeObserverFunc(PrefCallback *aKey,
+                 nsAutoPtr<PrefCallback> &aCallback,
+                 void *aArgs)
+{
+  // Calling NS_RELEASE below might trigger a call to
+  // nsPrefBranch::RemoveObserver, since some classes remove themselves from
+  // the pref branch on destruction.  We don't need to worry about this causing
+  // double-frees, however, because freeObserverList sets mFreeingObserverList
+  // to true, which prevents RemoveObserver calls from doing anything.
+
+  nsPrefBranch *prefBranch = aCallback->GetPrefBranch();
+  const char *pref = prefBranch->getPrefName(aCallback->GetDomain().get());
+  PREF_UnregisterCallback(pref, nsPrefBranch::NotifyObserver, aCallback);
+
+  return PL_DHASH_REMOVE;
+}
 
 void nsPrefBranch::freeObserverList(void)
 {
-  const char *pref;
-  PrefCallbackData *pCallback;
-
-  if (mObservers) {
-    // unregister the observers
-
-    PRInt32 i;
-    nsCAutoString domain;
-    for (i = 0; i < mObservers->Count(); ++i) {
-      pCallback = (PrefCallbackData *)mObservers->ElementAt(i);
-      if (pCallback) {
-        // We must pass a fully qualified preference name to remove the callback
-        pref = getPrefName(pCallback->pDomain);
-        // Remove this observer from our array so that nobody else can remove
-        // what we're trying to remove right now.
-        mObservers->ReplaceElementAt(nsnull, i);
-        PREF_UnregisterCallback(pref, NotifyObserver, pCallback);
-        if (pCallback->pWeakRef) {
-          NS_RELEASE(pCallback->pWeakRef);
-        } else {
-          NS_RELEASE(pCallback->pObserver);
-        }
-        nsMemory::Free(pCallback);
-      }
-    }
-
-    delete mObservers;
-    mObservers = 0;
-  }
+  // We need to prevent anyone from modifying mObservers while we're
+  // enumerating over it.  In particular, some clients will call
+  // RemoveObserver() when they're destructed; we need to keep those calls from
+  // touching mObservers.
+  mFreeingObserverList = PR_TRUE;
+  mObservers.Enumerate(&FreeObserverFunc, nsnull);
+  mFreeingObserverList = PR_FALSE;
 }
 
-nsresult
-nsPrefBranch::RemoveObserverFromList(const char *aDomain, nsISupports *aObserver)
+void
+nsPrefBranch::RemoveExpiredCallback(PrefCallback *aCallback)
 {
-  PRInt32 count = mObservers->Count();
-  if (!count) {
-    return NS_OK;
-  }
-
-#ifdef DEBUG
-  PRBool alreadyRemoved = PR_FALSE;
-#endif
-
-  for (PRInt32 i = 0; i < count; i++) {
-    PrefCallbackData *pCallback = (PrefCallbackData *)mObservers->ElementAt(i);
-
-#ifdef DEBUG
-    if (!pCallback) {
-      // Have to assume that the callback we're looking for was already
-      // removed in freeObserverList below.
-      alreadyRemoved = PR_TRUE;
-    }
-#endif
+  NS_PRECONDITION(aCallback->IsExpired(), "Callback should be expired.");
+  mObservers.Remove(aCallback);
+}
 
-    if (pCallback &&
-        pCallback->pCanonical == aObserver &&
-        !strcmp(pCallback->pDomain, aDomain)) {
-      // We must pass a fully qualified preference name to remove the callback
-      const char *pref = getPrefName(aDomain); // aDomain == nsnull only possible failure, trapped above
-      nsresult rv = PREF_UnregisterCallback(pref, NotifyObserver, pCallback);
-      if (NS_SUCCEEDED(rv)) {
-        // Remove this observer from our array so that nobody else can remove
-        // what we're trying to remove ourselves right now.
-        mObservers->RemoveElementAt(i);
-        if (pCallback->pWeakRef) {
-          NS_RELEASE(pCallback->pWeakRef);
-        } else {
-          NS_RELEASE(pCallback->pObserver);
-        }
-        NS_Free(pCallback);
-      }
-      return rv;
-    }
-  }
-
-  NS_WARN_IF_FALSE(alreadyRemoved,
-                   "Failed attempt to remove a pref observer, probably leaking");
-  return NS_OK;
-}
- 
 nsresult nsPrefBranch::GetDefaultFromPropertiesFile(const char *aPrefName, PRUnichar **return_buf)
 {
   nsresult rv;
 
   // the default value contains a URL to a .properties file
     
   nsXPIDLCString propertyFileURL;
   rv = PREF_CopyCharPref(aPrefName, getter_Copies(propertyFileURL), PR_TRUE);
--- a/modules/libpref/src/nsPrefBranch.h
+++ b/modules/libpref/src/nsPrefBranch.h
@@ -46,48 +46,204 @@
 #include "nsXPCOM.h"
 #include "nsISupportsPrimitives.h"
 #include "nsIRelativeFilePref.h"
 #include "nsILocalFile.h"
 #include "nsString.h"
 #include "nsVoidArray.h"
 #include "nsTArray.h"
 #include "nsWeakReference.h"
+#include "nsClassHashtable.h"
+#include "nsCRT.h"
+#include "prbit.h"
+#include "nsTraceRefcnt.h"
+
+class nsPrefBranch;
+
+class PrefCallback : public PLDHashEntryHdr {
+
+  public:
+    typedef PrefCallback* KeyType;
+    typedef const PrefCallback* KeyTypePointer;
+
+    static const PrefCallback* KeyToPointer(PrefCallback *aKey)
+    {
+      return aKey;
+    }
+
+    static PLDHashNumber HashKey(const PrefCallback *aKey)
+    {
+      PRUint32 strHash = nsCRT::HashCode(aKey->mDomain.BeginReading(),
+                                         aKey->mDomain.Length());
+
+      return PR_ROTATE_LEFT32(strHash, 4) ^
+             NS_PTR_TO_UINT32(aKey->mCanonical);
+    }
+
+
+  public:
+    // Create a PrefCallback with a strong reference to its observer.
+    PrefCallback(const char *aDomain, nsIObserver *aObserver,
+                 nsPrefBranch *aBranch)
+      : mDomain(aDomain),
+        mBranch(aBranch),
+        mWeakRef(nsnull),
+        mStrongRef(aObserver)
+    {
+      MOZ_COUNT_CTOR(PrefCallback);
+      nsCOMPtr<nsISupports> canonical = do_QueryInterface(aObserver);
+      mCanonical = canonical;
+    }
+
+    // Create a PrefCallback with a weak reference to its observer.
+    PrefCallback(const char *aDomain,
+                 nsISupportsWeakReference *aObserver,
+                 nsPrefBranch *aBranch)
+      : mDomain(aDomain),
+        mBranch(aBranch),
+        mWeakRef(do_GetWeakReference(aObserver)),
+        mStrongRef(nsnull)
+    {
+      MOZ_COUNT_CTOR(PrefCallback);
+      nsCOMPtr<nsISupports> canonical = do_QueryInterface(aObserver);
+      mCanonical = canonical;
+    }
+
+    // Copy constructor needs to be explicit or the linker complains.
+    PrefCallback(const PrefCallback *&aCopy)
+      : mDomain(aCopy->mDomain),
+        mBranch(aCopy->mBranch),
+        mWeakRef(aCopy->mWeakRef),
+        mStrongRef(aCopy->mStrongRef),
+        mCanonical(aCopy->mCanonical)
+    {
+      MOZ_COUNT_CTOR(PrefCallback);
+    }
+
+    ~PrefCallback()
+    {
+      MOZ_COUNT_DTOR(PrefCallback);
+    }
+
+    PRBool KeyEquals(const PrefCallback *aKey) const
+    {
+      // We want to be able to look up a weakly-referencing PrefCallback after
+      // its observer has died so we can remove it from the table.  Once the
+      // callback's observer dies, its canonical pointer is stale -- in
+      // particular, we may have allocated a new observer in the same spot in
+      // memory!  So we can't just compare canonical pointers to determine
+      // whether aKey refers to the same observer as this.
+      //
+      // Our workaround is based on the way we use this hashtable: When we ask
+      // the hashtable to remove a PrefCallback whose weak reference has
+      // expired, we use as the key for removal the same object as was inserted
+      // into the hashtable.  Thus we can say that if one of the keys' weak
+      // references has expired, the two keys are equal iff they're the same
+      // object.
+
+      if (IsExpired() || aKey->IsExpired())
+        return this == aKey;
+
+      if (mCanonical != aKey->mCanonical)
+        return PR_FALSE;
+
+      return mDomain.Equals(aKey->mDomain);
+    }
+
+    PrefCallback *GetKey() const
+    {
+      return const_cast<PrefCallback*>(this);
+    }
+
+    // Get a pointer to the callback's observer, or null if the observer was
+    // weakly referenced and has been destroyed.
+    nsIObserver *GetObserver() const
+    {
+      if (!IsWeak())
+        return mStrongRef;
+
+      nsCOMPtr<nsIObserver> observer = do_QueryReferent(mWeakRef);
+      return observer;
+    }
+
+    const nsCString& GetDomain() const
+    {
+      return mDomain;
+    }
+
+    nsPrefBranch* GetPrefBranch() const
+    {
+      return mBranch;
+    }
+
+    // Has this callback's weak reference died?
+    PRBool IsExpired() const
+    {
+      return IsWeak() && !GetObserver();
+    }
+
+    enum { ALLOW_MEMMOVE = PR_TRUE };
+
+  private:
+    nsCString             mDomain;
+    nsPrefBranch         *mBranch;
+
+    // Exactly one of mWeakRef and mStrongRef should be non-null.
+    nsWeakPtr             mWeakRef;
+    nsCOMPtr<nsIObserver> mStrongRef;
+
+    // We need a canonical nsISupports pointer, per bug 578392.
+    nsISupports          *mCanonical;
+
+    PRBool IsWeak() const
+    {
+      return !!mWeakRef;
+    }
+};
 
 class nsPrefBranch : public nsIPrefBranchInternal,
                      public nsIObserver,
                      public nsSupportsWeakReference
 {
 public:
   NS_DECL_ISUPPORTS
   NS_DECL_NSIPREFBRANCH
   NS_DECL_NSIPREFBRANCH2
   NS_DECL_NSIOBSERVER
 
   nsPrefBranch(const char *aPrefRoot, PRBool aDefaultBranch);
   virtual ~nsPrefBranch();
 
   PRInt32 GetRootLength() { return mPrefRootLength; }
 
-  nsresult RemoveObserverFromList(const char *aDomain, nsISupports *aObserver);
+  nsresult RemoveObserverFromMap(const char *aDomain, nsISupports *aObserver);
+
+  static nsresult NotifyObserver(const char *newpref, void *data);
 
 protected:
   nsPrefBranch()    /* disallow use of this constructer */
     { }
 
   nsresult   GetDefaultFromPropertiesFile(const char *aPrefName, PRUnichar **return_buf);
+  void RemoveExpiredCallback(PrefCallback *aCallback);
   const char *getPrefName(const char *aPrefName);
   void       freeObserverList(void);
 
+  friend PLDHashOperator
+    FreeObserverFunc(PrefCallback *aKey,
+                     nsAutoPtr<PrefCallback> &aCallback,
+                     void *aArgs);
+
 private:
   PRInt32               mPrefRootLength;
-  nsAutoVoidArray       *mObservers;
   nsCString             mPrefRoot;
   PRBool                mIsDefault;
 
+  PRBool                mFreeingObserverList;
+  nsClassHashtable<PrefCallback, PrefCallback> mObservers;
 };
 
 
 class nsPrefLocalizedString : public nsIPrefLocalizedString,
                               public nsISupportsString
 {
 public:
   nsPrefLocalizedString();
@@ -98,17 +254,17 @@ public:
   NS_FORWARD_NSISUPPORTSPRIMITIVE(mUnicodeString->)
 
   nsresult Init();
 
 private:
   NS_IMETHOD GetData(PRUnichar**);
   NS_IMETHOD SetData(const PRUnichar* aData);
   NS_IMETHOD SetDataWithLength(PRUint32 aLength, const PRUnichar *aData);
-                               
+
   nsCOMPtr<nsISupportsString> mUnicodeString;
 };
 
 
 class nsRelativeFilePref : public nsIRelativeFilePref
 {
 public:
   NS_DECL_ISUPPORTS
new file mode 100644
--- /dev/null
+++ b/modules/libpref/test/unit/test_bug577950.js
@@ -0,0 +1,63 @@
+/* ***** BEGIN LICENSE BLOCK *****
+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
+ *
+ * The contents of this file are subject to the Mozilla Public License Version
+ * 1.1 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * http://www.mozilla.org/MPL/
+ *
+ * Software distributed under the License is distributed on an "AS IS" basis,
+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
+ * for the specific language governing rights and limitations under the
+ * License.
+ *
+ * The Original Code is Mozilla code
+ *
+ * The Initial Developer of the Original Code is Mozilla Corporation
+ * Portions created by the Initial Developer are Copyright (C) 2010
+ * the Initial Developer. All Rights Reserved.
+ *
+ * Contributor(s):
+ *   Justin Lebar <justin.lebar@gmail.com>
+ *
+ * Alternatively, the contents of this file may be used under the terms of
+ * either the GNU General Public License Version 2 or later (the "GPL"), or
+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
+ * in which case the provisions of the GPL or the LGPL are applicable instead
+ * of those above. If you wish to allow use of your version of this file only
+ * under the terms of either the GPL or the LGPL, and not to allow others to
+ * use your version of this file under the terms of the MPL, indicate your
+ * decision by deleting the provisions above and replace them with the notice
+ * and other provisions required by the GPL or the LGPL. If you do not delete
+ * the provisions above, a recipient may use your version of this file under
+ * the terms of any one of the MPL, the GPL or the LGPL.
+ *
+ * ***** END LICENSE BLOCK ***** */
+
+function run_test() {
+  var ps = Cc["@mozilla.org/preferences-service;1"].
+            getService(Ci.nsIPrefService);
+
+  var pb2= Cc["@mozilla.org/preferences-service;1"].
+            getService(Ci.nsIPrefBranch2);
+
+  var pb = Cc["@mozilla.org/preferences-service;1"].
+            getService(Ci.nsIPrefBranch);
+
+  var observer = {
+    QueryInterface: function QueryInterface(aIID) {
+      if (aIID.equals(Ci.nsIObserver) ||
+          aIID.equals(Ci.nsISupports))
+         return this;
+      throw Components.results.NS_NOINTERFACE;
+    },
+
+    observe: function observe(aSubject, aTopic, aState) {
+      // Don't do anything.
+    }
+  }
+
+  /* Set the same pref twice.  This shouldn't leak. */
+  pb.addObserver("UserPref.nonexistent.setIntPref", observer, false);
+  pb.addObserver("UserPref.nonexistent.setIntPref", observer, false);
+}
--- a/xpcom/glue/nsClassHashtable.h
+++ b/xpcom/glue/nsClassHashtable.h
@@ -65,16 +65,28 @@ public:
    */
   PRBool Get(KeyType aKey, UserDataType* pData) const;
 
   /**
    * @copydoc nsBaseHashtable::Get
    * @returns NULL if the key is not present.
    */
   UserDataType Get(KeyType aKey) const;
+
+  /**
+   * Remove the entry for the given key from the hashtable and return it in
+   * aOut.  If the key is not in the hashtable, aOut's pointer is set to NULL.
+   *
+   * Normally, an entry is deleted when it's removed from an nsClassHashtable,
+   * but this function transfers ownership of the entry back to the caller
+   * through aOut -- the entry will be deleted when aOut goes out of scope.
+   *
+   * @param aKey the key to get and remove from the hashtable
+   */
+  void RemoveAndForget(KeyType aKey, nsAutoPtr<T> &aOut);
 };
 
 
 /**
  * Thread-safe version of nsClassHashtable
  * @param KeyClass a wrapper-class for the hashtable key, see nsHashKeys.h
  *   for a complete specification.
  * @param Class the class-type being wrapped
@@ -128,16 +140,33 @@ nsClassHashtable<KeyClass,T>::Get(KeyTyp
   typename base_type::EntryType* ent = this->GetEntry(aKey);
 
   if (!ent)
     return NULL;
 
   return ent->mData;
 }
 
+template<class KeyClass,class T>
+void
+nsClassHashtable<KeyClass,T>::RemoveAndForget(KeyType aKey, nsAutoPtr<T> &aOut)
+{
+  aOut = nsnull;
+  nsAutoPtr<T> ptr;
+
+  typename base_type::EntryType *ent = this->GetEntry(aKey);
+  if (!ent)
+    return;
+
+  // Transfer ownership from ent->mData into aOut.
+  aOut = ent->mData;
+
+  this->Remove(aKey);
+}
+
 
 //
 // nsClassHashtableMT definitions
 //
 
 template<class KeyClass,class T>
 PRBool
 nsClassHashtableMT<KeyClass,T>::Get(KeyType aKey, T** retVal) const