Bug 1029148 - store current search engine configuration outside of prefs, r=MattN.

browser/app/profile/firefox.js

@@ -1217,7 +1217,6 @@ pref("services.sync.prefs.sync.browser.link.open_newwindow", true);
 pref("services.sync.prefs.sync.browser.offline-apps.notify", true);
 pref("services.sync.prefs.sync.browser.safebrowsing.enabled", true);
 pref("services.sync.prefs.sync.browser.safebrowsing.malware.enabled", true);
-pref("services.sync.prefs.sync.browser.search.selectedEngine", true);
 pref("services.sync.prefs.sync.browser.search.update", true);
 pref("services.sync.prefs.sync.browser.sessionstore.restore_on_demand", true);
 pref("services.sync.prefs.sync.browser.startup.homepage", true);

browser/components/nsBrowserGlue.js

@@ -1343,7 +1343,7 @@ BrowserGlue.prototype = {
   },
 
   _migrateUI: function BG__migrateUI() {
-    const UI_VERSION = 22;
+    const UI_VERSION = 23;
     const BROWSER_DOCURL = "chrome://browser/content/browser.xul#";
     let currentUIVersion = 0;
     try {
@@ -1616,6 +1616,17 @@ BrowserGlue.prototype = {
       Services.prefs.clearUserPref("browser.syncPromoViewsLeftMap");
     }
 
+    if (currentUIVersion < 23) {
+      const kSelectedEnginePref = "browser.search.selectedEngine";
+      if (Services.prefs.prefHasUserValue(kSelectedEnginePref)) {
+        try {
+          let name = Services.prefs.getComplexValue(kSelectedEnginePref,
+                                                    Ci.nsIPrefLocalizedString).data;
+          Services.search.currentEngine = Services.search.getEngineByName(name);
+        } catch (ex) {}
+      }
+    }
+
     if (this._dirty)
       this._dataSource.QueryInterface(Ci.nsIRDFRemoteDataSource).Flush();
 

toolkit/components/search/nsSearchService.js

@@ -3254,18 +3254,27 @@ SearchService.prototype = {
     // Clear the engines, too, so we don't stick with the stale ones.
     this._engines = {};
     this.__sortedEngines = null;
+    this._currentEngine = null;
+    this._defaultEngine = null;
 
-    // Typically we'll re-init as a result of a pref observer,
+    // Clear the metadata service.
-    // so signal to 'callers' that we're done.
+    engineMetadataService._initState = engineMetadataService._InitStates.NOT_STARTED;
-    return this._asyncLoadEngines()
+    engineMetadataService._initializer = null;
-               .then(() => {
+
-                       Services.obs.notifyObservers(null, SEARCH_SERVICE_TOPIC, "reinit-complete");
+    Task.spawn(function* () {
-                       gInitialized = true;
+      try {
-                     },
+        yield engineMetadataService.init();
-                     (err) => {
+        yield this._asyncLoadEngines();
-                       LOG("Reinit failed: " + err);
+
-                       Services.obs.notifyObservers(null, SEARCH_SERVICE_TOPIC, "reinit-failed");
+        // Typically we'll re-init as a result of a pref observer,
-                     });
+        // so signal to 'callers' that we're done.
+        Services.obs.notifyObservers(null, SEARCH_SERVICE_TOPIC, "reinit-complete");
+        gInitialized = true;
+      } catch (err) {
+        LOG("Reinit failed: " + err);
+        Services.obs.notifyObservers(null, SEARCH_SERVICE_TOPIC, "reinit-failed");
+      }
+    }.bind(this));
   },
 
   _readCacheFile: function SRCH_SVC__readCacheFile(aFile) {
@@ -3790,13 +3799,21 @@ SearchService.prototype = {
                                       });
   },
 
-  _setEngineByPref: function SRCH_SVC_setEngineByPref(aEngineType, aPref) {
+  _getVerificationHash: function SRCH_SVC__getVerificationHash(aName) {
-    this._ensureInitialized();
+    let str = OS.Path.basename(OS.Constants.Path.profileDir) + aName;
-    let newEngine = this.getEngineByName(getLocalizedPref(aPref, ""));
-    if (!newEngine)
-      FAIL("Can't find engine in store!", Cr.NS_ERROR_UNEXPECTED);
 
-    this[aEngineType] = newEngine;
+    let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"]
+                      .createInstance(Ci.nsIScriptableUnicodeConverter);
+    converter.charset = "UTF-8";
+
+    // Data is an array of bytes.
+    let data = converter.convertToByteArray(str, {});
+    let hasher = Cc["@mozilla.org/security/hash;1"]
+                   .createInstance(Ci.nsICryptoHash);
+    hasher.init(hasher.SHA256);
+    hasher.update(data, data.length);
+
+    return hasher.finish(true);
   },
 
   // nsIBrowserSearchService
@@ -4125,9 +4142,6 @@ SearchService.prototype = {
 
     this._defaultEngine = newDefaultEngine;
 
-    // Set a flag to keep track that this setter was called properly, not by
-    // setting the pref alone.
-    this._changingDefaultEngine = true;
     let defPref = BROWSER_SEARCH_PREF + "defaultenginename";
     // If we change the default engine in the future, that change should impact
     // users who have switched away from and then back to the build's "default"
@@ -4140,7 +4154,6 @@ SearchService.prototype = {
     else {
       setLocalizedPref(defPref, this._defaultEngine.name);
     }
-    this._changingDefaultEngine = false;
 
     notifyAction(this._defaultEngine, SEARCH_ENGINE_DEFAULT);
   },
@@ -4148,13 +4161,16 @@ SearchService.prototype = {
   get currentEngine() {
     this._ensureInitialized();
     if (!this._currentEngine) {
-      let selectedEngine = getLocalizedPref(BROWSER_SEARCH_PREF +
+      let name = engineMetadataService.getGlobalAttr("current");
-                                            "selectedEngine");
+      if (engineMetadataService.getGlobalAttr("hash") == this._getVerificationHash(name)) {
-      this._currentEngine = this.getEngineByName(selectedEngine);
+        this._currentEngine = this.getEngineByName(name);
+      }
     }
 
     if (!this._currentEngine || this._currentEngine.hidden)
-      this._currentEngine = this.defaultEngine;
+      this._currentEngine = this._originalDefaultEngine;
+    if (!this._currentEngine || this._currentEngine.hidden)
+      this._currentEngine = this._getSortedEngines(false)[0];
     return this._currentEngine;
   },
 
@@ -4175,23 +4191,18 @@ SearchService.prototype = {
 
     this._currentEngine = newCurrentEngine;
 
-    var currentEnginePref = BROWSER_SEARCH_PREF + "selectedEngine";
-
-    // Set a flag to keep track that this setter was called properly, not by
-    // setting the pref alone.
-    this._changingCurrentEngine = true;
     // If we change the default engine in the future, that change should impact
     // users who have switched away from and then back to the build's "default"
     // engine. So clear the user pref when the currentEngine is set to the
     // build's default engine, so that the currentEngine getter falls back to
     // whatever the default is.
+    let newName = this._currentEngine.name;
     if (this._currentEngine == this._originalDefaultEngine) {
-      Services.prefs.clearUserPref(currentEnginePref);
+      newName = "";
     }
-    else {
+
-      setLocalizedPref(currentEnginePref, this._currentEngine.name);
+    engineMetadataService.setGlobalAttr("current", newName);
-    }
+    engineMetadataService.setGlobalAttr("hash", this._getVerificationHash(newName));
-    this._changingCurrentEngine = false;
 
     notifyAction(this._currentEngine, SEARCH_ENGINE_CURRENT);
   },
@@ -4374,26 +4385,12 @@ SearchService.prototype = {
         break;
 
       case "nsPref:changed":
-#ifdef MOZ_FENNEC
         if (aVerb == LOCALE_PREF) {
           // Locale changed. Re-init. We rely on observers, because we can't
           // return this promise to anyone.
           this._asyncReInit();
           break;
         }
-#endif
-
-        let currPref = BROWSER_SEARCH_PREF + "selectedEngine";
-        if (aVerb == currPref && !this._changingCurrentEngine) {
-          this._setEngineByPref("currentEngine", currPref);
-          break;
-        }
-
-        let defPref = BROWSER_SEARCH_PREF + "defaultenginename";
-        if (aVerb == defPref && !this._changingDefaultEngine) {
-          this._setEngineByPref("defaultEngine", defPref);
-        }
-        break;
     }
   },
 
@@ -4439,8 +4436,6 @@ SearchService.prototype = {
   _addObservers: function SRCH_SVC_addObservers() {
     Services.obs.addObserver(this, SEARCH_ENGINE_TOPIC, false);
     Services.obs.addObserver(this, QUIT_APPLICATION_TOPIC, false);
-    Services.prefs.addObserver(BROWSER_SEARCH_PREF + "defaultenginename", this, false);
-    Services.prefs.addObserver(BROWSER_SEARCH_PREF + "selectedEngine", this, false);
 
 #ifdef MOZ_FENNEC
     Services.prefs.addObserver(LOCALE_PREF, this, false);
@@ -4490,8 +4485,6 @@ SearchService.prototype = {
   _removeObservers: function SRCH_SVC_removeObservers() {
     Services.obs.removeObserver(this, SEARCH_ENGINE_TOPIC);
     Services.obs.removeObserver(this, QUIT_APPLICATION_TOPIC);
-    Services.prefs.removeObserver(BROWSER_SEARCH_PREF + "defaultenginename", this);
-    Services.prefs.removeObserver(BROWSER_SEARCH_PREF + "selectedEngine", this);
 
 #ifdef MOZ_FENNEC
     Services.prefs.removeObserver(LOCALE_PREF, this);
@@ -4651,6 +4644,11 @@ var engineMetadataService = {
     return record[aName];
   },
 
+  _globalFakeEngine: {_id: "[global]"},
+  getGlobalAttr: function epsGetGlobalAttr(name) {
+    return this.getAttr(this._globalFakeEngine, name);
+  },
+
   _setAttr: function epsSetAttr(engine, name, value) {
     // attr names must be lower case
     name = name.toLowerCase();
@@ -4684,6 +4682,10 @@ var engineMetadataService = {
     }
   },
 
+  setGlobalAttr: function epsGetGlobalAttr(key, value) {
+    this.setAttr(this._globalFakeEngine, key, value);
+  },
+
   /**
    * Bulk set metadata attributes for a number of engines.
    *
@@ -4718,14 +4720,10 @@ var engineMetadataService = {
    * (= 100ms). If the function is called again before the expiration of
    * the delay, commits are merged and the function is again delayed by
    * the same amount of time.
-   *
-   * @param aStore is an optional parameter specifying the object to serialize.
-   *               If not specified, this._store is used.
    */
-  _commit: function epsCommit(aStore) {
+  _commit: function epsCommit() {
     LOG("metadata _commit: start");
-    let store = aStore || this._store;
+    if (!this._store) {
-    if (!store) {
       LOG("metadata _commit: nothing to do");
       return;
     }
@@ -4734,7 +4732,7 @@ var engineMetadataService = {
       LOG("metadata _commit: initializing lazy writer");
       function writeCommit() {
         LOG("metadata writeCommit: start");
-        let data = gEncoder.encode(JSON.stringify(store));
+        let data = gEncoder.encode(JSON.stringify(engineMetadataService._store));
         let path = engineMetadataService._jsonFile;
         LOG("metadata writeCommit: path " + path);
         let promise = OS.File.writeAtomic(path, data, { tmpPath: path + ".tmp" });

toolkit/components/search/tests/xpcshell/test_prefSync.js

@@ -1,81 +0,0 @@
-/* Any copyright is dedicated to the Public Domain.
- * http://creativecommons.org/publicdomain/zero/1.0/ */
-
-/*
- * Test that currentEngine and defaultEngine properties are updated when the
- * prefs are set independently.
- */
-
-"use strict";
-
-const PREF_BRANCH = "browser.search.";
-
-/**
- * Wrapper for nsIPrefBranch::setComplexValue.
- * @param aPrefName
- *        The name of the pref to set.
- */
-function setLocalizedPref(aPrefName, aValue) {
-  let nsIPLS = Ci.nsIPrefLocalizedString;
-  let branch = Services.prefs.getBranch(PREF_BRANCH);
-  try {
-    var pls = Cc["@mozilla.org/pref-localizedstring;1"].
-              createInstance(Ci.nsIPrefLocalizedString);
-    pls.data = aValue;
-    branch.setComplexValue(aPrefName, nsIPLS, pls);
-  } catch (ex) {}
-}
-
-function run_test() {
-  removeMetadata();
-  updateAppInfo();
-  useHttpServer();
-
-  run_next_test();
-}
-
-add_task(function* test_prefSync() {
-  let [engine1, engine2] = yield addTestEngines([
-    { name: "Test search engine", xmlFileName: "engine.xml" },
-    { name: "A second test engine", xmlFileName: "engine2.xml" },
-  ]);
-
-  let search = Services.search;
-
-  // Initial sanity check:
-  search.defaultEngine = engine1;
-  do_check_eq(search.defaultEngine, engine1);
-  search.currentEngine = engine1;
-  do_check_eq(search.currentEngine, engine1);
-
-  setLocalizedPref("defaultenginename", engine2.name);
-  // Default engine should be synced with the pref
-  do_check_eq(search.defaultEngine, engine2);
-  // Current engine should've stayed the same
-  do_check_eq(search.currentEngine, engine1);
-  
-  setLocalizedPref("selectedEngine", engine2.name);
-  // Default engine should've stayed the same
-  do_check_eq(search.defaultEngine, engine2);
-  // Current engine should've been updated
-  do_check_eq(search.currentEngine, engine2);
-
-  // Test that setting the currentEngine to the original default engine clears
-  // the selectedEngine pref, rather than setting it. To do this we need to
-  // set the value of defaultenginename on the default branch.
-  let defaultBranch = Services.prefs.getDefaultBranch("");
-  let prefName = PREF_BRANCH + "defaultenginename";
-  let prefVal = "data:text/plain," + prefName + "=" + engine1.name;
-  defaultBranch.setCharPref(prefName, prefVal, true);
-  search.currentEngine = engine1;
-  // Current engine should've been updated
-  do_check_eq(search.currentEngine, engine1);
-  do_check_false(Services.prefs.prefHasUserValue("browser.search.selectedEngine"));
-
-  // Test that setting the defaultEngine to the original default engine clears
-  // the defaultenginename pref, rather than setting it.
-  do_check_true(Services.prefs.prefHasUserValue("browser.search.defaultenginename"));
-  search.defaultEngine = engine1;
-  do_check_eq(search.defaultEngine, engine1);
-  do_check_false(Services.prefs.prefHasUserValue("browser.search.defaultenginename"));
-});

toolkit/components/search/tests/xpcshell/test_selectedEngine.js

@@ -0,0 +1,168 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+Components.utils.import("resource://gre/modules/osfile.jsm");
+
+const kDefaultenginenamePref = "browser.search.defaultenginename";
+const kSelectedEnginePref = "browser.search.selectedEngine";
+
+const kTestEngineName = "Test search engine";
+
+function getDefaultEngineName() {
+  const nsIPLS = Ci.nsIPrefLocalizedString;
+  return Services.prefs.getComplexValue(kDefaultenginenamePref, nsIPLS).data;
+}
+
+function waitForNotification(aExpectedData) {
+  let deferred = Promise.defer();
+
+  const SEARCH_SERVICE_TOPIC = "browser-search-service";
+  Services.obs.addObserver(function observer(aSubject, aTopic, aData) {
+    if (aData != aExpectedData)
+      return;
+
+    Services.obs.removeObserver(observer, SEARCH_SERVICE_TOPIC);
+    deferred.resolve();
+  }, SEARCH_SERVICE_TOPIC, false);
+
+  return deferred.promise;
+}
+
+function asyncInit() {
+  let deferred = Promise.defer();
+
+  Services.search.init(function() {
+    do_check_true(Services.search.isInitialized);
+    deferred.resolve();
+  });
+
+  return deferred.promise;
+}
+
+function asyncReInit() {
+  let promise = waitForNotification("reinit-complete");
+
+  Services.search.QueryInterface(Ci.nsIObserver)
+          .observe(null, "nsPref:changed", "general.useragent.locale");
+
+  return promise;
+}
+
+// Check that the default engine matches the defaultenginename pref
+add_task(function* test_defaultEngine() {
+  yield asyncInit();
+
+  do_check_eq(Services.search.currentEngine.name, getDefaultEngineName());
+});
+
+// Giving prefs a user value shouldn't change the selected engine.
+add_task(function* test_selectedEngine() {
+  let defaultEngineName = getDefaultEngineName();
+  // Test the selectedEngine pref.
+  Services.prefs.setCharPref(kSelectedEnginePref, kTestEngineName);
+
+  yield asyncReInit();
+  do_check_eq(Services.search.currentEngine.name, defaultEngineName);
+
+  Services.prefs.clearUserPref(kSelectedEnginePref);
+
+  // Test the defaultenginename pref.
+  Services.prefs.setCharPref(kDefaultenginenamePref, kTestEngineName);
+
+  yield asyncReInit();
+  do_check_eq(Services.search.currentEngine.name, defaultEngineName);
+
+  Services.prefs.clearUserPref(kDefaultenginenamePref);
+});
+
+// Setting the search engine should be persisted across restarts.
+add_task(function* test_persistAcrossRestarts() {
+  // Set the engine through the API.
+  Services.search.currentEngine = Services.search.getEngineByName(kTestEngineName);
+  do_check_eq(Services.search.currentEngine.name, kTestEngineName);
+  yield waitForNotification("write-metadata-to-disk-complete");
+
+  // Check that the a hash was saved.
+  let path = OS.Path.join(OS.Constants.Path.profileDir, "search-metadata.json");
+  let bytes = yield OS.File.read(path);
+  let json = JSON.parse(new TextDecoder().decode(bytes));
+  do_check_eq(json["[global]"].hash.length, 44);
+
+  // Re-init and check the engine is still the same.
+  yield asyncReInit();
+  do_check_eq(Services.search.currentEngine.name, kTestEngineName);
+
+  // Cleanup (set the engine back to default).
+  Services.search.currentEngine = Services.search.defaultEngine;
+  do_check_eq(Services.search.currentEngine.name, getDefaultEngineName());
+});
+
+// An engine set without a valid hash should be ignored.
+add_task(function* test_ignoreInvalidHash() {
+  // Set the engine through the API.
+  Services.search.currentEngine = Services.search.getEngineByName(kTestEngineName);
+  do_check_eq(Services.search.currentEngine.name, kTestEngineName);
+  yield waitForNotification("write-metadata-to-disk-complete");
+
+  // Then mess with the file.
+  let path = OS.Path.join(OS.Constants.Path.profileDir, "search-metadata.json");
+  let bytes = yield OS.File.read(path);
+  let json = JSON.parse(new TextDecoder().decode(bytes));
+
+  // Make the hask invalid.
+  json["[global]"].hash = "invalid";
+
+  let data = new TextEncoder().encode(JSON.stringify(json));
+  let promise = OS.File.writeAtomic(path, data);//, { tmpPath: path + ".tmp" });
+
+  // Re-init the search service, and check that the json file is ignored.
+  yield asyncReInit();
+  do_check_eq(Services.search.currentEngine.name, getDefaultEngineName());
+});
+
+// Resetting the engine to the default should remove the saved value.
+add_task(function* test_settingToDefault() {
+  // Set the engine through the API.
+  Services.search.currentEngine = Services.search.getEngineByName(kTestEngineName);
+  do_check_eq(Services.search.currentEngine.name, kTestEngineName);
+  yield waitForNotification("write-metadata-to-disk-complete");
+
+  // Check that the current engine was saved.
+  let path = OS.Path.join(OS.Constants.Path.profileDir, "search-metadata.json");
+  let bytes = yield OS.File.read(path);
+  let json = JSON.parse(new TextDecoder().decode(bytes));
+  do_check_eq(json["[global]"].current, kTestEngineName);
+
+  // Then set the engine back to the default through the API.
+  Services.search.currentEngine =
+    Services.search.getEngineByName(getDefaultEngineName());
+  yield waitForNotification("write-metadata-to-disk-complete");
+
+  // Check that the current engine is no longer saved in the JSON file.
+  bytes = yield OS.File.read(path);
+  json = JSON.parse(new TextDecoder().decode(bytes));
+  do_check_eq(json["[global]"].current, "");
+});
+
+
+function run_test() {
+  removeMetadata();
+  removeCacheFile();
+
+  do_check_false(Services.search.isInitialized);
+
+  let engineDummyFile = gProfD.clone();
+  engineDummyFile.append("searchplugins");
+  engineDummyFile.append("test-search-engine.xml");
+  let engineDir = engineDummyFile.parent;
+  engineDir.create(Ci.nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
+
+  do_get_file("data/engine.xml").copyTo(engineDir, "engine.xml");
+
+  do_register_cleanup(function() {
+    removeMetadata();
+    removeCacheFile();
+  });
+
+  run_next_test();
+}

toolkit/components/search/tests/xpcshell/xpcshell.ini

@@ -32,7 +32,6 @@ support-files =
 [test_save_sorted_engines.js]
 [test_purpose.js]
 [test_defaultEngine.js]
-[test_prefSync.js]
 [test_notifications.js]
 [test_parseSubmissionURL.js]
 [test_SearchStaticData.js]
@@ -46,3 +45,4 @@ support-files =
 [test_sync_fallback.js]
 [test_sync_delay_fallback.js]
 [test_rel_searchform.js]
+[test_selectedEngine.js]