Bug 1435253 - Handle more types of failure during add-on study enrollment r?Gijs draft
authorMike Cooper <mcooper@mozilla.com>
Tue, 06 Feb 2018 16:37:38 -0800
changeset 752198 ff35d2a1229a74c018b26f73f10109efcfdb0a52
parent 751859 172f8f653720f399c8ff8a6e49161b1fd6121475
push id98197
push userbmo:mcooper@mozilla.com
push dateWed, 07 Feb 2018 18:51:48 +0000
reviewersGijs
bugs1435253
milestone60.0a1
Bug 1435253 - Handle more types of failure during add-on study enrollment r?Gijs MozReview-Commit-ID: BOUFuvt5wYO
browser/extensions/shield-recipe-client/docs/data-collection.rst
browser/extensions/shield-recipe-client/lib/AddonStudies.jsm
browser/extensions/shield-recipe-client/lib/TelemetryEvents.jsm
browser/extensions/shield-recipe-client/test/browser/browser_AddonStudies.js
--- a/browser/extensions/shield-recipe-client/docs/data-collection.rst
+++ b/browser/extensions/shield-recipe-client/docs/data-collection.rst
@@ -150,16 +150,29 @@ Enrollment
    value
       The name of the study (``recipe.arguments.slug``).
    extra
       addonId
          The add-on's ID (example: ``"feature-study@shield.mozilla.com"``).
       addonVersion
          The add-on's version (example: ``"1.2.3"``).
 
+Enroll Failure
+   method
+      The string ``"enrollFailed"``
+   object
+      The string ``"addon_study"``
+   value
+      The name of the study (``recipe.arguments.slug``).
+   reason
+      A string containing the filename and line number of the code
+      that failed, and the name of the error thrown. This information
+      is purposely limited to avoid leaking personally identifiable
+      information. This should be considered a bug.
+
 Unenrollment
    method
       The string ``"unenroll"``.
    object
       The string ``"addon_study"``.
    value
       The name of the study (``recipe.arguments.name``).
    extra
@@ -181,10 +194,10 @@ Unenrollment
            filter.
          * ``"uninstalled"``: The study's add-on as uninstalled by some
            mechanism. For example, this could be a user action or the
            add-on self-uninstalling.
          * ``"uninstalled-sideload"``: The study's add-on was
            uninstalled while Shield was inactive. This could be that
            the add-on is no longer compatible, or was manually removed
            from a profile.
-         * ``"unknown"``: A reason was not specificied. This should be
+         * ``"unknown"``: A reason was not specified. This should be
            considered a bug.
--- a/browser/extensions/shield-recipe-client/lib/AddonStudies.jsm
+++ b/browser/extensions/shield-recipe-client/lib/AddonStudies.jsm
@@ -252,51 +252,58 @@ this.AddonStudies = {
       throw new Error("Required arguments (recipeId, name, description, addonUrl) missing.");
     }
 
     const db = await getDatabase();
     if (await getStore(db).get(recipeId)) {
       throw new Error(`A study for recipe ${recipeId} already exists.`);
     }
 
-    const 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(),
-    };
+    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(),
+      };
 
-    TelemetryEvents.sendEvent("enroll", "addon_study", name, {
-      addonId: install.addon.id,
-      addonVersion: install.addon.version,
-    });
-
-    try {
       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);
 
-      TelemetryEvents.sendEvent("unenroll", "addon_study", name, {
-        reason: "install-failure",
-        addonId: install.addon.id,
-        addonVersion: install.addon.version,
+      // 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 {
-      Services.obs.notifyObservers(addonFile, "flush-cache-entry");
-      await OS.File.remove(addonFile.path);
+      if (addonFile) {
+        Services.obs.notifyObservers(addonFile, "flush-cache-entry");
+        await OS.File.remove(addonFile.path);
+      }
     }
   },
 
   /**
    * Download a remote add-on and store it in a temporary nsIFile.
    * @param {String} addonUrl
    * @returns {nsIFile}
    */
--- a/browser/extensions/shield-recipe-client/lib/TelemetryEvents.jsm
+++ b/browser/extensions/shield-recipe-client/lib/TelemetryEvents.jsm
@@ -16,16 +16,23 @@ const TelemetryEvents = {
     Services.telemetry.registerEvents(TELEMETRY_CATEGORY, {
       enroll: {
         methods: ["enroll"],
         objects: ["preference_study", "addon_study"],
         extra_keys: ["experimentType", "branch", "addonId", "addonVersion"],
         record_on_release: true,
       },
 
+      enroll_failure: {
+        methods: ["enrollFailed"],
+        objects: ["addon_study"],
+        extra_keys: ["reason"],
+        record_on_release: true,
+      },
+
       unenroll: {
         methods: ["unenroll"],
         objects: ["preference_study", "addon_study"],
         extra_keys: ["reason", "didResetValue", "addonId", "addonVersion"],
         record_on_release: true,
       },
     });
   },
--- a/browser/extensions/shield-recipe-client/test/browser/browser_AddonStudies.js
+++ b/browser/extensions/shield-recipe-client/test/browser/browser_AddonStudies.js
@@ -137,29 +137,41 @@ decorate_task(
       /already exists/,
       "start rejects when a study exists with the given recipeId already."
     );
   }
 );
 
 decorate_task(
   withStub(Addons, "applyInstall"),
+  withStub(TelemetryEvents, "sendEvent"),
   withWebExtension(),
-  async function testStartAddonCleanup(applyInstallStub, [addonId, addonFile]) {
-    applyInstallStub.rejects(new Error("Fake failure"));
+  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(startArgsFactory({addonUrl})),
+      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]) {