Bug 1472523: Part 4 - Avoid unnecessary domain string duplication in preference observers. r?njn draft
authorKris Maglione <maglione.k@gmail.com>
Sun, 01 Jul 2018 02:24:10 -0700
changeset 813934 e6635b5c825da641c62432873cfa2361bfb3be50
parent 813933 b3cb0ba103fc239e42459e77cd389db0b5ecde18
child 814231 b8d27b6b40213e21ab36b387c4eff0af4591ee1d
push id115049
push usermaglione.k@gmail.com
push dateWed, 04 Jul 2018 05:52:30 +0000
reviewersnjn
bugs1472523
milestone63.0a1
Bug 1472523: Part 4 - Avoid unnecessary domain string duplication in preference observers. r?njn MozReview-Commit-ID: EMCgMRTDqDn
modules/libpref/Preferences.cpp
modules/libpref/Preferences.h
modules/libpref/nsIPrefBranch.idl
netwerk/cache/nsCacheService.cpp
toolkit/components/extensions/WebExtensionPolicy.cpp
toolkit/components/reputationservice/LoginReputation.cpp
toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
widget/android/PrefsHelper.h
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -78,16 +78,20 @@
 #include "nsXPCOM.h"
 #include "nsXULAppAPI.h"
 #include "nsZipArchive.h"
 #include "plbase64.h"
 #include "PLDHashTable.h"
 #include "plstr.h"
 #include "prlink.h"
 
+#ifdef MOZ_MEMORY
+#include "mozmemory.h"
+#endif
+
 #ifdef XP_WIN
 #include "windows.h"
 #endif
 
 using namespace mozilla;
 
 #ifdef DEBUG
 
@@ -1681,17 +1685,22 @@ private:
                                      const nsAString& aValue);
   nsresult CheckSanityOfStringLength(const char* aPrefName,
                                      const nsACString& aValue);
   nsresult CheckSanityOfStringLength(const char* aPrefName,
                                      const uint32_t aLength);
 
   void RemoveExpiredCallback(PrefCallback* aCallback);
 
-  PrefName GetPrefName(const char* aPrefName) const;
+  PrefName GetPrefName(const char* aPrefName) const
+  {
+    return GetPrefName(nsDependentCString(aPrefName));
+  }
+
+  PrefName GetPrefName(const nsACString& aPrefName) const;
 
   void FreeObserverList(void);
 
   const nsCString mPrefRoot;
   PrefValueKind mKind;
 
   bool mFreeingObserverList;
   nsClassHashtable<PrefCallback, PrefCallback> mObservers;
@@ -2375,23 +2384,22 @@ nsPrefBranch::GetChildList(const char* a
     *aChildArray = outArray;
   }
   *aCount = numPrefs;
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
-nsPrefBranch::AddObserver(const char* aDomain,
-                          nsIObserver* aObserver,
-                          bool aHoldWeak)
+nsPrefBranch::AddObserverImpl(const nsACString& aDomain,
+                              nsIObserver* aObserver,
+                              bool aHoldWeak)
 {
   PrefCallback* pCallback;
 
-  NS_ENSURE_ARG(aDomain);
   NS_ENSURE_ARG(aObserver);
 
   nsCString prefName;
   GetPrefName(aDomain).get(prefName);
 
   // Hold a weak reference to the observer if so requested.
   if (aHoldWeak) {
     nsCOMPtr<nsISupportsWeakReference> weakRefFactory =
@@ -2427,19 +2435,19 @@ nsPrefBranch::AddObserver(const char* aD
                                 pCallback,
                                 Preferences::PrefixMatch,
                                 /* isPriority */ false);
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
-nsPrefBranch::RemoveObserver(const char* aDomain, nsIObserver* aObserver)
+nsPrefBranch::RemoveObserverImpl(const nsACString& aDomain,
+                                 nsIObserver* aObserver)
 {
-  NS_ENSURE_ARG(aDomain);
   NS_ENSURE_ARG(aObserver);
 
   nsresult rv = NS_OK;
 
   // 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.
   //
@@ -2568,26 +2576,25 @@ nsPrefBranch::GetDefaultFromPropertiesFi
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   return bundle->GetStringFromName(aPrefName, aReturn);
 }
 
 nsPrefBranch::PrefName
-nsPrefBranch::GetPrefName(const char* aPrefName) const
+nsPrefBranch::GetPrefName(const nsACString& aPrefName) const
 {
   MOZ_ASSERT(aPrefName);
 
-  // For speed, avoid strcpy if we can.
   if (mPrefRoot.IsEmpty()) {
-    return PrefName(aPrefName);
-  }
-
-  return PrefName(mPrefRoot + nsDependentCString(aPrefName));
+    return PrefName(PromiseFlatCString(aPrefName));
+  }
+
+  return PrefName(mPrefRoot + aPrefName);
 }
 
 //----------------------------------------------------------------------------
 // nsPrefLocalizedString
 //----------------------------------------------------------------------------
 
 nsPrefLocalizedString::nsPrefLocalizedString() = default;
 
@@ -4577,77 +4584,96 @@ Preferences::GetType(const char* aPrefNa
       return PREF_BOOL;
 
     default:
       MOZ_CRASH();
   }
 }
 
 /* static */ nsresult
