Bug 1413811 (part 1) - Avoid PrefCallback for pref callbacks registered with Preferences::RegisterCallback(). r=glandium.
authorNicholas Nethercote <nnethercote@mozilla.com>
Thu, 02 Nov 2017 17:11:51 +1100
changeset 443513 b7ade96f752be032b1de492c2520c68d242d04ec
parent 443512 179dae92e4d794e7f45ad080ff01908c80691f31
child 443514 044253938e1c21d20026e88be2ff9a4f4f3beb27
push id1618
push userCallek@gmail.com
push dateThu, 11 Jan 2018 17:45:48 +0000
treeherdermozilla-release@882ca853e05a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
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 1413811 (part 1) - Avoid PrefCallback for pref callbacks registered with Preferences::RegisterCallback(). r=glandium. Pref callbacks registered via Preferences::Add*VarCache() are wrapped in a ValueObserver -- which groups all callback requests that have the same prefname and callback function -- and the ValueObserver is put into gObserverTable. This is reasonable. Observers registered via nsPrefBranch::Add{Weak,Strong}Observer() are wrapped in a PrefCallback, and the PrefCallback is put into sRootBranch->mObservers. This is also reasonable. Pref callbacks registered via Preferences::RegisterCallback() are conceptually similar to those registered via Preferences::Add*VarCache(). However, they are implemented by using *both* of the above mechanisms: they are wrapped in a ValueObserver which is put into gObserverTable, *and* that ValueObserver is then wrapped in a PrefCallback which is put into sRootBranch->mObservers. Using both mechanisms isn't necessary, so this patch removes the PrefCallback/mObservers part. This makes Preferences::RegisterCallback() work in much the same way as Preferences::Add*VarCache(). Specifically: - Preferences::RegisterCallback() now calls PREF_RegisterCallback() instead of Preferences::AddStrongObserver(). This makes it more similar to RegisterPriorityCallback(). - Preferences::UnregisterCallback() now explicitly calls PREF_UnregisterCallback() instead of Preferences::RemoveObserver (which previously happened via ~ValueObserver() when the ValueObserver was removed from gObserverTable and its refcount dropped to zerod). MozReview-Commit-ID: 1tEQNeYrBUU
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -3258,17 +3258,17 @@ public:
   PrefChangedFunc mCallback;
   Preferences::MatchKind mMatchKind;
 class ValueObserver final
   : public nsIObserver
   , public ValueObserverHashKey
-  ~ValueObserver() { Preferences::RemoveObserver(this, mPrefName.get()); }
+  ~ValueObserver() = default;
   ValueObserver(const char* aPref,
                 PrefChangedFunc aCallback,
                 Preferences::MatchKind aMatchKind)
@@ -4988,19 +4988,20 @@ Preferences::RegisterCallback(PrefChange
   gObserverTable->Get(&hashKey, getter_AddRefs(observer));
   if (observer) {
     return NS_OK;
   observer = new ValueObserver(aPref, aCallback, aMatchKind);
-  nsresult rv = AddStrongObserver(observer, aPref);
+  PREF_RegisterCallback(aPref,
+                        NotifyObserver,
+                        static_cast<nsIObserver*>(observer),
+                        /* isPriority */ false);
   gObserverTable->Put(observer, observer);
   return NS_OK;
 /* static */ nsresult
 Preferences::RegisterCallbackAndCall(PrefChangedFunc aCallback,
                                      const char* aPref,
                                      void* aClosure,
@@ -5032,16 +5033,19 @@ Preferences::UnregisterCallback(PrefChan
   gObserverTable->Get(&hashKey, getter_AddRefs(observer));
   if (!observer) {
     return NS_OK;
   if (observer->HasNoClosures()) {
     // Delete the callback since its list of closures is empty.
+      PREF_UnregisterCallback(aPref, NotifyObserver, observer));
   return NS_OK;
 // We insert cache observers using RegisterPriorityCallback to ensure they are
 // called prior to ordinary pref observers. Doing this ensures that ordinary
 // observers will never get stale values from cache variables.