Bug 1382817 - Prefs shouldn't start out dirty or be written almost immediately after being read, r?milan draft
authorBenjamin Smedberg <benjamin@smedbergs.us>
Thu, 20 Jul 2017 16:19:39 -0400
changeset 612566 138f69b831f327de18885a8ac3504719f1d39dea
parent 610649 dece50457378ac4934afe9fb3c2a8054e8894588
child 638447 156771dc53d4d567ce2f6cb7c08161d718d9a189
push id69538
push userbsmedberg@mozilla.com
push dateThu, 20 Jul 2017 20:51:01 +0000
reviewersmilan
bugs1382817
milestone56.0a1
Bug 1382817 - Prefs shouldn't start out dirty or be written almost immediately after being read, r?milan MozReview-Commit-ID: BFLMsMQjn9w
modules/libpref/Preferences.cpp
modules/libpref/Preferences.h
toolkit/xre/nsXREDirProvider.cpp
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -95,23 +95,29 @@ void
 Preferences::DirtyCallback()
 {
   if (!XRE_IsParentProcess()) {
     // TODO: this should really assert because you can't set prefs in a
     // content process. But so much code currently does this that we just
     // ignore it for now.
     return;
   }
-  if (gHashTable && sPreferences && !sPreferences->mDirty) {
+  if (!gHashTable || !sPreferences) {
+    return;
+  }
+  if (sPreferences->mProfileShutdown) {
+    NS_WARNING("Setting user pref after profile shutdown.");
+    return;
+  }
+  if (!sPreferences->mDirty) {
     sPreferences->mDirty = true;
 
-    NS_WARNING_ASSERTION(!sPreferences->mProfileShutdown,
-                         "Setting user pref after profile shutdown.");
-
-    if (sPreferences->AllowOffMainThreadSave() && !sPreferences->mSavePending) {
+    if (sPreferences->mCurrentFile &&
+        sPreferences->AllowOffMainThreadSave()
+        && !sPreferences->mSavePending) {
       sPreferences->mSavePending = true;
       static const int PREF_DELAY_MS = 500;
       NS_DelayedDispatchToCurrentThread(
         mozilla::NewRunnableMethod("Preferences::SavePrefFileAsynchronous",
                                    sPreferences,
                                    &Preferences::SavePrefFileAsynchronous),
         PREF_DELAY_MS);
     }
@@ -777,34 +783,40 @@ Preferences::Init()
 
   observerService->AddObserver(this, "load-extension-defaults", true);
   observerService->AddObserver(this, "suspend_process_notification", true);
 
   return(rv);
 }
 
 // static
-nsresult
-Preferences::ResetAndReadUserPrefs()
+void
+Preferences::InitializeUserPrefs()
 {
+  MOZ_ASSERT(!sPreferences->mCurrentFile, "Should only initialize prefs once");
+
+  // prefs which are set before we initialize the profile are silently discarded.
+  // This is stupid, but there are various tests which depend on this behavior.
   sPreferences->ResetUserPrefs();
 
-  MOZ_ASSERT(!sPreferences->mCurrentFile, "Should only initialize prefs once");
+  nsCOMPtr<nsIFile> prefsFile = sPreferences->ReadSavedPrefs();
+  sPreferences->ReadUserOverridePrefs();
 
-  nsresult rv = sPreferences->UseDefaultPrefFile();
-  sPreferences->UseUserPrefFile();
+  sPreferences->mDirty = false;
+
+  // Don't set mCurrentFile until we're done so that dirty flags work properly
+  sPreferences->mCurrentFile = prefsFile.forget();
 
   // Migrate the old prerelease telemetry pref
   if (!Preferences::GetBool(kOldTelemetryPref, true)) {
     Preferences::SetBool(kTelemetryPref, false);
     Preferences::ClearUser(kOldTelemetryPref);
   }
 
   sPreferences->NotifyServiceObservers(NS_PREFSERVICE_READ_TOPIC_ID);
-  return rv;
 }
 
 NS_IMETHODIMP
 Preferences::Observe(nsISupports *aSubject, const char *aTopic,
                      const char16_t *someData)
 {
   if (MOZ_UNLIKELY(!XRE_IsParentProcess())) {
     return NS_ERROR_NOT_AVAILABLE;
@@ -1077,46 +1089,44 @@ Preferences::NotifyServiceObservers(cons
     return NS_ERROR_FAILURE;
 
   nsISupports *subject = (nsISupports *)((nsIPrefService *)this);
   observerService->NotifyObservers(subject, aTopic, nullptr);
 
   return NS_OK;
 }
 
-nsresult
-Preferences::UseDefaultPrefFile()
+already_AddRefed<nsIFile>
+Preferences::ReadSavedPrefs()
 {
   nsCOMPtr<nsIFile> file;
   nsresult rv = NS_GetSpecialDirectory(NS_APP_PREFS_50_FILE,
                                        getter_AddRefs(file));
   if (NS_WARN_IF(NS_FAILED(rv))) {
-    return rv;
+    return nullptr;
   }
 
-  mCurrentFile = file;
-
   rv = openPrefFile(file);
   if (rv == NS_ERROR_FILE_NOT_FOUND) {
     // this is a normal case for new users
     Telemetry::ScalarSet(Telemetry::ScalarID::PREFERENCES_CREATED_NEW_USER_PREFS_FILE, true);
     rv = NS_OK;
   } else if (NS_FAILED(rv)) {
     // Save a backup copy of the current (invalid) prefs file, since all prefs
     // from the error line to the end of the file will be lost (bug 361102).
     // TODO we should notify the user about it (bug 523725).
     Telemetry::ScalarSet(Telemetry::ScalarID::PREFERENCES_PREFS_FILE_WAS_INVALID, true);
     MakeBackupPrefFile(file);
   }
 
-  return rv;
+  return file.forget();
 }
 
 void
-Preferences::UseUserPrefFile()
+Preferences::ReadUserOverridePrefs()
 {
   nsCOMPtr<nsIFile> aFile;
   nsresult rv = NS_GetSpecialDirectory(NS_APP_PREFS_50_DIR,
                                        getter_AddRefs(aFile));
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return;
   }
 
--- a/modules/libpref/Preferences.h
+++ b/modules/libpref/Preferences.h
@@ -67,19 +67,19 @@ public:
   nsresult Init();
 
   /**
    * Returns true if the Preferences service is available, false otherwise.
    */
   static bool IsServiceAvailable();
 
   /**
-   * Reset loaded user prefs then read them
+   * Initialize user prefs from prefs.js/user.js
    */
-  static nsresult ResetAndReadUserPrefs();
+  static void InitializeUserPrefs();
 
   /**
    * Returns the singleton instance which is addreffed.
    */
   static Preferences* GetInstanceForService();
 
   /**
    * Finallizes global members.
@@ -431,23 +431,27 @@ public:
   nsresult SavePrefFileBlocking();
   nsresult SavePrefFileAsynchronous();
 
 protected:
   virtual ~Preferences();
 
   nsresult NotifyServiceObservers(const char *aSubject);
   /**
-   * Reads the default pref file or, if that failed, try to save a new one.
+   * Loads the prefs.js file from the profile, or creates a new one.
    *
-   * @return NS_OK if either action succeeded,
-   *         or the error code related to the read attempt.
+   * @return the prefs file if successful, or nullptr on failure.
    */
-  nsresult UseDefaultPrefFile();
-  void UseUserPrefFile();
+  already_AddRefed<nsIFile> ReadSavedPrefs();
+
+  /**
+   * Loads the user.js file from the profile if present.
+   */
+  void ReadUserOverridePrefs();
+
   nsresult MakeBackupPrefFile(nsIFile *aFile);
 
   // Default pref file save can be blocking or not.
   enum class SaveMethod {
     Blocking,
     Asynchronous
   };
 
--- a/toolkit/xre/nsXREDirProvider.cpp
+++ b/toolkit/xre/nsXREDirProvider.cpp
@@ -1021,30 +1021,31 @@ nsXREDirProvider::GetDirectory(nsIFile* 
   NS_ENSURE_TRUE(mProfileDir, NS_ERROR_NOT_INITIALIZED);
 
   return mProfileDir->Clone(aResult);
 }
 
 NS_IMETHODIMP
 nsXREDirProvider::DoStartup()
 {
+  nsresult rv;
+
   if (!mProfileNotified) {
     nsCOMPtr<nsIObserverService> obsSvc =
       mozilla::services::GetObserverService();
     if (!obsSvc) return NS_ERROR_FAILURE;
 
     mProfileNotified = true;
 
     /*
        Setup prefs before profile-do-change to be able to use them to track
        crashes and because we want to begin crash tracking before other code run
        from this notification since they may cause crashes.
     */
-    nsresult rv = mozilla::Preferences::ResetAndReadUserPrefs();
-    if (NS_FAILED(rv)) NS_WARNING("Failed to setup pref service.");
+    mozilla::Preferences::InitializeUserPrefs();
 
     bool safeModeNecessary = false;
     nsCOMPtr<nsIAppStartup> appStartup (do_GetService(NS_APPSTARTUP_CONTRACTID));
     if (appStartup) {
       rv = appStartup->TrackStartupCrashBegin(&safeModeNecessary);
       if (NS_FAILED(rv) && rv != NS_ERROR_NOT_AVAILABLE)
         NS_WARNING("Error while beginning startup crash tracking");