-Preferences::AddStrongObserver(nsIObserver* aObserver, const char* aPref)
+Preferences::AddStrongObserver(nsIObserver* aObserver, const nsACString& aPref)
 {
   MOZ_ASSERT(aObserver);
   NS_ENSURE_TRUE(InitStaticMembers(), NS_ERROR_NOT_AVAILABLE);
   return sPreferences->mRootBranch->AddObserver(aPref, aObserver, false);
 }
 
 /* static */ nsresult
-Preferences::AddWeakObserver(nsIObserver* aObserver, const char* aPref)
+Preferences::AddWeakObserver(nsIObserver* aObserver, const nsACString& aPref)
 {
   MOZ_ASSERT(aObserver);
   NS_ENSURE_TRUE(InitStaticMembers(), NS_ERROR_NOT_AVAILABLE);
   return sPreferences->mRootBranch->AddObserver(aPref, aObserver, true);
 }
 
 /* static */ nsresult
-Preferences::RemoveObserver(nsIObserver* aObserver, const char* aPref)
+Preferences::RemoveObserver(nsIObserver* aObserver, const nsACString& aPref)
 {
   MOZ_ASSERT(aObserver);
   if (sShutdown) {
     MOZ_ASSERT(!sPreferences);
     return NS_OK; // Observers have been released automatically.
   }
   NS_ENSURE_TRUE(sPreferences, NS_ERROR_NOT_AVAILABLE);
   return sPreferences->mRootBranch->RemoveObserver(aPref, aObserver);
 }
 
