Bug 1435253 - Handle more types of failure during add-on study enrollment r?Gijs
MozReview-Commit-ID: BOUFuvt5wYO
--- 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]) {