Bug 1177130: Multipackage XPIs with no valid XPIs should appear as failed downloads. r=rhelmer

toolkit/mozapps/extensions/amWebInstallListener.js

@@ -119,8 +119,8 @@ Installer.prototype = {
         if (install.linkedInstalls) {
           install.linkedInstalls.forEach(function(aInstall) {
             aInstall.addListener(this);
-            // App disabled items are not compatible and so fail to install
-            if (aInstall.addon.appDisabled)
+            // Corrupt or incompatible items fail to install
+            if (aInstall.state == AddonManager.STATE_DOWNLOAD_FAILED || aInstall.addon.appDisabled)
               failed.push(aInstall);
             else
               installs.push(aInstall);
@@ -132,7 +132,7 @@ Installer.prototype = {
         break;
       default:
         logger.warn("Download of " + install.sourceURI.spec + " in unexpected state " +
-             install.state);
+                    install.state);
       }
     }
 

toolkit/mozapps/extensions/internal/XPIProvider.jsm

@@ -5126,7 +5126,7 @@ AddonInstall.prototype = {
     }
 
     let self = this;
-    this.loadManifest().then(() => {
+    this.loadManifest(this.file).then(() => {
       XPIDatabase.getVisibleAddonForID(self.addon.id, function initLocalInstall_getVisibleAddon(aAddon) {
         self.existingAddon = aAddon;
         if (aAddon)
@@ -5162,6 +5162,10 @@ AddonInstall.prototype = {
       logger.warn("Invalid XPI", message);
       this.state = AddonManager.STATE_DOWNLOAD_FAILED;
       this.error = error;
+      AddonManagerPrivate.callInstallListeners("onNewInstall",
+                                               self.listeners,
+                                               self.wrapper);
+
       aCallback(this);
     });
   },
@@ -5337,6 +5341,45 @@ AddonInstall.prototype = {
       this.addon.releaseNotesURI = this.releaseNotesURI.spec;
   },
 
+  /**
+   * Fills out linkedInstalls with AddonInstall instances for the other files
+   * in a multi-package XPI.
+   *
+   * @param  aFiles
+   *         An array of { entryName, file } for each remaining file from the
+   *         multi-package XPI.
+   */
+  _createLinkedInstalls: Task.async(function* AI_createLinkedInstalls(aFiles) {
+    if (aFiles.length == 0)
+      return;
+
+    // Create new AddonInstall instances for every remaining file
+    if (!this.linkedInstalls)
+      this.linkedInstalls = [];
+
+    for (let { entryName, file } of aFiles) {
+      logger.debug("Creating linked install from " + entryName);
+      let install = yield new Promise(resolve => AddonInstall.createInstall(resolve, file));
+
+      // Make the new install own its temporary file
+      install.ownsTempFile = true;
+
+      this.linkedInstalls.push(install);
+
+      // If one of the internal XPIs was multipackage then move its linked
+      // installs to the outer install
+      if (install.linkedInstalls) {
+        this.linkedInstalls.push(...install.linkedInstalls);
+        install.linkedInstalls = null;
+      }
+
+      install.sourceURI = this.sourceURI;
+      install.releaseNotesURI = this.releaseNotesURI;
+      if (install.state != AddonManager.STATE_DOWNLOAD_FAILED)
+        install.updateAddonURIs();
+    }
+  }),
+
   /**
    * Loads add-on manifests from a multi-package XPI file. Each of the
    * XPI and JAR files contained in the XPI will be extracted. Any that
@@ -5347,97 +5390,58 @@ AddonInstall.prototype = {
    * @param  aZipReader
    *         An open nsIZipReader for the multi-package XPI's files. This will
    *         be closed before this method returns.
-   * @param  aCallback
-   *         A function to call when all of the add-on manifests have been
-   *         loaded. Because this loadMultipackageManifests is an internal API
-   *         we don't exception-wrap this callback
    */
   _loadMultipackageManifests: Task.async(function* AI_loadMultipackageManifests(aZipReader) {
     let files = [];
     let entries = aZipReader.findEntries("(*.[Xx][Pp][Ii]|*.[Jj][Aa][Rr])");
     while (entries.hasMore()) {
       let entryName = entries.getNext();
-      var target = getTemporaryFile();
+      let file = getTemporaryFile();
       try {
-        aZipReader.extract(entryName, target);
-        files.push(target);
+        aZipReader.extract(entryName, file);
+        files.push({ entryName, file });
       }
       catch (e) {
         logger.warn("Failed to extract " + entryName + " from multi-package " +
              "XPI", e);
-        target.remove(false);
+        file.remove(false);
       }
     }
 
     aZipReader.close();
 
     if (files.length == 0) {
-      throw new Error("Multi-package XPI does not contain any packages " +
-                      "to install");
+      return Promise.reject([AddonManager.ERROR_CORRUPT_FILE,
+                             "Multi-package XPI does not contain any packages to install"]);
     }
 
     let addon = null;
 
-    // Find the first file that has a valid install manifest and use it for
+    // Find the first file that is a valid install and use it for
     // the add-on that this AddonInstall instance will install.
-    while (files.length > 0) {
+    for (let { entryName, file } of files) {
       this.removeTemporaryFile();
-      this.file = files.shift();
-      this.ownsTempFile = true;
       try {
-        addon = yield loadManifestFromZipFile(this.file);
-        break;
+        yield this.loadManifest(file);
+        logger.debug("Base multi-package XPI install came from " + entryName);
+        this.file = file;
+        this.ownsTempFile = true;
+
+        yield this._createLinkedInstalls(files.filter(f => f.file != file));
+        return;
       }
       catch (e) {
-        logger.warn(this.file.leafName + " cannot be installed from multi-package " +
-             "XPI", e);
+        // _createLinkedInstalls will log errors when it tries to process this
+        // file
       }
     }
 
-    if (!addon) {
-      // No valid add-on was found
-      return;
-    }
+    // No valid add-on was found, delete all the temporary files
+    for (let { file } of files)
+      file.remove(true);
 
-    this.addon = addon;
-
-    this.updateAddonURIs();
-
-    this.addon._install = this;
-    this.name = this.addon.selectedLocale.name;
-    this.type = this.addon.type;
-    this.version = this.addon.version;
-
-    // Setting the iconURL to something inside the XPI locks the XPI and
-    // makes it impossible to delete on Windows.
-    //let newIcon = createWrapper(this.addon).iconURL;
-    //if (newIcon)
-    //  this.iconURL = newIcon;
-
-    // Create new AddonInstall instances for every remaining file
-    if (files.length > 0) {
-      this.linkedInstalls = [];
-      let self = this;
-      for (let file of files) {
-        let install = yield new Promise(resolve => AddonInstall.createInstall(resolve, file));
-
-        // Ignore bad add-ons (createInstall will have logged the error)
-        if (install.state == AddonManager.STATE_DOWNLOAD_FAILED) {
-          // Manually remove the temporary file
-          file.remove(true);
-        }
-        else {
-          // Make the new install own its temporary file
-          install.ownsTempFile = true;
-
-          self.linkedInstalls.push(install)
-
-          install.sourceURI = self.sourceURI;
-          install.releaseNotesURI = self.releaseNotesURI;
-          install.updateAddonURIs();
-        }
-      }
-    }
+    return Promise.reject([AddonManager.ERROR_CORRUPT_FILE,
+                           "Multi-package XPI does not contain any valid packages to install"]);
   }),
 
   /**
@@ -5449,11 +5453,11 @@ AddonInstall.prototype = {
    * @throws if the add-on does not contain a valid install manifest or the
    *         XPI is incorrectly signed
    */
-  loadManifest: Task.async(function* AI_loadManifest() {
+  loadManifest: Task.async(function* AI_loadManifest(file) {
     let zipreader = Cc["@mozilla.org/libjar/zip-reader;1"].
                     createInstance(Ci.nsIZipReader);
     try {
-      zipreader.open(this.file);
+      zipreader.open(file);
     }
     catch (e) {
       zipreader.close();
@@ -5771,7 +5775,7 @@ AddonInstall.prototype = {
         }
 
         let self = this;
-        this.loadManifest().then(() => {
+        this.loadManifest(this.file).then(() => {
           if (self.addon.isCompatible) {
             self.downloadCompleted();
           }
@@ -5860,9 +5864,10 @@ AddonInstall.prototype = {
         self.install();
 
         if (self.linkedInstalls) {
-          self.linkedInstalls.forEach(function(aInstall) {
-            aInstall.install();
-          });
+          for (let install of self.linkedInstalls) {
+            if (install.state == AddonManager.STATE_DOWNLOADED)
+              install.install();
+          }
         }
       }
     });

toolkit/mozapps/extensions/test/addons/test_install7/addon1.xpi

@@ -0,0 +1 @@
+This isn't a valid zip file.

toolkit/mozapps/extensions/test/addons/test_install7/addon2.xpi

@@ -0,0 +1 @@
+This isn't a valid zip file.

toolkit/mozapps/extensions/test/addons/test_install7/install.rdf

@@ -0,0 +1,10 @@
+<?xml version="1.0"?>
+
+<!-- A multi-package XPI -->
+<RDF xmlns="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
+     xmlns:em="http://www.mozilla.org/2004/em-rdf#">
+
+  <Description about="urn:mozilla:install-manifest">
+    <em:type>32</em:type>
+  </Description>
+</RDF>

toolkit/mozapps/extensions/test/addons/test_install8/install.rdf

@@ -0,0 +1,10 @@
+<?xml version="1.0"?>
+
+<!-- A multi-package XPI -->
+<RDF xmlns="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
+     xmlns:em="http://www.mozilla.org/2004/em-rdf#">
+
+  <Description about="urn:mozilla:install-manifest">
+    <em:type>32</em:type>
+  </Description>
+</RDF>

toolkit/mozapps/extensions/test/xpcshell/head_addons.js

@@ -1108,9 +1108,13 @@ const AddonListener = {
 const InstallListener = {
   onNewInstall: function(install) {
     if (install.state != AddonManager.STATE_DOWNLOADED &&
+        install.state != AddonManager.STATE_DOWNLOAD_FAILED &&
         install.state != AddonManager.STATE_AVAILABLE)
       do_throw("Bad install state " + install.state);
-    do_check_eq(install.error, 0);
+    if (install.state != AddonManager.STATE_DOWNLOAD_FAILED)
+      do_check_eq(install.error, 0);
+    else
+      do_check_neq(install.error, 0);
     do_check_eq("onNewInstall", getExpectedInstall());
     return check_test_completed(arguments);
   },

toolkit/mozapps/extensions/test/xpcshell/test_install.js

@@ -647,6 +647,8 @@ function check_test_10(install) {
 // correct sourceURI.
 function run_test_11() {
   prepare_test({ }, [
+    "onNewInstall",
+    "onNewInstall",
     "onNewInstall",
     "onNewInstall",
     "onNewInstall",
@@ -657,11 +659,22 @@ function run_test_11() {
     ensure_test_completed();
     do_check_neq(install, null);
     do_check_neq(install.linkedInstalls, null);
-    do_check_eq(install.linkedInstalls.length, 3);
+    do_check_eq(install.linkedInstalls.length, 5);
 
     // Might be in any order so sort them based on ID
     let installs = [install].concat(install.linkedInstalls);
     installs.sort(function(a, b) {
+      if (a.state != b.state) {
+        if (a.state == AddonManager.STATE_DOWNLOAD_FAILED)
+          return 1;
+        else if (b.state == AddonManager.STATE_DOWNLOAD_FAILED)
+          return -1;
+      }
+
+      // Don't care what order the failed installs show up in
+      if (a.state == AddonManager.STATE_DOWNLOAD_FAILED)
+        return 0;
+
       if (a.addon.id < b.addon.id)
         return -1;
       if (a.addon.id > b.addon.id)
@@ -709,6 +722,12 @@ function run_test_11() {
     do_check_true(hasFlag(installs[3].addon.operationsRequiringRestart,
                           AddonManager.OP_NEEDS_RESTART_INSTALL));
 
+    do_check_eq(installs[4].state, AddonManager.STATE_DOWNLOAD_FAILED);
+    do_check_eq(installs[4].error, AddonManager.ERROR_CORRUPT_FILE);
+
+    do_check_eq(installs[5].state, AddonManager.STATE_DOWNLOAD_FAILED);
+    do_check_eq(installs[5].error, AddonManager.ERROR_CORRUPT_FILE);
+
     AddonManager.getAllInstalls(function(aInstalls) {
       do_check_eq(aInstalls.length, 4);
 
@@ -815,6 +834,8 @@ function run_test_12() {
         "onNewInstall",
         "onNewInstall",
         "onNewInstall",
+        "onNewInstall",
+        "onNewInstall",
         "onDownloadEnded"
       ],
       "addon4@tests.mozilla.org": [
@@ -839,11 +860,22 @@ function run_test_12() {
 }
 
 function check_test_12() {
-  do_check_eq(gInstall.linkedInstalls.length, 3);
+  do_check_eq(gInstall.linkedInstalls.length, 5);
 
   // Might be in any order so sort them based on ID
   let installs = [gInstall].concat(gInstall.linkedInstalls);
   installs.sort(function(a, b) {
+    if (a.state != b.state) {
+      if (a.state == AddonManager.STATE_DOWNLOAD_FAILED)
+        return 1;
+      else if (b.state == AddonManager.STATE_DOWNLOAD_FAILED)
+        return -1;
+    }
+
+    // Don't care what order the failed installs show up in
+    if (a.state == AddonManager.STATE_DOWNLOAD_FAILED)
+      return 0;
+
     if (a.addon.id < b.addon.id)
       return -1;
     if (a.addon.id > b.addon.id)
@@ -883,6 +915,12 @@ function check_test_12() {
   do_check_eq(installs[3].name, "Multi Test 4");
   do_check_eq(installs[3].state, AddonManager.STATE_INSTALLED);
 
+  do_check_eq(installs[4].state, AddonManager.STATE_DOWNLOAD_FAILED);
+  do_check_eq(installs[4].error, AddonManager.ERROR_CORRUPT_FILE);
+
+  do_check_eq(installs[5].state, AddonManager.STATE_DOWNLOAD_FAILED);
+  do_check_eq(installs[5].error, AddonManager.ERROR_CORRUPT_FILE);
+
   restartManager();
 
   AddonManager.getAddonsByIDs(["addon4@tests.mozilla.org",
@@ -1755,7 +1793,45 @@ function check_test_29(install) {
 
   prepare_test({}, [
     "onDownloadCancelled"
-  ], do_test_finished);
+  ], run_test_30);
   install.cancel();
   return false;
 }
+
+// Tests that a multi-package XPI with no add-ons inside shows up as a
+// corrupt file
+function run_test_30() {
+  prepare_test({ }, [
+    "onNewInstall"
+  ]);
+
+  AddonManager.getInstallForFile(do_get_addon("test_install7"), function(install) {
+    ensure_test_completed();
+
+    do_check_neq(install, null);
+    do_check_eq(install.state, AddonManager.STATE_DOWNLOAD_FAILED);
+    do_check_eq(install.error, AddonManager.ERROR_CORRUPT_FILE);
+    do_check_eq(install.linkedInstalls, null);
+
+    run_test_31();
+  });
+}
+
+// Tests that a multi-package XPI with no valid add-ons inside shows up as a
+// corrupt file
+function run_test_31() {
+  prepare_test({ }, [
+    "onNewInstall"
+  ]);
+
+  AddonManager.getInstallForFile(do_get_addon("test_install8"), function(install) {
+    ensure_test_completed();
+
+    do_check_neq(install, null);
+    do_check_eq(install.state, AddonManager.STATE_DOWNLOAD_FAILED);
+    do_check_eq(install.error, AddonManager.ERROR_CORRUPT_FILE);
+    do_check_eq(install.linkedInstalls, null);
+
+    end_test();
+  });
+}

toolkit/mozapps/extensions/test/xpcshell/test_install_strictcompat.js

@@ -636,6 +636,8 @@ function check_test_10(install) {
 // correct sourceURI.
 function run_test_11() {
   prepare_test({ }, [
+    "onNewInstall",
+    "onNewInstall",
     "onNewInstall",
     "onNewInstall",
     "onNewInstall",
@@ -646,11 +648,22 @@ function run_test_11() {
     ensure_test_completed();
     do_check_neq(install, null);
     do_check_neq(install.linkedInstalls, null);
-    do_check_eq(install.linkedInstalls.length, 3);
+    do_check_eq(install.linkedInstalls.length, 5);
 
     // Might be in any order so sort them based on ID
     let installs = [install].concat(install.linkedInstalls);
     installs.sort(function(a, b) {
+      if (a.state != b.state) {
+        if (a.state == AddonManager.STATE_DOWNLOAD_FAILED)
+          return 1;
+        else if (b.state == AddonManager.STATE_DOWNLOAD_FAILED)
+          return -1;
+      }
+
+      // Don't care what order the failed installs show up in
+      if (a.state == AddonManager.STATE_DOWNLOAD_FAILED)
+        return 0;
+
       if (a.addon.id < b.addon.id)
         return -1;
       if (a.addon.id > b.addon.id)
@@ -698,6 +711,12 @@ function run_test_11() {
     do_check_true(hasFlag(installs[3].addon.operationsRequiringRestart,
                           AddonManager.OP_NEEDS_RESTART_INSTALL));
 
+    do_check_eq(installs[4].state, AddonManager.STATE_DOWNLOAD_FAILED);
+    do_check_eq(installs[4].error, AddonManager.ERROR_CORRUPT_FILE);
+
+    do_check_eq(installs[5].state, AddonManager.STATE_DOWNLOAD_FAILED);
+    do_check_eq(installs[5].error, AddonManager.ERROR_CORRUPT_FILE);
+
     AddonManager.getAllInstalls(function(aInstalls) {
       do_check_eq(aInstalls.length, 4);
 
@@ -806,6 +825,8 @@ function run_test_12() {
         "onNewInstall",
         "onNewInstall",
         "onNewInstall",
+        "onNewInstall",
+        "onNewInstall",
         "onDownloadEnded"
       ],
       "addon4@tests.mozilla.org": [
@@ -830,11 +851,22 @@ function run_test_12() {
 }
 
 function check_test_12() {
-  do_check_eq(gInstall.linkedInstalls.length, 3);
+  do_check_eq(gInstall.linkedInstalls.length, 5);
 
   // Might be in any order so sort them based on ID
   let installs = [gInstall].concat(gInstall.linkedInstalls);
   installs.sort(function(a, b) {
+    if (a.state != b.state) {
+      if (a.state == AddonManager.STATE_DOWNLOAD_FAILED)
+        return 1;
+      else if (b.state == AddonManager.STATE_DOWNLOAD_FAILED)
+        return -1;
+    }
+
+    // Don't care what order the failed installs show up in
+    if (a.state == AddonManager.STATE_DOWNLOAD_FAILED)
+      return 0;
+
     if (a.addon.id < b.addon.id)
       return -1;
     if (a.addon.id > b.addon.id)
@@ -874,6 +906,12 @@ function check_test_12() {
   do_check_eq(installs[3].name, "Multi Test 4");
   do_check_eq(installs[3].state, AddonManager.STATE_INSTALLED);
 
+  do_check_eq(installs[4].state, AddonManager.STATE_DOWNLOAD_FAILED);
+  do_check_eq(installs[4].error, AddonManager.ERROR_CORRUPT_FILE);
+
+  do_check_eq(installs[5].state, AddonManager.STATE_DOWNLOAD_FAILED);
+  do_check_eq(installs[5].error, AddonManager.ERROR_CORRUPT_FILE);
+
   restartManager();
 
   AddonManager.getAddonsByIDs(["addon4@tests.mozilla.org",
@@ -1650,5 +1688,43 @@ function finish_test_27(aInstall) {
 
   ensure_test_completed();
 
-  end_test();
+  run_test_30();
+}
+
+// Tests that a multi-package XPI with no add-ons inside shows up as a
+// corrupt file
+function run_test_30() {
+  prepare_test({ }, [
+    "onNewInstall"
+  ]);
+
+  AddonManager.getInstallForFile(do_get_addon("test_install7"), function(install) {
+    ensure_test_completed();
+
+    do_check_neq(install, null);
+    do_check_eq(install.state, AddonManager.STATE_DOWNLOAD_FAILED);
+    do_check_eq(install.error, AddonManager.ERROR_CORRUPT_FILE);
+    do_check_eq(install.linkedInstalls, null);
+
+    run_test_31();
+  });
+}
+
+// Tests that a multi-package XPI with no valid add-ons inside shows up as a
+// corrupt file
+function run_test_31() {
+  prepare_test({ }, [
+    "onNewInstall"
+  ]);
+
+  AddonManager.getInstallForFile(do_get_addon("test_install8"), function(install) {
+    ensure_test_completed();
+
+    do_check_neq(install, null);
+    do_check_eq(install.state, AddonManager.STATE_DOWNLOAD_FAILED);
+    do_check_eq(install.error, AddonManager.ERROR_CORRUPT_FILE);
+    do_check_eq(install.linkedInstalls, null);
+
+    end_test();
+  });
 }

toolkit/mozapps/extensions/test/xpcshell/test_signed_multi.js

@@ -0,0 +1,55 @@
+// Enable signature checks for these tests
+Services.prefs.setBoolPref(PREF_XPI_SIGNATURES_REQUIRED, true);
+// Disable update security
+Services.prefs.setBoolPref(PREF_EM_CHECK_UPDATE_SECURITY, false);
+
+const DATA = "data/signing_checks/";
+
+// Each multi-package XPI contains one valid theme and one other add-on that
+// has the following error state:
+const ADDONS = {
+  "multi_signed.xpi": 0,
+  "multi_badid.xpi": AddonManager.ERROR_CORRUPT_FILE,
+  "multi_broken.xpi": AddonManager.ERROR_CORRUPT_FILE,
+  "multi_unsigned.xpi": AddonManager.ERROR_SIGNEDSTATE_REQUIRED,
+};
+
+function createInstall(filename) {
+  return new Promise(resolve => {
+    AddonManager.getInstallForFile(do_get_file(DATA + filename), resolve, "application/x-xpinstall");
+  });
+}
+
+function run_test() {
+  createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "4", "4");
+  startupManager();
+
+  run_next_test();
+}
+
+function* test_addon(filename) {
+  do_print("Testing " + filename);
+
+  let install = yield createInstall(filename);
+  do_check_eq(install.state, AddonManager.STATE_DOWNLOADED);
+  do_check_eq(install.error, 0);
+
+  do_check_neq(install.linkedInstalls, null);
+  do_check_eq(install.linkedInstalls.length, 1);
+
+  let linked = install.linkedInstalls[0];
+  do_print(linked.state);
+  do_check_eq(linked.error, ADDONS[filename]);
+  if (linked.error == 0) {
+    do_check_eq(linked.state, AddonManager.STATE_DOWNLOADED);
+    linked.cancel();
+  }
+  else {
+    do_check_eq(linked.state, AddonManager.STATE_DOWNLOAD_FAILED);
+  }
+
+  install.cancel();
+}
+
+for (let filename of Object.keys(ADDONS))
+  add_task(test_addon.bind(null, filename));

toolkit/mozapps/extensions/test/xpcshell/xpcshell-shared.ini

@@ -242,6 +242,7 @@ fail-if = buildapp == "mulet" || os == "android"
 [test_signed_install.js]
 run-sequentially = Uses hardcoded ports in xpi files.
 [test_signed_migrate.js]
+[test_signed_multi.js]
 [test_startup.js]
 # Bug 676992: test consistently fails on Android
 fail-if = os == "android"