Bug 1123974 - geoip result should not override users already with browser.search.isUS=true. r=florian, a=sledru
authorMark Hammond <mhammond@skippinet.com.au>
Wed, 04 Feb 2015 14:20:19 +1100
changeset 243729 fafd3abc1d01
parent 243728 8174cebcfbbd
child 243730 2ad6015cdd1a
push id4454
push userryanvm@gmail.com
push date2015-02-09 19:34 +0000
treeherdermozilla-beta@8600a7b2e3a6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersflorian, sledru
bugs1123974
milestone36.0
Bug 1123974 - geoip result should not override users already with browser.search.isUS=true. r=florian, a=sledru
toolkit/components/search/nsSearchService.js
toolkit/components/search/tests/xpcshell/head_search.js
toolkit/components/search/tests/xpcshell/test_location.js
toolkit/components/search/tests/xpcshell/test_location_error.js
toolkit/components/search/tests/xpcshell/test_location_malformed_json.js
toolkit/components/search/tests/xpcshell/test_location_migrate_countrycode_isUS.js
toolkit/components/search/tests/xpcshell/test_location_migrate_no_countrycode_isUS.js
toolkit/components/search/tests/xpcshell/test_location_migrate_no_countrycode_notUS.js
toolkit/components/search/tests/xpcshell/test_location_sync.js
toolkit/components/search/tests/xpcshell/test_location_timeout.js
toolkit/components/search/tests/xpcshell/test_location_timeout_xhr.js
toolkit/components/search/tests/xpcshell/xpcshell.ini
--- a/toolkit/components/search/nsSearchService.js
+++ b/toolkit/components/search/nsSearchService.js
@@ -417,43 +417,94 @@ function geoSpecificDefaultsEnabled() {
   let distroID;
   try {
     distroID = Services.prefs.getCharPref("distribution.id");
   } catch (e) {}
 
   return (geoSpecificDefaults && !distroID);
 }
 
-// Hacky method that tries to determine if this user is in a US geography, and
-// using an en-US build.
-function getIsUS() {
-  // If we've set the pref before, just return that result.
-  let cachePref = "browser.search.isUS";
+// Some notes on countryCode and region prefs:
+// * A "countryCode" pref is set via a geoip lookup.  It always reflects the
+//   result of that geoip request.
+// * A "region" pref, once set, is the region actually used for search.  In
+//   most cases it will be identical to the countryCode pref.
+// * The value of "region" and "countryCode" will only not agree in one edge
+//   case - 34/35 users who have previously been configured to use US defaults
+//   based purely on a timezone check will have "region" forced to US,
+//   regardless of what countryCode geoip returns.
+// * We may want to know if we are in the US before we have *either*
+//   countryCode or region - in which case we fallback to a timezone check,
+//   but we don't persist that value anywhere in the expectation we will
+//   eventually get a countryCode/region.
+
+// A method that "migrates" prefs if necessary.
+function migrateRegionPrefs() {
+  // If we already have a "region" pref there's nothing to do.
+  if (Services.prefs.prefHasUserValue("browser.search.region")) {
+    return;
+  }
+
+  // If we have 'isUS' but no 'countryCode' then we are almost certainly
+  // a profile from Fx 34/35 that set 'isUS' based purely on a timezone
+  // check. If this said they were US, we force region to be US.
+  // (But if isUS was false, we leave region alone - we will do a geoip request
+  // and set the region accordingly)
   try {
-    return Services.prefs.getBoolPref(cachePref);
-  } catch(e) {}
-
+    if (Services.prefs.getBoolPref("browser.search.isUS") &&
+        !Services.prefs.prefHasUserValue("browser.search.countryCode")) {
+      Services.prefs.setCharPref("browser.search.region", "US");
+    }
+  } catch (ex) {
+    // no isUS pref, nothing to do.
+  }
+  // If we have a countryCode pref but no region pref, just force region
+  // to be the countryCode.
+  try {
+    let countryCode = Services.prefs.getCharPref("browser.search.countryCode");
+    if (!Services.prefs.prefHasUserValue("browser.search.region")) {
+      Services.prefs.setCharPref("browser.search.region", countryCode);
+    }
+  } catch (ex) {
+    // no countryCode pref, nothing to do.
+  }
+}
+
+// A method to determine if we are in the United States (US) for the search
+// service.
+// It uses a browser.search.region pref (which typically comes from a geoip
+// request) or if that doesn't exist, falls back to a hacky timezone check.
+function getIsUS() {
+  // Regardless of the region or countryCode, non en-US builds are not
+  // considered to be in the US from the POV of the search service.
   if (getLocale() != "en-US") {
-    Services.prefs.setBoolPref(cachePref, false);
     return false;
   }
+
+  // If we've got a region pref, trust it.
+  try {
+    return Services.prefs.getCharPref("browser.search.region") == "US";
+  } catch(e) {}
+
+  // So we are en-US but have no region pref - fallback to hacky timezone check.
   let isNA = isUSTimezone();
-  Services.prefs.setBoolPref(cachePref, isNA);
+  LOG("getIsUS() fell back to a timezone check with the result=" + isNA);
   return isNA;
 }
 
-// Helper method to modify preference keys with geo-specific modifiers, if needed
+// Helper method to modify preference keys with geo-specific modifiers, if needed.
 function getGeoSpecificPrefName(basepref) {
   if (!geoSpecificDefaultsEnabled())
     return basepref;
   if (getIsUS())
     return basepref + ".US";
   return basepref;
 }
 
+// A method that tries to determine if this user is in a US geography.
 function isUSTimezone() {
   // Timezone assumptions! We assume that if the system clock's timezone is
   // between Newfoundland and Hawaii, that the user is in North America.
 
   // This includes all of South America as well, but we have relatively few
   // en-US users there, so that's OK.
 
   // 150 minutes = 2.5 hours (UTC-2.5), which is
@@ -486,21 +537,19 @@ let ensureKnownCountryCode = Task.async(
   Services.telemetry.getHistogramById("SEARCH_SERVICE_COUNTRY_FETCH_CAUSED_SYNC_INIT").add(gInitialized);
 });
 
 // Store the result of the geoip request as well as any other values and
 // telemetry which depend on it.
 function storeCountryCode(cc) {
   // Set the country-code itself.
   Services.prefs.setCharPref("browser.search.countryCode", cc);
-  // and update our "isUS" cache pref if it is US - that will prevent a
-  // fallback to the timezone check.
-  // However, only do this if the locale also matches.
-  if (getLocale() == "en-US") {
-    Services.prefs.setBoolPref("browser.search.isUS", (cc == "US"));
+  // And set the region pref if we don't already have a value.
+  if (!Services.prefs.prefHasUserValue("browser.search.region")) {
+    Services.prefs.setCharPref("browser.search.region", cc);
   }
   // and telemetry...
   let isTimezoneUS = isUSTimezone();
   if (cc == "US" && !isTimezoneUS) {
     Services.telemetry.getHistogramById("SEARCH_SERVICE_US_COUNTRY_MISMATCHED_TIMEZONE").add(1);
   }
   if (cc != "US" && isTimezoneUS) {
     Services.telemetry.getHistogramById("SEARCH_SERVICE_US_TIMEZONE_MISMATCHED_COUNTRY").add(1);
@@ -515,16 +564,17 @@ function fetchCountryCode() {
     SUCCESS_WITHOUT_DATA: 1,
     XHRTIMEOUT: 2,
     ERROR: 3,
     // Note that we expect to add finer-grained error types here later (eg,
     // dns error, network error, ssl error, etc) with .ERROR remaining as the
     // generic catch-all that doesn't fit into other categories.
   };
   let endpoint = Services.urlFormatter.formatURLPref("browser.search.geoip.url");
+  LOG("_fetchCountryCode starting with endpoint " + endpoint);
   // As an escape hatch, no endpoint means no geoip.
   if (!endpoint) {
     return Promise.resolve();
   }
   let startTime = Date.now();
   return new Promise(resolve => {
     // Instead of using a timeout on the xhr object itself, we simulate one
     // using a timer and let the XHR request complete.  This allows us to
@@ -3126,16 +3176,17 @@ SearchService.prototype = {
   },
 
   // Synchronous implementation of the initializer.
   // Used by |_ensureInitialized| as a fallback if initialization is not
   // complete.
   _syncInit: function SRCH_SVC__syncInit() {
     LOG("_syncInit start");
     this._initStarted = true;
+    migrateRegionPrefs();
     try {
       this._syncLoadEngines();
     } catch (ex) {
       this._initRV = Cr.NS_ERROR_FAILURE;
       LOG("_syncInit: failure loading engines: " + ex);
     }
     this._addObservers();
 
@@ -3151,16 +3202,17 @@ SearchService.prototype = {
 
   /**
    * Asynchronous implementation of the initializer.
    *
    * @returns {Promise} A promise, resolved successfully if the initialization
    * succeeds.
    */
   _asyncInit: function SRCH_SVC__asyncInit() {
+    migrateRegionPrefs();
     return Task.spawn(function() {
       LOG("_asyncInit start");
       try {
         yield checkForSyncCompletion(ensureKnownCountryCode());
       } catch (ex if ex.result != Cr.NS_ERROR_ALREADY_INITIALIZED) {
         LOG("_asyncInit: failure determining country code: " + ex);
       }
       try {
--- a/toolkit/components/search/tests/xpcshell/head_search.js
+++ b/toolkit/components/search/tests/xpcshell/head_search.js
@@ -286,16 +286,40 @@ let addTestEngines = Task.async(function
       }
     });
   }
 
   return engines;
 });
 
 /**
+ * Installs a test engine into the test profile.
+ */
+function installTestEngine() {
+  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();
+  });
+}
+
+
+/**
  * Returns a promise that is resolved when an observer notification from the
  * search service fires with the specified data.
  *
  * @param aExpectedData
  *        The value the observer notification sends that causes us to resolve
  *        the promise.
  */
 function waitForSearchNotification(aExpectedData) {
--- a/toolkit/components/search/tests/xpcshell/test_location.js
+++ b/toolkit/components/search/tests/xpcshell/test_location.js
@@ -1,34 +1,20 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 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();
-  });
+  installTestEngine();
 
   Services.prefs.setCharPref("browser.search.geoip.url", 'data:application/json,{"country_code": "AU"}');
   Services.search.init(() => {
     equal(Services.prefs.getCharPref("browser.search.countryCode"), "AU", "got the correct country code.");
-    equal(Services.prefs.getBoolPref("browser.search.isUS"), false, "AU is not in the US.")
+    equal(Services.prefs.getCharPref("browser.search.region"), "AU", "region pref also set to the countryCode.")
+    // No isUS pref is ever written
+    ok(!Services.prefs.prefHasUserValue("browser.search.isUS"), "no isUS pref")
     // check we have "success" recorded in telemetry
     checkCountryResultTelemetry(TELEMETRY_RESULT_ENUM.SUCCESS);
     // a false value for each of SEARCH_SERVICE_COUNTRY_TIMEOUT and SEARCH_SERVICE_COUNTRY_FETCH_CAUSED_SYNC_INIT
     for (let hid of ["SEARCH_SERVICE_COUNTRY_TIMEOUT",
                      "SEARCH_SERVICE_COUNTRY_FETCH_CAUSED_SYNC_INIT"]) {
       let histogram = Services.telemetry.getHistogramById(hid);
       let snapshot = histogram.snapshot();
       deepEqual(snapshot.counts, [1,0,0]); // boolean probe so 3 buckets, expect 1 result for |0|.
--- a/toolkit/components/search/tests/xpcshell/test_location_error.js
+++ b/toolkit/components/search/tests/xpcshell/test_location_error.js
@@ -1,29 +1,13 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 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();
-  });
+  installTestEngine();
 
   // using a port > 2^32 causes an error to be reported.
   let url = "http://localhost:111111111";
 
   Services.prefs.setCharPref("browser.search.geoip.url", url);
   Services.search.init(() => {
     try {
       Services.prefs.getCharPref("browser.search.countryCode");
--- a/toolkit/components/search/tests/xpcshell/test_location_malformed_json.js
+++ b/toolkit/components/search/tests/xpcshell/test_location_malformed_json.js
@@ -1,53 +1,57 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
-function run_test() {
-  removeMetadata();
-  removeCacheFile();
-
-  do_check_false(Services.search.isInitialized);
+// A console listener so we can listen for a log message from nsSearchService.
+function promiseTimezoneMessage() {
+  return new Promise(resolve => {
+    let listener = {
+      QueryInterface: XPCOMUtils.generateQI([Ci.nsIConsoleListener]),
+      observe : function (msg) {
+        if (msg.message.startsWith("getIsUS() fell back to a timezone check with the result=")) {
+          Services.console.unregisterListener(listener);
+          resolve(msg);
+        }
+      }
+    };
+    Services.console.registerListener(listener);
+  });
+}
 
-  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);
+function run_test() {
+  installTestEngine();
 
-  do_get_file("data/engine.xml").copyTo(engineDir, "engine.xml");
-
-  do_register_cleanup(function() {
-    removeMetadata();
-    removeCacheFile();
-  });
+  // setup a console listener for the timezone fallback message.
+  let promiseTzMessage = promiseTimezoneMessage();
 
   // Here we have malformed JSON
   Services.prefs.setCharPref("browser.search.geoip.url", 'data:application/json,{"country_code"');
   Services.search.init(() => {
-    try {
-      Services.prefs.getCharPref("browser.search.countryCode");
-      ok(false, "should be no countryCode pref");
-    } catch (_) {}
-    try {
-      Services.prefs.getCharPref("browser.search.isUS");
-      ok(false, "should be no isUS pref yet either");
-    } catch (_) {}
-    // fetch the engines - this should force the timezone check
+    ok(!Services.prefs.prefHasUserValue("browser.search.countryCode"), "should be no countryCode pref");
+    ok(!Services.prefs.prefHasUserValue("browser.search.region"), "should be no region pref");
+    ok(!Services.prefs.prefHasUserValue("browser.search.isUS"), "should never be an isUS pref");
+    // fetch the engines - this should force the timezone check, but still
+    // doesn't persist any prefs.
     Services.search.getEngines();
-    equal(Services.prefs.getBoolPref("browser.search.isUS"),
-          isUSTimezone(),
-          "should have set isUS based on current timezone.");
+    ok(!Services.prefs.prefHasUserValue("browser.search.countryCode"), "should be no countryCode pref");
+    ok(!Services.prefs.prefHasUserValue("browser.search.region"), "should be no region pref");
+    ok(!Services.prefs.prefHasUserValue("browser.search.isUS"), "should never be an isUS pref");
     // should have recorded SUCCESS_WITHOUT_DATA
     checkCountryResultTelemetry(TELEMETRY_RESULT_ENUM.SUCCESS_WITHOUT_DATA);
     // and false values for timeout and forced-sync-init.
     for (let hid of ["SEARCH_SERVICE_COUNTRY_TIMEOUT",
                      "SEARCH_SERVICE_COUNTRY_FETCH_CAUSED_SYNC_INIT"]) {
       let histogram = Services.telemetry.getHistogramById(hid);
       let snapshot = histogram.snapshot();
       deepEqual(snapshot.counts, [1,0,0]); // boolean probe so 3 buckets, expect 1 result for |0|.
     }
 
-    do_test_finished();
-    run_next_test();
+    // Check we saw the timezone fallback message.
+    promiseTzMessage.then(msg => {
+      print("Timezone message:", msg.message);
+      ok(msg.message.endsWith(isUSTimezone().toString()), "fell back to timezone and it matches our timezone");
+      do_test_finished();
+      run_next_test();
+    });
   });
   do_test_pending();
 }
new file mode 100644
--- /dev/null
+++ b/toolkit/components/search/tests/xpcshell/test_location_migrate_countrycode_isUS.js
@@ -0,0 +1,24 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+// Here we are testing the "migration" when both isUS and countryCode are
+// set.
+function run_test() {
+  installTestEngine();
+
+  // Set the prefs we care about.
+  Services.prefs.setBoolPref("browser.search.isUS", true);
+  Services.prefs.setCharPref("browser.search.countryCode", "US");
+  // And the geoip request that will return AU - it shouldn't be used.
+  Services.prefs.setCharPref("browser.search.geoip.url", 'data:application/json,{"country_code": "AU"}');
+  Services.search.init(() => {
+    // "region" and countryCode should still reflect US.
+    equal(Services.prefs.getCharPref("browser.search.region"), "US", "got the correct region.");
+    equal(Services.prefs.getCharPref("browser.search.countryCode"), "US", "got the correct country code.");
+    // should be no geoip evidence.
+    checkCountryResultTelemetry(null);
+    do_test_finished();
+    run_next_test();
+  });
+  do_test_pending();
+}
copy from toolkit/components/search/tests/xpcshell/test_location.js
copy to toolkit/components/search/tests/xpcshell/test_location_migrate_no_countrycode_isUS.js
--- a/toolkit/components/search/tests/xpcshell/test_location.js
+++ b/toolkit/components/search/tests/xpcshell/test_location_migrate_no_countrycode_isUS.js
@@ -1,34 +1,24 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
+// Here we are testing the "migration" from the isUS pref being true but when
+// no country-code exists.
 function run_test() {
-  removeMetadata();
-  removeCacheFile();
-
-  do_check_false(Services.search.isInitialized);
+  installTestEngine();
 
-  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();
-  });
-
+  // Set the pref we care about.
+  Services.prefs.setBoolPref("browser.search.isUS", true);
+  // And the geoip request that will return *not* US.
   Services.prefs.setCharPref("browser.search.geoip.url", 'data:application/json,{"country_code": "AU"}');
   Services.search.init(() => {
+    // "region" should be set to US, but countryCode to AU.
+    equal(Services.prefs.getCharPref("browser.search.region"), "US", "got the correct region.");
     equal(Services.prefs.getCharPref("browser.search.countryCode"), "AU", "got the correct country code.");
-    equal(Services.prefs.getBoolPref("browser.search.isUS"), false, "AU is not in the US.")
     // check we have "success" recorded in telemetry
     checkCountryResultTelemetry(TELEMETRY_RESULT_ENUM.SUCCESS);
     // a false value for each of SEARCH_SERVICE_COUNTRY_TIMEOUT and SEARCH_SERVICE_COUNTRY_FETCH_CAUSED_SYNC_INIT
     for (let hid of ["SEARCH_SERVICE_COUNTRY_TIMEOUT",
                      "SEARCH_SERVICE_COUNTRY_FETCH_CAUSED_SYNC_INIT"]) {
       let histogram = Services.telemetry.getHistogramById(hid);
       let snapshot = histogram.snapshot();
       deepEqual(snapshot.counts, [1,0,0]); // boolean probe so 3 buckets, expect 1 result for |0|.
copy from toolkit/components/search/tests/xpcshell/test_location.js
copy to toolkit/components/search/tests/xpcshell/test_location_migrate_no_countrycode_notUS.js
--- a/toolkit/components/search/tests/xpcshell/test_location.js
+++ b/toolkit/components/search/tests/xpcshell/test_location_migrate_no_countrycode_notUS.js
@@ -1,34 +1,24 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
+// Here we are testing the "migration" from the isUS pref being false but when
+// no country-code exists.
 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);
+  installTestEngine();
 
-  do_get_file("data/engine.xml").copyTo(engineDir, "engine.xml");
-
-  do_register_cleanup(function() {
-    removeMetadata();
-    removeCacheFile();
-  });
-
-  Services.prefs.setCharPref("browser.search.geoip.url", 'data:application/json,{"country_code": "AU"}');
+  // Set the pref we care about.
+  Services.prefs.setBoolPref("browser.search.isUS", false);
+  // And the geoip request that will return US.
+  Services.prefs.setCharPref("browser.search.geoip.url", 'data:application/json,{"country_code": "US"}');
   Services.search.init(() => {
-    equal(Services.prefs.getCharPref("browser.search.countryCode"), "AU", "got the correct country code.");
-    equal(Services.prefs.getBoolPref("browser.search.isUS"), false, "AU is not in the US.")
+    // "region" and countryCode should reflect US.
+    equal(Services.prefs.getCharPref("browser.search.region"), "US", "got the correct region.");
+    equal(Services.prefs.getCharPref("browser.search.countryCode"), "US", "got the correct country code.");
     // check we have "success" recorded in telemetry
     checkCountryResultTelemetry(TELEMETRY_RESULT_ENUM.SUCCESS);
     // a false value for each of SEARCH_SERVICE_COUNTRY_TIMEOUT and SEARCH_SERVICE_COUNTRY_FETCH_CAUSED_SYNC_INIT
     for (let hid of ["SEARCH_SERVICE_COUNTRY_TIMEOUT",
                      "SEARCH_SERVICE_COUNTRY_FETCH_CAUSED_SYNC_INIT"]) {
       let histogram = Services.telemetry.getHistogramById(hid);
       let snapshot = histogram.snapshot();
       deepEqual(snapshot.counts, [1,0,0]); // boolean probe so 3 buckets, expect 1 result for |0|.
--- a/toolkit/components/search/tests/xpcshell/test_location_sync.js
+++ b/toolkit/components/search/tests/xpcshell/test_location_sync.js
@@ -12,60 +12,66 @@ function getCountryCodePref() {
 function getIsUSPref() {
   try {
     return Services.prefs.getBoolPref("browser.search.isUS");
   } catch (_) {
     return undefined;
   }
 }
 
-function run_test() {
-  removeMetadata();
-  removeCacheFile();
-
-  ok(!Services.search.isInitialized);
+// A console listener so we can listen for a log message from nsSearchService.
+function promiseTimezoneMessage() {
+  return new Promise(resolve => {
+    let listener = {
+      QueryInterface: XPCOMUtils.generateQI([Ci.nsIConsoleListener]),
+      observe : function (msg) {
+        if (msg.message.startsWith("getIsUS() fell back to a timezone check with the result=")) {
+          Services.console.unregisterListener(listener);
+          resolve(msg);
+        }
+      }
+    };
+    Services.console.registerListener(listener);
+  });
+}
 
-  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();
-  });
+function run_test() {
+  installTestEngine();
 
   run_next_test();
 }
 
 // Force a sync init and ensure the right thing happens (ie, that no xhr
 // request is made and we fall back to the timezone-only trick)
 add_task(function* test_simple() {
   deepEqual(getCountryCodePref(), undefined, "no countryCode pref");
   deepEqual(getIsUSPref(), undefined, "no isUS pref");
 
   // Still set a geoip pref so we can (indirectly) check it wasn't used.
   Services.prefs.setCharPref("browser.search.geoip.url", 'data:application/json,{"country_code": "AU"}');
 
   ok(!Services.search.isInitialized);
 
+  // setup a console listener for the timezone fallback message.
+  let promiseTzMessage = promiseTimezoneMessage();
+
   // fetching the engines forces a sync init, and should have caused us to
   // check the timezone.
   let engines = Services.search.getEngines();
   ok(Services.search.isInitialized);
-  deepEqual(getIsUSPref(), isUSTimezone(), "isUS pref was set by sync init.");
 
   // a little wait to check we didn't do the xhr thang.
   yield new Promise(resolve => {
     do_timeout(500, resolve);
   });
 
+  let msg = yield promiseTzMessage;
+  print("Timezone message:", msg.message);
+  ok(msg.message.endsWith(isUSTimezone().toString()), "fell back to timezone and it matches our timezone");
+
   deepEqual(getCountryCodePref(), undefined, "didn't do the geoip xhr");
   // and no telemetry evidence of geoip.
   for (let hid of [
     "SEARCH_SERVICE_COUNTRY_FETCH_RESULT",
     "SEARCH_SERVICE_COUNTRY_FETCH_TIME_MS",
     "SEARCH_SERVICE_COUNTRY_TIMEOUT",
     "SEARCH_SERVICE_US_COUNTRY_MISMATCHED_TIMEZONE",
     "SEARCH_SERVICE_US_TIMEZONE_MISMATCHED_COUNTRY",
--- a/toolkit/components/search/tests/xpcshell/test_location_timeout.js
+++ b/toolkit/components/search/tests/xpcshell/test_location_timeout.js
@@ -23,48 +23,30 @@ function startServer(continuePromise) {
 }
 
 function getProbeSum(probe, sum) {
   let histogram = Services.telemetry.getHistogramById(probe);
   return histogram.snapshot().sum;
 }
 
 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();
-  });
+  installTestEngine();
 
   let resolveContinuePromise;
   let continuePromise = new Promise(resolve => {
     resolveContinuePromise = resolve;
   });
 
   let server = startServer(continuePromise);
   let url = "http://localhost:" + server.identity.primaryPort + "/lookup_country";
   Services.prefs.setCharPref("browser.search.geoip.url", url);
   Services.prefs.setIntPref("browser.search.geoip.timeout", 50);
   Services.search.init(() => {
-    try {
-      Services.prefs.getCharPref("browser.search.countryCode");
-      ok(false, "not expecting countryCode to be set");
-    } catch (ex) {}
+    ok(!Services.prefs.prefHasUserValue("browser.search.countryCode"), "should be no countryCode pref");
+    ok(!Services.prefs.prefHasUserValue("browser.search.region"), "should be no region pref");
     // should be no result recorded at all.
     checkCountryResultTelemetry(null);
 
     // should have set the flag indicating we saw a timeout.
     let histogram = Services.telemetry.getHistogramById("SEARCH_SERVICE_COUNTRY_TIMEOUT");
     let snapshot = histogram.snapshot();
     deepEqual(snapshot.counts, [0,1,0]);
     // should not yet have SEARCH_SERVICE_COUNTRY_FETCH_TIME_MS recorded as our
@@ -77,17 +59,18 @@ function run_test() {
       // The telemetry "sum" will be the actual time in ms - just check it's non-zero.
       ok(getProbeSum("SEARCH_SERVICE_COUNTRY_FETCH_TIME_MS") != 0);
       // should have reported the fetch ended up being successful
       checkCountryResultTelemetry(TELEMETRY_RESULT_ENUM.SUCCESS);
 
       // and should have the result of the response that finally came in, and
       // everything dependent should also be updated.
       equal(Services.prefs.getCharPref("browser.search.countryCode"), "AU");
-      equal(Services.prefs.getBoolPref("browser.search.isUS"), false);
+      equal(Services.prefs.getCharPref("browser.search.region"), "AU");
+      ok(!Services.prefs.prefHasUserValue("browser.search.isUS"), "should never have an isUS pref");
 
       do_test_finished();
       server.stop(run_next_test);
     });
     // now tell the server to send its response.  That will end up causing the
     // search service to notify of that the response was received.
     resolveContinuePromise();
   });
--- a/toolkit/components/search/tests/xpcshell/test_location_timeout_xhr.js
+++ b/toolkit/components/search/tests/xpcshell/test_location_timeout_xhr.js
@@ -25,50 +25,32 @@ function startServer(continuePromise) {
 
 function verifyProbeSum(probe, sum) {
   let histogram = Services.telemetry.getHistogramById(probe);
   let snapshot = histogram.snapshot();
   equal(snapshot.sum, sum, probe);
 }
 
 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();
-  });
+  installTestEngine();
 
   let resolveContinuePromise;
   let continuePromise = new Promise(resolve => {
     resolveContinuePromise = resolve;
   });
 
   let server = startServer(continuePromise);
   let url = "http://localhost:" + server.identity.primaryPort + "/lookup_country";
   Services.prefs.setCharPref("browser.search.geoip.url", url);
   // The timeout for the timer.
   Services.prefs.setIntPref("browser.search.geoip.timeout", 10);
   let promiseXHRStarted = waitForSearchNotification("geoip-lookup-xhr-starting");
   Services.search.init(() => {
-    try {
-      Services.prefs.getCharPref("browser.search.countryCode");
-      ok(false, "not expecting countryCode to be set");
-    } catch (ex) {}
+    ok(!Services.prefs.prefHasUserValue("browser.search.countryCode"), "should be no countryCode pref");
+    ok(!Services.prefs.prefHasUserValue("browser.search.region"), "should be no region pref");
     // should be no result recorded at all.
     checkCountryResultTelemetry(null);
 
     // should have set the flag indicating we saw a timeout.
     let histogram = Services.telemetry.getHistogramById("SEARCH_SERVICE_COUNTRY_TIMEOUT");
     let snapshot = histogram.snapshot();
     deepEqual(snapshot.counts, [0,1,0]);
 
@@ -82,21 +64,19 @@ function run_test() {
       xhr.timeout = 10;
       // wait for the xhr timeout to fire.
       waitForSearchNotification("geoip-lookup-xhr-complete").then(() => {
         // should have the XHR timeout recorded.
         checkCountryResultTelemetry(TELEMETRY_RESULT_ENUM.XHRTIMEOUT);
         // still should not have a report of how long the response took as we
         // only record that on success responses.
         verifyProbeSum("SEARCH_SERVICE_COUNTRY_FETCH_TIME_MS", 0);
-        // and we don't know the country code.
-        try {
-          Services.prefs.getCharPref("browser.search.countryCode");
-          ok(false, "not expecting countryCode to be set");
-        } catch (ex) {}
+        // and we still don't know the country code or region.
+        ok(!Services.prefs.prefHasUserValue("browser.search.countryCode"), "should be no countryCode pref");
+        ok(!Services.prefs.prefHasUserValue("browser.search.region"), "should be no region pref");
 
         // unblock the server even though nothing is listening.
         resolveContinuePromise();
 
         do_test_finished();
         server.stop(run_next_test);
       });
     });
--- a/toolkit/components/search/tests/xpcshell/xpcshell.ini
+++ b/toolkit/components/search/tests/xpcshell/xpcshell.ini
@@ -26,16 +26,19 @@ support-files =
 [test_identifiers.js]
 [test_invalid_engine_from_dir.js]
 [test_init_async_multiple.js]
 [test_init_async_multiple_then_sync.js]
 [test_json_cache.js]
 [test_location.js]
 [test_location_error.js]
 [test_location_malformed_json.js]
+[test_location_migrate_countrycode_isUS.js]
+[test_location_migrate_no_countrycode_isUS.js]
+[test_location_migrate_no_countrycode_notUS.js]
 [test_location_sync.js]
 [test_location_timeout.js]
 [test_location_timeout_xhr.js]
 [test_nodb.js]
 [test_nodb_pluschanges.js]
 [test_save_sorted_engines.js]
 [test_purpose.js]
 [test_defaultEngine.js]