Bug 633062 p5 - Remove event loop spinning from addonutils.js. r?markh
MozReview-Commit-ID: 1PSX4tOieEH
--- a/services/sync/modules/addonutils.js
+++ b/services/sync/modules/addonutils.js
@@ -20,43 +20,33 @@ ChromeUtils.defineModuleGetter(this, "Ad
function AddonUtilsInternal() {
this._log = Log.repository.getLogger("Sync.AddonUtils");
this._log.Level = Log.Level[Svc.Prefs.get("log.logger.addonutils")];
}
AddonUtilsInternal.prototype = {
/**
* Obtain an AddonInstall object from an AddonSearchResult instance.
*
- * The callback will be invoked with the result of the operation. The
- * callback receives 2 arguments, error and result. Error will be falsy
- * on success or some kind of error value otherwise. The result argument
- * will be an AddonInstall on success or null on failure. It is possible
- * for the error to be falsy but result to be null. This could happen if
- * an install was not found.
+ * The returned promise will be an AddonInstall on success or null (failure or
+ * addon not found)
*
* @param addon
* AddonSearchResult to obtain install from.
- * @param cb
- * Function to be called with result of operation.
*/
- getInstallFromSearchResult:
- function getInstallFromSearchResult(addon, cb) {
-
+ getInstallFromSearchResult(addon) {
this._log.debug("Obtaining install for " + addon.id);
// We should theoretically be able to obtain (and use) addon.install if
// it is available. However, the addon.sourceURI rewriting won't be
// reflected in the AddonInstall, so we can't use it. If we ever get rid
// of sourceURI rewriting, we can avoid having to reconstruct the
// AddonInstall.
- AddonManager.getInstallForURL(
+ return AddonManager.getInstallForURL(
addon.sourceURI.spec,
- function handleInstall(install) {
- cb(null, install);
- },
+ null,
"application/x-xpinstall",
undefined,
addon.name,
addon.iconURL,
addon.version
);
},
@@ -65,53 +55,40 @@ AddonUtilsInternal.prototype = {
*
* The options argument defines extra options to control the install.
* Recognized keys in this map are:
*
* syncGUID - Sync GUID to use for the new add-on.
* enabled - Boolean indicating whether the add-on should be enabled upon
* install.
*
- * When complete it calls a callback with 2 arguments, error and result.
- *
- * If error is falsy, result is an object. If error is truthy, result is
- * null.
- *
* The result object has the following keys:
*
* id ID of add-on that was installed.
* install AddonInstall that was installed.
* addon Addon that was installed.
*
* @param addon
* AddonSearchResult to install add-on from.
* @param options
* Object with additional metadata describing how to install add-on.
- * @param cb
- * Function to be invoked with result of operation.
*/
- installAddonFromSearchResult:
- function installAddonFromSearchResult(addon, options, cb) {
+ async installAddonFromSearchResult(addon, options) {
this._log.info("Trying to install add-on from search result: " + addon.id);
- this.getInstallFromSearchResult(addon, (error, install) => {
- if (error) {
- cb(error, null);
- return;
- }
+ const install = await this.getInstallFromSearchResult(addon);
+ if (!install) {
+ throw new Error("AddonInstall not available: " + addon.id);
+ }
- if (!install) {
- cb(new Error("AddonInstall not available: " + addon.id), null);
- return;
- }
+ try {
+ this._log.info("Installing " + addon.id);
+ let log = this._log;
- try {
- this._log.info("Installing " + addon.id);
- let log = this._log;
-
+ return new Promise((res, rej) => {
let listener = {
onInstallStarted: function onInstallStarted(install) {
if (!options) {
return;
}
if (options.syncGUID) {
log.info("Setting syncGUID of " + install.name + ": " +
@@ -125,36 +102,36 @@ AddonUtilsInternal.prototype = {
log.info("Marking add-on as disabled for install: " +
install.name);
install.addon.userDisabled = true;
}
},
onInstallEnded(install, addon) {
install.removeListener(listener);
- cb(null, {id: addon.id, install, addon});
+ res({id: addon.id, install, addon});
},
onInstallFailed(install) {
install.removeListener(listener);
- cb(new Error("Install failed: " + install.error), null);
+ rej(new Error("Install failed: " + install.error));
},
onDownloadFailed(install) {
install.removeListener(listener);
- cb(new Error("Download failed: " + install.error), null);
+ rej(new Error("Download failed: " + install.error));
}
};
install.addListener(listener);
install.install();
- } catch (ex) {
- this._log.error("Error installing add-on", ex);
- cb(ex, null);
- }
- });
+ });
+ } catch (ex) {
+ this._log.error("Error installing add-on", ex);
+ throw ex;
+ }
},
/**
* Uninstalls the addon instance.
*
* @param addon
* Addon instance to uninstall.
*/
@@ -215,143 +192,130 @@ AddonUtilsInternal.prototype = {
* installedIDs Array of add-on IDs that were installed.
* installs Array of AddonInstall instances that were installed.
* addons Array of Addon instances that were installed.
* errors Array of errors encountered. Only has elements if error is
* truthy.
*
* @param installs
* Array of objects describing add-ons to install.
- * @param cb
- * Function to be called when all actions are complete.
*/
- installAddons: function installAddons(installs, cb) {
- if (!cb) {
- throw new Error("Invalid argument: cb is not defined.");
- }
-
+ async installAddons(installs) {
let ids = [];
for (let addon of installs) {
ids.push(addon.id);
}
- AddonRepository.getAddonsByIDs(ids, {
- searchSucceeded: (addons, addonsLength, total) => {
- this._log.info("Found " + addonsLength + "/" + ids.length +
+ const {addons, addonsLength} = await new Promise((res, rej) => {
+ AddonRepository.getAddonsByIDs(ids, {
+ searchSucceeded: (addons, addonsLength, total) => {
+ res({addons, addonsLength});
+ },
+ searchFailed() {
+ rej(new Error("AddonRepository search failed"));
+ }
+ });
+ });
+
+ this._log.info("Found " + addonsLength + "/" + ids.length +
" add-ons during repository search.");
- let ourResult = {
- installedIDs: [],
- installs: [],
- addons: [],
- skipped: [],
- errors: []
- };
+ let ourResult = {
+ installedIDs: [],
+ installs: [],
+ addons: [],
+ skipped: [],
+ errors: []
+ };
- if (!addonsLength) {
- cb(null, ourResult);
- return;
- }
+ if (!addonsLength) {
+ return ourResult;
+ }
- let expectedInstallCount = 0;
- let finishedCount = 0;
- let installCallback = function installCallback(error, result) {
- finishedCount++;
+ let toInstall = [];
- if (error) {
- ourResult.errors.push(error);
- } else {
- ourResult.installedIDs.push(result.id);
- ourResult.installs.push(result.install);
- ourResult.addons.push(result.addon);
- }
+ // Rewrite the "src" query string parameter of the source URI to note
+ // that the add-on was installed by Sync and not something else so
+ // server-side metrics aren't skewed (bug 708134). The server should
+ // ideally send proper URLs, but this solution was deemed too
+ // complicated at the time the functionality was implemented.
+ for (let addon of addons) {
+ // Find the specified options for this addon.
+ let options;
+ for (let install of installs) {
+ if (install.id == addon.id) {
+ options = install;
+ break;
+ }
+ }
+ if (!this.canInstallAddon(addon, options)) {
+ ourResult.skipped.push(addon.id);
+ continue;
+ }
- if (finishedCount >= expectedInstallCount) {
- if (ourResult.errors.length > 0) {
- cb(new Error("1 or more add-ons failed to install"), ourResult);
- } else {
- cb(null, ourResult);
- }
- }
- };
+ // We can go ahead and attempt to install it.
+ toInstall.push(addon);
- let toInstall = [];
+ // We should always be able to QI the nsIURI to nsIURL. If not, we
+ // still try to install the add-on, but we don't rewrite the URL,
+ // potentially skewing metrics.
+ try {
+ addon.sourceURI.QueryInterface(Ci.nsIURL);
+ } catch (ex) {
+ this._log.warn("Unable to QI sourceURI to nsIURL: " +
+ addon.sourceURI.spec);
+ continue;
+ }
- // Rewrite the "src" query string parameter of the source URI to note
- // that the add-on was installed by Sync and not something else so
- // server-side metrics aren't skewed (bug 708134). The server should
- // ideally send proper URLs, but this solution was deemed too
- // complicated at the time the functionality was implemented.
- for (let addon of addons) {
- // Find the specified options for this addon.
- let options;
- for (let install of installs) {
- if (install.id == addon.id) {
- options = install;
- break;
- }
- }
- if (!this.canInstallAddon(addon, options)) {
- ourResult.skipped.push(addon.id);
- continue;
- }
+ let params = addon.sourceURI.query.split("&").map(
+ function rewrite(param) {
- // We can go ahead and attempt to install it.
- toInstall.push(addon);
+ if (param.indexOf("src=") == 0) {
+ return "src=sync";
+ }
+ return param;
+ });
- // We should always be able to QI the nsIURI to nsIURL. If not, we
- // still try to install the add-on, but we don't rewrite the URL,
- // potentially skewing metrics.
- try {
- addon.sourceURI.QueryInterface(Ci.nsIURL);
- } catch (ex) {
- this._log.warn("Unable to QI sourceURI to nsIURL: " +
- addon.sourceURI.spec);
- continue;
- }
+ addon.sourceURI.query = params.join("&");
+ }
- let params = addon.sourceURI.query.split("&").map(
- function rewrite(param) {
+ if (!toInstall.length) {
+ return ourResult;
+ }
- if (param.indexOf("src=") == 0) {
- return "src=sync";
- }
- return param;
- });
-
- addon.sourceURI.query = params.join("&");
+ const installPromises = [];
+ // Start all the installs asynchronously. They will report back to us
+ // as they finish, eventually triggering the global callback.
+ for (let addon of toInstall) {
+ let options = {};
+ for (let install of installs) {
+ if (install.id == addon.id) {
+ options = install;
+ break;
}
-
- expectedInstallCount = toInstall.length;
-
- if (!expectedInstallCount) {
- cb(null, ourResult);
- return;
- }
+ }
- // Start all the installs asynchronously. They will report back to us
- // as they finish, eventually triggering the global callback.
- for (let addon of toInstall) {
- let options = {};
- for (let install of installs) {
- if (install.id == addon.id) {
- options = install;
- break;
- }
- }
+ installPromises.push((async () => {
+ try {
+ const result = await this.installAddonFromSearchResult(addon, options);
+ ourResult.installedIDs.push(result.id);
+ ourResult.installs.push(result.install);
+ ourResult.addons.push(result.addon);
+ } catch (error) {
+ ourResult.errors.push(error);
+ }
+ })());
+ }
- this.installAddonFromSearchResult(addon, options, installCallback);
- }
-
- },
+ await Promise.all(installPromises);
- searchFailed: function searchFailed() {
- cb(new Error("AddonRepository search failed"), null);
- },
- });
+ if (ourResult.errors.length > 0) {
+ throw new Error("1 or more add-ons failed to install");
+ }
+ return ourResult;
},
/**
* Returns true if we are able to install the specified addon, false
* otherwise. It is expected that this will log the reason if it returns
* false.
*
* @param addon
--- a/services/sync/modules/engines/addons.js
+++ b/services/sync/modules/engines/addons.js
@@ -300,27 +300,24 @@ AddonsStore.prototype = {
await Store.prototype.applyIncoming.call(this, record);
},
/**
* Provides core Store API to create/install an add-on from a record.
*/
async create(record) {
- let cb = Async.makeSpinningCallback();
- AddonUtils.installAddons([{
+ // This will throw if there was an error. This will get caught by the sync
+ // engine and the record will try to be applied later.
+ const results = await AddonUtils.installAddons([{
id: record.addonID,
syncGUID: record.id,
enabled: record.enabled,
requireSecureURI: this._extensionsPrefs.get("install.requireSecureOrigin", true),
- }], cb);
-
- // This will throw if there was an error. This will get caught by the sync
- // engine and the record will try to be applied later.
- let results = cb.wait();
+ }]);
if (results.skipped.includes(record.addonID)) {
this._log.info("Add-on skipped: " + record.addonID);
// Just early-return for skipped addons - we don't want to arrange to
// try again next time because the condition that caused up to skip
// will remain true for this addon forever.
return;
}
@@ -365,17 +362,17 @@ AddonsStore.prototype = {
let addon = await this.getAddonByID(record.addonID);
// update() is called if !this.itemExists. And, since itemExists consults
// the reconciler only, we need to take care of some corner cases.
//
// First, the reconciler could know about an add-on that was uninstalled
// and no longer present in the add-ons manager.
if (!addon) {
- this.create(record);
+ await this.create(record);
return;
}
// It's also possible that the add-on is non-restartless and has pending
// install/uninstall activity.
//
// We wouldn't get here if the incoming record was for a deletion. So,
// check for pending uninstall and cancel if necessary.
--- a/services/sync/tests/unit/test_addon_utils.js
+++ b/services/sync/tests/unit/test_addon_utils.js
@@ -41,34 +41,32 @@ function createAndStartHTTPServer(port =
}
function run_test() {
syncTestLogging();
run_next_test();
}
-add_test(function test_handle_empty_source_uri() {
+add_task(async function test_handle_empty_source_uri() {
_("Ensure that search results without a sourceURI are properly ignored.");
let server = createAndStartHTTPServer();
const ID = "missing-sourceuri@tests.mozilla.org";
- let cb = Async.makeSpinningCallback();
- AddonUtils.installAddons([{id: ID, requireSecureURI: false}], cb);
- let result = cb.wait();
+ const result = await AddonUtils.installAddons([{id: ID, requireSecureURI: false}]);
Assert.ok("installedIDs" in result);
Assert.equal(0, result.installedIDs.length);
Assert.ok("skipped" in result);
Assert.ok(result.skipped.includes(ID));
- server.stop(run_next_test);
+ await promiseStopServer(server);
});
add_test(function test_ignore_untrusted_source_uris() {
_("Ensures that source URIs from insecure schemes are rejected.");
const bad = ["http://example.com/foo.xpi",
"ftp://example.com/foo.xpi",
"silly://example.com/foo.xpi"];
@@ -88,50 +86,45 @@ add_test(function test_ignore_untrusted_
let addon = {sourceURI, name: "good", id: "good"};
let canInstall = AddonUtils.canInstallAddon(addon);
Assert.ok(canInstall, "Correctly accepted a good URL");
}
run_next_test();
});
-add_test(function test_source_uri_rewrite() {
+add_task(async function test_source_uri_rewrite() {
_("Ensure that a 'src=api' query string is rewritten to 'src=sync'");
// This tests for conformance with bug 708134 so server-side metrics aren't
// skewed.
// We resort to monkeypatching because of the API design.
let oldFunction = AddonUtils.__proto__.installAddonFromSearchResult;
let installCalled = false;
AddonUtils.__proto__.installAddonFromSearchResult =
- function testInstallAddon(addon, metadata, cb) {
+ async function testInstallAddon(addon, metadata) {
Assert.equal(SERVER_ADDRESS + "/require.xpi?src=sync",
addon.sourceURI.spec);
installCalled = true;
- AddonUtils.getInstallFromSearchResult(addon, function(error, install) {
- Assert.equal(null, error);
- Assert.equal(SERVER_ADDRESS + "/require.xpi?src=sync",
- install.sourceURI.spec);
-
- cb(null, {id: addon.id, addon, install});
- }, false);
+ const install = await AddonUtils.getInstallFromSearchResult(addon);
+ Assert.equal(SERVER_ADDRESS + "/require.xpi?src=sync",
+ install.sourceURI.spec);
+ return {id: addon.id, addon, install};
};
let server = createAndStartHTTPServer();
- let installCallback = Async.makeSpinningCallback();
let installOptions = {
id: "rewrite@tests.mozilla.org",
requireSecureURI: false,
};
- AddonUtils.installAddons([installOptions], installCallback);
+ await AddonUtils.installAddons([installOptions]);
- installCallback.wait();
Assert.ok(installCalled);
AddonUtils.__proto__.installAddonFromSearchResult = oldFunction;
- server.stop(run_next_test);
+ await promiseStopServer(server);
});
--- a/services/sync/tps/extensions/tps/resource/modules/addons.jsm
+++ b/services/sync/tps/extensions/tps/resource/modules/addons.jsm
@@ -5,17 +5,16 @@
var EXPORTED_SYMBOLS = ["Addon", "STATE_ENABLED", "STATE_DISABLED"];
const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
ChromeUtils.import("resource://gre/modules/AddonManager.jsm");
ChromeUtils.import("resource://gre/modules/addons/AddonRepository.jsm");
ChromeUtils.import("resource://gre/modules/NetUtil.jsm");
-ChromeUtils.import("resource://services-common/async.js");
ChromeUtils.import("resource://services-sync/addonutils.js");
ChromeUtils.import("resource://services-sync/util.js");
ChromeUtils.import("resource://tps/logger.jsm");
const ADDONSGETURL = "http://127.0.0.1:4567/";
const STATE_ENABLED = 1;
const STATE_DISABLED = 2;
@@ -80,23 +79,21 @@ Addon.prototype = {
} else if (state) {
throw new Error("Don't know how to handle state: " + state);
} else {
// No state, so just checking that it exists.
return true;
}
},
- install: function install() {
+ async install() {
// For Install, the id parameter initially passed is really the filename
// for the addon's install .xml; we'll read the actual id from the .xml.
- let cb = Async.makeSpinningCallback();
- AddonUtils.installAddons([{id: this.id, requireSecureURI: false}], cb);
- let result = cb.wait();
+ const result = await AddonUtils.installAddons([{id: this.id, requireSecureURI: false}]);
Logger.AssertEqual(1, result.installedIDs.length, "Exactly 1 add-on was installed.");
Logger.AssertEqual(this.id, result.installedIDs[0],
"Add-on was installed successfully: " + this.id);
},
async setEnabled(flag) {
Logger.AssertTrue((await this.find()), "Add-on is available.");
--- a/services/sync/tps/extensions/tps/resource/tps.jsm
+++ b/services/sync/tps/extensions/tps/resource/tps.jsm
@@ -479,17 +479,17 @@ var TPS = {
async HandleAddons(addons, action, state) {
this.shouldValidateAddons = true;
for (let entry of addons) {
Logger.logInfo("executing action " + action.toUpperCase() +
" on addon " + JSON.stringify(entry));
let addon = new Addon(this, entry);
switch (action) {
case ACTION_ADD:
- addon.install();
+ await addon.install();
break;
case ACTION_DELETE:
await addon.uninstall();
break;
case ACTION_VERIFY:
Logger.AssertTrue((await addon.find(state)), "addon " + addon.id + " not found");
break;
case ACTION_VERIFY_NOT: