Bug 1399936: Migrate active preference experiment values to new storage. draft
authorMichael Kelly <mkelly@mozilla.com>
Mon, 18 Sep 2017 21:41:51 -0700
changeset 666751 b1af72c9087826ef14465c99f89a6b1d35505bb8
parent 666750 28fadaace7bd2189a6c13db6765bf04048a41789
child 667155 992785025587ec887b08c907dccf63b9a96c0645
push id80488
push userbmo:mkelly@mozilla.com
push dateTue, 19 Sep 2017 04:42:30 +0000
bugs1399936
milestone56.0
Bug 1399936: Migrate active preference experiment values to new storage. If a user is enrolled in a preference experiment when Firefox upgrades, the pref branch for storing experiment values will be empty on startup, causing no prefs to be set. This, in turn, triggers the check in PreferenceExperiments.init for prefs that differ from their experimental value, and unenrolls the user. The solution is to store a pref flag marking if the user has been migrated. If they have not, we take a one-time hit to startup perf and load the experiment storage, saving it to the new pref storage and marking the user as migrated. MozReview-Commit-ID: 74VHHt18qnw
browser/extensions/shield-recipe-client/bootstrap.js
browser/extensions/shield-recipe-client/test/browser/browser_PreferenceExperiments.js
browser/extensions/shield-recipe-client/test/browser/browser_bootstrap.js
browser/extensions/shield-recipe-client/test/browser/head.js
--- a/browser/extensions/shield-recipe-client/bootstrap.js
+++ b/browser/extensions/shield-recipe-client/bootstrap.js
@@ -8,23 +8,26 @@ Cu.import("resource://gre/modules/AppCon
 Cu.import("resource://gre/modules/Log.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 
 XPCOMUtils.defineLazyModuleGetter(this, "LogManager",
   "resource://shield-recipe-client/lib/LogManager.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "ShieldRecipeClient",
   "resource://shield-recipe-client/lib/ShieldRecipeClient.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "PreferenceExperiments",
+  "resource://shield-recipe-client/lib/PreferenceExperiments.jsm");
 
 // Act as both a normal bootstrap.js and a JS module so that we can test
 // startup methods without having to install/uninstall the add-on.
 this.EXPORTED_SYMBOLS = ["Bootstrap"];
 
 const REASON_APP_STARTUP = 1;
 const UI_AVAILABLE_NOTIFICATION = "sessionstore-windows-restored";
+const STARTUP_EXPERIMENT_MIGRATED_PREF = "extensions.shield-recipe-client.startupExperimentMigrated";
 const STARTUP_EXPERIMENT_PREFS_BRANCH = "extensions.shield-recipe-client.startupExperimentPrefs.";
 const PREF_LOGGING_LEVEL = "extensions.shield-recipe-client.logging.level";
 const BOOTSTRAP_LOGGER_NAME = "extensions.shield-recipe-client.bootstrap";
 const DEFAULT_PREFS = {
   "extensions.shield-recipe-client.api_url": "https://normandy.cdn.mozilla.net/api/v1",
   "extensions.shield-recipe-client.dev_mode": false,
   "extensions.shield-recipe-client.enabled": true,
   "extensions.shield-recipe-client.startup_delay_seconds": 300,
@@ -58,20 +61,28 @@ this.Bootstrap = {
           prefBranch.setBoolPref(name, value);
           break;
         default:
           throw new Error(`Invalid default preference type ${typeof value}`);
       }
     }
   },
 
-  initExperimentPrefs() {
+  async initExperimentPrefs() {
     const defaultBranch = Services.prefs.getDefaultBranch("");
     const experimentBranch = Services.prefs.getBranch(STARTUP_EXPERIMENT_PREFS_BRANCH);
 
+    // If the user has upgraded from a Shield version that doesn't save experiment prefs on
+    // shutdown, we need to manually import them. This incurs a one-time startup i/o hit, but we
+    // already had this startup i/o hit anyway in the affected versions. (bug 1399936)
+    if (!Services.prefs.getBoolPref(STARTUP_EXPERIMENT_MIGRATED_PREF, false)) {
+      await PreferenceExperiments.saveStartupPrefs();
+      Services.prefs.setBoolPref(STARTUP_EXPERIMENT_MIGRATED_PREF, true);
+    }
+
     for (const prefName of experimentBranch.getChildList("")) {
       const experimentPrefType = experimentBranch.getPrefType(prefName);
       const realPrefType = defaultBranch.getPrefType(prefName);
 
       if (realPrefType !== Services.prefs.PREF_INVALID && realPrefType !== experimentPrefType) {
         log.error(`Error setting startup pref ${prefName}; pref type does not match.`);
         continue;
       }
@@ -107,20 +118,20 @@ this.Bootstrap = {
       ShieldRecipeClient.startup();
     }
   },
 
   install() {
     // Nothing to do during install
   },
 
-  startup(data, reason) {
+  async startup(data, reason) {
     // Initialization that needs to happen before the first paint on startup.
     this.initShieldPrefs(DEFAULT_PREFS);
-    this.initExperimentPrefs();
+    await this.initExperimentPrefs();
 
     // If the app is starting up, wait until the UI is available before finishing
     // init.
     if (reason === REASON_APP_STARTUP) {
       Services.obs.addObserver(this, UI_AVAILABLE_NOTIFICATION);
     } else {
       ShieldRecipeClient.startup();
     }
--- a/browser/extensions/shield-recipe-client/test/browser/browser_PreferenceExperiments.js
+++ b/browser/extensions/shield-recipe-client/test/browser/browser_PreferenceExperiments.js
@@ -4,30 +4,16 @@ Cu.import("resource://gre/modules/Prefer
 Cu.import("resource://gre/modules/TelemetryEnvironment.jsm", this);
 Cu.import("resource://shield-recipe-client/lib/PreferenceExperiments.jsm", this);
 
 // Save ourselves some typing
 const {withMockExperiments} = PreferenceExperiments;
 const DefaultPreferences = new Preferences({defaultBranch: true});
 const startupPrefs = "extensions.shield-recipe-client.startupExperimentPrefs";
 
-function experimentFactory(attrs) {
-  return Object.assign({
-    name: "fakename",
-    branch: "fakebranch",
-    expired: false,
-    lastSeen: new Date().toJSON(),
-    preferenceName: "fake.preference",
-    preferenceValue: "falkevalue",
-    preferenceType: "string",
-    previousPreferenceValue: "oldfakevalue",
-    preferenceBranchType: "default",
-  }, attrs);
-}
-
 // clearAllExperimentStorage
 add_task(withMockExperiments(async function(experiments) {
   experiments["test"] = experimentFactory({name: "test"});
   ok(await PreferenceExperiments.has("test"), "Mock experiment is detected.");
   await PreferenceExperiments.clearAllExperimentStorage();
   ok(
     !(await PreferenceExperiments.has("test")),
     "clearAllExperimentStorage removed all stored experiments",
--- a/browser/extensions/shield-recipe-client/test/browser/browser_bootstrap.js
+++ b/browser/extensions/shield-recipe-client/test/browser/browser_bootstrap.js
@@ -1,11 +1,12 @@
 "use strict";
 
 Cu.import("resource://shield-recipe-client/lib/ShieldRecipeClient.jsm", this);
+Cu.import("resource://shield-recipe-client/lib/PreferenceExperiments.jsm", this);
 
 // We can't import bootstrap.js directly since it isn't in the jar manifest, but
 // we can use Addon.getResourceURI to get a path to the file and import using
 // that instead.
 Cu.import("resource://gre/modules/AddonManager.jsm", this);
 const bootstrapPromise = AddonManager.getAddonByID("shield-recipe-client@mozilla.org").then(addon => {
   const bootstrapUri = addon.getResourceURI("bootstrap.js");
   const {Bootstrap} = Cu.import(bootstrapUri.spec, {});
@@ -166,17 +167,17 @@ decorate_task(
     );
   },
 );
 
 decorate_task(
   withBootstrap,
   withStub(ShieldRecipeClient, "startup"),
   async function testStartupDelayed(Bootstrap, startupStub) {
-    Bootstrap.startup(undefined, 1); // 1 == APP_STARTUP
+    await Bootstrap.startup(undefined, 1); // 1 == APP_STARTUP
     ok(
       !startupStub.called,
       "When started at app startup, do not call ShieldRecipeClient.startup immediately.",
     );
 
     Bootstrap.observe(null, "sessionstore-windows-restored");
     ok(
       startupStub.called,
@@ -184,15 +185,59 @@ decorate_task(
     );
   },
 );
 
 decorate_task(
   withBootstrap,
   withStub(ShieldRecipeClient, "startup"),
   async function testStartupDelayed(Bootstrap, startupStub) {
-    Bootstrap.startup(undefined, 3); // 1 == ADDON_ENABLED
+    await Bootstrap.startup(undefined, 3); // 1 == ADDON_ENABLED
     ok(
       startupStub.called,
       "When the add-on is enabled outside app startup, call ShieldRecipeClient.startup immediately.",
     );
   },
 );
+
+decorate_task(
+  withBootstrap,
+  withMockPreferences,
+  PreferenceExperiments.withMockExperiments,
+  async function testMigrateOldPreferenceExperiments(Bootstrap, mockPreferences, mockExperiments) {
+    const defaultBranch = Services.prefs.getDefaultBranch("");
+    const experimentBranch = Services.prefs.getBranch("extensions.shield-recipe-client.startupExperimentPrefs.");
+    experimentBranch.deleteBranch("");
+
+    mockExperiments.test = experimentFactory({
+      preferenceName: "test.shieldMigrate",
+      preferenceType: "integer",
+      preferenceValue: 7,
+    });
+    mockPreferences.set("test.shieldMigrate", 10, "default");
+    mockPreferences.set("extensions.shield-recipe-client.startupExperimentMigrated", false, "user");
+
+    await Bootstrap.initExperimentPrefs();
+
+    is(
+      defaultBranch.getIntPref("test.shieldMigrate"),
+      7,
+      "initExperimentPrefs initialized test.shieldMigrate after migrating the experiment prefs.",
+    );
+    ok(
+      Services.prefs.getBoolPref("extensions.shield-recipe-client.startupExperimentMigrated"),
+      "initExperimentPrefs set the migration pref after finishing the migration.",
+    );
+  },
+);
+
+decorate_task(
+  withBootstrap,
+  withMockPreferences,
+  withStub(PreferenceExperiments, "saveStartupPrefs"),
+  async function testAlreadyMigrated(Bootstrap, mockPreferences, saveStartupPrefsStub) {
+    mockPreferences.set("extensions.shield-recipe-client.startupExperimentMigrated", true, "user");
+
+    await Bootstrap.initExperimentPrefs();
+
+    sinon.assert.notCalled(saveStartupPrefsStub);
+  },
+);
--- a/browser/extensions/shield-recipe-client/test/browser/head.js
+++ b/browser/extensions/shield-recipe-client/test/browser/head.js
@@ -260,16 +260,30 @@ this.studyFactory = function(attrs) {
     active: true,
     addonId: "fake@example.com",
     addonUrl: "http://test/addon.xpi",
     addonVersion: "1.0.0",
     studyStartDate: new Date(),
   }, attrs);
 };
 
+this.experimentFactory = function(attrs) {
+  return Object.assign({
+    name: "fakename",
+    branch: "fakebranch",
+    expired: false,
+    lastSeen: new Date().toJSON(),
+    preferenceName: "fake.preference",
+    preferenceValue: "falkevalue",
+    preferenceType: "string",
+    previousPreferenceValue: "oldfakevalue",
+    preferenceBranchType: "default",
+  }, attrs);
+}
+
 this.withStub = function(...stubArgs) {
   return function wrapper(testFunction) {
     return async function wrappedTestFunction(...args) {
       const stub = sinon.stub(...stubArgs);
       try {
         await testFunction(...args, stub);
       } finally {
         stub.restore();