Bug 579236: Fix shutdown crash and lesser bugs with remote pref observers. r=dwitte
authorChris Jones <jones.chris.g@gmail.com>
Wed, 21 Jul 2010 13:42:32 -0500
changeset 48134 c428d17cd3e74d97c28881888adde8c007346446
parent 48133 8b85a12f424d3648cbc19fcd6f2a9b5848fe3701
child 48135 6ee9399bde01da63f0d95e3f7bd8f811262407d4
push id14589
push usercjones@mozilla.com
push dateFri, 23 Jul 2010 17:12:29 +0000
treeherdermozilla-central@49185ce20807 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdwitte
bugs579236
milestone2.0b3pre
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 579236: Fix shutdown crash and lesser bugs with remote pref observers. r=dwitte
dom/ipc/ContentChild.cpp
dom/ipc/ContentChild.h
--- a/dom/ipc/ContentChild.cpp
+++ b/dom/ipc/ContentChild.cpp
@@ -40,33 +40,146 @@
 #include "ContentChild.h"
 #include "TabChild.h"
 
 #include "mozilla/ipc/TestShellChild.h"
 #include "mozilla/net/NeckoChild.h"
 #include "mozilla/ipc/XPCShellEnvironment.h"
 #include "mozilla/jsipc/PContextWrapperChild.h"
 
+#include "nsIObserverService.h"
+#include "nsTObserverArray.h"
+#include "nsIObserver.h"
+#include "nsIPrefService.h"
+#include "nsIPrefBranch.h"
+#include "nsServiceManagerUtils.h"
 #include "nsXULAppAPI.h"
