Bug 1440780 - Add Normandy action for add-on studies r=aswan

This ports the code from the Normandy server github repo to run as a local
action, instead of being fetched from the server.

The original code is here:
c0a8c53707/client/actions/opt-out-study

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

--HG--
extra : moz-landing-system : lando

toolkit/components/normandy/actions/AddonStudyAction.jsm

@@ -0,0 +1,269 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+/*
+ * This action handles the life cycle of add-on based studies. Currently that
+ * means installing the add-on the first time the recipe applies to this client,
+ * and uninstalling them when the recipe no longer applies.
+ */
+
+"use strict";
+
+ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
+ChromeUtils.import("resource://normandy/actions/BaseAction.jsm");
+
+XPCOMUtils.defineLazyModuleGetters(this, {
+  Services: "resource://gre/modules/Services.jsm",
+  PromiseUtils: "resource://gre/modules/PromiseUtils.jsm",
+  AddonManager: "resource://gre/modules/AddonManager.jsm",
+  ActionSchemas: "resource://normandy/actions/schemas/index.js",
+  AddonStudies: "resource://normandy/lib/AddonStudies.jsm",
+  TelemetryEvents: "resource://normandy/lib/TelemetryEvents.jsm",
+});
+
+var EXPORTED_SYMBOLS = ["AddonStudyAction"];
+
+const OPT_OUT_STUDIES_ENABLED_PREF = "app.shield.optoutstudies.enabled";
+
+class AddonStudyEnrollError extends Error {
+  constructor(studyName, reason) {
+    let message;
+    switch (reason) {
+      case "conflicting-addon-id": {
+        message = "an add-on with this ID is already installed";
+        break;
+      }
+      case "download-failure": {
+        message = "the add-on failed to download";
+        break;
+      }
+      default: {
+        throw new Error(`Unexpected AddonStudyEnrollError reason: ${reason}`);
+      }
+    }
+    super(new Error(`Cannot install study add-on for ${studyName}: ${message}.`));
+    this.studyName = studyName;
+    this.reason = reason;
+  }
+}
+
+class AddonStudyAction extends BaseAction {
+  get schema() {
+    return ActionSchemas["addon-study"];
+  }
+
+  /**
+   * This hook is executed once before any recipes have been processed, it is
+   * responsible for:
+   *
+   *   - Checking if the user has opted out of studies, and if so, it disables the action.
+   *   - Setting up tracking of seen recipes, for use in _finalize.
+   */
+  _preExecution() {
+    // Check opt-out preference
+    if (!Services.prefs.getBoolPref(OPT_OUT_STUDIES_ENABLED_PREF, true)) {
+      this.log.info("User has opted-out of opt-out experiments, disabling action.");
+      this.disable();
+    }
+
+    this.seenRecipeIds = new Set();
+  }
+
+  /**
+   * This hook is executed once for each recipe that currently applies to this
+   * client. It is responsible for:
+   *
+   *   - Enrolling studies the first time they are seen.
+   *   - Marking studies as having been seen in this session.
+   *
+   * If the recipe fails to enroll, it should throw to properly report its status.
+   */
+  async _run(recipe) {
+    this.seenRecipeIds.add(recipe.id);
+
+    const hasStudy = await AddonStudies.has(recipe.id);
+    if (recipe.arguments.isEnrollmentPaused || hasStudy) {
+      // Recipe does not need anything done
+      return;
+    }
+
+    await this.enroll(recipe);
+  }
+
+  /**
+   * This hook is executed once after all recipes that apply to this client
+   * have been processed. It is responsible for unenrolling the client from any
+   * studies that no longer apply, based on this.seenRecipeIds.
+   */
+  async _finalize() {
+    const activeStudies = (await AddonStudies.getAll()).filter(study => study.active);
+
+    for (const study of activeStudies) {
+      if (!this.seenRecipeIds.has(study.recipeId)) {
+        this.log.debug(`Stopping study for recipe ${study.recipeId}`);
+        try {
+          await this.unenroll(study.recipeId, "recipe-not-seen");
+        } catch (err) {
+          Cu.reportError(err);
+        }
+      }
+    }
+  }
+
+  /**
+   * Enroll in the study represented by the given recipe.
+   * @param recipe Object describing the study to enroll in.
+   */
+  async enroll(recipe) {
+    // This function first downloads the add-on to get its metadata. Then it
+    // uses that metadata to record a study in `AddonStudies`. Then, it finishes
+    // installing the add-on, and finally sends telemetry. If any of these steps
+    // fails, the previous ones are undone, as needed.
+    //
+    // This ordering is important because the only intermediate states we can be
+    // in are:
+    //   1. The add-on is only downloaded, in which case AddonManager will clean it up.
+    //   2. The study has been recorded, in which case we will unenroll on next
+    //      start up, assuming that the add-on was uninstalled while the browser was
+    //      shutdown.
+    //   3. After installation is complete, but before telemetry, in which case we
+    //      lose an enroll event. This is acceptable.
+    //
+    // This way we a shutdown, crash or unexpected error can't leave Normandy in
+    // a long term inconsistent state. The main thing avoided is having a study
+    // add-on installed but no record of it, which would leave it permanently
+    // installed.
+
+    const { addonUrl, name, description } = recipe.arguments;
+
+    const downloadDeferred = PromiseUtils.defer();
+    const installDeferred = PromiseUtils.defer();
+
+    const install = await AddonManager.getInstallForURL(addonUrl, "application/x-xpinstall");
+
+    const listener = {
+      onDownloadFailed() {
+        downloadDeferred.reject(new AddonStudyEnrollError(name, "download-failure"));
+      },
+
+      onDownloadEnded() {
+        downloadDeferred.resolve();
+        return false; // temporarily pause installation for Normandy bookkeeping
+      },
+
+      onInstallStarted(cbInstall) {
+        if (cbInstall.existingAddon) {
+          installDeferred.reject(new AddonStudyEnrollError(name, "conflicting-addon-id"));
+          return false; // cancel the installation, no upgrades allowed
+        }
+        return true;
+      },
+
+      onInstallFailed() {
+        installDeferred.reject(new AddonStudyEnrollError(name, "failed-to-install"));
+      },
+
+      onInstallEnded() {
+        installDeferred.resolve();
+      },
+    };
+
+    install.addListener(listener);
+
+    // Download the add-on
+    try {
+      install.install();
+      await downloadDeferred.promise;
+    } catch (err) {
+      this.reportEnrollError(err);
+      install.removeListener(listener);
+      return;
+    }
+
+    const addonId = install.addon.id;
+
+    const study = {
+      recipeId: recipe.id,
+      name,
+      description,
+      addonId,
+      addonVersion: install.addon.version,
+      addonUrl,
+      active: true,
+      studyStartDate: new Date(),
+    };
+
+    try {
+      await AddonStudies.add(study);
+    } catch (err) {
+      this.reportEnrollError(err);
+      install.removeListener(listener);
+      install.cancel();
+      throw err;
+    }
+
+    // finish paused installation
+    try {
+      install.install();
+      await installDeferred.promise;
+    } catch (err) {
+      this.reportEnrollError(err);
+      install.removeListener(listener);
+      await AddonStudies.delete(recipe.id);
+      throw err;
+    }
+
+    // All done, report success to Telemetry and cleanup
+    TelemetryEvents.sendEvent("enroll", "addon_study", name, {
+      addonId: install.addon.id,
+      addonVersion: install.addon.version,
+    });
+
+    install.removeListener(listener);
+  }
+
+  reportEnrollError(error) {
+    if (error instanceof AddonStudyEnrollError) {
+      // One of our known errors. Report it nicely to telemetry
+      TelemetryEvents.sendEvent("enrollFailed", "addon_study", error.studyName, { reason: error.reason });
+    } else {
+      /*
+        * Some unknown error. Add some helpful details, and report it to
+        * telemetry. The actual stack trace and error message could possibly
+        * contain PII, so we don't include them here. Instead include some
+        * information that should still be helpful, and is less likely to be
+        * unsafe.
+        */
+      const safeErrorMessage = `${error.fileName}:${error.lineNumber}:${error.columnNumber} ${error.name}`;
+      TelemetryEvents.sendEvent("enrollFailed", "addon_study", error.studyName, {
+        reason: safeErrorMessage.slice(0, 80),  // max length is 80 chars
+      });
+    }
+  }
+
+  /**
+   * Unenrolls the client from the study with a given recipe ID.
+   * @param recipeId The recipe ID of an enrolled study
+   * @param reason The reason for this unenrollment, to be used in Telemetry
+   * @throws If the specified study does not exist, or if it is already inactive.
+   */
+  async unenroll(recipeId, reason = "unknown") {
+    const study = await AddonStudies.get(recipeId);
+    if (!study) {
+      throw new Error(`No study found for recipe ${recipeId}.`);
+    }
+    if (!study.active) {
+      throw new Error(`Cannot stop study for recipe ${recipeId}; it is already inactive.`);
+    }
+
+    await AddonStudies.markAsEnded(study, reason);
+
+    const addon = await AddonManager.getAddonByID(study.addonId);
+    if (addon) {
+      await addon.uninstall();
+    } else {
+      this.log.warn(`Could not uninstall addon ${study.addonId} for recipe ${study.recipeId}: it is not installed.`);
+    }
+  }
+}

