From 5491e15085d3cd32b596804de65d9295bbeb675e Mon Sep 17 00:00:00 2001 From: Edouard Oger Date: Fri, 8 Dec 2017 15:34:29 -0500 Subject: [PATCH] Bug 633062 p5 - Remove event loop spinning from addonutils.js. r=markh MozReview-Commit-ID: 1PSX4tOieEH --HG-- extra : rebase_source : e8804e1428c78d47fbc776e74da61d5de4e5f402 --- services/sync/modules/addonutils.js | 264 ++++++++---------- services/sync/modules/engines/addons.js | 13 +- services/sync/tests/unit/test_addon_utils.js | 29 +- .../tps/resource/modules/addons.jsm | 7 +- .../sync/tps/extensions/tps/resource/tps.jsm | 2 +- 5 files changed, 133 insertions(+), 182 deletions(-) diff --git a/services/sync/modules/addonutils.js b/services/sync/modules/addonutils.js index 25f6202ade62..7ecca4baeb63 100644 --- a/services/sync/modules/addonutils.js +++ b/services/sync/modules/addonutils.js @@ -25,21 +25,13 @@ 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 @@ -47,11 +39,9 @@ AddonUtilsInternal.prototype = { // 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, @@ -70,11 +60,6 @@ AddonUtilsInternal.prototype = { * 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. @@ -85,28 +70,20 @@ AddonUtilsInternal.prototype = { * 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) { @@ -130,26 +107,26 @@ AddonUtilsInternal.prototype = { 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; + } }, /** @@ -220,133 +197,120 @@ AddonUtilsInternal.prototype = { * * @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 toInstall = []; + + // 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 expectedInstallCount = 0; - let finishedCount = 0; - let installCallback = function installCallback(error, result) { - finishedCount++; + // We can go ahead and attempt to install it. + toInstall.push(addon); - if (error) { - ourResult.errors.push(error); - } else { - ourResult.installedIDs.push(result.id); - ourResult.installs.push(result.install); - ourResult.addons.push(result.addon); - } + // 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; + } - if (finishedCount >= expectedInstallCount) { - if (ourResult.errors.length > 0) { - cb(new Error("1 or more add-ons failed to install"), ourResult); - } else { - cb(null, ourResult); - } - } - }; + let params = addon.sourceURI.query.split("&").map( + function rewrite(param) { - let toInstall = []; - - // 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; - } - - // We can go ahead and attempt to install it. - toInstall.push(addon); - - // 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; - } - - let params = addon.sourceURI.query.split("&").map( - function rewrite(param) { - - if (param.indexOf("src=") == 0) { - return "src=sync"; - } - return param; - }); - - addon.sourceURI.query = params.join("&"); + if (param.indexOf("src=") == 0) { + return "src=sync"; } + return param; + }); - expectedInstallCount = toInstall.length; + addon.sourceURI.query = params.join("&"); + } - if (!expectedInstallCount) { - cb(null, ourResult); - return; + if (!toInstall.length) { + return ourResult; + } + + 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; } + } - // 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; - } - } - - this.installAddonFromSearchResult(addon, options, installCallback); + 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); } + })()); + } - }, + 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; }, /** diff --git a/services/sync/modules/engines/addons.js b/services/sync/modules/engines/addons.js index b877e9eb53a9..93b2b177999b 100644 --- a/services/sync/modules/engines/addons.js +++ b/services/sync/modules/engines/addons.js @@ -305,17 +305,14 @@ AddonsStore.prototype = { * 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); @@ -370,7 +367,7 @@ AddonsStore.prototype = { // 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; } diff --git a/services/sync/tests/unit/test_addon_utils.js b/services/sync/tests/unit/test_addon_utils.js index 269f443c4805..dd211bdf61a4 100644 --- a/services/sync/tests/unit/test_addon_utils.js +++ b/services/sync/tests/unit/test_addon_utils.js @@ -46,16 +46,14 @@ function run_test() { 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); @@ -63,7 +61,7 @@ add_test(function test_handle_empty_source_uri() { 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() { @@ -93,7 +91,7 @@ add_test(function test_ignore_untrusted_source_uris() { 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 @@ -104,34 +102,29 @@ add_test(function test_source_uri_rewrite() { 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); }); diff --git a/services/sync/tps/extensions/tps/resource/modules/addons.jsm b/services/sync/tps/extensions/tps/resource/modules/addons.jsm index 8f042e3ec9be..3a804feb183f 100644 --- a/services/sync/tps/extensions/tps/resource/modules/addons.jsm +++ b/services/sync/tps/extensions/tps/resource/modules/addons.jsm @@ -10,7 +10,6 @@ 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"); @@ -85,13 +84,11 @@ Addon.prototype = { } }, - 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], diff --git a/services/sync/tps/extensions/tps/resource/tps.jsm b/services/sync/tps/extensions/tps/resource/tps.jsm index f1aa1cbfff6d..29f2b1c15c5e 100644 --- a/services/sync/tps/extensions/tps/resource/tps.jsm +++ b/services/sync/tps/extensions/tps/resource/tps.jsm @@ -484,7 +484,7 @@ var TPS = { let addon = new Addon(this, entry); switch (action) { case ACTION_ADD: - addon.install(); + await addon.install(); break; case ACTION_DELETE: await addon.uninstall();