Bug 1702759 - Remove load_dump preference r=robwu

This is a breaking change. It is not possible to control the loading of dumps from a preference anymore.

Dumps are only loaded if server is pointed at PROD. The recommended way to prevent dumps loading in tests is to store a high timestamp in the local DB.

Differential Revision: https://phabricator.services.mozilla.com/D146049

browser/components/newtab/docs/v2-system-addon/remote_cfr.md

@@ -61,8 +61,6 @@ Until [support for the DEV environment](https://github.com/mozilla-extensions/re
   Services.prefs.setCharPref("services.settings.server", "https://settings.dev.mozaws.net/v1");
   // Dev collections are signed with the STAGE infrastructure, use STAGE's hash:
   Services.prefs.setCharPref("security.content.signature.root_hash" "3C:01:44:6A:BE:90:36:CE:A9:A0:9A:CA:A3:A5:20:AC:62:8F:20:A7:AE:32:CE:86:1C:B2:EF:B7:0F:A0:C7:45");
-  // Prevent packaged dumps to interfere.
-  Services.prefs.setBoolPref("services.settings.load_dump", false);
   // Pull data from the preview bucket.
   RemoteSettings.enablePreviewMode(true);
 ```

modules/libpref/init/all.js

@@ -2055,7 +2055,6 @@ pref("security.insecure_field_warning.ignore_local_ip_address", true);
 
 // Remote settings preferences
 pref("services.settings.poll_interval", 86400); // 24H
-pref("services.settings.server", "https://firefox.settings.services.mozilla.com/v1");
 
 // The percentage of clients who will report uptake telemetry as
 // events instead of just a histogram. This only applies on Release;

services/settings/RemoteSettingsClient.jsm

@@ -30,13 +30,6 @@ const TELEMETRY_COMPONENT = "remotesettings";
 
 XPCOMUtils.defineLazyGetter(this, "console", () => Utils.log);
 
-XPCOMUtils.defineLazyPreferenceGetter(
-  this,
-  "gLoadDump",
-  "services.settings.load_dump",
-  true
-);
-
 /**
  * cacheProxy returns an object Proxy that will memoize properties of the target.
  * @param {Object} target the object to wrap.
@@ -359,7 +352,7 @@ class RemoteSettingsClient extends EventEmitter {
         if (!this._importingPromise) {
           // Prevent parallel loading when .get() is called multiple times.
           this._importingPromise = (async () => {
-            const importedFromDump = gLoadDump
+            const importedFromDump = Utils.LOAD_DUMPS
               ? await this._importJSONDump()
               : -1;
             if (importedFromDump < 0) {
@@ -521,8 +514,7 @@ class RemoteSettingsClient extends EventEmitter {
    *                                    This will be compared to the local timestamp, and will be used for
    *                                    cache busting if local data is out of date.
    * @param {Object} options            additional advanced options.
-   * @param {bool}   options.loadDump   load initial dump from disk on first sync (default: true, unless
-   *                                    `services.settings.load_dump` says otherwise).
+   * @param {bool}   options.loadDump   load initial dump from disk on first sync (default: true if server is prod)
    * @param {bool}   options.sendEvents send `"sync"` events (default: `true`)
    * @param {string} options.trigger    label to identify what triggered this sync (eg. ``"timer"``, default: `"manual"`)
    * @return {Promise}                  which rejects on sync or process failure.
@@ -530,7 +522,7 @@ class RemoteSettingsClient extends EventEmitter {
   async maybeSync(expectedTimestamp, options = {}) {
     // Should the clients try to load JSON dump? (mainly disabled in tests)
     const {
-      loadDump = gLoadDump,
+      loadDump = Utils.LOAD_DUMPS,
       trigger = "manual",
       sendEvents = true,
     } = options;

services/settings/Utils.jsm

@@ -45,19 +45,7 @@ XPCOMUtils.defineLazyGetter(this, "log", () => {
   });
 });
 
-// Overriding the server URL is normally disabled on Beta and Release channels,
-// except under some conditions.
-XPCOMUtils.defineLazyGetter(this, "allowServerURLOverride", () => {
-  if (!AppConstants.RELEASE_OR_BETA) {
-    // Always allow to override the server URL on Nightly/DevEdition.
-    return true;
-  }
-
-  if (AppConstants.MOZ_APP_NAME === "thunderbird") {
-    // Always allow to override the server URL for Thunderbird.
-    return true;
-  }
-
+XPCOMUtils.defineLazyGetter(this, "isRunningTests", () => {
   const env = Cc["@mozilla.org/process/environment;1"].getService(
     Ci.nsIEnvironment
   );
@@ -66,6 +54,24 @@ XPCOMUtils.defineLazyGetter(this, "allowServerURLOverride", () => {
     // usually true when running tests.
     return true;
   }
+  return false;
+});
+
+// Overriding the server URL is normally disabled on Beta and Release channels,
+// except under some conditions.
+XPCOMUtils.defineLazyGetter(this, "allowServerURLOverride", () => {
+  if (!AppConstants.RELEASE_OR_BETA) {
+    // Always allow to override the server URL on Nightly/DevEdition.
+    return true;
+  }
+
+  if (isRunningTests) {
+    return true;
+  }
+
+  const env = Cc["@mozilla.org/process/environment;1"].getService(
+    Ci.nsIEnvironment
+  );
 
   if (env.get("MOZ_REMOTE_SETTINGS_DEVTOOLS") === "1") {
     // Allow to override the server URL when using remote settings devtools.
@@ -78,7 +84,8 @@ XPCOMUtils.defineLazyGetter(this, "allowServerURLOverride", () => {
 XPCOMUtils.defineLazyPreferenceGetter(
   this,
   "gServerURL",
-  "services.settings.server"
+  "services.settings.server",
+  AppConstants.REMOTE_SETTINGS_SERVER_URL
 );
 
 XPCOMUtils.defineLazyPreferenceGetter(
@@ -96,7 +103,7 @@ var Utils = {
   get SERVER_URL() {
     return allowServerURLOverride
       ? gServerURL
-      : "https://firefox.settings.services.mozilla.com/v1";
+      : AppConstants.REMOTE_SETTINGS_SERVER_URL;
   },
 
   CHANGES_PATH: "/buckets/monitor/collections/changes/changeset",
@@ -106,6 +113,14 @@ var Utils = {
    */
   log,
 
+  get LOAD_DUMPS() {
+    // Load dumps only if pulling data from the production server, or in tests.
+    return (
+      this.SERVER_URL == AppConstants.REMOTE_SETTINGS_SERVER_URL ||
+      isRunningTests
+    );
+  },
+
   get PREVIEW_MODE() {
     // We want to offer the ability to set preview mode via a preference
     // for consumers who want to pull from the preview bucket on startup.

services/settings/test/unit/test_remote_settings.js

@@ -266,16 +266,6 @@ add_task(
 );
 add_task(clear_state);
 
-add_task(async function test_get_does_not_load_dump_when_pref_is_false() {
-  Services.prefs.setBoolPref("services.settings.load_dump", false);
-
-  const data = await clientWithDump.get();
-
-  equal(data.map(r => r.id).join(", "), "pt-BR, xx"); // No dump, 2 pulled from test server.
-  Services.prefs.clearUserPref("services.settings.load_dump");
-});
-add_task(clear_state);
-
 add_task(async function test_get_loads_dump_only_once_if_called_in_parallel() {
   const backup = clientWithDump._importJSONDump;
   let callCount = 0;

services/settings/test/unit/test_remote_settings_release_prefs.js

@@ -103,6 +103,49 @@ add_task(
 );
 add_task(clear_state);
 
+add_task(
+  {
+    skip_if: () => !AppConstants.RELEASE_OR_BETA,
+  },
+  async function test_load_dumps_will_always_be_loaded_in_release() {
+    Services.prefs.setCharPref(
+      "services.settings.server",
+      "http://localhost:8888/v1"
+    );
+
+    const { Utils } = ChromeUtils.import(UTILS_MODULE);
+
+    Assert.equal(
+      Utils.SERVER_URL,
+      AppConstants.REMOTE_SETTINGS_SERVER_URL,
+      "Server url pref was not read"
+    );
+    Assert.ok(Utils.LOAD_DUMPS, "Dumps will always be loaded");
+  }
+);
+
+add_task(
+  {
+    skip_if: () => AppConstants.RELEASE_OR_BETA,
+  },
+  async function test_load_dumps_can_be_disabled_in_dev_nightly() {
+    Services.prefs.setCharPref(
+      "services.settings.server",
+      "http://localhost:8888/v1"
+    );
+
+    const { Utils } = ChromeUtils.import(UTILS_MODULE);
+
+    Assert.notEqual(
+      Utils.SERVER_URL,
+      AppConstants.REMOTE_SETTINGS_SERVER_URL,
+      "Server url pref was read"
+    );
+    Assert.ok(!Utils.LOAD_DUMPS, "Dumps are not loaded if server is not prod");
+  }
+);
+add_task(clear_state);
+
 add_task(
   async function test_server_url_can_be_changed_in_all_versions_if_running_for_devtools() {
     env.set("MOZ_REMOTE_SETTINGS_DEVTOOLS", "1");
@@ -133,3 +176,32 @@ add_task(
   }
 );
 add_task(clear_state);
+
+add_task(
+  async function test_dumps_are_not_loaded_if_server_is_not_prod_if_running_for_devtools() {
+    env.set("MOZ_REMOTE_SETTINGS_DEVTOOLS", "1");
+    Services.prefs.setCharPref(
+      "services.settings.server",
+      "http://localhost:8888/v1"
+    );
+
+    const { Utils } = ChromeUtils.import(UTILS_MODULE);
+    Assert.ok(!Utils.LOAD_DUMPS, "Dumps won't be loaded");
+  }
+);
+add_task(clear_state);
+
+add_task(
+  async function test_dumps_are_loaded_if_server_is_prod_if_running_for_devtools() {
+    env.set("MOZ_REMOTE_SETTINGS_DEVTOOLS", "1");
+    Services.prefs.setCharPref(
+      "services.settings.server",
+      AppConstants.REMOTE_SETTINGS_SERVER_URL
+    );
+
+    const { Utils } = ChromeUtils.import(UTILS_MODULE);
+
+    Assert.ok(Utils.LOAD_DUMPS, "dumps are loaded if prod");
+  }
+);
+add_task(clear_state);

toolkit/modules/AppConstants.jsm

@@ -418,6 +418,13 @@ this.AppConstants = Object.freeze({
     false,
 #endif
 
+  REMOTE_SETTINGS_SERVER_URL:
+#ifdef MOZ_THUNDERBIRD
+    "https://thunderbird-settings.thunderbird.net/v1",
+#else
+    "https://firefox.settings.services.mozilla.com/v1",
+#endif
+
   REMOTE_SETTINGS_VERIFY_SIGNATURE:
 #ifdef MOZ_THUNDERBIRD
     false,

toolkit/mozapps/extensions/internal/AddonTestUtils.jsm

@@ -686,21 +686,6 @@ var AddonTestUtils = {
       extensionsMLBF: BlocklistPrivate.ExtensionBlocklistMLBF,
     };
 
-    // Since we load the specified test data, we shouldn't let the
-    // packaged JSON dumps to interfere.
-    const pref = "services.settings.load_dump";
-    const backup = Services.prefs.getBoolPref(pref, null);
-    Services.prefs.setBoolPref(pref, false);
-    if (this.testScope) {
-      this.testScope.registerCleanupFunction(() => {
-        if (backup === null) {
-          Services.prefs.clearUserPref(pref);
-        } else {
-          Services.prefs.setBoolPref(pref, backup);
-        }
-      });
-    }
-
     for (const [dataProp, blocklistObj] of Object.entries(blocklistMapping)) {
       let newData = data[dataProp];
       if (!newData) {