toolkit/components/normandy/actions/BaseAction.jsm

@@ -20,17 +20,19 @@ var EXPORTED_SYMBOLS = ["BaseAction"];
  */
 class BaseAction {
   constructor() {
-    this.finalized = false;
-    this.failed = false;
+    this.state = BaseAction.STATE_PREPARING;
     this.log = LogManager.getLogger(`action.${this.name}`);
 
     try {
       this._preExecution();
+      // if _preExecution changed the state, don't overwrite it
+      if (this.state === BaseAction.STATE_PREPARING) {
+        this.state = BaseAction.STATE_READY;
+      }
     } catch (err) {
-      this.failed = true;
       err.message = `Could not initialize action ${this.name}: ${err.message}`;
       Cu.reportError(err);
-      Uptake.reportAction(this.name, Uptake.ACTION_PRE_EXECUTION_ERROR);
+      this.fail(Uptake.ACTION_PRE_EXECUTION_ERROR);
     }
   }
 
@@ -41,6 +43,27 @@ class BaseAction {
     };
   }
 
+  /**
+   * Disable the action for a non-error reason, such as the user opting out of
+   * this type of action.
+   */
+  disable() {
+    this.state = BaseAction.STATE_DISABLED;
+  }
+
+  fail() {
+    switch (this.state) {
+      case BaseAction.STATE_PREPARING: {
+        Uptake.reportAction(this.name, Uptake.ACTION_PRE_EXECUTION_ERROR);
+        break;
+      }
+      default: {
+        Cu.reportError(new Error("BaseAction.fail() called at unexpected time"));
+      }
+    }
+    this.state = BaseAction.STATE_FAILED;
+  }
+
   // Gets the name of the action. Does not necessarily match the
   // server slug for the action.
   get name() {
@@ -63,13 +86,13 @@ class BaseAction {
    * @throws If this action has already been finalized.
    */
   async runRecipe(recipe) {
-    if (this.finalized) {
+    if (this.state === BaseAction.STATE_FINALIZED) {
       throw new Error("Action has already been finalized");
     }
 
-    if (this.failed) {
+    if (this.state !== BaseAction.STATE_READY) {
       Uptake.reportRecipe(recipe.id, Uptake.RECIPE_ACTION_DISABLED);
-      this.log.warn(`Skipping recipe ${recipe.name} because ${this.name} failed during preExecution.`);
+      this.log.warn(`Skipping recipe ${recipe.name} because ${this.name} was disabled during preExecution.`);
       return;
     }
 
@@ -107,24 +130,46 @@ class BaseAction {
    * recipes will be assumed to have been seen.
    */
   async finalize() {
-    if (this.finalized) {
-      throw new Error("Action has already been finalized");
+    let status;
+    switch (this.state) {
+      case BaseAction.STATE_FINALIZED: {
+        throw new Error("Action has already been finalized");
+      }
+      case BaseAction.STATE_READY: {
+        try {
+          await this._finalize();
+          status = Uptake.ACTION_SUCCESS;
+        } catch (err) {
+          status = Uptake.ACTION_POST_EXECUTION_ERROR;
+            // Sometimes Error.message can be updated in place. This gives better messages when debugging errors.
+          try {
+            err.message = `Could not run postExecution hook for ${this.name}: ${err.message}`;
+          } catch (err) {
+            // Sometimes Error.message cannot be updated. Log a warning, and move on.
+            this.log.debug(`Could not run postExecution hook for ${this.name}`);
+          }
+
+          Cu.reportError(err);
+        }
+        break;
+      }
+      case BaseAction.STATE_DISABLED: {
+        this.log.debug(`Skipping post-execution hook for ${this.name} because it is disabled.`);
+        status = Uptake.ACTION_SUCCESS;
+        break;
+      }
+      case BaseAction.STATE_FAILED: {
+        this.log.debug(`Skipping post-execution hook for ${this.name} because it failed during pre-execution.`);
+        // Don't report a status. A status should have already been reported by this.fail().
+        break;
+      }
+      default: {
+        throw new Error(`Unexpected state during finalize: ${this.state}`);
+      }
     }
 
-    if (this.failed) {
-      this.log.info(`Skipping post-execution hook for ${this.name} due to earlier failure.`);
-      return;
-    }
-
-    let status = Uptake.ACTION_SUCCESS;
-    try {
-      await this._finalize();
-    } catch (err) {
-      status = Uptake.ACTION_POST_EXECUTION_ERROR;
-      err.message = `Could not run postExecution hook for ${this.name}: ${err.message}`;
-      Cu.reportError(err);
-    } finally {
-      this.finalized = true;
+    this.state = BaseAction.STATE_FINALIZED;
+    if (status) {
       Uptake.reportAction(this.name, status);
     }
   }
@@ -138,3 +183,9 @@ class BaseAction {
     // Does nothing, may be overridden
   }
 }
+
+BaseAction.STATE_PREPARING = "ACTION_PREPARING";
+BaseAction.STATE_READY = "ACTION_READY";
+BaseAction.STATE_DISABLED = "ACTION_DISABLED";
+BaseAction.STATE_FAILED = "ACTION_FAILED";
+BaseAction.STATE_FINALIZED = "ACTION_FINALIZED";

toolkit/components/normandy/actions/schemas/index.js

@@ -61,8 +61,45 @@ const ActionSchemas = {
       },
     },
   },
+
+  "addon-study": {
+    $schema: "http://json-schema.org/draft-04/schema#",
+    title: "Enroll a user in an opt-out SHIELD study",
+    type: "object",
+    required: [
+      "name",
+      "description",
+      "addonUrl"
+    ],
+    properties: {
+      name: {
+        description: "User-facing name of the study",
+        type: "string",
+        minLength: 1
+      },
+      description: {
+        description: "User-facing description of the study",
+        type: "string",
+        minLength: 1
+      },
+      addonUrl: {
+        description: "URL of the add-on XPI file",
+        type: "string",
+        format: "uri",
+        minLength: 1
+      },
+      isEnrollmentPaused: {
+        description: "If true, new users will not be enrolled in the study.",
+        type: "boolean",
+        default: false
+      }
+    }
+  }
 };
 
+// Legacy name used on Normandy server
+ActionSchemas["opt-out-study"] = ActionSchemas["addon-study"];
+
 // If running in Node.js, export the schemas.
 if (typeof module !== "undefined") {
   /* globals module */

toolkit/components/normandy/actions/schemas/package.json

@@ -1,6 +1,6 @@
 {
   "name": "@mozilla/normandy-action-argument-schemas",
-  "version": "0.3.0",
+  "version": "0.4.0",
   "description": "Schemas for Normandy action arguments",
   "main": "index.js",
   "author": "Michael Cooper <mcooper@mozilla.com>",

toolkit/components/normandy/lib/ActionsManager.jsm

@@ -5,6 +5,7 @@ XPCOMUtils.defineLazyModuleGetters(this, {
   ActionSandboxManager: "resource://normandy/lib/ActionSandboxManager.jsm",
   NormandyApi: "resource://normandy/lib/NormandyApi.jsm",
   Uptake: "resource://normandy/lib/Uptake.jsm",
+  AddonStudyAction: "resource://normandy/actions/AddonStudyAction.jsm",
   ConsoleLogAction: "resource://normandy/actions/ConsoleLogAction.jsm",
   PreferenceRolloutAction: "resource://normandy/actions/PreferenceRolloutAction.jsm",
   PreferenceRollbackAction: "resource://normandy/actions/PreferenceRollbackAction.jsm",
@@ -28,10 +29,14 @@ class ActionsManager {
     this.finalized = false;
     this.remoteActionSandboxes = {};
 
+    const addonStudyAction = new AddonStudyAction();
+
     this.localActions = {
+      "addon-study": addonStudyAction,
       "console-log": new ConsoleLogAction(),
       "preference-rollout": new PreferenceRolloutAction(),
       "preference-rollback": new PreferenceRollbackAction(),
+      "opt-out-study": addonStudyAction, // Legacy name used on Normandy server
     };
   }
 

toolkit/components/normandy/lib/AddonStudies.jsm

@@ -28,20 +28,15 @@
 
 ChromeUtils.import("resource://gre/modules/osfile.jsm");
 ChromeUtils.import("resource://gre/modules/Services.jsm");
-ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
 
-ChromeUtils.defineModuleGetter(this, "FileUtils", "resource://gre/modules/FileUtils.jsm");
 ChromeUtils.defineModuleGetter(this, "IndexedDB", "resource://gre/modules/IndexedDB.jsm");
 ChromeUtils.defineModuleGetter(this, "AddonManager", "resource://gre/modules/AddonManager.jsm");
-ChromeUtils.defineModuleGetter(this, "Addons", "resource://normandy/lib/Addons.jsm");
 ChromeUtils.defineModuleGetter(
   this, "CleanupManager", "resource://normandy/lib/CleanupManager.jsm"
 );
 ChromeUtils.defineModuleGetter(this, "LogManager", "resource://normandy/lib/LogManager.jsm");
 ChromeUtils.defineModuleGetter(this, "TelemetryEvents", "resource://normandy/lib/TelemetryEvents.jsm");
 
-XPCOMUtils.defineLazyGlobalGetters(this, ["fetch"]); /* globals fetch */
-
 var EXPORTED_SYMBOLS = ["AddonStudies"];
 
 const DB_NAME = "shield";
@@ -87,29 +82,6 @@ function getStore(db) {
   return db.objectStore(STORE_NAME, "readwrite");
 }
 
-/**
- * Mark a study object as having ended. Modifies the study in-place.
- * @param {IDBDatabase} db
- * @param {Study} study
- * @param {String} reason Why the study is ending.
- */
-async function markAsEnded(db, study, reason) {
-  if (reason === "unknown") {
-    log.warn(`Study ${study.name} ending for unknown reason.`);
-  }
-
-  study.active = false;
-  study.studyEndDate = new Date();
-  await getStore(db).put(study);
-
-  Services.obs.notifyObservers(study, STUDY_ENDED_TOPIC, `${study.recipeId}`);
-  TelemetryEvents.sendEvent("unenroll", "addon_study", study.name, {
-    addonId: study.addonId,
-    addonVersion: study.addonVersion,
-    reason,
-  });
-}
-
 var AddonStudies = {
   /**
    * Test wrapper that temporarily replaces the stored studies with the given
@@ -151,11 +123,10 @@ var AddonStudies = {
     // If an active study's add-on has been removed since we last ran, stop the
     // study.
     const activeStudies = (await this.getAll()).filter(study => study.active);
-    const db = await getDatabase();
     for (const study of activeStudies) {
       const addon = await AddonManager.getAddonByID(study.addonId);
       if (!addon) {
-        await markAsEnded(db, study, "uninstalled-sideload");
+        await this.markAsEnded(study, "uninstalled-sideload");
       }
     }
     await this.close();
@@ -178,7 +149,7 @@ var AddonStudies = {
       // Use a dedicated DB connection instead of the shared one so that we can
       // close it without fear of affecting other users of the shared connection.
       const db = await openDatabase();
-      await markAsEnded(db, matchingStudy, "uninstalled");
+      await this.markAsEnded(matchingStudy, "uninstalled");
       await db.close();
     }
   },
@@ -234,122 +205,45 @@ var AddonStudies = {
   },
 
   /**
-   * Start a new study. Installs an add-on and stores the study info.
-   * @param {Object} options
-   * @param {Number} options.recipeId
-   * @param {String} options.name
-   * @param {String} options.description
-   * @param {String} options.addonUrl
-   * @throws
-   *   If any of the required options aren't given.
-   *   If a study for the given recipeID already exists in storage.
-   *   If add-on installation fails.
+   * Add a study to storage.
+   * @return {Promise<void, Error>} Resolves when the study is stored, or rejects with an error.
    */
-  async start({recipeId, name, description, addonUrl}) {
-    if (!recipeId || !name || !description || !addonUrl) {
-      throw new Error("Required arguments (recipeId, name, description, addonUrl) missing.");
-    }
-
+  async add(study) {
     const db = await getDatabase();
-    if (await getStore(db).get(recipeId)) {
-      throw new Error(`A study for recipe ${recipeId} already exists.`);
-    }
-
-    let addonFile;
-    try {
-      addonFile = await this.downloadAddonToTemporaryFile(addonUrl);
-      const install = await AddonManager.getInstallForFile(addonFile);
-      const study = {
-        recipeId,
-        name,
-        description,
-        addonId: install.addon.id,
-        addonVersion: install.addon.version,
-        addonUrl,
-        active: true,
-        studyStartDate: new Date(),
-      };
-
-      await getStore(db).add(study);
-      await Addons.applyInstall(install, false);
-
-      TelemetryEvents.sendEvent("enroll", "addon_study", name, {
-        addonId: install.addon.id,
-        addonVersion: install.addon.version,
-      });
-
-      return study;
-    } catch (err) {
-      await getStore(db).delete(recipeId);
-
-      // The actual stack trace and error message could possibly
-      // contain PII, so we don't include them here. Instead include
-      // some information that should still be helpful, and is less
-      // likely to be unsafe.
-      const safeErrorMessage = `${err.fileName}:${err.lineNumber}:${err.columnNumber} ${err.name}`;
-      TelemetryEvents.sendEvent("enrollFailed", "addon_study", name, {
-        reason: safeErrorMessage.slice(0, 80),  // max length is 80 chars
-      });
-
-      throw err;
-    } finally {
-      if (addonFile) {
-        Services.obs.notifyObservers(addonFile, "flush-cache-entry");
-        await OS.File.remove(addonFile.path);
-      }
-    }
+    return getStore(db).add(study);
   },
 
   /**
-   * Download a remote add-on and store it in a temporary nsIFile.
-   * @param {String} addonUrl
-   * @returns {nsIFile}
+   * Remove a study from storage
+   * @param recipeId The recipeId of the study to delete
+   * @return {Promise<void, Error>} Resolves when the study is deleted, or rejects with an error.
    */
-  async downloadAddonToTemporaryFile(addonUrl) {
-    const response = await fetch(addonUrl);
-    if (!response.ok) {
-      throw new Error(`Download for ${addonUrl} failed: ${response.status} ${response.statusText}`);
-    }
-
-    // Create temporary file to store add-on.
-    const path = OS.Path.join(OS.Constants.Path.tmpDir, "study.xpi");
-    const {file, path: uniquePath} = await OS.File.openUnique(path);
-
-    // Write the add-on to the file
-    try {
-      const xpiArrayBufferView = new Uint8Array(await response.arrayBuffer());
-      await file.write(xpiArrayBufferView);
-    } finally {
-      await file.close();
-    }
-
-    return new FileUtils.File(uniquePath);
+  async delete(recipeId) {
+    const db = await getDatabase();
+    return getStore(db).delete(recipeId);
   },
 
   /**
-   * Stop an active study, uninstalling the associated add-on.
-   * @param {Number} recipeId
-   * @param {String} reason Why the study is ending. Optional, defaults to "unknown".
-   * @throws
-   *   If no study is found with the given recipeId.
-   *   If the study is already inactive.
+   * Mark a study object as having ended. Modifies the study in-place.
+   * @param {IDBDatabase} db
+   * @param {Study} study
+   * @param {String} reason Why the study is ending.
    */
-  async stop(recipeId, reason = "unknown") {
+  async markAsEnded(study, reason) {
+    if (reason === "unknown") {
+      log.warn(`Study ${study.name} ending for unknown reason.`);
+    }
+
+    study.active = false;
+    study.studyEndDate = new Date();
     const db = await getDatabase();
-    const study = await getStore(db).get(recipeId);
-    if (!study) {
-      throw new Error(`No study found for recipe ${recipeId}.`);
-    }
-    if (!study.active) {
-      throw new Error(`Cannot stop study for recipe ${recipeId}; it is already inactive.`);
-    }
+    await getStore(db).put(study);
 
-    await markAsEnded(db, study, reason);
-
-    try {
-      await Addons.uninstall(study.addonId);
-    } catch (err) {
-      log.warn(`Could not uninstall addon ${study.addonId} for recipe ${study.recipeId}:`, err);
-    }
+    Services.obs.notifyObservers(study, STUDY_ENDED_TOPIC, `${study.recipeId}`);
+    TelemetryEvents.sendEvent("unenroll", "addon_study", study.name, {
+      addonId: study.addonId,
+      addonVersion: study.addonVersion,
+      reason,
+    });
   },
 };

toolkit/components/normandy/lib/Addons.jsm

@@ -1,118 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this
- * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-
-"use strict";
-
-ChromeUtils.defineModuleGetter(this, "AddonManager", "resource://gre/modules/AddonManager.jsm");
-
-var EXPORTED_SYMBOLS = ["Addons"];
-
-/**
- * SafeAddons store info about an add-on. They are single-depth
- * objects to simplify cloning, and have no methods so they are safe
- * to pass to sandboxes and filter expressions.
- *
- * @typedef {Object} SafeAddon
- * @property {string} id
- *   Add-on id, such as "shield-recipe-client@mozilla.com" or "{4ea51ac2-adf2-4af8-a69d-17b48c558a12}"
- * @property {Date} installDate
- * @property {boolean} isActive
- * @property {string} name
- * @property {string} type
- *   "extension", "theme", etc.
- * @property {string} version
- */
-
-var Addons = {
-  /**
-   * Get information about an installed add-on by ID.
-   *
-   * @param {string} addonId
-   * @returns {SafeAddon?} Add-on with given ID, or null if not found.
-   * @throws If addonId is not specified or not a string.
-   */
-  async get(addonId) {
-    const addon = await AddonManager.getAddonByID(addonId);
-    if (!addon) {
-      return null;
-    }
-    return this.serializeForSandbox(addon);
-  },
-
-  /**
-   * Installs an add-on
-   *
-   * @param {string} addonUrl
-   *   Url to download the .xpi for the add-on from.
-   * @param {object} options
-   * @param {boolean} options.update=false
-   *   If true, will update an existing installed add-on with the same ID.
-   * @async
-   * @returns {string}
-   *   Add-on ID that was installed
-   * @throws {string}
-   *   If the add-on can not be installed, or overwriting is disabled and an
-   *   add-on with a matching ID is already installed.
-   */
-  async install(addonUrl, options) {
-    const installObj = await AddonManager.getInstallForURL(addonUrl, "application/x-xpinstall");
-    return this.applyInstall(installObj, options);
-  },
-
-  async applyInstall(addonInstall, {update = false} = {}) {
-    const result = new Promise((resolve, reject) => addonInstall.addListener({
-      onInstallStarted(cbInstall) {
-        if (cbInstall.existingAddon && !update) {
-          reject(new Error(`
-            Cannot install add-on ${cbInstall.addon.id}; an existing add-on
-            with the same ID exists and updating is disabled.
-          `));
-          return false;
-        }
-        return true;
-      },
-      onInstallEnded(cbInstall, addon) {
-        resolve(addon.id);
-      },
-      onInstallFailed(cbInstall) {
-        reject(new Error(`AddonInstall error code: [${cbInstall.error}]`));
-      },
-      onDownloadFailed(cbInstall) {
-        reject(new Error(`Download failed: [${cbInstall.sourceURI.spec}]`));
-      },
-    }));
-    addonInstall.install();
-    return result;
-  },
-
-  /**
-   * Uninstalls an add-on by ID.
-   * @param addonId {string} Add-on ID to uninstall.
-   * @async
-   * @throws If no add-on with `addonId` is installed.
-   */
-  async uninstall(addonId) {
-    const addon = await AddonManager.getAddonByID(addonId);
-    if (addon === null) {
-      throw new Error(`No addon with ID [${addonId}] found.`);
-    }
-    await addon.uninstall();
-    return null;
-  },
-
-  /**
-   * Make a safe serialization of an add-on
-   * @param addon {Object} An add-on object as returned from AddonManager.
-   */
-  serializeForSandbox(addon) {
-    return {
-      id: addon.id,
-      installDate: new Date(addon.installDate),
-      isActive: addon.isActive,
-      name: addon.name,
-      type: addon.type,
-      version: addon.version,
-    };
-  },
-};

toolkit/components/normandy/lib/NormandyDriver.jsm

@@ -9,7 +9,6 @@ ChromeUtils.import("resource://gre/modules/Preferences.jsm");
 ChromeUtils.import("resource:///modules/ShellService.jsm");
 ChromeUtils.import("resource://gre/modules/AddonManager.jsm");
 ChromeUtils.import("resource://gre/modules/Timer.jsm");
-ChromeUtils.import("resource://normandy/lib/Addons.jsm");
 ChromeUtils.import("resource://normandy/lib/LogManager.jsm");
 ChromeUtils.import("resource://normandy/lib/Storage.jsm");
 ChromeUtils.import("resource://normandy/lib/Heartbeat.jsm");
@@ -19,8 +18,6 @@ ChromeUtils.import("resource://normandy/lib/PreferenceExperiments.jsm");
 ChromeUtils.defineModuleGetter(
   this, "Sampling", "resource://gre/modules/components-utils/Sampling.jsm");
 ChromeUtils.defineModuleGetter(this, "UpdateUtils", "resource://gre/modules/UpdateUtils.jsm");
-ChromeUtils.defineModuleGetter(
-  this, "AddonStudies", "resource://normandy/lib/AddonStudies.jsm");
 
 const {generateUUID} = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);
 
@@ -157,12 +154,6 @@ var NormandyDriver = function(sandboxManager) {
       sandboxManager.removeHold(`setTimeout-${token}`);
     },
 
-    addons: {
-      get: sandboxManager.wrapAsync(Addons.get.bind(Addons), {cloneInto: true}),
-      install: sandboxManager.wrapAsync(Addons.install.bind(Addons)),
-      uninstall: sandboxManager.wrapAsync(Addons.uninstall.bind(Addons)),
-    },
-
     // Sampling
     ratioSample: sandboxManager.wrapAsync(Sampling.ratioSample),
 
@@ -187,18 +178,6 @@ var NormandyDriver = function(sandboxManager) {
       has: sandboxManager.wrapAsync(PreferenceExperiments.has.bind(PreferenceExperiments)),
     },
 
-    // Study storage API
-    studies: {
-      start: sandboxManager.wrapAsync(
-        AddonStudies.start.bind(AddonStudies),
-        {cloneArguments: true, cloneInto: true}
-      ),
-      stop: sandboxManager.wrapAsync(AddonStudies.stop.bind(AddonStudies)),
-      get: sandboxManager.wrapAsync(AddonStudies.get.bind(AddonStudies), {cloneInto: true}),
-      getAll: sandboxManager.wrapAsync(AddonStudies.getAll.bind(AddonStudies), {cloneInto: true}),
-      has: sandboxManager.wrapAsync(AddonStudies.has.bind(AddonStudies)),
-    },
-
     // Preference read-only API
     preferences: {
       getBool: wrapPrefGetter(Services.prefs.getBoolPref),

toolkit/components/normandy/lib/ShieldPreferences.jsm

@@ -3,14 +3,14 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 "use strict";
 
-ChromeUtils.import("resource://gre/modules/Services.jsm");
+ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
 
-ChromeUtils.defineModuleGetter(
-  this, "AddonStudies", "resource://normandy/lib/AddonStudies.jsm"
-);
-ChromeUtils.defineModuleGetter(
-  this, "CleanupManager", "resource://normandy/lib/CleanupManager.jsm"
-);
+XPCOMUtils.defineLazyModuleGetters(this, {
+  Services: "resource://gre/modules/Services.jsm",
+  AddonStudyAction: "resource://normandy/actions/AddonStudyAction.jsm",
+  AddonStudies: "resource://normandy/lib/AddonStudies.jsm",
+  CleanupManager: "resource://normandy/lib/CleanupManager.jsm",
+});
 
 var EXPORTED_SYMBOLS = ["ShieldPreferences"];
 
@@ -24,6 +24,7 @@ var ShieldPreferences = {
   init() {
     // Watch for changes to the Opt-out pref
     Services.prefs.addObserver(PREF_OPT_OUT_STUDIES_ENABLED, this);
+
     CleanupManager.addCleanupHandler(() => {
       Services.prefs.removeObserver(PREF_OPT_OUT_STUDIES_ENABLED, this);
     });
@@ -44,9 +45,14 @@ var ShieldPreferences = {
       case PREF_OPT_OUT_STUDIES_ENABLED: {
         prefValue = Services.prefs.getBoolPref(PREF_OPT_OUT_STUDIES_ENABLED);
         if (!prefValue) {
+          const action = new AddonStudyAction();
           for (const study of await AddonStudies.getAll()) {
             if (study.active) {
-              await AddonStudies.stop(study.recipeId, "general-opt-out");
+              try {
+                await action.unenroll(study.recipeId, "general-opt-out");
+              } catch (err) {
+                Cu.reportError(err);
+              }
             }
           }
         }

toolkit/components/normandy/test/browser/browser.ini

@@ -8,12 +8,12 @@ head = head.js
 skip-if = !healthreport || !telemetry
 [browser_about_studies.js]
 skip-if = true # bug 1442712
+[browser_actions_AddonStudyAction.js]
 [browser_actions_ConsoleLogAction.js]
 [browser_actions_PreferenceRolloutAction.js]
 [browser_actions_PreferenceRollbackAction.js]
 [browser_ActionSandboxManager.js]
 [browser_ActionsManager.js]
-[browser_Addons.js]
 [browser_AddonStudies.js]
 skip-if = (verify && (os == 'linux'))
 [browser_BaseAction.js]

toolkit/components/normandy/test/browser/browser_AddonStudies.js

@@ -1,9 +1,9 @@
 "use strict";
 
 ChromeUtils.import("resource://gre/modules/IndexedDB.jsm", this);
+ChromeUtils.import("resource://gre/modules/AddonManager.jsm", this);
 ChromeUtils.import("resource://testing-common/TestUtils.jsm", this);
 ChromeUtils.import("resource://testing-common/AddonTestUtils.jsm", this);
-ChromeUtils.import("resource://normandy/lib/Addons.jsm", this);
 ChromeUtils.import("resource://normandy/lib/AddonStudies.jsm", this);
 ChromeUtils.import("resource://normandy/lib/TelemetryEvents.jsm", this);
 
@@ -114,197 +114,6 @@ decorate_task(
   }
 );
 
-add_task(async function testStartRequiredArguments() {
-  const requiredArguments = startArgsFactory();
-  for (const key in requiredArguments) {
-    const args = Object.assign({}, requiredArguments);
-    delete args[key];
-    await Assert.rejects(
-      AddonStudies.start(args),
-      /Required arguments/,
-      `start rejects when missing required argument ${key}.`
-    );
-  }
-});
-
-decorate_task(
-  AddonStudies.withStudies([
-    studyFactory(),
-  ]),
-  async function testStartExisting([study]) {
-    await Assert.rejects(
-      AddonStudies.start(startArgsFactory({recipeId: study.recipeId})),
-      /already exists/,
-      "start rejects when a study exists with the given recipeId already."
-    );
-  }
-);
-
-decorate_task(
-  withStub(Addons, "applyInstall"),
-  withSendEventStub,
-  withWebExtension(),
-  async function testStartAddonCleanup(applyInstallStub, sendEventStub, [addonId, addonFile]) {
-    const fakeError = new Error("Fake failure");
-    fakeError.fileName = "fake/filename.js";
-    fakeError.lineNumber = 42;
-    fakeError.columnNumber = 54;
-    applyInstallStub.rejects(fakeError);
-
-    const addonUrl = Services.io.newFileURI(addonFile).spec;
-    const args = startArgsFactory({addonUrl});
-    await Assert.rejects(
-      AddonStudies.start(args),
-      /Fake failure/,
-      "start rejects when the Addons.applyInstall function rejects"
-    );
-
-    const addon = await Addons.get(addonId);
-    ok(!addon, "If something fails during start after the add-on is installed, it is uninstalled.");
-
-    Assert.deepEqual(
-      sendEventStub.getCall(0).args,
-      ["enrollFailed", "addon_study", args.name, {reason: "fake/filename.js:42:54 Error"}],
-      "AddonStudies.start() should send an enroll-failed event when applyInstall rejects",
-    );
-  }
-);
-
-const testOverwriteId = "testStartAddonNoOverwrite@example.com";
-decorate_task(
-  withInstalledWebExtension({version: "1.0", id: testOverwriteId}),
-  withWebExtension({version: "2.0", id: testOverwriteId}),
-  async function testStartAddonNoOverwrite([installedId, installedFile], [id, addonFile]) {
-    const addonUrl = Services.io.newFileURI(addonFile).spec;
-    await Assert.rejects(
-      AddonStudies.start(startArgsFactory({addonUrl})),
-      /updating is disabled/,
-      "start rejects when the study add-on is already installed"
-    );
-
-    await Addons.uninstall(testOverwriteId);
-  }
-);
-
-decorate_task(
-  withWebExtension({version: "2.0"}),
-  withSendEventStub,
-  AddonStudies.withStudies(),
-  async function testStart([addonId, addonFile], sendEventStub) {
-    const startupPromise = AddonTestUtils.promiseWebExtensionStartup(addonId);
-    const addonUrl = Services.io.newFileURI(addonFile).spec;
-
-    let addon = await Addons.get(addonId);
-    is(addon, null, "Before start is called, the add-on is not installed.");
-
-    const args = startArgsFactory({
-      name: "Test Study",
-      description: "Test Desc",
-      addonUrl,
-    });
-    await AddonStudies.start(args);
-    await startupPromise;
-
-    addon = await Addons.get(addonId);
-    ok(addon, "After start is called, the add-on is installed.");
-
-    const study = await AddonStudies.get(args.recipeId);
-    Assert.deepEqual(
-      study,
-      {
-        recipeId: args.recipeId,
-        name: args.name,
-        description: args.description,
-        addonId,
-        addonVersion: "2.0",
-        addonUrl,
-        active: true,
-        studyStartDate: study.studyStartDate,
-      },
-      "start saves study data to storage",
-    );
-    ok(study.studyStartDate, "start assigns a value to the study start date.");
-
-    Assert.deepEqual(
-      sendEventStub.getCall(0).args,
-      ["enroll", "addon_study", args.name, {addonId, addonVersion: "2.0"}],
-      "AddonStudies.start() should send the correct telemetry event"
-    );
-
-    await AddonStudies.stop(args.recipeId);
-  }
-);
-
-decorate_task(
-  AddonStudies.withStudies(),
-  async function testStopNoStudy() {
-    await Assert.rejects(
-      AddonStudies.stop("does-not-exist"),
-      /No study found/,
-      "stop rejects when no study exists for the given recipe."
-    );
-  }
-);
-
-decorate_task(
-  AddonStudies.withStudies([
-    studyFactory({active: false}),
-  ]),
-  async function testStopInactiveStudy([study]) {
-    await Assert.rejects(
-      AddonStudies.stop(study.recipeId),
-      /already inactive/,
-      "stop rejects when the requested study is already inactive."
-    );
-  }
-);
-
-const testStopId = "testStop@example.com";
-decorate_task(
-  AddonStudies.withStudies([
-    studyFactory({active: true, addonId: testStopId, studyEndDate: null}),
-  ]),
-  withInstalledWebExtension({id: testStopId}),
-  withSendEventStub,
-  async function testStop([study], [addonId, addonFile], sendEventStub) {
-    await AddonStudies.stop(study.recipeId, "test-reason");
-    const newStudy = await AddonStudies.get(study.recipeId);
-    ok(!newStudy.active, "stop marks the study as inactive.");
-    ok(newStudy.studyEndDate, "stop saves the study end date.");
-
-    const addon = await Addons.get(addonId);
-    is(addon, null, "stop uninstalls the study add-on.");
-
-    Assert.deepEqual(
-      sendEventStub.getCall(0).args,
-      ["unenroll", "addon_study", study.name, {
-        addonId,
-        addonVersion: study.addonVersion,
-        reason: "test-reason",
-      }],
-      "stop should send the correct telemetry event"
-    );
-  }
-);
-
-decorate_task(
-  AddonStudies.withStudies([
-    studyFactory({active: true, addonId: "testStopWarn@example.com", studyEndDate: null}),
-  ]),
-  async function testStopWarn([study]) {
-    const addon = await Addons.get("testStopWarn@example.com");
-    is(addon, null, "Before start is called, the add-on is not installed.");
-
-    // If the add-on is not installed, log a warning to the console, but do not
-    // throw.
-    await new Promise(resolve => {
-      SimpleTest.waitForExplicitFinish();
-      SimpleTest.monitorConsole(resolve, [{message: /Could not uninstall addon/}]);
-      AddonStudies.stop(study.recipeId).then(() => SimpleTest.endMonitorConsole());
-    });
-  }
-);
-
 decorate_task(
   AddonStudies.withStudies([
     studyFactory({active: true, addonId: "does.not.exist@example.com", studyEndDate: null}),
@@ -355,9 +164,10 @@ decorate_task(
   AddonStudies.withStudies([
     studyFactory({active: true, addonId: "installed@example.com", studyEndDate: null}),
   ]),
-  withInstalledWebExtension({id: "installed@example.com"}),
+  withInstalledWebExtension({id: "installed@example.com"}, /* expectUninstall: */ true),
   async function testInit([study], [id, addonFile]) {
-    await Addons.uninstall(id);
+    const addon = await AddonManager.getAddonByID(id);
+    await addon.uninstall();
     await TestUtils.topicObserved("shield-study-ended");
 
     const newStudy = await AddonStudies.get(study.recipeId);
@@ -368,18 +178,3 @@ decorate_task(
     );
   }
 );
-
-// stop should pass "unknown" to TelemetryEvents for `reason` if none specified
-decorate_task(
-  AddonStudies.withStudies([studyFactory({ active: true })]),
-  withSendEventStub,
-  async function testStopUnknownReason([study], sendEventStub) {
-    await AddonStudies.stop(study.recipeId);
-    is(
-      sendEventStub.getCall(0).args[3].reason,
-      "unknown",
-      "stop should send the correct telemetry event",
-      "AddonStudies.stop() should use unknown as the default reason",
-    );
-  }
-);

toolkit/components/normandy/test/browser/browser_BaseAction.js

@@ -4,23 +4,21 @@ ChromeUtils.import("resource://normandy/actions/BaseAction.jsm", this);
 ChromeUtils.import("resource://normandy/lib/Uptake.jsm", this);
 
 class NoopAction extends BaseAction {
-  _run(recipe) {
-    // does nothing
-  }
-}
-
-class FailPreExecutionAction extends BaseAction {
   constructor() {
     super();
+    // this._testPreExecutionFlag is set by _preExecution, called in the constructor
+    if (this._testPreExecutionFlag === undefined) {
+      this._testPreExecutionFlag = false;
+    }
     this._testRunFlag = false;
     this._testFinalizeFlag = false;
   }
 
   _preExecution() {
-    throw new Error("Test error");
+    this._testPreExecutionFlag = true;
   }
 
-  _run() {
+  _run(recipe) {
     this._testRunFlag = true;
   }
 
@@ -29,41 +27,43 @@ class FailPreExecutionAction extends BaseAction {
   }
 }
 
-class FailRunAction extends BaseAction {
-  constructor() {
-    super();
-    this._testRunFlag = false;
-    this._testFinalizeFlag = false;
-  }
-
-  _run(recipe) {
+class FailPreExecutionAction extends NoopAction {
+  _preExecution() {
     throw new Error("Test error");
   }
-
-  _finalize() {
-    this._testFinalizeFlag = true;
-  }
 }
 
-class FailFinalizeAction extends BaseAction {
+class FailRunAction extends NoopAction {
   _run(recipe) {
-    // does nothing
+    throw new Error("Test error");
   }
+}
 
+class FailFinalizeAction extends NoopAction {
   _finalize() {
     throw new Error("Test error");
   }
 }
 
-let _recipeId = 1;
-function recipeFactory(overrides) {
-  let defaults = {
-    id: _recipeId++,
-    arguments: {},
-  };
-  Object.assign(defaults, overrides);
-  return defaults;
-}
+// Test that constructor and override methods are run
+decorate_task(
+  withStub(Uptake, "reportRecipe"),
+  withStub(Uptake, "reportAction"),
+  async () => {
+    const action = new NoopAction();
+    is(action._testPreExecutionFlag, true, "_preExecution should be called on a new action");
+    is(action._testRunFlag, false, "_run has should not have been called on a new action");
+    is(action._testFinalizeFlag, false, "_finalize should not be called on a new action");
+
+    const recipe = recipeFactory();
+    await action.runRecipe(recipe);
+    is(action._testRunFlag, true, "_run should be called when a recipe is executed");
+    is(action._testFinalizeFlag, false, "_finalize should not have been called when a recipe is executed");
+
+    await action.finalize();
+    is(action._testFinalizeFlag, true, "_finalizeExecution should be called when finalize was called");
+  }
+);
 
 // Test that per-recipe uptake telemetry is recorded
 decorate_task(
@@ -86,7 +86,7 @@ decorate_task(
   async function(reportActionStub) {
     const action = new NoopAction();
     await action.finalize();
-    ok(action.finalized, "Action should be marked as finalized");
+    ok(action.state == NoopAction.STATE_FINALIZED, "Action should be marked as finalized");
     Assert.deepEqual(
       reportActionStub.args,
       [[action.name, Uptake.ACTION_SUCCESS]],
@@ -127,16 +127,18 @@ decorate_task(
   async function(reportRecipeStub, reportActionStub) {
     const recipe = recipeFactory();
     const action = new FailPreExecutionAction();
-    ok(action.failed, "Action should fail during pre-execution fail");
+    is(action.state, FailPreExecutionAction.STATE_FAILED, "Action should fail during pre-execution fail");
 
-    // Should not throw, even though the action is in a failed state.
+    // Should not throw, even though the action is in a disabled state.
     await action.runRecipe(recipe);
+    is(action.state, FailPreExecutionAction.STATE_FAILED, "Action should remain failed");
 
-    // Should not throw, even though the action is in a failed state.
+    // Should not throw, even though the action is in a disabled state.
     await action.finalize();
+    is(action.state, FailPreExecutionAction.STATE_FINALIZED, "Action should be finalized");
 
-    is(action._testRunFlag, false, "_run should not have been caled");
-    is(action._testFinalizeFlag, false, "_finalize should not have been caled");
+    is(action._testRunFlag, false, "_run should not have been called");
+    is(action._testFinalizeFlag, false, "_finalize should not have been called");
 
     Assert.deepEqual(
       reportRecipeStub.args,
@@ -160,8 +162,9 @@ decorate_task(
     const recipe = recipeFactory();
     const action = new FailRunAction();
     await action.runRecipe(recipe);
+    is(action.state, FailRunAction.STATE_READY, "Action should not be marked as failed due to a recipe failure");
     await action.finalize();
-    ok(!action.failed, "Action should not be marked as failed due to a recipe failure");
+    is(action.state, FailRunAction.STATE_FINALIZED, "Action should be marked as finalized after finalize is called");
 
     ok(action._testFinalizeFlag, "_finalize should have been called");
 
@@ -202,3 +205,37 @@ decorate_task(
     );
   },
 );
+
+// Disable disables an action
+decorate_task(
+  withStub(Uptake, "reportRecipe"),
+  withStub(Uptake, "reportAction"),
+  async function(reportRecipeStub, reportActionStub) {
+    const recipe = recipeFactory();
+    const action = new NoopAction();
+
+    action.disable();
+    is(action.state, NoopAction.STATE_DISABLED, "Action should be marked as disabled");
+
+    // Should not throw, even though the action is disabled
+    await action.runRecipe(recipe);
+
+    // Should not throw, even though the action is disabled
+    await action.finalize();
+
+    is(action._testRunFlag, false, "_run should not have been called");
+    is(action._testFinalizeFlag, false, "_finalize should not have been called");
+
+    Assert.deepEqual(
+      reportActionStub.args,
+      [[action.name, Uptake.ACTION_SUCCESS]],
+      "Action should not report pre execution error",
+    );
+
+    Assert.deepEqual(
+      reportRecipeStub.args,
+      [[recipe.id, Uptake.RECIPE_ACTION_DISABLED]],
+      "Recipe should report recipe status as action disabled",
+    );
+  },
+);

toolkit/components/normandy/test/browser/browser_ClientEnvironment.js

@@ -105,7 +105,9 @@ add_task(async function testExperiments() {
 add_task(withDriver(Assert, async function testAddonsInContext(driver) {
   // Create before install so that the listener is added before startup completes.
   const startupPromise = AddonTestUtils.promiseWebExtensionStartup("normandydriver@example.com");
-  const addonId = await driver.addons.install(TEST_XPI_URL);
+  const addonInstall = await AddonManager.getInstallForURL(TEST_XPI_URL, "application/x-xpinstall");
+  await addonInstall.install();
+  const addonId = addonInstall.addon.id;
   await startupPromise;
 
   const addons = await ClientEnvironment.addons;
@@ -118,7 +120,8 @@ add_task(withDriver(Assert, async function testAddonsInContext(driver) {
     type: "extension",
   }, "addons should be available in context");
 
-  await driver.addons.uninstall(addonId);
+  const addon = await AddonManager.getAddonByID(addonId);
+  await addon.uninstall();
 }));
 
 add_task(async function isFirstRun() {

toolkit/components/normandy/test/browser/browser_NormandyDriver.js

@@ -1,8 +1,6 @@
 "use strict";
 
 ChromeUtils.import("resource://gre/modules/AppConstants.jsm");
-ChromeUtils.import("resource://testing-common/AddonTestUtils.jsm", this);
-ChromeUtils.import("resource://normandy/lib/AddonStudies.jsm", this);
 ChromeUtils.import("resource://normandy/lib/NormandyDriver.jsm", this);
 ChromeUtils.import("resource://normandy/lib/PreferenceExperiments.jsm", this);
 
@@ -16,49 +14,6 @@ add_task(withDriver(Assert, async function uuids(driver) {
   isnot(uuid1, uuid2, "uuids are unique");
 }));
 
-add_task(withDriver(Assert, async function installXpi(driver) {
-  // Test that we can install an XPI from any URL
-  // Create before install so that the listener is added before startup completes.
-  const startupPromise = AddonTestUtils.promiseWebExtensionStartup("normandydriver@example.com");
-
-  var addonId = await driver.addons.install(TEST_XPI_URL);
-  is(addonId, "normandydriver@example.com", "Expected test addon was installed");
-  isnot(addonId, null, "Addon install was successful");
-
-  // Wait until the add-on is fully started up to uninstall it.
-  await startupPromise;
-
-  const uninstallMsg = await driver.addons.uninstall(addonId);
-  is(uninstallMsg, null, `Uninstall returned an unexpected message [${uninstallMsg}]`);
-}));
-
-add_task(withDriver(Assert, async function uninstallInvalidAddonId(driver) {
-  const invalidAddonId = "not_a_valid_xpi_id@foo.bar";
-  try {
-    await driver.addons.uninstall(invalidAddonId);
-    ok(false, `Uninstalling an invalid XPI should fail. addons.uninstall resolved successfully though.`);
-  } catch (e) {
-    ok(true, `This is the expected failure`);
-  }
-}));
-
-
-add_task(withDriver(Assert, async function installXpiBadURL(driver) {
-  let xpiUrl;
-  if (AppConstants.platform === "win") {
-    xpiUrl = "file:///C:/invalid_xpi.xpi";
-  } else {
-    xpiUrl = "file:///tmp/invalid_xpi.xpi";
-  }
-
-  try {
-    await driver.addons.install(xpiUrl);
-    ok(false, "Installation succeeded on an XPI that doesn't exist");
-  } catch (reason) {
-    ok(true, `Installation was rejected: [${reason}]`);
-  }
-}));
-
 add_task(withDriver(Assert, async function userId(driver) {
   // Test that userId is a UUID
   ok(UUID_REGEX.test(driver.userId), "userId is a uuid");
@@ -131,109 +86,6 @@ decorate_task(
   }
 );
 
-add_task(withDriver(Assert, async function getAddon(driver, sandboxManager) {
-  const ADDON_ID = "normandydriver@example.com";
-  let addon = await driver.addons.get(ADDON_ID);
-  Assert.equal(addon, null, "Add-on is not yet installed");
-
-  await driver.addons.install(TEST_XPI_URL);
-  addon = await driver.addons.get(ADDON_ID);
-
-  Assert.notEqual(addon, null, "Add-on object was returned");
-  ok(addon.installDate instanceof sandboxManager.sandbox.Date, "installDate should be a Date object");
-
-  Assert.deepEqual(addon, {
-    id: "normandydriver@example.com",
-    name: "normandy_fixture",
-    version: "1.0",
-    installDate: addon.installDate,
-    isActive: true,
-    type: "extension",
-  }, "Add-on is installed");
-
-  await driver.addons.uninstall(ADDON_ID);
-  addon = await driver.addons.get(ADDON_ID);
-
-  Assert.equal(addon, null, "Add-on has been uninstalled");
-}));
-
-decorate_task(
-  withSandboxManager(Assert),
-  async function testAddonsGetWorksInSandbox(sandboxManager) {
-    const driver = new NormandyDriver(sandboxManager);
-    sandboxManager.cloneIntoGlobal("driver", driver, {cloneFunctions: true});
-
-    // Assertion helpers
-    sandboxManager.addGlobal("is", is);
-    sandboxManager.addGlobal("deepEqual", (...args) => Assert.deepEqual(...args));
-
-    const ADDON_ID = "normandydriver@example.com";
-
-    await driver.addons.install(TEST_XPI_URL);
-
-    await sandboxManager.evalInSandbox(`
-      (async function sandboxTest() {
-        const addon = await driver.addons.get("${ADDON_ID}");
-
-        deepEqual(addon, {
-          id: "${ADDON_ID}",
-          name: "normandy_fixture",
-          version: "1.0",
-          installDate: addon.installDate,
-          isActive: true,
-          type: "extension",
-        }, "Add-on is accesible in the driver");
-      })();
-    `);
-
-    await driver.addons.uninstall(ADDON_ID);
-  }
-);
-
-decorate_task(
-  withSandboxManager(Assert),
-  withWebExtension({id: "driver-addon-studies@example.com"}),
-  AddonStudies.withStudies(),
-  async function testAddonStudies(sandboxManager, [addonId, addonFile]) {
-    const addonUrl = Services.io.newFileURI(addonFile).spec;
-    const driver = new NormandyDriver(sandboxManager);
-    sandboxManager.cloneIntoGlobal("driver", driver, {cloneFunctions: true});
-
-    // Assertion helpers
-    sandboxManager.addGlobal("is", is);
-    sandboxManager.addGlobal("ok", ok);
-
-    await sandboxManager.evalInSandbox(`
-      (async function sandboxTest() {
-        const recipeId = 5;
-        let hasStudy = await driver.studies.has(recipeId);
-        ok(!hasStudy, "studies.has returns false if the study hasn't been started yet.");
-
-        await driver.studies.start({
-          recipeId,
-          name: "fake",
-          description: "fake",
-          addonUrl: "${addonUrl}",
-        });
-        hasStudy = await driver.studies.has(recipeId);
-        ok(hasStudy, "studies.has returns true after the study has been started.");
-
-        let study = await driver.studies.get(recipeId);
-        is(
-          study.addonId,
-          "driver-addon-studies@example.com",
-          "studies.get fetches studies from within a sandbox."
-        );
-        ok(study.active, "Studies are marked as active after being started by the driver.");
-
-        await driver.studies.stop(recipeId);
-        study = await driver.studies.get(recipeId);
-        ok(!study.active, "Studies are marked as inactive after being stopped by the driver.");
-      })();
-    `);
-  }
-);
-
 decorate_task(
   withPrefEnv({
     set: [

toolkit/components/normandy/test/browser/browser_ShieldPreferences.js

@@ -2,8 +2,11 @@
 
 ChromeUtils.import("resource://gre/modules/Services.jsm", this);
 ChromeUtils.import("resource://normandy/lib/AddonStudies.jsm", this);
+ChromeUtils.import("resource://normandy/lib/ShieldPreferences.jsm", this);
 
-const OPT_OUT_PREF = "app.shield.optoutstudies.enabled";
+const OPT_OUT_STUDIES_ENABLED_PREF = "app.shield.optoutstudies.enabled";
+
+ShieldPreferences.init();
 
 decorate_task(
   withMockPreferences,
@@ -12,12 +15,13 @@ decorate_task(
     studyFactory({active: true}),
   ]),
   async function testDisableStudiesWhenOptOutDisabled(mockPreferences, [study1, study2]) {
-    mockPreferences.set(OPT_OUT_PREF, true);
+
+    mockPreferences.set(OPT_OUT_STUDIES_ENABLED_PREF, true);
     const observers = [
       studyEndObserved(study1.recipeId),
       studyEndObserved(study2.recipeId),
     ];
-    Services.prefs.setBoolPref(OPT_OUT_PREF, false);
+    Services.prefs.setBoolPref(OPT_OUT_STUDIES_ENABLED_PREF, false);
     await Promise.all(observers);
 
     const newStudy1 = await AddonStudies.get(study1.recipeId);

toolkit/components/normandy/test/browser/browser_actions_AddonStudyAction.js

@@ -0,0 +1,301 @@
+"use strict";
+
+ChromeUtils.import("resource://gre/modules/Services.jsm", this);
+ChromeUtils.import("resource://normandy/actions/AddonStudyAction.jsm", this);
+ChromeUtils.import("resource://normandy/lib/AddonStudies.jsm", this);
+ChromeUtils.import("resource://normandy/lib/Uptake.jsm", this);
+
+const FIXTURE_ADDON_ID = "normandydriver@example.com";
+const FIXTURE_ADDON_URL = "http://example.com/browser/toolkit/components/normandy/test/browser/fixtures/normandy.xpi";
+
+function addonStudyRecipeFactory(overrides = {}) {
+  let args = {
+    name: "Fake name",
+    description: "fake description",
+    addonUrl: "https://example.com/study.xpi",
+  };
+  if (Object.hasOwnProperty.call(overrides, "arguments")) {
+    args = Object.assign(args, overrides.arguments);
+    delete overrides.arguments;
+  }
+  return recipeFactory(Object.assign({ action: "addon-study", arguments: args }, overrides));
+}
+
+/**
+ * Test decorator that checks that the test cleans up all add-ons installed
+ * during the test. Likely needs to be the first decorator used.
+ */
+function ensureAddonCleanup(testFunction) {
+  return async function wrappedTestFunction(...args) {
+    const beforeAddons = new Set(await AddonManager.getAllAddons());
+
+    try {
+      await testFunction(...args);
+    } finally {
+      const afterAddons = new Set(await AddonManager.getAllAddons());
+      Assert.deepEqual(beforeAddons, afterAddons, "The add-ons should be same before and after the test");
+    }
+  };
+}
+
+// Test that enroll is not called if recipe is already enrolled
+decorate_task(
+  ensureAddonCleanup,
+  AddonStudies.withStudies([studyFactory()]),
+  withSendEventStub,
+  async function enrollTwiceFail([study], sendEventStub) {
+    const recipe = recipeFactory({
+      id: study.recipeId,
+      type: "addon-study",
+      arguments: {
+        name: study.name,
+        description: study.description,
+        addonUrl: study.addonUrl,
+      },
+    });
+    const action = new AddonStudyAction();
+    const enrollSpy = sinon.spy(action, "enroll");
+    await action.runRecipe(recipe);
+    Assert.deepEqual(enrollSpy.args, [], "enroll should not be called");
+    Assert.deepEqual(sendEventStub.args, [], "no events should be sent");
+  },
+);
+
+// Test that if the add-on fails to install, the database is cleaned up and the
+// error is correctly reported.
+decorate_task(
+  ensureAddonCleanup,
+  withSendEventStub,
+  AddonStudies.withStudies([]),
+  async function enrollFailInstall(sendEventStub) {
+    const recipe = addonStudyRecipeFactory({ arguments: { addonUrl: "https://example.com/404.xpi" }});
+    const action = new AddonStudyAction();
+    await action.enroll(recipe);
+
+    const studies = await AddonStudies.getAll();
+    Assert.deepEqual(studies, [], "the study should not be in the database");
+
+    Assert.deepEqual(
+      sendEventStub.args,
+      [["enrollFailed", "addon_study", recipe.arguments.name, {reason: "download-failure"}]],
+      "An enrollFailed event should be sent",
+    );
+  }
+);
+
+// Test that in the case of a study add-on conflicting with a non-study add-on, the study does not enroll
+decorate_task(
+  ensureAddonCleanup,
+  AddonStudies.withStudies([]),
+  withSendEventStub,
+  withInstalledWebExtension({ version: "0.1", id: FIXTURE_ADDON_ID }),
+  async function conflictingEnrollment(studies, sendEventStub, [installedAddonId, installedAddonFile]) {
+    is(installedAddonId, FIXTURE_ADDON_ID, "Generated, installed add-on should have the same ID as the fixture");
+    const addonUrl = FIXTURE_ADDON_URL;
+    const recipe = addonStudyRecipeFactory({ arguments: { name: "conflicting", addonUrl } });
+    const action = new AddonStudyAction();
+    await action.runRecipe(recipe);
+
+    const addon = await AddonManager.getAddonByID(FIXTURE_ADDON_ID);
+    is(addon.version, "0.1", "The installed add-on should not be replaced");
+
+    Assert.deepEqual(await AddonStudies.getAll(), [], "There should be no enrolled studies");
+
+    Assert.deepEqual(
+      sendEventStub.args,
+      [["enrollFailed", "addon_study", recipe.arguments.name, { reason: "conflicting-addon-id" }]],
+      "A enrollFailed event should be sent",
+    );
+  },
+);
+
+// Test a successful enrollment
+decorate_task(
+  ensureAddonCleanup,
+  withSendEventStub,
+  AddonStudies.withStudies(),
+  async function successfulEnroll(sendEventStub, studies) {
+    const webExtStartupPromise = AddonTestUtils.promiseWebExtensionStartup(FIXTURE_ADDON_ID);
+    const addonUrl = FIXTURE_ADDON_URL;
+
+    let addon = await AddonManager.getAddonByID(FIXTURE_ADDON_ID);
+    is(addon, null, "Before enroll, the add-on is not installed");
+
+    const recipe = addonStudyRecipeFactory({ arguments: { name: "success", addonUrl } });
+    const action = new AddonStudyAction();
+    await action.runRecipe(recipe);
+
+    await webExtStartupPromise;
+    addon = await AddonManager.getAddonByID(FIXTURE_ADDON_ID);
+    ok(addon, "After start is called, the add-on is installed");
+
+    const study = await AddonStudies.get(recipe.id);
+    Assert.deepEqual(
+      study,
+      {
+        recipeId: recipe.id,
+        name: recipe.arguments.name,
+        description: recipe.arguments.description,
+        addonId: FIXTURE_ADDON_ID,
+        addonVersion: "1.0",
+        addonUrl,
+        active: true,
+        studyStartDate: study.studyStartDate,
+      },
+      "study data should be stored",
+    );
+    ok(study.studyStartDate, "a start date should be assigned");
+    is(study.studyEndDate, null, "an end date should not be assigned");
+
+    Assert.deepEqual(
+      sendEventStub.args,
+      [["enroll", "addon_study", recipe.arguments.name, { addonId: FIXTURE_ADDON_ID, addonVersion: "1.0" }]],
+      "an enrollment event should be sent",
+    );
+
+    // cleanup
+    await addon.uninstall();
+  },
+);
+
+// Test that unenrolling fails if the study doesn't exist
+decorate_task(
+  ensureAddonCleanup,
+  AddonStudies.withStudies(),
+  async function unenrollNonexistent(studies) {
+    const action = new AddonStudyAction();
+    await Assert.rejects(
+      action.unenroll(42),
+      /no study found/i,
+      "unenroll should fail when no study exists"
+    );
+  }
+);
+
+// Test that unenrolling an inactive experiment fails
+decorate_task(
+  ensureAddonCleanup,
+  AddonStudies.withStudies([
+    studyFactory({active: false}),
+  ]),
+  withSendEventStub,
+  async ([study], sendEventStub) => {
+    const action = new AddonStudyAction();
+    await Assert.rejects(
+      action.unenroll(study.recipeId),
+      /cannot stop study.*already inactive/i,
+      "unenroll should fail when the requested study is inactive"
+    );
+  }
+);
+
+// test a successful unenrollment
+const testStopId = "testStop@example.com";
+decorate_task(
+  ensureAddonCleanup,
+  AddonStudies.withStudies([
+    studyFactory({active: true, addonId: testStopId, studyEndDate: null}),
+  ]),
+  withInstalledWebExtension({id: testStopId}, /* expectUninstall: */ true),
+  withSendEventStub,
+  async function unenrollTest([study], [addonId, addonFile], sendEventStub) {
+    let addon = await AddonManager.getAddonByID(addonId);
+    ok(addon, "the add-on should be installed before unenrolling");
+
+    const action = new AddonStudyAction();
+    await action.unenroll(study.recipeId, "test-reason");
+
+    const newStudy = AddonStudies.get(study.recipeId);
+    is(!newStudy, false, "stop should mark the study as inactive");
+    ok(newStudy.studyEndDate !== null, "the study should have an end date");
+
+    addon = await AddonManager.getAddonByID(addonId);
+    is(addon, null, "the add-on should be uninstalled after unenrolling");
+
+    Assert.deepEqual(
+      sendEventStub.args,
+      [["unenroll", "addon_study", study.name, {
+        addonId,
+        addonVersion: study.addonVersion,
+        reason: "test-reason"
+      }]],
+      "an unenroll event should be sent",
+    );
+  },
+);
+
+// If the add-on for a study isn't installed, a warning should be logged, but the action is still successful
+decorate_task(
+  ensureAddonCleanup,
+  AddonStudies.withStudies([
+    studyFactory({active: true, addonId: "missingAddon@example.com", studyEndDate: null}),
+  ]),
+  withSendEventStub,
+  async function unenrollTest([study], sendEventStub) {
+    const action = new AddonStudyAction();
+
+    SimpleTest.waitForExplicitFinish();
+    SimpleTest.monitorConsole(() => SimpleTest.finish(), [{message: /could not uninstall addon/i}]);
+    await action.unenroll(study.recipeId);
+
+    Assert.deepEqual(
+      sendEventStub.args,
+      [["unenroll", "addon_study", study.name, {
+        addonId: study.addonId,
+        addonVersion: study.addonVersion,
+        reason: "unknown"
+      }]],
+      "an unenroll event should be sent",
+    );
+
+    SimpleTest.endMonitorConsole();
+  },
+);
+
+// Test that the action respects the study opt-out
+decorate_task(
+  ensureAddonCleanup,
+  withSendEventStub,
+  withMockPreferences,
+  AddonStudies.withStudies([]),
+  async function testOptOut(sendEventStub, mockPreferences) {
+    mockPreferences.set("app.shield.optoutstudies.enabled", false);
+    const action = new AddonStudyAction();
+    is(action.state, AddonStudyAction.STATE_DISABLED, "the action should be disabled");
+    const enrollSpy = sinon.spy(action, "enroll");
+    const recipe = addonStudyRecipeFactory();
+    await action.runRecipe(recipe);
+    await action.finalize();
+    is(action.state, AddonStudyAction.STATE_FINALIZED, "the action should be finalized");
+    Assert.deepEqual(enrollSpy.args, [], "enroll should not be called");
+    Assert.deepEqual(sendEventStub.args, [], "no events should be sent");
+  },
+);
+
+// Test that the action does not execute paused recipes
+decorate_task(
+  ensureAddonCleanup,
+  withSendEventStub,
+  AddonStudies.withStudies([]),
+  async function testOptOut(sendEventStub) {
+    const action = new AddonStudyAction();
+    const enrollSpy = sinon.spy(action, "enroll");
+    const recipe = addonStudyRecipeFactory({arguments: {isEnrollmentPaused: true}});
+    await action.runRecipe(recipe);
+    await action.finalize();
+    Assert.deepEqual(enrollSpy.args, [], "enroll should not be called");
+    Assert.deepEqual(sendEventStub.args, [], "no events should be sent");
+  },
+);
+
+// Test that enroll is not called if recipe is already enrolled
+decorate_task(
+  ensureAddonCleanup,
+  AddonStudies.withStudies([studyFactory()]),
+  async function enrollTwiceFail([study]) {
+    const action = new AddonStudyAction();
+    const unenrollSpy = sinon.stub(action, "unenroll");
+    await action.finalize();
+    Assert.deepEqual(unenrollSpy.args, [[study.recipeId, "recipe-not-seen"]], "unenroll should be called");
+  },
+);

toolkit/components/normandy/test/browser/head.js

@@ -2,7 +2,6 @@ ChromeUtils.import("resource://gre/modules/Preferences.jsm", this);
 ChromeUtils.import("resource://testing-common/AddonTestUtils.jsm", this);
 ChromeUtils.import("resource://testing-common/TestUtils.jsm", this);
 ChromeUtils.import("resource://normandy-content/AboutPages.jsm", this);
-ChromeUtils.import("resource://normandy/lib/Addons.jsm", this);
 ChromeUtils.import("resource://normandy/lib/SandboxManager.jsm", this);
 ChromeUtils.import("resource://normandy/lib/NormandyDriver.jsm", this);
 ChromeUtils.import("resource://normandy/lib/NormandyApi.jsm", this);
@@ -71,21 +70,30 @@ this.withWebExtension = function(manifestOverrides = {}) {
   };
 };
 
-this.withInstalledWebExtension = function(manifestOverrides = {}) {
+this.withCorruptedWebExtension = function() {
+  // This should be an invalid manifest version, so that installing this add-on fails.
+  return this.withWebExtension({ manifest_version: -1 });
+};
+
+this.withInstalledWebExtension = function(manifestOverrides = {}, expectUninstall = false) {
   return function wrapper(testFunction) {
     return decorate(
       withWebExtension(manifestOverrides),
       async function wrappedTestFunction(...args) {
         const [id, file] = args[args.length - 1];
         const startupPromise = AddonTestUtils.promiseWebExtensionStartup(id);
-        const url = Services.io.newFileURI(file).spec;
-        await Addons.install(url);
+        const addonInstall = await AddonManager.getInstallForFile(file, "application/x-xpinstall");
+        await addonInstall.install();
         await startupPromise;
+
         try {
           await testFunction(...args);
         } finally {
-          if (await Addons.get(id)) {
-            await Addons.uninstall(id);
+          const addonToUninstall = await AddonManager.getAddonByID(id);
+          if (addonToUninstall) {
+            await addonToUninstall.uninstall();
+          } else {
+            ok(expectUninstall, "Add-on should not be unexpectedly uninstalled during test");
           }
         }
       }
@@ -231,7 +239,7 @@ this.withPrefEnv = function(inPrefs) {
 
 /**
  * Combine a list of functions right to left. The rightmost function is passed
- * to the preceeding function as the argument; the result of this is passed to
+ * to the preceding function as the argument; the result of this is passed to
  * the next function until all are exhausted. For example, this:
  *
  * decorate(func1, func2, func3);
@@ -354,3 +362,11 @@ this.withSendEventStub = function(testFunction) {
     }
   };
 };
+
+let _recipeId = 1;
+this.recipeFactory = function(overrides = {}) {
+  return Object.assign({
+    id: _recipeId++,
+    arguments: overrides.arguments || {},
+  }, overrides);
+};