Bug 633062 p5 - Remove event loop spinning from addonutils.js. r?markh draft
authorEdouard Oger <eoger@fastmail.com>
Fri, 08 Dec 2017 15:34:29 -0500
changeset 749590 c3238b5ac6bb2c3e362e712a8718d2ac4114bf28
parent 749589 da71b1d5fb337ec94dc9d182eeda6d8617a86d9f
child 749591 0ea264c5e47ff45a1e2c1ca30a3122e7c9116011
push id97449
push userbmo:eoger@fastmail.com
push dateWed, 31 Jan 2018 19:28:47 +0000
reviewersmarkh
bugs633062
milestone60.0a1
Bug 633062 p5 - Remove event loop spinning from addonutils.js. r?markh MozReview-Commit-ID: 1PSX4tOieEH
services/sync/modules/addonutils.js
services/sync/modules/engines/addons.js
services/sync/tests/unit/test_addon_utils.js
services/sync/tps/extensions/tps/resource/modules/addons.jsm
services/sync/tps/extensions/tps/resource/tps.jsm
--- 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: