Bug 633062 p5 - Remove event loop spinning from addonutils.js. r=markh

MozReview-Commit-ID: 1PSX4tOieEH --HG-- extra : rebase_source : e8804e1428c78d47fbc776e74da61d5de4e5f402
2017-12-08 15:34:29 -05:00 · 2017-12-08 15:34:29 -05:00 · 5491e15085
--- 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;
  },

  /**
--- 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;
    }

--- 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);
 });
--- 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],
--- 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();