Bug 1331313 - Let add-on install() / uninstall() wait for the expected add-on. draft
authorHenrik Skupin <mail@hskupin.info>
Wed, 23 Aug 2017 15:53:45 +0200
changeset 659955 4d6ee8f049e82120e415645d0ee7014b17358664
parent 659065 3ecda4678c49ca255c38b1697142b9118cdd27e7
child 730100 5a08a2d5adcb15a28041873fa7b613f843d8ad8d
push id78249
push userbmo:hskupin@gmail.com
push dateWed, 06 Sep 2017 12:03:09 +0000
bugs1331313
milestone57.0a1
Bug 1331313 - Let add-on install() / uninstall() wait for the expected add-on. Currently the listener for addon installs misses a check for the addon id, to only resolve the promise when it has been called for the expected addon. This can cause race-conditions if other addons are getting installed at the same time. The same applies to uninstall which doesn't wait at all until the operation has been completed. MozReview-Commit-ID: 5GsomMoAVZ1
testing/marionette/addon.js
testing/marionette/harness/marionette_harness/tests/unit/test_addons.py
testing/marionette/harness/marionette_harness/tests/unit/webextension-invalid.xpi
--- a/testing/marionette/addon.js
+++ b/testing/marionette/addon.js
@@ -25,16 +25,53 @@ addon.Errors = {
   [-5]: "ERROR_SIGNEDSTATE_REQUIRED: The addon must be signed and isn't.",
 };
 
 function lookupError(code) {
   let msg = addon.Errors[code];
   return new UnknownError(msg);
 }
 
+async function installAddon(file) {
+  let install = await AddonManager.getInstallForFile(file);
+
+  return new Promise((resolve, reject) => {
+    if (install.error != 0) {
+      reject(new UnknownError(lookupError(install.error)));
+    }
+
+    let addonId = install.addon.id;
+
+    let success = install => {
+      if (install.addon.id === addonId) {
+        install.removeListener(listener);
+        resolve(install.addon);
+      }
+    };
+
+    let fail = install => {
+      if (install.addon.id === addonId) {
+        install.removeListener(listener);
+        reject(new UnknownError(lookupError(install.error)));
+      }
+    };
+
+    let listener = {
+      onDownloadCancelled: fail,
+      onDownloadFailed: fail,
+      onInstallCancelled: fail,
+      onInstallFailed: fail,
+      onInstallEnded: success,
+    };
+
+    install.addListener(listener);
+    install.install();
+  });
+}
+
 /**
  * Install a Firefox addon.
  *
  * If the addon is restartless, it can be used right away.  Otherwise a
  * restart is required.
  *
  * Temporary addons will automatically be uninstalled on shutdown and
  * do not need to be signed, though they must be restartless.
@@ -45,61 +82,61 @@ function lookupError(code) {
  *     True to install the addon temporarily, false (default) otherwise.
  *
  * @return {Promise.<string>}
  *     Addon ID.
  *
  * @throws {UnknownError}
  *     If there is a problem installing the addon.
  */
