Bug 600110 - Pref follow up patch. Remove dead code, fix observers. r=dwitte. a=blocking-fennec
authorDoug Turner <dougt@dougt.org>
Tue, 28 Sep 2010 10:02:37 -0700
changeset 54698 e79ce9228dee01088afc34cdd19c7f46a99a84c1
parent 54697 59f911418d495945c9ef794864cc9ddba4350dcb
child 54699 8492ccf470395dca880d25a062cd8bcfe5f87e86
push id1
push userroot
push dateMon, 20 Oct 2014 17:29:22 +0000
reviewersdwitte, blocking-fennec
bugs600110
milestone2.0b7pre
Bug 600110 - Pref follow up patch. Remove dead code, fix observers. r=dwitte. a=blocking-fennec
dom/ipc/ContentChild.cpp
dom/ipc/ContentChild.h
dom/ipc/ContentParent.cpp
dom/ipc/PContent.ipdl
modules/libpref/public/nsIPrefService.idl
modules/libpref/src/nsPrefService.cpp
modules/libpref/src/prefapi.cpp
modules/libpref/src/prefapi_private_data.h
--- a/dom/ipc/ContentChild.cpp
+++ b/dom/ipc/ContentChild.cpp
@@ -49,17 +49,16 @@
 #include "mozilla/ipc/XPCShellEnvironment.h"
 #include "mozilla/jsipc/PContextWrapperChild.h"
 #include "mozilla/dom/ExternalHelperAppChild.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 "History.h"
 #include "nsDocShellCID.h"
 #include "nsNetUtil.h"
 
@@ -73,121 +72,16 @@
 #include "nsIGeolocationProvider.h"
 
 using namespace mozilla::ipc;
 using namespace mozilla::net;
 using namespace mozilla::places;
 
 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&);
