Bug 1367813 - 1) Add telemetry for prefs.js not existing or being corrupted, and the presence of a user.js file. 2) Rename and change the nsIPrefService.readUserPrefs API. The new API reads user prefs from a file but doesn't remember that location or save changed preferences to that location. r=milan data-r=rweiss

MozReview-Commit-ID: FD5npJlB24W

--HG--
extra : rebase_source : d30bac5044ecb8d86253e8bca3e8d47371fb9d1f
extra : source : 4f0ee0f75a307095e4a04afe4413e5bb6ea8e502

modules/libpref/Preferences.cpp

@@ -11,6 +11,7 @@
 #include "mozilla/Attributes.h"
 #include "mozilla/HashFunctions.h"
 #include "mozilla/ServoStyleSet.h"
+#include "mozilla/Telemetry.h"
 #include "mozilla/UniquePtrExtensions.h"
 
 #include "nsXULAppAPI.h"
@@ -54,6 +55,8 @@
 #include "nsThreadUtils.h"
 #include "GeckoProfiler.h"
 
+using namespace mozilla;
+
 #ifdef DEBUG
 #define ENSURE_MAIN_PROCESS(message, pref) do {                                \
   if (MOZ_UNLIKELY(!XRE_IsParentProcess())) {                                  \
@@ -91,6 +94,12 @@ static NS_DEFINE_CID(kZipReaderCID, NS_ZIPREADER_CID);
 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) {
     sPreferences->mDirty = true;
   }
@@ -765,7 +774,20 @@ nsresult
 Preferences::ResetAndReadUserPrefs()
 {
   sPreferences->ResetUserPrefs();
-  return sPreferences->ReadUserPrefs(nullptr);
+
+  MOZ_ASSERT(!sPreferences->mCurrentFile, "Should only initialize prefs once");
+
+  nsresult rv = sPreferences->UseDefaultPrefFile();
+  sPreferences->UseUserPrefFile();
+
+  // 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
@@ -800,39 +822,19 @@ Preferences::Observe(nsISupports *aSubject, const char *aTopic,
 
 
 NS_IMETHODIMP
-Preferences::ReadUserPrefs(nsIFile *aFile)
+Preferences::ReadUserPrefsFromFile(nsIFile *aFile)
 {
   if (MOZ_UNLIKELY(!XRE_IsParentProcess())) {
     NS_ERROR("must load prefs from parent process");
     return NS_ERROR_NOT_AVAILABLE;
   }
 
-  nsresult rv;
-
-  if (nullptr == aFile) {
-    // We should not be re-reading the user preferences, but if we
-    // are going to try, make sure there are no outstanding saves
-    if (AllowOffMainThreadSave()) {
-      PreferencesWriter::Flush();
-    }
-
-    rv = UseDefaultPrefFile();
-    // A user pref file is optional.
-    // Ignore all errors related to it, so we retain 'rv' value :-|
-    (void) UseUserPrefFile();
-
-    // Migrate the old prerelease telemetry pref
-    if (!Preferences::GetBool(kOldTelemetryPref, true)) {
-      Preferences::SetBool(kTelemetryPref, false);
-      Preferences::ClearUser(kOldTelemetryPref);
-    }
-
-    NotifyServiceObservers(NS_PREFSERVICE_READ_TOPIC_ID);
-  } else {
-    rv = ReadAndOwnUserPrefFile(aFile);
+  if (!aFile) {
+    NS_ERROR("ReadUserPrefsFromFile requires a parameter");
+    return NS_ERROR_INVALID_ARG;
   }
 
-  return rv;
+  return openPrefFile(aFile);
 }
 
 NS_IMETHODIMP
@@ -1063,46 +1065,48 @@ Preferences::NotifyServiceObservers(const char *aTopic)
 nsresult
 Preferences::UseDefaultPrefFile()
 {
-  nsCOMPtr<nsIFile> aFile;
-  nsresult rv = NS_GetSpecialDirectory(NS_APP_PREFS_50_FILE, getter_AddRefs(aFile));
+  nsCOMPtr<nsIFile> file;
+  nsresult rv = NS_GetSpecialDirectory(NS_APP_PREFS_50_FILE,
+                                       getter_AddRefs(file));
+  if (NS_WARN_IF(NS_FAILED(rv))) {
+    return rv;
+  }
 
-  if (NS_SUCCEEDED(rv)) {
-    rv = ReadAndOwnUserPrefFile(aFile);
-    // Most likely cause of failure here is that the file didn't
-    // exist, so save a new one. mUserPrefReadFailed will be
-    // used to catch an error in actually reading the file.
-    if (NS_FAILED(rv)) {
-      if (NS_FAILED(SavePrefFileInternal(aFile, SaveMethod::Blocking)))
-        NS_ERROR("Failed to save new shared pref file");
-      else
-        rv = NS_OK;
-    }
+  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;
 }
 
-nsresult
+void
 Preferences::UseUserPrefFile()
 {
-  nsresult rv = NS_OK;
   nsCOMPtr<nsIFile> aFile;
-  nsDependentCString prefsDirProp(NS_APP_PREFS_50_DIR);
-
-  rv = NS_GetSpecialDirectory(prefsDirProp.get(), getter_AddRefs(aFile));
-  if (NS_SUCCEEDED(rv) && aFile) {
-    rv = aFile->AppendNative(NS_LITERAL_CSTRING("user.js"));
-    if (NS_SUCCEEDED(rv)) {
-      bool exists = false;
-      aFile->Exists(&exists);
-      if (exists) {
-        rv = openPrefFile(aFile);
-      } else {
-        rv = NS_ERROR_FILE_NOT_FOUND;
-      }
-    }
+  nsresult rv = NS_GetSpecialDirectory(NS_APP_PREFS_50_DIR,
+                                       getter_AddRefs(aFile));
+  if (NS_WARN_IF(NS_FAILED(rv))) {
+    return;
+  }
+
+  aFile->AppendNative(NS_LITERAL_CSTRING("user.js"));
+  rv = openPrefFile(aFile);
+  if (rv != NS_ERROR_FILE_NOT_FOUND) {
+    // If the file exists and was at least partially read, record that
+    // in telemetry as it may be a sign of pref injection.
+    Telemetry::ScalarSet(Telemetry::ScalarID::PREFERENCES_READ_USER_JS, true);
   }
-  return rv;
 }
 
 nsresult
@@ -1130,40 +1134,6 @@ Preferences::MakeBackupPrefFile(nsIFile *aFile)
   return rv;
 }
 
-nsresult
-Preferences::ReadAndOwnUserPrefFile(nsIFile *aFile)
-{
-  NS_ENSURE_ARG(aFile);
-  
-  if (mCurrentFile == aFile)
-    return NS_OK;
-
-  // Since we're changing the pref file, we may have to make
-  // sure the outstanding writes are handled first.
-  if (AllowOffMainThreadSave()) {
-    PreferencesWriter::Flush();
-  }
-
-  mCurrentFile = aFile;
-
-  nsresult rv = NS_OK;
-  bool exists = false;
-  mCurrentFile->Exists(&exists);
-  if (exists) {
-    rv = openPrefFile(mCurrentFile);
-    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).
-      MakeBackupPrefFile(mCurrentFile);
-    }
-  } else {
-    rv = NS_ERROR_FILE_NOT_FOUND;
-  }
-
-  return rv;
-}
-
 nsresult
 Preferences::SavePrefFileInternal(nsIFile *aFile, SaveMethod aSaveMethod)
 {

modules/libpref/Preferences.h

@@ -442,9 +442,7 @@ protected:
    *         or the error code related to the read attempt.
    */
   nsresult UseDefaultPrefFile();
-  nsresult UseUserPrefFile();
-  nsresult ReadAndOwnUserPrefFile(nsIFile *aFile);
-  nsresult ReadAndOwnSharedUserPrefFile(nsIFile *aFile);
+  void UseUserPrefFile();
   nsresult MakeBackupPrefFile(nsIFile *aFile);
 
   // Default pref file save can be blocking or not.

modules/libpref/nsIPrefService.idl

@@ -30,22 +30,6 @@ interface nsIFile;
 [scriptable, uuid(1f84fd56-3956-40df-b86a-1ea01402ee96)]
 interface nsIPrefService : nsISupports
 {
-  /**
-   * Called to read in the preferences specified in a user preference file.
-   *
-   * @param aFile The file to be read.
-   *
-   * @note
-   * If nullptr is passed in for the aFile parameter the default preferences
-   * file(s) [prefs.js, user.js] will be read and processed.
-   *
-   * @throws Error File failed to read or contained invalid data.
-   *
-   * @see savePrefFile
-   * @see nsIFile
-   */
-  void readUserPrefs(in nsIFile aFile);
-
   /**
    * Called to completely flush and re-initialize the preferences system.
    *
@@ -125,6 +109,17 @@ interface nsIPrefService : nsISupports
    * that have not been written to disk
    */
   readonly attribute boolean dirty;
+
+  /**
+   * Read in the preferences specified in a user preference file. This method
+   * does not clear user preferences that were already set.
+   *
+   * @param aFile The file to be read.
+   *
+   * @throws Error File failed to read or contained invalid data.
+   * @note This method is intended for internal unit testing only!
+   */
+  void readUserPrefsFromFile(in nsIFile aFile);
 };
 
 %{C++

modules/libpref/test/unit/head_libPrefs.js

@@ -17,11 +17,10 @@ function do_check_throws(f, result, stack)
   try {
     f();
   } catch (exc) {
-    if (exc.result == result)
-      return;
-    do_throw("expected result " + result + ", caught " + exc, stack);
+    equal(exc.result, result, "Correct exception was thrown");
+    return;
   }
-  do_throw("expected result " + result + ", none thrown", stack);
+  ok(false, "expected result " + result + ", none thrown");
 }
 
 var dirSvc = Cc["@mozilla.org/file/directory_service;1"].getService(Ci.nsIProperties);

modules/libpref/test/unit/test_bug506224.js

@@ -22,7 +22,7 @@ function run_test() {
 
   prefs.unlockPref(PREF_NAME);
   prefs.setCharPref(PREF_NAME, "test1");
-  ps.readUserPrefs(file);
+  ps.readUserPrefsFromFile(file);
 
   do_check_eq("test1", userprefs.getCharPref(PREF_NAME));
   do_check_eq(false, userprefs.prefHasUserValue(PREF_NAME));

modules/libpref/test/unit/test_dirtyPrefs.js

@@ -20,17 +20,6 @@ function run_test() {
   let prefFile = do_get_profile();
   prefFile.append("prefs.js");
 
-  // dirty flag only applies to the default pref file save, not all of them,
-  // so we need to set the default pref file first.  Chances are, the file
-  // does not exist, but we don't need it to - we just want to set the
-  // name/location to match
-  //
-  try {
-    ps.readUserPrefs(prefFile);
-  } catch (e) {
-    // we're fine if the file isn't there
-  }
-
   //**************************************************************************//
   // prefs are not dirty after a write
   ps.savePrefFile(null);

modules/libpref/test/unit/test_extprefs.js

@@ -50,7 +50,7 @@ function run_test() {
     ps.getBoolPref("testPref.bool1");
   }, Cr.NS_ERROR_UNEXPECTED);
   
-  ps.readUserPrefs(prefFile);
+  ps.readUserPrefsFromFile(prefFile);
 
   do_check_true(ps.getBoolPref("testPref.bool1"));
   ps.setBoolPref("testPref.bool1", false);

modules/libpref/test/unit/test_libPrefs.js

@@ -309,6 +309,7 @@ function run_test() {
   let savePrefFile = do_get_cwd();
   savePrefFile.append("data");
   savePrefFile.append("savePref.js");
+
   if (savePrefFile.exists())
     savePrefFile.remove(false);
   savePrefFile.create(Ci.nsIFile.NORMAL_FILE_TYPE, 0o666);
@@ -317,7 +318,7 @@ function run_test() {
 
   // load a preexisting pref file
   let prefFile = do_get_file("data/testPref.js");
-  ps.readUserPrefs(prefFile);
+  ps.readUserPrefsFromFile(prefFile);
 
   // former prefs should have been replaced/lost
   do_check_throws(function() {
@@ -337,7 +338,12 @@ function run_test() {
   do_check_eq(pb.getCharPref("char2"), "älskar");
 
   // loading our former savePrefFile should allow us to read former prefs
-  ps.readUserPrefs(savePrefFile);
+
+  // Hack alert: on Windows nsLocalFile caches the size of savePrefFile from
+  // the .create() call above as 0. We call .exists() to reset the cache.
+  savePrefFile.exists();
+
+  ps.readUserPrefsFromFile(savePrefFile);
   // cleanup the file now we don't need it
   savePrefFile.remove(false);
   do_check_eq(ps.getBoolPref("ReadPref.bool"), true);

modules/libpref/test/unit/test_stickyprefs.js

@@ -18,7 +18,7 @@ do_register_cleanup(saveAndReload);
 function resetAndLoad(filenames) {
   ps.resetPrefs();
   for (let filename of filenames) {
-    ps.readUserPrefs(do_get_file(filename));
+    ps.readUserPrefsFromFile(do_get_file(filename));
   }
 }
 
@@ -33,7 +33,12 @@ function saveAndReload() {
 
   // Now reset the pref service and re-read what we saved.
   ps.resetPrefs();
-  ps.readUserPrefs(file);
+
+  // Hack alert: on Windows nsLocalFile caches the size of savePrefFile from
+  // the .create() call above as 0. We call .exists() to reset the cache.
+  file.exists();
+
+  ps.readUserPrefsFromFile(file);
 }
 
 function run_test() {

services/sync/tests/unit/test_prefs_store.js

@@ -25,15 +25,7 @@ function makePersona(id) {
 function run_test() {
   _("Test fixtures.");
   // read our custom prefs file before doing anything.
-  Services.prefs.readUserPrefs(do_get_file("prefs_test_prefs_store.js"));
-  // Now we've read from this file, any writes the pref service makes will be
-  // back to this prefs_test_prefs_store.js directly in the obj dir. This
-  // upsets things in confusing ways :) We avoid this by explicitly telling the
-  // pref service to use a file in our profile dir.
-  let prefFile = do_get_profile();
-  prefFile.append("prefs.js");
-  Services.prefs.savePrefFile(prefFile);
-  Services.prefs.readUserPrefs(prefFile);
+  Services.prefs.readUserPrefsFromFile(do_get_file("prefs_test_prefs_store.js"));
 
   let store = Service.engineManager.get("prefs")._store;
   let prefs = new Preferences();

toolkit/components/telemetry/Scalars.yaml

@@ -317,6 +317,45 @@ security:
     record_in_processes:
       - main
 
+preferences:
+  created_new_user_prefs_file:
+    bug_numbers:
+      - 1367813
+    description: >-
+      A boolean indicating that profile/prefs.js was not found and it is being
+      created for the first time in this session.
+    expires: "62"
+    kind: boolean
+    notification_emails:
+      - bsmedberg@mozilla.com
+    release_channel_collection: opt-out
+    record_in_processes:
+      - main
+  prefs_file_was_invalid:
+    bug_numbers:
+      - 1367813
+    description: >-
+      Set to true if a failure occurred reading profile/prefs.js.
+    expires: "62"
+    kind: boolean
+    notification_emails:
+      - bsmedberg@mozilla.com
+    release_channel_collection: opt-out
+    record_in_processes:
+      - main
+  read_user_js:
+    bug_numbers:
+      - 1367813
+    description: >-
+      Set to true if user.js exists and was read.
+    expires: "62"
+    kind: boolean
+    notification_emails:
+      - bsmedberg@mozilla.com
+    release_channel_collection: opt-out
+    record_in_processes:
+      - main
+
 # The following section contains WebRTC nICEr scalars
 # For more info on ICE, see https://tools.ietf.org/html/rfc5245
 # For more info on STUN, see https://tools.ietf.org/html/rfc5389