-addon.install = function(path, temporary = false) {
-  return new Promise((resolve, reject) => {
-    let file = new FileUtils.File(path);
-
-    let listener = {
-      onInstallEnded(install, addon) {
-        resolve(addon.id);
-      },
-
-      onInstallFailed(install) {
-        reject(lookupError(install.error));
-      },
+addon.install = async function(path, temporary = false) {
+  let file = new FileUtils.File(path);
+  let addon;
 
-      onInstalled(addon) {
-        AddonManager.removeAddonListener(listener);
-        resolve(addon.id);
-      },
-    };
+  try {
+    if (temporary) {
+      addon = await AddonManager.installTemporaryAddon(file);
+    } else {
+      addon = await installAddon(file);
+    }
+  } catch (e) {
+    throw new UnknownError(
+        `Could not install add-on at '${path}': ${e.message}`);
+  }
 
-    if (!temporary) {
-      AddonManager.getInstallForFile(file, function(aInstall) {
-        if (aInstall.error !== 0) {
-          reject(lookupError(aInstall.error));
-        }
-        aInstall.addListener(listener);
-        aInstall.install();
-      });
-    } else {
-      AddonManager.addAddonListener(listener);
-      AddonManager.installTemporaryAddon(file);
-    }
-  });
+  return addon.id;
 };
 
 /**
  * Uninstall a Firefox addon.
  *
  * If the addon is restartless it will be uninstalled right away.
  * Otherwise, Firefox must be restarted for the change to take effect.
  *
  * @param {string} id
  *     ID of the addon to uninstall.
  *
  * @return {Promise}
+ *
+ * @throws {UnknownError}
+ *     If there is a problem uninstalling the addon.
  */
-addon.uninstall = function(id) {
-  return new Promise(resolve => {
-    AddonManager.getAddonByID(id, function(addon) {
-      addon.uninstall();
-      resolve();
-    });
+addon.uninstall = async function(id) {
+  return AddonManager.getAddonByID(id).then(addon => {
+    let listener = {
+      onOperationCancelled: addon => {
+        if (addon.id === id) {
+          AddonManager.removeAddonListener(listener);
+          throw new UnknownError(`Uninstall of ${id} has been canceled`);
+        }
+      },
+      onUninstalled: addon => {
+        if (addon.id === id) {
+          AddonManager.removeAddonListener(listener);
+          Promise.resolve();
+        }
+      },
+    };
+
+    AddonManager.addAddonListener(listener);
+    addon.uninstall();
   });
 };
--- a/testing/marionette/harness/marionette_harness/tests/unit/test_addons.py
+++ b/testing/marionette/harness/marionette_harness/tests/unit/test_addons.py
@@ -1,58 +1,93 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 import os
 
 from marionette_driver.addons import Addons, AddonInstallException
-from marionette_harness import MarionetteTestCase, skip
+from marionette_harness import MarionetteTestCase
 
 
 here = os.path.abspath(os.path.dirname(__file__))
 
 
 class TestAddons(MarionetteTestCase):
 
     def setUp(self):
-        MarionetteTestCase.setUp(self)
+        super(TestAddons, self).setUp()
+
         self.addons = Addons(self.marionette)
+        self.preinstalled_addons = self.all_addon_ids
+
+    def tearDown(self):
+        self.reset_addons()
+
+        super(TestAddons, self).tearDown()
 
     @property
     def all_addon_ids(self):
-        with self.marionette.using_context('chrome'):
+        with self.marionette.using_context("chrome"):
             addons = self.marionette.execute_async_script("""
               Components.utils.import("resource://gre/modules/AddonManager.jsm");
-              AddonManager.getAllAddons(function(addons){
-                let ids = addons.map(function(x) {
-                  return x.id;
-                });
+              AddonManager.getAllAddons(function(addons) {
+                let ids = addons.map(x => x.id);
                 marionetteScriptFinished(ids);
               });
             """)
 
-        return addons
+        return set(addons)
 
-    def test_install_and_remove_temporary_unsigned_addon(self):
-        addon_path = os.path.join(here, 'webextension-unsigned.xpi')
+    def reset_addons(self):
+        with self.marionette.using_context("chrome"):
+            for addon in (self.all_addon_ids - self.preinstalled_addons):
+                addon_id = self.marionette.execute_async_script("""
+                  Components.utils.import("resource://gre/modules/AddonManager.jsm");
+                  return new Promise(resolve => {
+                    AddonManager.getAddonByID(arguments[0], function(addon) {
+                      addon.uninstall();
+                      marionetteScriptFinished(addon.id);
+                    });
+                  });
+                """, script_args=(addon,))
+                self.assertEqual(addon_id, addon,
+                                 msg="Failed to uninstall {}".format(addon))
+
+    def test_temporary_install_and_remove_unsigned_addon(self):
+        addon_path = os.path.join(here, "webextension-unsigned.xpi")
 
         addon_id = self.addons.install(addon_path, temp=True)
         self.assertIn(addon_id, self.all_addon_ids)
+        self.assertEqual(addon_id, "{d3e7c1f1-2e35-4a49-89fe-9f46eb8abf0a}")
 
         self.addons.uninstall(addon_id)
         self.assertNotIn(addon_id, self.all_addon_ids)
 
-    def test_install_unsigned_addon(self):
-        addon_path = os.path.join(here, 'webextension-unsigned.xpi')
+    def test_temporary_install_invalid_addon(self):
+        addon_path = os.path.join(here, "webextension-invalid.xpi")
+
+        with self.assertRaises(AddonInstallException):
+            self.addons.install(addon_path, temp=True)
+        self.assertNotIn("{d3e7c1f1-2e35-4a49-89fe-9f46eb8abf0a}", self.all_addon_ids)
+
+    def test_install_and_remove_signed_addon(self):
+        addon_path = os.path.join(here, "webextension-signed.xpi")
+
+        addon_id = self.addons.install(addon_path)
+        self.assertIn(addon_id, self.all_addon_ids)
+        self.assertEqual(addon_id, "{d3e7c1f1-2e35-4a49-89fe-9f46eb8abf0a}")
+
+        self.addons.uninstall(addon_id)
+        self.assertNotIn(addon_id, self.all_addon_ids)
+
+    def test_install_invalid_addon(self):
+        addon_path = os.path.join(here, "webextension-invalid.xpi")
 
         with self.assertRaises(AddonInstallException):
             self.addons.install(addon_path)
-
-    @skip("Need to get the test extension signed")
-    def test_install_and_remove_signed_addon(self):
-        addon_path = os.path.join(here, 'mn-restartless-signed.xpi')
+        self.assertNotIn("{d3e7c1f1-2e35-4a49-89fe-9f46eb8abf0a}", self.all_addon_ids)
 
-        addon_id = self.addons.install(addon_path)
-        self.assertIn(addon_id, self.all_addon_ids)
+    def test_install_unsigned_addon_fails(self):
+        addon_path = os.path.join(here, "webextension-unsigned.xpi")
 
-        self.addons.uninstall(addon_id)
-        self.assertNotIn(addon_id, self.all_addon_ids)
+        with self.assertRaises(AddonInstallException):
+            self.addons.install(addon_path)
new file mode 100644
index 0000000000000000000000000000000000000000..bd1177462ea7ba987f2b31c3f9ff9b200edb5e90
GIT binary patch
literal 295
zc$^FHW@Zs#U|`^2h^i9z-jTV)vK+`;0mQrvG7Pzid6{Xc#U*-K#rb)mA)E}%yBp?4
z&H>`m3T_5QmamKq3}EfK-Hv>R6a-wq&q*@MHaxm$+r2cdW<@2{))MC(yBF!*{k!LT
zohXCE%aaw%Ukt^$7`dJ|wdd)iZ~htbN9sSjb^cHFtA#%{^l2`A!2316ZZ}s&`saxK
zmv7GtxBhfhWBHTL<D0K6Nw+-P_w>z|*6E#M<^j$b7hHt(1H2iT<d|`}UxEPyfKFs+
dXaup)e8URy4Vq5^yjj^G+87yvf%HDGApkgdUgrP+