-};
-
 class AlertObserver
 {
 public:
 
     AlertObserver(nsIObserver *aObserver, const nsString& aData)
         : mData(aData)
         , mObserver(aObserver)
     {
@@ -217,17 +111,16 @@ private:
     nsCOMPtr<nsIObserver> mObserver;
     nsString mData;
 };
 
 
 ContentChild* ContentChild::sSingleton;
 
 ContentChild::ContentChild()
-    : mDead(false)
 {
 }
 
 ContentChild::~ContentChild()
 {
 }
 
 bool
@@ -354,24 +247,16 @@ ContentChild::ActorDestroy(ActorDestroyR
 
 #ifndef DEBUG
     // In release builds, there's no point in the content process
     // going through the full XPCOM shutdown path, because it doesn't
     // keep persistent state.
     QuickExit();
 #endif
 
-    // 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();
     mAlertObservers.Clear();
     XRE_ShutdownChildProcess();
 }
 
 void
 ContentChild::ProcessingError(Result what)
 {
     switch (what) {
@@ -394,88 +279,35 @@ ContentChild::ProcessingError(Result wha
 void
 ContentChild::QuickExit()
 {
     NS_WARNING("content process _exit()ing");
     _exit(0);
 }
 
 nsresult
-ContentChild::AddRemotePrefObserver(const nsCString& aDomain, 
-                                    const nsCString& aPrefRoot, 
-                                    nsIObserver* aObserver, 
-                                    PRBool aHoldWeak)
-{
-    if (aObserver) {
-        mPrefObservers.AppendElement(
-            new PrefObserver(aObserver, aHoldWeak, aPrefRoot, aDomain));
-    }
-    return NS_OK;
-}
-
-nsresult
-ContentChild::RemoveRemotePrefObserver(const nsCString& aDomain, 
-                                       const nsCString& aPrefRoot, 
-                                       nsIObserver* aObserver)
-{
-    if (mDead) {
-        // Silently ignore, we're about to exit.  See comment in
-        // ActorDestroy().
-        return NS_OK;
-    }
-
-    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("RemoveRemotePrefObserver(): no observer was matched!");
-    return NS_ERROR_UNEXPECTED;
-}
-
-nsresult
 ContentChild::AddRemoteAlertObserver(const nsString& aData,
                                      nsIObserver* aObserver)
 {
     NS_ASSERTION(aObserver, "Adding a null observer?");
     mAlertObservers.AppendElement(new AlertObserver(aObserver, aData));
     return NS_OK;
 }
 
 bool
-ContentChild::RecvNotifyRemotePrefObserver(const nsCString& aPref)
+ContentChild::RecvPreferenceUpdate(const nsCString& aPref)
 {
-    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;
-    }
+    nsCOMPtr<nsIPrefServiceInternal> prefs = do_GetService("@mozilla.org/preferences-service;1");
+    prefs->ReadPrefBuffer(aPref);
     return true;
 }
 
 bool
 ContentChild::RecvNotifyAlertsObserver(const nsCString& aType, const nsString& aData)
 {
-    printf("ContentChild::RecvNotifyAlertsObserver %s\n", aType.get() );
-
     for (PRUint32 i = 0; i < mAlertObservers.Length();
          /*we mutate the array during the loop; ++i iff no mutation*/) {
         AlertObserver* observer = mAlertObservers[i];
         if (observer->Observes(aData) && observer->Notify(aType)) {
             // if aType == alertfinished, this alert is done.  we can
             // remove the observer.
             if (aType.Equals(nsDependentCString("alertfinished"))) {
                 mAlertObservers.RemoveElementAt(i);
--- a/dom/ipc/ContentChild.h
+++ b/dom/ipc/ContentChild.h
@@ -93,32 +93,20 @@ public:
 
     virtual bool RecvRegisterChrome(const nsTArray<ChromePackage>& packages,
                                     const nsTArray<ResourceMapping>& resources,
                                     const nsTArray<OverrideMapping>& overrides);
 
     virtual bool RecvSetOffline(const PRBool& offline);
 
     virtual bool RecvNotifyVisited(const IPC::URI& aURI);
-
-    /**
-     * 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);
-
     // auto remove when alertfinished is received.
     nsresult AddRemoteAlertObserver(const nsString& aData, nsIObserver* aObserver);
 
-    virtual bool RecvNotifyRemotePrefObserver(const nsCString& aDomain);
+    virtual bool RecvPreferenceUpdate(const nsCString& aDomain);
     
     virtual bool RecvNotifyAlertsObserver(const nsCString& aType, const nsString& aData);
 
     virtual bool RecvAsyncMessage(const nsString& aMsg, const nsString& aJSON);
 
     virtual bool RecvGeolocationUpdate(const GeoPosition& somewhere);
 
 private:
--- a/dom/ipc/ContentParent.cpp
+++ b/dom/ipc/ContentParent.cpp
@@ -40,16 +40,17 @@
 #include "ContentParent.h"
 
 #include "TabParent.h"
 #include "History.h"
 #include "mozilla/ipc/TestShellParent.h"
 #include "mozilla/net/NeckoParent.h"
 #include "nsIPrefBranch.h"
 #include "nsIPrefBranch2.h"
+#include "nsIPrefService.h"
 #include "nsIPrefLocalizedString.h"
 #include "nsIObserverService.h"
 #include "nsContentUtils.h"
 #include "nsAutoPtr.h"
 #include "nsCOMPtr.h"
 #include "nsServiceManagerUtils.h"
 #include "nsThreadUtils.h"
 #include "nsChromeRegistryChrome.h"
@@ -266,17 +267,20 @@ ContentParent::Observe(nsISupports* aSub
 
     if (!mIsAlive || !mSubprocess)
         return NS_OK;
 
     // listening for remotePrefs...
     if (!strcmp(aTopic, "nsPref:changed")) {
         // We know prefs are ASCII here.
         NS_LossyConvertUTF16toASCII strData(aData);
-        if (!SendNotifyRemotePrefObserver(strData))
+        nsCString prefBuffer;
+        nsCOMPtr<nsIPrefServiceInternal> prefs = do_GetService("@mozilla.org/preferences-service;1");
+        prefs->SerializePreference(strData, prefBuffer);
+        if (!SendPreferenceUpdate(prefBuffer))
             return NS_ERROR_NOT_AVAILABLE;
     }
     else if (!strcmp(aTopic, NS_IPC_IOSERVICE_SET_OFFLINE_TOPIC)) {
       NS_ConvertUTF16toUTF8 dataStr(aData);
       const char *offline = dataStr.get();
       if (!SendSetOffline(!strcmp(offline, "true") ? true : false))
           return NS_ERROR_NOT_AVAILABLE;
     }
--- a/dom/ipc/PContent.ipdl
+++ b/dom/ipc/PContent.ipdl
@@ -70,17 +70,17 @@ child:
 
     RegisterChrome(ChromePackage[] packages, ResourceMapping[] resources,
                    OverrideMapping[] overrides);
 
     async SetOffline(PRBool offline);
 
     async NotifyVisited(URI uri);
 
-    NotifyRemotePrefObserver(nsCString aDomain);
+    PreferenceUpdate(nsCString pref);
 
     NotifyAlertsObserver(nsCString topic, nsString data);
 
     GeolocationUpdate(GeoPosition somewhere);
 
 parent:
     PNecko();
 
--- a/modules/libpref/public/nsIPrefService.idl
+++ b/modules/libpref/public/nsIPrefService.idl
@@ -165,16 +165,18 @@ interface nsIPrefServiceInternal : nsISu
    * @return NS_OK The file was read and processed.
    * @return Other The file failed to read or contained invalid data.
    *
    * @see readUserPrefs
    */
   void readExtensionPrefs(in nsILocalFile aFile);
 
   ACString serializePreferences();
+  ACString serializePreference(in ACString aPrefName);
+  void readPrefBuffer(in ACString aBuffer);
 };
 
 %{C++
 
 #define NS_PREFSERVICE_CID                             \
   { /* {1cd91b88-1dd2-11b2-92e1-ed22ed298000} */       \
     0x1cd91b88,                                        \
     0x1dd2,                                            \
--- a/modules/libpref/src/nsPrefService.cpp
+++ b/modules/libpref/src/nsPrefService.cpp
@@ -143,22 +143,17 @@ nsresult nsPrefService::Init()
 #ifdef MOZ_IPC
   using mozilla::dom::ContentChild;
   if (XRE_GetProcessType() == GeckoProcessType_Content) {
 
     ContentChild* cpc = ContentChild::GetSingleton();
     nsCAutoString prefs;
     cpc->SendReadPrefs(&prefs);
 
-    PrefParseState ps;
-    PREF_InitParseState(&ps, PREF_ReaderCallback, NULL);
-    nsresult rv = PREF_ParseBuf(&ps, prefs.get(), prefs.Length());
-    PREF_FinalizeParseState(&ps);
-
-    return rv;
+    return ReadPrefBuffer(prefs);
   }
 #endif
 
   nsXPIDLCString lockFileName;
   /*
    * The following is a small hack which will allow us to only load the library
    * which supports the netscape.cfg file if the preference is defined. We
    * test for the existence of the pref, set in the all.js (mozilla) or
@@ -354,16 +349,44 @@ NS_IMETHODIMP nsPrefService::SerializePr
       NS_Free(*walker);
     }
   }
   PR_Free(valueArray);
 
   return NS_OK;
 }
 
+NS_IMETHODIMP nsPrefService::SerializePreference(const nsACString& aPrefName, nsACString& aBuffer)
+{
+  PrefHashEntry *pref = pref_HashTableLookup(nsDependentCString(aPrefName).get());
+  if (!pref)
+    return NS_ERROR_NOT_AVAILABLE;
+
+  char* prefArray = nsnull;
+
+  pref_saveArgs saveArgs;
+  saveArgs.prefArray = &prefArray;
+  saveArgs.saveTypes = SAVE_ALL_AND_DEFAULTS;
+
+  pref_savePref(&gHashTable, pref, 0, &saveArgs);
+  aBuffer = prefArray;
+  PR_Free(prefArray);
+
+  return NS_OK;
+}
+
+NS_IMETHODIMP nsPrefService::ReadPrefBuffer(const nsACString& aBuffer)
+{
+  PrefParseState ps;
+  PREF_InitParseState(&ps, PREF_ReaderCallback, NULL);
+  nsresult rv = PREF_ParseBuf(&ps, nsDependentCString(aBuffer).get(), aBuffer.Length());
+  PREF_FinalizeParseState(&ps);
+  return rv;
+}
+
 NS_IMETHODIMP nsPrefService::GetBranch(const char *aPrefRoot, nsIPrefBranch **_retval)
 {
   nsresult rv;
 
   if ((nsnull != aPrefRoot) && (*aPrefRoot != '\0')) {
     // TODO: - cache this stuff and allow consumers to share branches (hold weak references I think)
     nsPrefBranch* prefBranch = new nsPrefBranch(aPrefRoot, PR_FALSE);
     if (!prefBranch)
--- a/modules/libpref/src/prefapi.cpp
+++ b/modules/libpref/src/prefapi.cpp
@@ -179,17 +179,16 @@ struct CallbackNode {
     struct CallbackNode*    next;
 };
 
 /* -- Prototypes */
 static nsresult pref_DoCallback(const char* changed_pref);
 
 
 static nsresult pref_HashPref(const char *key, PrefValue value, PrefType type, PRBool defaultPref);
-static inline PrefHashEntry* pref_HashTableLookup(const void *key);
 
 #define PREF_HASHTABLE_INITIAL_SIZE	2048
 
 nsresult PREF_Init()
 {
     if (!gHashTable.ops) {
         if (!PL_DHashTableInit(&gHashTable, &pref_HashTableOps, nsnull,
                                sizeof(PrefHashEntry),
@@ -382,17 +381,16 @@ pref_CompareStrings(const void *v1, cons
             return -1;
     }
     else if (!s2)
         return 1;
     else
         return strcmp(s1, s2);
 }
 
-
 PRBool PREF_HasUserPref(const char *pref_name)
 {
     if (!gHashTable.ops)
         return PR_FALSE;
 
     PrefHashEntry *pref = pref_HashTableLookup(pref_name);
     if (!pref) return PR_FALSE;
 
@@ -673,17 +671,17 @@ static void pref_SetValue(PrefValue* old
             break;
 
         default:
             *oldValue = newValue;
     }
     gDirty = PR_TRUE;
 }
 
-static inline PrefHashEntry* pref_HashTableLookup(const void *key)
+PrefHashEntry* pref_HashTableLookup(const void *key)
 {
     PrefHashEntry* result =
         static_cast<PrefHashEntry*>(PL_DHashTableOperate(&gHashTable, key, PL_DHASH_LOOKUP));
 
     if (PL_DHASH_ENTRY_IS_FREE(result))
         return nsnull;
 
     return result;
--- a/modules/libpref/src/prefapi_private_data.h
+++ b/modules/libpref/src/prefapi_private_data.h
@@ -47,8 +47,9 @@ struct pref_saveArgs {
   char **prefArray;
   pref_SaveTypes saveTypes;
 };
 
 PLDHashOperator
 pref_savePref(PLDHashTable *table, PLDHashEntryHdr *heh, PRUint32 i, void *arg);
 
 int pref_CompareStrings(const void *v1, const void *v2, void* unused);
+PrefHashEntry* pref_HashTableLookup(const void *key);