+template<typename T>
+static void
+AssertNotMallocAllocated(T* aPtr)
+{
+#if defined(DEBUG) && defined(MOZ_MEMORY)
+  jemalloc_ptr_info_t info;
+  jemalloc_ptr_info((void*)aPtr, &info);
+  MOZ_ASSERT(info.tag == TagUnknown);
+#endif
+}
+
 /* static */ nsresult
 Preferences::AddStrongObservers(nsIObserver* aObserver, const char** aPrefs)
 {
   MOZ_ASSERT(aObserver);
   for (uint32_t i = 0; aPrefs[i]; i++) {
-    nsresult rv = AddStrongObserver(aObserver, aPrefs[i]);
+    AssertNotMallocAllocated(aPrefs[i]);
+
+    nsCString pref;
+    pref.AssignLiteral(aPrefs[i], strlen(aPrefs[i]));
+    nsresult rv = AddStrongObserver(aObserver, pref);
     NS_ENSURE_SUCCESS(rv, rv);
   }
   return NS_OK;
 }
 
 /* static */ nsresult
 Preferences::AddWeakObservers(nsIObserver* aObserver, const char** aPrefs)
 {
   MOZ_ASSERT(aObserver);
   for (uint32_t i = 0; aPrefs[i]; i++) {
-    nsresult rv = AddWeakObserver(aObserver, aPrefs[i]);
+    AssertNotMallocAllocated(aPrefs[i]);
+
+    nsCString pref;
+    pref.AssignLiteral(aPrefs[i], strlen(aPrefs[i]));
+    nsresult rv = AddWeakObserver(aObserver, pref);
     NS_ENSURE_SUCCESS(rv, rv);
   }
   return NS_OK;
 }
 
 /* static */ nsresult
 Preferences::RemoveObservers(nsIObserver* aObserver, const char** aPrefs)
 {
   MOZ_ASSERT(aObserver);
   if (sShutdown) {
     MOZ_ASSERT(!sPreferences);
     return NS_OK; // Observers have been released automatically.
   }
   NS_ENSURE_TRUE(sPreferences, NS_ERROR_NOT_AVAILABLE);
 
   for (uint32_t i = 0; aPrefs[i]; i++) {
-    nsresult rv = RemoveObserver(aObserver, aPrefs[i]);
+    nsresult rv = RemoveObserver(aObserver, nsDependentCString(aPrefs[i]));
     NS_ENSURE_SUCCESS(rv, rv);
   }
   return NS_OK;
 }
 
 /* static */ nsresult
 Preferences::RegisterCallback(PrefChangedFunc aCallback,
                               const nsACString& aPrefNode,
--- a/modules/libpref/Preferences.h
+++ b/modules/libpref/Preferences.h
@@ -232,22 +232,45 @@ public:
   // Clears user set pref. Fails if run outside the parent process.
   static nsresult ClearUser(const char* aPrefName);
 
   // Whether the pref has a user value or not.
   static bool HasUserValue(const char* aPref);
 
   // Adds/Removes the observer for the root pref branch. See nsIPrefBranch.idl
   // for details.
-  static nsresult AddStrongObserver(nsIObserver* aObserver, const char* aPref);
-  static nsresult AddWeakObserver(nsIObserver* aObserver, const char* aPref);
-  static nsresult RemoveObserver(nsIObserver* aObserver, const char* aPref);
+  static nsresult AddStrongObserver(nsIObserver* aObserver,
+                                    const nsACString& aPref);
+  static nsresult AddWeakObserver(nsIObserver* aObserver,
+                                  const nsACString& aPref);
+  static nsresult RemoveObserver(nsIObserver* aObserver,
+                                 const nsACString& aPref);
+
+  template<int N>
+  static nsresult AddStrongObserver(nsIObserver* aObserver,
+                                    const char (&aPref)[N])
+  {
+    return AddStrongObserver(aObserver, nsLiteralCString(aPref));
+  }
+  template<int N>
+  static nsresult AddWeakObserver(nsIObserver* aObserver,
+                                  const char (&aPref)[N])
+  {
+    return AddWeakObserver(aObserver, nsLiteralCString(aPref));
+  }
+  template<int N>
+  static nsresult RemoveObserver(nsIObserver* aObserver, const char (&aPref)[N])
+  {
+    return RemoveObserver(aObserver, nsLiteralCString(aPref));
+  }
 
   // Adds/Removes two or more observers for the root pref branch. Pass to
   // aPrefs an array of const char* whose last item is nullptr.
+  // Note: All preference strings *must* be statically-allocated string
+  // literals.
   static nsresult AddStrongObservers(nsIObserver* aObserver,
                                      const char** aPrefs);
   static nsresult AddWeakObservers(nsIObserver* aObserver, const char** aPrefs);
   static nsresult RemoveObservers(nsIObserver* aObserver, const char** aPrefs);
 
   // Registers/Unregisters the callback function for the aPref.
   static nsresult RegisterCallback(PrefChangedFunc aCallback,
                                    const nsACString& aPref,
--- a/modules/libpref/nsIPrefBranch.idl
+++ b/modules/libpref/nsIPrefBranch.idl
@@ -1,15 +1,19 @@
 /* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "nsISupports.idl"
 
+%{C++
+#include "nsLiteralString.h"
+%}
+
 interface nsIObserver;
 
 /**
  * The nsIPrefBranch interface is used to manipulate the preferences data. This
  * object may be obtained from the preferences service (nsIPrefService) and
  * used to get and set default and/or user preferences across the application.
  *
  * This object is created with a "root" value which describes the base point in
@@ -18,17 +22,17 @@ interface nsIObserver;
  * For example, if this object is created with the root "browser.startup.",
  * the preferences "browser.startup.page", "browser.startup.homepage",
  * and "browser.startup.homepage_override" can be accessed by simply passing
  * "page", "homepage", or "homepage_override" to the various Get/Set methods.
  *
  * @see nsIPrefService
  */
 
-[scriptable, uuid(55d25e49-793f-4727-a69f-de8b15f4b985)]
+[scriptable, builtinclass, uuid(55d25e49-793f-4727-a69f-de8b15f4b985)]
 interface nsIPrefBranch : nsISupports
 {
 
   /**
    * Values describing the basic preference types.
    *
    * @see getPrefType
    */
@@ -416,34 +420,62 @@ interface nsIPrefBranch : nsISupports
    * @note
    * It is not safe to change observers during this callback in Gecko 
    * releases before 1.9. If you want a safe way to remove a pref observer,
    * please use an nsITimer.
    *
    * @see nsIObserver
    * @see removeObserver
    */
-  void addObserver(in string aDomain, in nsIObserver aObserver,
+  [binaryname(AddObserverImpl)]
+  void addObserver(in ACString aDomain, in nsIObserver aObserver,
                    [optional] in boolean aHoldWeak);
 
   /**
    * Remove a preference change observer.
    *
    * @param aDomain   The preference which is being observed for changes.
    * @param aObserver An observer previously registered with addObserver().
    *
    * @note
    * Note that you must call removeObserver() on the same nsIPrefBranch
    * instance on which you called addObserver() in order to remove aObserver;
    * otherwise, the observer will not be removed.
    *
    * @see nsIObserver
    * @see addObserver
    */
-  void removeObserver(in string aDomain, in nsIObserver aObserver);
+  [binaryname(RemoveObserverImpl)]
+  void removeObserver(in ACString aDomain, in nsIObserver aObserver);
+
+  %{C++
+  nsresult AddObserver(const nsACString& aDomain, nsIObserver* aObserver,
+                       bool aHoldWeak = false)
+  {
+    return AddObserverImpl(aDomain, aObserver, aHoldWeak);
+  }
+
+  template <int N>
+  nsresult AddObserver(const char (&aDomain)[N], nsIObserver* aObserver,
+                       bool aHoldWeak = false)
+  {
+    return AddObserverImpl(nsLiteralCString(aDomain), aObserver, aHoldWeak);
+  }
+
+  nsresult RemoveObserver(const nsACString& aDomain, nsIObserver* aObserver)
+  {
+    return RemoveObserverImpl(aDomain, aObserver);
+  }
+
+  template <int N>
+  nsresult RemoveObserver(const char (&aDomain)[N], nsIObserver* aObserver)
+  {
+    return RemoveObserverImpl(nsLiteralCString(aDomain), aObserver);
+  }
+  %}
 };
 
 
 %{C++
 
 #define NS_PREFBRANCH_CONTRACTID "@mozilla.org/preferencesbranch;1"
 /**
  * Notification sent when a preference changes.
--- a/netwerk/cache/nsCacheService.cpp
+++ b/netwerk/cache/nsCacheService.cpp
@@ -334,17 +334,19 @@ nsCacheProfilePrefObserver::Install()
             rv2 = rv;
     }
 
     // install preferences observer
     nsCOMPtr<nsIPrefBranch> branch = do_GetService(NS_PREFSERVICE_CONTRACTID);
     if (!branch) return NS_ERROR_FAILURE;
 
     for (auto& pref : prefList) {
-        rv = branch->AddObserver(pref, this, false);
+        nsCString prefStr;
+        prefStr.AssignLiteral(pref, strlen(pref));
+        rv = branch->AddObserver(prefStr, this, false);
         if (NS_FAILED(rv))
             rv2 = rv;
     }
 
     // Determine if we have a profile already
     //     Install() is called *after* the profile-after-change notification
     //     when there is only a single profile, or it is specified on the
     //     commandline at startup.
@@ -377,17 +379,17 @@ nsCacheProfilePrefObserver::Remove()
     }
 
     // remove Pref Service observers
     nsCOMPtr<nsIPrefBranch> prefs =
         do_GetService(NS_PREFSERVICE_CONTRACTID);
     if (!prefs)
         return;
     for (auto& pref : prefList)
-        prefs->RemoveObserver(pref, this); // remove cache pref observers
+        prefs->RemoveObserver(nsDependentCString(pref), this); // remove cache pref observers
 }
 
 void
 nsCacheProfilePrefObserver::SetDiskCacheCapacity(int32_t capacity)
 {
     mDiskCacheCapacity = std::max(0, capacity);
 }
 
--- a/toolkit/components/extensions/WebExtensionPolicy.cpp
+++ b/toolkit/components/extensions/WebExtensionPolicy.cpp
@@ -277,19 +277,19 @@ namespace {
   class AtomSetPref : public nsIObserver
                     , public nsSupportsWeakReference
   {
   public:
     NS_DECL_ISUPPORTS
     NS_DECL_NSIOBSERVER
 
     static already_AddRefed<AtomSetPref>
-    Create(const char* aPref)
+    Create(const nsCString& aPref)
     {
-      RefPtr<AtomSetPref> self = new AtomSetPref(aPref);
+      RefPtr<AtomSetPref> self = new AtomSetPref(aPref.get());
       Preferences::AddWeakObserver(self, aPref);
       return self.forget();
     }
 
     const AtomSet& Get() const;
 
     bool Contains(const nsAtom* aAtom) const
     {
@@ -349,17 +349,17 @@ WebExtensionPolicy::IsRestrictedDoc(cons
   return IsRestrictedURI(aDoc.PrincipalURL());
 }
 
 /* static */ bool
 WebExtensionPolicy::IsRestrictedURI(const URLInfo &aURI)
 {
   static RefPtr<AtomSetPref> domains;
   if (!domains) {
-    domains = AtomSetPref::Create(kRestrictedDomainPref);
+    domains = AtomSetPref::Create(nsLiteralCString(kRestrictedDomainPref));
     ClearOnShutdown(&domains);
   }
 
   if (domains->Contains(aURI.HostAtom())) {
     return true;
   }
 
   if (AddonManagerWebAPI::IsValidSite(aURI.URI())) {
--- a/toolkit/components/reputationservice/LoginReputation.cpp
+++ b/toolkit/components/reputationservice/LoginReputation.cpp
@@ -25,16 +25,17 @@ static bool sPasswordProtectionEnabled =
 LazyLogModule gLoginReputationLogModule("LoginReputation");
 #define LR_LOG(args) MOZ_LOG(gLoginReputationLogModule, mozilla::LogLevel::Debug, args)
 #define LR_LOG_ENABLED() MOZ_LOG_TEST(gLoginReputationLogModule, mozilla::LogLevel::Debug)
 
 static Atomic<bool> gShuttingDown(false);
 
 static const char* kObservedPrefs[] = {
   PREF_PASSWORD_ALLOW_TABLE,
+  nullptr,
 };
 
 // -------------------------------------------------------------------------
 // ReputationQueryParam
 //
 // Concrete class for nsILoginReputationQuery to hold query parameters
 //
 class ReputationQueryParam final : public nsILoginReputationQuery
@@ -268,19 +269,17 @@ LoginReputationService::Enable()
   MOZ_ASSERT(XRE_IsParentProcess());
   MOZ_ASSERT(sPasswordProtectionEnabled);
 
   LR_LOG(("Enable login reputation service"));
 
   nsresult rv = mLoginWhitelist->Init();
   Unused << NS_WARN_IF(NS_FAILED(rv));
 
-  for (const char* pref : kObservedPrefs) {
-    Preferences::AddStrongObserver(this, pref);
-  }
+  Preferences::AddStrongObservers(this, kObservedPrefs);
 
   return NS_OK;
 }
 
 nsresult
 LoginReputationService::Disable()
 {
   MOZ_ASSERT(XRE_IsParentProcess());
@@ -289,19 +288,17 @@ LoginReputationService::Disable()
 
   nsresult rv = mLoginWhitelist->Uninit();
   Unused << NS_WARN_IF(NS_FAILED(rv));
 
   mQueryRequests.Clear();
 
   nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
   if (prefs) {
-    for (const char* pref : kObservedPrefs) {
-      prefs->RemoveObserver(pref, this);
-    }
+    Preferences::RemoveObservers(this, kObservedPrefs);
   }
 
   return NS_OK;
 }
 
 nsresult
 LoginReputationService::Shutdown()
 {
--- a/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
+++ b/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ -1668,17 +1668,17 @@ nsUrlClassifierDBService::Init()
   // XXX: Do we *really* need to be able to change all of these at runtime?
   // Note: These observers should only be added when everything else above has
   //       succeeded. Failing to do so can cause long shutdown times in certain
   //       situations. See Bug 1247798 and Bug 1244803.
   Preferences::AddUintVarCache(&sGethashNoise, GETHASH_NOISE_PREF,
     GETHASH_NOISE_DEFAULT);
 
   for (uint8_t i = 0; i < kObservedPrefs.Length(); i++) {
-    Preferences::AddStrongObserver(this, kObservedPrefs[i].get());
+    Preferences::AddStrongObserver(this, kObservedPrefs[i]);
   }
 
   return NS_OK;
 }
 
 // nsChannelClassifier is the only consumer of this interface.
 NS_IMETHODIMP
 nsUrlClassifierDBService::Classify(nsIPrincipal* aPrincipal,
@@ -2491,17 +2491,17 @@ nsUrlClassifierDBService::Shutdown()
 
   Telemetry::AutoTimer<Telemetry::URLCLASSIFIER_SHUTDOWN_TIME> timer;
 
   mCompleters.Clear();
 
   nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
   if (prefs) {
     for (uint8_t i = 0; i < kObservedPrefs.Length(); i++) {
-      prefs->RemoveObserver(kObservedPrefs[i].get(), this);
+      prefs->RemoveObserver(kObservedPrefs[i], this);
     }
   }
 
   // 1. Synchronize with worker thread and update thread by
   //    *synchronously* dispatching an event to worker thread
   //    for shutting down the update thread. The reason not
   //    shutting down update thread directly from main thread
   //    is to avoid racing for Classifier::mUpdateThread
--- a/widget/android/PrefsHelper.h
+++ b/widget/android/PrefsHelper.h
@@ -259,32 +259,32 @@ public:
         nsTArray<jni::Object::LocalRef> nameRefArray(
                 aPrefsToObserve->GetElements());
         nsAppShell* const appShell = nsAppShell::Get();
         MOZ_ASSERT(appShell);
 
         for (jni::Object::LocalRef& nameRef : nameRefArray) {
             jni::String::LocalRef nameStr(std::move(nameRef));
             MOZ_ALWAYS_SUCCEEDS(Preferences::AddStrongObserver(
-                    appShell, nameStr->ToCString().get()));
+                    appShell, nameStr->ToCString()));
         }
     }
 
     static void RemoveObserver(const jni::Class::LocalRef& aCls,
                                jni::ObjectArray::Param aPrefsToUnobserve)
     {
         nsTArray<jni::Object::LocalRef> nameRefArray(
                 aPrefsToUnobserve->GetElements());
         nsAppShell* const appShell = nsAppShell::Get();
         MOZ_ASSERT(appShell);
 
         for (jni::Object::LocalRef& nameRef : nameRefArray) {
             jni::String::LocalRef nameStr(std::move(nameRef));
             MOZ_ALWAYS_SUCCEEDS(Preferences::RemoveObserver(
-                    appShell, nameStr->ToCString().get()));
+                    appShell, nameStr->ToCString()));
         }
     }
 
     static void OnPrefChange(const char16_t* aData)
     {
         const nsCString& name = NS_LossyConvertUTF16toASCII(aData);
 
         int32_t type = -1;