Bug 326491 - leaked observer service leaks things on shutdown - r=darin

Author: benjamin%smedbergs.us

docshell/base/nsDocShell.cpp

@@ -93,7 +93,6 @@
 #include "nsIPrefBranch.h"
 #include "nsIPrefBranch2.h"
 #include "nsIWritablePropertyBag2.h"
-#include "nsObserverService.h"
 
 // we want to explore making the document own the load group
 // so we can associate the document URI with the load group.

xpcom/build/nsXPCOMCID.h

@@ -84,6 +84,13 @@
  */
 #define NS_ARRAY_CONTRACTID  "@mozilla.org/array;1"
 
+/**
+ * Observer Service ContractID
+ * The observer service implements the global nsIObserverService object.
+ * It should be used from the main thread only.
+ */
+#define NS_OBSERVERSERVICE_CONTRACTID "@mozilla.org/observer-service;1"
+
 /**
  * The following are the CIDs and Contract IDs of the nsISupports wrappers for 
  * primative types.  

xpcom/build/nsXPComInit.cpp

@@ -682,8 +682,9 @@ NS_ShutdownXPCOM(nsIServiceManager* servMgr)
         nsCOMPtr <nsIEventQueue> currentQ;
         NS_GetCurrentEventQ(getter_AddRefs(currentQ), eqs);
 
-        nsCOMPtr<nsIObserverService> observerService =
-                 do_GetService("@mozilla.org/observer-service;1");
+        nsRefPtr<nsObserverService> observerService;
+        CallGetService("@mozilla.org/observer-service;1",
+                       (nsObserverService**) getter_AddRefs(observerService));
 
         if (observerService)
         {
@@ -722,10 +723,13 @@ NS_ShutdownXPCOM(nsIServiceManager* servMgr)
 
         // We save the "xpcom-shutdown-loaders" observers to notify after
         // the observerservice is gone.
-        if (observerService)
+        if (observerService) {
             observerService->
                 EnumerateObservers(NS_XPCOM_SHUTDOWN_LOADERS_OBSERVER_ID,
                                    getter_AddRefs(moduleLoaders));
+
+            observerService->Shutdown();
+        }
     }
 
     // XPCOM is officially in shutdown mode NOW

xpcom/ds/nsObserverList.cpp

@@ -37,53 +37,29 @@
 
 #include "nsObserverList.h"
 
-#include "pratom.h"
-#include "nsAutoLock.h"
 #include "nsAutoPtr.h"
-#include "nsCOMPtr.h"
-#include "nsIObserver.h"
+#include "nsCOMArray.h"
 #include "nsISimpleEnumerator.h"
-#include "nsIWeakReference.h"
-
-nsObserverList::nsObserverList(nsresult &rv)
-{
-    MOZ_COUNT_CTOR(nsObserverList);
-    mLock = PR_NewLock();
-    if (!mLock)
-        rv = NS_ERROR_OUT_OF_MEMORY;
-}
-
-nsObserverList::~nsObserverList(void)
-{
-    MOZ_COUNT_DTOR(nsObserverList);
-    if (mLock)
-        PR_DestroyLock(mLock);
-}
 
 nsresult
 nsObserverList::AddObserver(nsIObserver* anObserver, PRBool ownsWeak)
 {
-    NS_ENSURE_ARG(anObserver);
-
-    nsAutoLock lock(mLock);
-
-    nsCOMPtr<nsISupports> observerRef;
-    if (ownsWeak) {
-        nsCOMPtr<nsISupportsWeakReference>
-            weakRefFactory(do_QueryInterface(anObserver));
-        NS_ASSERTION(weakRefFactory,
-                     "Doesn't implement nsISupportsWeakReference");
-        if (weakRefFactory)
-            weakRefFactory->
-                GetWeakReference((nsIWeakReference**)(nsISupports**)
-                                 getter_AddRefs(observerRef));
-    } else {
-        observerRef = anObserver;
+    NS_ASSERTION(anObserver, "Null input");
+
+    if (!ownsWeak) {
+        ObserverRef* o = mObservers.AppendElement(anObserver);
+        if (!o)
+            return NS_ERROR_OUT_OF_MEMORY;
+
+        return NS_OK;
     }
-    if (!observerRef)
-        return NS_ERROR_FAILURE;
+        
+    nsCOMPtr<nsIWeakReference> weak = do_GetWeakReference(anObserver);
+    if (!weak)
+        return NS_NOINTERFACE;
 
-    if (!mObservers.AppendObject(observerRef))
+    ObserverRef *o = mObservers.AppendElement(weak);
+    if (!o)
         return NS_ERROR_OUT_OF_MEMORY;
 
     return NS_OK;
@@ -92,93 +68,91 @@ nsObserverList::AddObserver(nsIObserver* anObserver, PRBool ownsWeak)
 nsresult
 nsObserverList::RemoveObserver(nsIObserver* anObserver)
 {
-    NS_ENSURE_ARG(anObserver);
+    NS_ASSERTION(anObserver, "Null input");
 
-    nsAutoLock lock(mLock);
-
-    if (mObservers.RemoveObject(anObserver))
+    if (mObservers.RemoveElement(NS_STATIC_CAST(nsISupports*, anObserver)))
         return NS_OK;
 
-    nsCOMPtr<nsISupportsWeakReference>
-        weakRefFactory(do_QueryInterface(anObserver));
-    if (!weakRefFactory)
-        return NS_ERROR_FAILURE;
-
-    nsCOMPtr<nsIWeakReference> observerRef;
-    weakRefFactory->GetWeakReference(getter_AddRefs(observerRef));
-
+    nsCOMPtr<nsIWeakReference> observerRef = do_GetWeakReference(anObserver);
     if (!observerRef)
         return NS_ERROR_FAILURE;
 
-    if (!mObservers.RemoveObject(observerRef))
+    if (!mObservers.RemoveElement(observerRef))
         return NS_ERROR_FAILURE;
 
     return NS_OK;
 }
 
-class nsObserverEnumerator : public nsISimpleEnumerator
-{
-public:
-    NS_DECL_ISUPPORTS
-    NS_DECL_NSISIMPLEENUMERATOR
-
-    nsObserverEnumerator(nsCOMArray<nsISupports> &aObservers);
-
-private:
-    ~nsObserverEnumerator() { }
-
-    PRUint32 mIndex; // Counts down, ends at 0
-    nsCOMArray<nsISupports> mObservers;
-};
-
 nsresult
 nsObserverList::GetObserverList(nsISimpleEnumerator** anEnumerator)
 {
-    nsAutoLock lock(mLock);
-
-    nsRefPtr<nsObserverEnumerator> e(new nsObserverEnumerator(mObservers));
+    nsRefPtr<nsObserverEnumerator> e(new nsObserverEnumerator(this));
     if (!e)
         return NS_ERROR_OUT_OF_MEMORY;
 
     NS_ADDREF(*anEnumerator = e);
     return NS_OK;
 }
 
-nsObserverEnumerator::nsObserverEnumerator(nsCOMArray<nsISupports> &aObservers)
+void
+nsObserverList::FillObserverArray(nsCOMArray<nsIObserver> &aArray)
 {
-    for (PRInt32 i = 0; i < aObservers.Count(); ++i) {
-        nsCOMPtr<nsIWeakReference> weak(do_QueryInterface(aObservers[i]));
-        if (weak) {
-            nsCOMPtr<nsISupports> strong(do_QueryReferent(weak));
-            if (strong)
-                mObservers.AppendObject(strong);
+    aArray.SetCapacity(mObservers.Length());
+
+    for (PRInt32 i = mObservers.Length() - 1; i >= 0; --i) {
+        if (mObservers[i].isWeakRef) {
+            nsCOMPtr<nsIObserver> o(do_QueryReferent(mObservers[i].asWeak()));
+            if (o) {
+                aArray.AppendObject(o);
+            }
+            else {
+                // the object has gone away, remove the weakref
+                mObservers.RemoveElementAt(i);
+            }
         }
         else {
-            mObservers.AppendObject(aObservers[i]);
+            aArray.AppendObject(mObservers[i].asObserver());
         }
     }
+}
+
+void
+nsObserverList::NotifyObservers(nsISupports *aSubject,
+                                const char *aTopic,
+                                const PRUnichar *someData)
+{
+    nsCOMArray<nsIObserver> observers;
+    FillObserverArray(observers);
 
-    mIndex = mObservers.Count();
+    for (PRInt32 i = 0; i < observers.Count(); ++i) {
+        observers[i]->Observe(aSubject, aTopic, someData);
+    }
 }
 
 NS_IMPL_ISUPPORTS1(nsObserverEnumerator, nsISimpleEnumerator)
 
+nsObserverEnumerator::nsObserverEnumerator(nsObserverList* aObserverList)
+    : mIndex(0)
+{
+    aObserverList->FillObserverArray(mObservers);
+}
+
 NS_IMETHODIMP
 nsObserverEnumerator::HasMoreElements(PRBool *aResult)
 {
-    *aResult = (mIndex > 0);
+    *aResult = (mIndex < mObservers.Count());
     return NS_OK;
 }
 
 NS_IMETHODIMP
 nsObserverEnumerator::GetNext(nsISupports* *aResult)
 {
-    if (!mIndex) {
+    if (mIndex == mObservers.Count()) {
         NS_ERROR("Enumerating after HasMoreElements returned false.");
         return NS_ERROR_UNEXPECTED;
     }
 
-    --mIndex;
     NS_ADDREF(*aResult = mObservers[mIndex]);
+    ++mIndex;
     return NS_OK;
 }

xpcom/ds/nsObserverList.h

@@ -39,25 +39,75 @@
 #define nsObserverList_h___
 
 #include "nsISupports.h"
+#include "nsTArray.h"
+#include "nsCOMPtr.h"
 #include "nsCOMArray.h"
+#include "nsIObserver.h"
+#include "nsIWeakReference.h"
+#include "nsHashKeys.h"
+#include "nsISimpleEnumerator.h"
 
-class nsISimpleEnumerator;
-class nsIObserver;
+struct ObserverRef
+{
+  ObserverRef(const ObserverRef& o) :
+    isWeakRef(o.isWeakRef), ref(o.ref) { }
+  
+  ObserverRef(nsIObserver* aObserver) : isWeakRef(PR_FALSE), ref(aObserver) { }
+  ObserverRef(nsIWeakReference* aWeak) : isWeakRef(PR_TRUE), ref(aWeak) { }
+
+  PRBool isWeakRef;
+  nsCOMPtr<nsISupports> ref;
+
+  nsIObserver* asObserver() {
+    NS_ASSERTION(!isWeakRef, "Isn't a strong ref.");
+    return NS_STATIC_CAST(nsIObserver*, (nsISupports*) ref);
+  }
+
+  nsIWeakReference* asWeak() {
+    NS_ASSERTION(isWeakRef, "Isn't a weak ref.");
+    return NS_STATIC_CAST(nsIWeakReference*, (nsISupports*) ref);
+  }
 
-class nsObserverList
+  PRBool operator==(nsISupports* b) const { return ref == b; }
+};
+
+class nsObserverList : public nsCharPtrHashKey
 {
 public:
-  nsObserverList(nsresult &rv);
-  ~nsObserverList();
+  nsObserverList(const char *key) : nsCharPtrHashKey(key)
+  { MOZ_COUNT_CTOR(nsObserverList); }
+
+  ~nsObserverList() { MOZ_COUNT_DTOR(nsObserverList); }
 
   nsresult AddObserver(nsIObserver* anObserver, PRBool ownsWeak);
   nsresult RemoveObserver(nsIObserver* anObserver);
+
+  void NotifyObservers(nsISupports *aSubject,
+                       const char *aTopic,
+                       const PRUnichar *someData);
   nsresult GetObserverList(nsISimpleEnumerator** anEnumerator);
-     
-protected:
-  PRLock* mLock;
-  nsCOMArray<nsISupports> mObservers;
+
+  // Fill an array with the observers of this category.
+  // The array is filled in last-added-first order.
+  void FillObserverArray(nsCOMArray<nsIObserver> &aArray);
+
+private:
+  nsTArray<ObserverRef> mObservers;
 };
 
+class nsObserverEnumerator : public nsISimpleEnumerator
+{
+public:
+    NS_DECL_ISUPPORTS
+    NS_DECL_NSISIMPLEENUMERATOR
+
+    nsObserverEnumerator(nsObserverList* aObserverList);
+
+private:
+    ~nsObserverEnumerator() { }
+
+    PRInt32 mIndex; // Counts up from 0
+    nsCOMArray<nsIObserver> mObservers;
+};
 
 #endif /* nsObserverList_h___ */