Bug 1413400 (part 2) - Make Preferences::sPreferences a StaticRefPtr. r=froydnj. draft
authorNicholas Nethercote <nnethercote@mozilla.com>
Wed, 01 Nov 2017 13:55:28 +1100
changeset 690052 89466a4b32db124fbe8db3ae29e7258afb0eac96
parent 690051 113caa173429fa38aa27c78ded69e2b33011824d
child 738467 8c78912b5cc7901bfd910de1a3593433a0f24a9d
push id87187
push usernnethercote@mozilla.com
push dateWed, 01 Nov 2017 11:43:21 +0000
reviewersfroydnj
bugs1413400
milestone58.0a1
Bug 1413400 (part 2) - Make Preferences::sPreferences a StaticRefPtr. r=froydnj. The notable part of this change is Shutdown(). I've made it just null out sPreferences, contrary to the old comment, which was strange for a couple of reasons: - ~Preferences() used to null out sPreference, which is backwards compare to how this sort of thing normally works. - In both the before and after cases, as far as I can tell, Preferences::Shutdown() is called but ~Preferences() is never called; something keeps the singleton Preferences instance alive until process termination. MozReview-Commit-ID: Ab0ui31rVcI
modules/libpref/Preferences.cpp
modules/libpref/Preferences.h
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -3163,17 +3163,17 @@ Preferences::HandleDirty()
     sPreferences->mDirty = true;
 
     if (sPreferences->mCurrentFile && sPreferences->AllowOffMainThreadSave() &&
         !sPreferences->mSavePending) {
       sPreferences->mSavePending = true;
       static const int PREF_DELAY_MS = 500;
       NS_DelayedDispatchToCurrentThread(
         mozilla::NewRunnableMethod("Preferences::SavePrefFileAsynchronous",
-                                   sPreferences,
+                                   sPreferences.get(),
                                    &Preferences::SavePrefFileAsynchronous),
         PREF_DELAY_MS);
     }
   }
 }
 
 static nsresult
 openPrefFile(nsIFile* aFile);
@@ -3206,17 +3206,17 @@ static const char kPrefFileHeader[] =
   " * To make a manual change to preferences, you can visit the URL "
   "about:config"
   NS_LINEBREAK
   " */"
   NS_LINEBREAK
   NS_LINEBREAK;
 // clang-format on
 
-Preferences* Preferences::sPreferences = nullptr;
+StaticRefPtr<Preferences> Preferences::sPreferences;
 bool Preferences::sShutdown = false;
 
 // This globally enables or disables OMT pref writing, both sync and async.
 static int32_t sAllowOMTPrefWrite = -1;
 
 class ValueObserverHashKey : public PLDHashEntryHdr
 {
 public:
@@ -3703,22 +3703,20 @@ Preferences::GetInstanceForService()
   }
 
   if (sShutdown) {
     gCacheDataDesc = "shutting down in GetInstanceForService()";
     return nullptr;
   }
 
   sPreferences = new Preferences();
-  NS_ADDREF(sPreferences);
-
   Result<Ok, const char*> res = sPreferences->Init();
   if (res.isErr()) {
+    sPreferences = nullptr;
     gCacheDataDesc = res.unwrapErr();
-    NS_RELEASE(sPreferences);
     return nullptr;
   }
 
   gCacheData = new nsTArray<nsAutoPtr<CacheData>>();
   gCacheDataDesc = "set by GetInstanceForService()";
 
   gObserverTable = new nsRefPtrHashtable<ValueObserverHashKey, ValueObserver>();
 
@@ -3753,23 +3751,17 @@ Preferences::InitStaticMembers()
   return sPreferences != nullptr;
 }
 
 /* static */ void
 Preferences::Shutdown()
 {
   if (!sShutdown) {
     sShutdown = true; // Don't create the singleton instance after here.
-
-    // Don't set sPreferences to nullptr here. The instance may be grabbed by
-    // other modules. The utility methods of Preferences should be available
-    // until the singleton instance actually released.
-    if (sPreferences) {
-      sPreferences->Release();
-    }
+    sPreferences = nullptr;
   }
 }
 
 //-----------------------------------------------------------------------------
 
 //
 // Constructor/Destructor
 //
@@ -3777,26 +3769,24 @@ Preferences::Shutdown()
 Preferences::Preferences()
   : mRootBranch(new nsPrefBranch("", false))
   , mDefaultRootBranch(new nsPrefBranch("", true))
 {
 }
 
 Preferences::~Preferences()
 {
-  NS_ASSERTION(sPreferences == this, "Isn't this the singleton instance?");
+  MOZ_ASSERT(!sPreferences);
 
   delete gObserverTable;
   gObserverTable = nullptr;
 
   delete gCacheData;
   gCacheData = nullptr;
 
-  sPreferences = nullptr;
-
   PREF_Cleanup();
 }
 
 //
 // nsISupports Implementation
 //
 
 NS_IMPL_ADDREF(Preferences)
--- a/modules/libpref/Preferences.h
+++ b/modules/libpref/Preferences.h
@@ -8,16 +8,17 @@
 #define mozilla_Preferences_h
 
 #ifndef MOZILLA_INTERNAL_API
 #error "This header is only usable from within libxul (MOZILLA_INTERNAL_API)."
 #endif
 
 #include "mozilla/Atomics.h"
 #include "mozilla/MemoryReporting.h"
+#include "mozilla/StaticPtr.h"
 #include "nsCOMPtr.h"
 #include "nsIObserver.h"
 #include "nsIPrefBranch.h"
 #include "nsIPrefService.h"
 #include "nsTArray.h"
 #include "nsWeakReference.h"
 
 class nsIFile;
@@ -401,17 +402,17 @@ private:
   bool mProfileShutdown = false;
   // We wait a bit after prefs are dirty before writing them. In this period,
   // mDirty and mSavePending will both be true.
   bool mSavePending = false;
 
   nsCOMPtr<nsIPrefBranch> mRootBranch;
   nsCOMPtr<nsIPrefBranch> mDefaultRootBranch;
 
-  static Preferences* sPreferences;
+  static StaticRefPtr<Preferences> sPreferences;
   static bool sShutdown;
 
   // Init static members. Returns true on success.
   static bool InitStaticMembers();
 };
 
 } // namespace mozilla