+#include "nsWeakReference.h"
 
 #include "base/message_loop.h"
 #include "base/task.h"
 
 #include "nsChromeRegistryContent.h"
 #include "mozilla/chrome/RegistryMessageUtils.h"
 
 using namespace mozilla::ipc;
 using namespace mozilla::net;
 
 namespace mozilla {
 namespace dom {
 
+class PrefObserver
+{
+public:
+    /**
+     * Pass |aHoldWeak=true| to force this to hold a weak ref to
+     * |aObserver|.  Otherwise, this holds a strong ref.
+     *
+     * XXX/cjones: what do domain and prefRoot mean?
+     */
+    PrefObserver(nsIObserver *aObserver, bool aHoldWeak,
+                 const nsCString& aPrefRoot, const nsCString& aDomain)
+        : mPrefRoot(aPrefRoot)
+        , mDomain(aDomain)
+    {
+        if (aHoldWeak) {
+            nsCOMPtr<nsISupportsWeakReference> supportsWeakRef = 
+                do_QueryInterface(aObserver);
+            if (supportsWeakRef)
+                mWeakObserver = do_GetWeakReference(aObserver);
+        } else {
+            mObserver = aObserver;
+        }
+    }
+
+    ~PrefObserver() {}
+
+    /**
+     * Return true if this observer can no longer receive
+     * notifications.
+     */
+    bool IsDead() const
+    {
+        nsCOMPtr<nsIObserver> observer = GetObserver();
+        return !!observer;
+    }
+
+    /**
+     * Return true iff a request to remove observers matching
+     * <aObserver, aDomain, aPrefRoot> entails removal of this.
+     */
+    bool ShouldRemoveFrom(nsIObserver* aObserver,
+                          const nsCString& aPrefRoot,
+                          const nsCString& aDomain) const
+    {
+        nsCOMPtr<nsIObserver> observer = GetObserver();
+        return (observer == aObserver &&
+                mDomain == aDomain && mPrefRoot == aPrefRoot);
+    }
+
+    /**
+     * Return true iff this should be notified of changes to |aPref|.
+     */
+    bool Observes(const nsCString& aPref) const
+    {
+        nsCAutoString myPref(mPrefRoot);
+        myPref += mDomain;
+        return StringBeginsWith(aPref, myPref);
+    }
+
+    /**
+     * Notify this of a pref change that's relevant to our interests
+     * (see Observes() above).  Return false iff this no longer cares
+     * to observe any more pref changes.
+     */
+    bool Notify() const
+    {
+        nsCOMPtr<nsIObserver> observer = GetObserver();
+        if (!observer) {
+            return false;
+        }
+
+        nsCOMPtr<nsIPrefBranch> prefBranch;
+        nsCOMPtr<nsIPrefService> prefService =
+            do_GetService(NS_PREFSERVICE_CONTRACTID);
+        if (prefService) {
+            prefService->GetBranch(mPrefRoot.get(), 
+                                   getter_AddRefs(prefBranch));
+            observer->Observe(prefBranch, "nsPref:changed",
+                              NS_ConvertASCIItoUTF16(mDomain).get());
+        }
+        return true;
+    }
+
+private:
+    already_AddRefed<nsIObserver> GetObserver() const
+    {
+        nsCOMPtr<nsIObserver> observer =
+            mObserver ? mObserver : do_QueryReferent(mWeakObserver);
+        return observer.forget();
+    }
+
+    // We only either hold a strong or a weak reference to the
+    // observer, so only either mObserver or
+    // GetReferent(mWeakObserver) is ever non-null.
+    nsCOMPtr<nsIObserver> mObserver;
+    nsWeakPtr mWeakObserver;
+    nsCString mPrefRoot;
+    nsCString mDomain;
+
+    // disable these
+    PrefObserver(const PrefObserver&);
+    PrefObserver& operator=(const PrefObserver&);
+};
+
+
 ContentChild* ContentChild::sSingleton;
 
 ContentChild::ContentChild()
+    : mDead(false)
 {
 }
 
 ContentChild::~ContentChild()
 {
 }
 
 bool
@@ -154,70 +267,81 @@ ContentChild::RecvSetOffline(const PRBoo
 }
 
 void
 ContentChild::ActorDestroy(ActorDestroyReason why)
 {
     if (AbnormalShutdown == why)
         NS_WARNING("shutting down because of crash!");
 
-    ClearPrefObservers();
+    // We might be holding the last ref to some of the observers in
+    // mPrefObserverArray.  Some of them try to unregister themselves
+    // in their dtors (sketchy).  To side-step uaf problems and so
+    // forth, we set this mDead flag.  Then, if during a Clear() a
+    // being-deleted observer tries to unregister itself, it hits the
+    // |if (mDead)| special case below and we're safe.
+    mDead = true;
+    mPrefObservers.Clear();
+
     XRE_ShutdownChildProcess();
 }
 
 nsresult
-ContentChild::AddRemotePrefObserver(const nsCString &aDomain, 
-                                    const nsCString &aPrefRoot, 
-                                    nsIObserver *aObserver, 
+ContentChild::AddRemotePrefObserver(const nsCString& aDomain, 
+                                    const nsCString& aPrefRoot, 
+                                    nsIObserver* aObserver, 
                                     PRBool aHoldWeak)
 {
-    nsPrefObserverStorage* newObserver = 
-        new nsPrefObserverStorage(aObserver, aDomain, aPrefRoot, aHoldWeak);
-
-    mPrefObserverArray.AppendElement(newObserver);
+    if (aObserver) {
+        mPrefObservers.AppendElement(
+            new PrefObserver(aObserver, aHoldWeak, aPrefRoot, aDomain));
+    }
     return NS_OK;
 }
 
 nsresult
-ContentChild::RemoveRemotePrefObserver(const nsCString &aDomain, 
-                                       const nsCString &aPrefRoot, 
-                                       nsIObserver *aObserver)
+ContentChild::RemoveRemotePrefObserver(const nsCString& aDomain, 
+                                       const nsCString& aPrefRoot, 
+                                       nsIObserver* aObserver)
 {
-    if (mPrefObserverArray.IsEmpty())
+    if (mDead) {
+        // Silently ignore, we're about to exit.  See comment in
+        // ActorDestroy().
         return NS_OK;
+    }
 
-    nsPrefObserverStorage *entry;
-    for (PRUint32 i = 0; i < mPrefObserverArray.Length(); ++i) {
-        entry = mPrefObserverArray[i];
-        if (entry && entry->GetObserver() == aObserver &&
-                     entry->GetDomain().Equals(aDomain)) {
-            // Remove this observer from our array
-            mPrefObserverArray.RemoveElementAt(i);
+    for (PRUint32 i = 0; i < mPrefObservers.Length();
+         /*we mutate the array during the loop; ++i iff no mutation*/) {
+        PrefObserver* observer = mPrefObservers[i];
+        if (observer->IsDead()) {
+            mPrefObservers.RemoveElementAt(i);
+            continue;
+        } else if (observer->ShouldRemoveFrom(aObserver, aPrefRoot, aDomain)) {
+            mPrefObservers.RemoveElementAt(i);
             return NS_OK;
         }
+        ++i;
     }
-    NS_WARNING("No preference Observer was matched !");
+
+    NS_WARNING("RemoveRemotePrefObserver(): no observer was matched!");
     return NS_ERROR_UNEXPECTED;
 }
 
 bool
-ContentChild::RecvNotifyRemotePrefObserver(const nsCString& aDomain)
+ContentChild::RecvNotifyRemotePrefObserver(const nsCString& aPref)
 {
-    nsPrefObserverStorage *entry;
-    for (PRUint32 i = 0; i < mPrefObserverArray.Length(); ) {
-        entry = mPrefObserverArray[i];
-        nsCAutoString prefName(entry->GetPrefRoot() + entry->GetDomain());
-        // aDomain here is returned from our global pref observer from parent
-        // so it's really a full pref name (i.e. root + domain)
-        if (StringBeginsWith(aDomain, prefName)) {
-            if (!entry->NotifyObserver()) {
-                // remove the observer from the list
-                mPrefObserverArray.RemoveElementAt(i);
-                continue;
-            }
+    for (PRUint32 i = 0; i < mPrefObservers.Length();
+         /*we mutate the array during the loop; ++i iff no mutation*/) {
+        PrefObserver* observer = mPrefObservers[i];
+        if (observer->Observes(aPref) &&
+            !observer->Notify()) {
+            // |observer| had a weak ref that went away, so it no
+            // longer cares about pref changes
+            mPrefObservers.RemoveElementAt(i);
+            continue;
         }
         ++i;
     }
     return true;
 }
 
 } // namespace dom
 } // namespace mozilla
--- a/dom/ipc/ContentChild.h
+++ b/dom/ipc/ContentChild.h
@@ -37,95 +37,34 @@
  *
  * ***** END LICENSE BLOCK ***** */
 
 #ifndef mozilla_dom_ContentChild_h
 #define mozilla_dom_ContentChild_h
 
 #include "mozilla/dom/PContentChild.h"
 
-#include "nsIObserverService.h"
-#include "nsTObserverArray.h"
-#include "nsIObserver.h"
-#include "nsIPrefService.h"
-#include "nsIPrefBranch.h"
-#include "nsServiceManagerUtils.h"
 #include "nsTArray.h"
-#include "nsAutoPtr.h"
-#include "nsWeakReference.h"
 
 struct ChromePackage;
+class nsIObserver;
 struct ResourceMapping;
 struct OverrideMapping;
 
 namespace mozilla {
 namespace dom {
 
+class PrefObserver;
+
 class ContentChild : public PContentChild
 {
 public:
     ContentChild();
     virtual ~ContentChild();
 
-    class nsPrefObserverStorage {
-    public:
-        nsPrefObserverStorage(nsIObserver *aObserver, nsCString aDomain,
-                              nsCString aPrefRoot, bool aHoldWeak) {
-            mDomain = aDomain;
-            mPrefRoot = aPrefRoot;
-            mObserver = aObserver;
-            if (aHoldWeak) {
-                nsCOMPtr<nsISupportsWeakReference> weakRefFactory = 
-                    do_QueryInterface(aObserver);
-                if (weakRefFactory)
-                    mWeakRef = do_GetWeakReference(aObserver);
-            } else {
-                mWeakRef = nsnull;
-            }
-        }
-
-        ~nsPrefObserverStorage() {
-        }
-
-        bool NotifyObserver() {
-            nsCOMPtr<nsIObserver> observer;
-            if (mWeakRef) {
-                observer = do_QueryReferent(mWeakRef);
-                if (!observer) {
-                    // this weak referenced observer went away, tell
-                    // the caller so he can remove the observer from the list
-                    return false;
-                }
-            } else {
-                observer = mObserver;
-            }
-
-            nsCOMPtr<nsIPrefBranch> prefBranch;
-            nsCOMPtr<nsIPrefService> prefService = 
-                                      do_GetService(NS_PREFSERVICE_CONTRACTID);
-            if (prefService) {
-                prefService->GetBranch(mPrefRoot.get(), 
-                                       getter_AddRefs(prefBranch));
-                observer->Observe(prefBranch, "nsPref:changed",
-                                  NS_ConvertASCIItoUTF16(mDomain).get());
-            }
-            return true;
-        }
-
-        nsIObserver* GetObserver() { return mObserver; }
-        const nsCString& GetDomain() { return mDomain; }
-        const nsCString& GetPrefRoot() { return mPrefRoot; }
-
-    private:
-        nsCOMPtr<nsIObserver> mObserver;
-        nsWeakPtr mWeakRef;
-        nsCString mPrefRoot;
-        nsCString mDomain;
-    };
-
     bool Init(MessageLoop* aIOLoop,
               base::ProcessHandle aParentHandle,
               IPC::Channel* aChannel);
 
     static ContentChild* GetSingleton() {
         NS_ASSERTION(sSingleton, "not initialized");
         return sSingleton;
     }
@@ -144,38 +83,38 @@ public:
     virtual bool DeallocPNecko(PNeckoChild*);
 
     virtual bool RecvRegisterChrome(const nsTArray<ChromePackage>& packages,
                                     const nsTArray<ResourceMapping>& resources,
                                     const nsTArray<OverrideMapping>& overrides);
 
     virtual bool RecvSetOffline(const PRBool& offline);
 
-    nsresult AddRemotePrefObserver(const nsCString &aDomain, 
-                                   const nsCString &aPrefRoot, 
-                                   nsIObserver *aObserver, PRBool aHoldWeak);
-    nsresult RemoveRemotePrefObserver(const nsCString &aDomain, 
-                                      const nsCString &aPrefRoot, 
-                                      nsIObserver *aObserver);
-    inline void ClearPrefObservers() {
-        mPrefObserverArray.Clear();
-    }
+    /**
+     * Notify |aObserver| of changes to |aPrefRoot|.|aDomain|.  If
+     * |aHoldWeak|, only a weak reference to |aObserver| is held.
+     */
+    nsresult AddRemotePrefObserver(const nsCString& aDomain, 
+                                   const nsCString& aPrefRoot, 
+                                   nsIObserver* aObserver, PRBool aHoldWeak);
+    nsresult RemoveRemotePrefObserver(const nsCString& aDomain, 
+                                      const nsCString& aPrefRoot, 
+                                      nsIObserver* aObserver);
 
     virtual bool RecvNotifyRemotePrefObserver(
             const nsCString& aDomain);
-    
-
 
 private:
     NS_OVERRIDE
     virtual void ActorDestroy(ActorDestroyReason why);
 
-    static ContentChild* sSingleton;
+    nsTArray<nsAutoPtr<PrefObserver> > mPrefObservers;
+    bool mDead;
 
-    nsTArray< nsAutoPtr<nsPrefObserverStorage> > mPrefObserverArray;
+    static ContentChild* sSingleton;
 
     DISALLOW_EVIL_CONSTRUCTORS(ContentChild);
 };
 
 } // namespace dom
 } // namespace mozilla
 
 #endif