552742 - Support multi-package XPIs

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P2)

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Is this somehow correlated to collections on AMO?

Comment 2

[Dave Townsend [:mossop]]

It is a way of installing multiple extensions out of a single XPI file. Some extensions on AMO use it, I'll find some once I get round to looking at this.

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

I think AMO is planning on allowing installing whole collections, which would presumably use multi-package XPIs. A few other consumers seem to be blocked on this as well.

Comment 4

[mcdavis941 (sporadically reading bugmail)]

(In reply to comment #2)
> Some extensions on AMO use it

If I'm not mistaken, this is one (em:type 32):

https://addons.mozilla.org/en-US/firefox/addon/14313/

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #3)
> I think AMO is planning on allowing installing whole collections, which would
> presumably use multi-package XPIs. A few other consumers seem to be blocked on
> this as well.

See bug 497306. Fred, is that still the active bug? Do you know something?

Comment 6

[Fred Wenzel [:wenzel]]

(In reply to comment #3)
> I think AMO is planning on allowing installing whole collections, which would
> presumably use multi-package XPIs.

Installing multiple add-ons at once and single xpis containing multiple add-ons at once are two distinct problems. The former does not require the latter.

> See bug 497306. Fred, is that still the active bug? Do you know something?

That being said, multiple add-ons installs are coming. There is bug 561226, the first comment of which links to the "Electric Bandwagon" specs, which explicitly mention multiple add-on installs. (In fact, this has long been technically possible, and I've written the Fashion Your Firefox code before which to support this, though at the time a client bug caused problems installing more than one *signed* add-on at once; that's long fixed now.)

Long story short, multiple add-ons installs are coming, yet unrelated to multi-package XPIs.

CCing fligtar who's welcome to add his 5 cents to the pile.

Comment 7

[Dave Garrett]

For anyone who wants the info:
https://developer.mozilla.org/en/Multiple_Item_Packaging

It's just an XPI containing normal XPIs/JARs and an install.rdf with:
<em:type>32</em:type>

Some themes have started using this so they can package an extension with it to alter the GUI in some way beyond what a plain theme can do or add an options dialog, etc. Others are using it to install some other extension along with their addon (sometimes a dependency or partner addon; sometimes junk to change search providers or the like). I just bumped into this because it can also be used as a slight hack to migrate users from beta->stable extension update channels on AMO. There's plenty of other uses for it as well. I think its initial use case was so an organization or someone with a lot of profiles could install a full set of addons at once with a custom built multi-XPI. The CLEO extension can generate these easily through a GUI for those who want it.

Comment 8

[Pardal Freudenthal (:ShareBird)]

Marked as regression since it used to work on previous versions.

Comment 9

[Dave Townsend [:mossop]]

Note to self: general plan will be to dispatch multiple onNewInstall notifications after download to the Ui can keep track of all the extra add-ons that come as a part of the install. An interesting choice will be which of the contained add-ons is "installed" by the original AddonInstall, but that probably doesn't matter all that much I think.

Comment 10

[Dave Townsend [:mossop]]

The plan to implement this does not involve any real API change so I think we don't need to rush it into b2.

Comment 11

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This adds support for multi-package XPIs. Basically when an AddonInstall is loading the main XPI manifest and detects it is a multi-package it extracts all the xpi and jar files from the package and then tries to find one with a valid install manifest. It uses this as the Addon for that AddonInstall instance. It then tries to create new AddonInstall instances for the other files. The one minor API change is to expose these additional instances as linkedInstalls from the main one so the install confirmation code can include them in the list.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 460746 [details] [diff] [review]
patch rev 1

Does the UI automatically work with this, or does it need some tweaking?

Comment 13

[Dave Townsend [:mossop]]

(In reply to comment #12)
> Comment on attachment 460746 [details] [diff] [review]
> patch rev 1
> 
> Does the UI automatically work with this, or does it need some tweaking?

Works automatically in my tests

Comment 14

[Robert Strong (they/them - no direct email)]

I made it part of the way through the review and should have it done Friday (today)

Comment 15

[Robert Strong (they/them - no direct email)]

Comment on attachment 460746 [details] [diff] [review]
patch rev 1

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>@@ -558,22 +565,22 @@ function loadManifestFromDir(aDir) {
>   }
>   finally {
>     bis.close();
>     fis.close();
>   }
> }
> 
> /**
>- * Loads an AddonInternal object from an add-on in a zip file.
>+ * Loads an AddonInternal object from an nsIZipReader for an add-on.
>  *
>  * @param  aZipReader
>- *         An nsIZipReader open reading from the add-on XPI file
>+ *         An nsIZipReader open reading from the add-on's XPI file
nit: could you reword the "An nsIZipReader open reading from" part as well? It doesn't seem all that well formed.

>  * @return an AddonInternal object
>- * @throws if the directory does not contain a valid install manifest
>+ * @throws if the zip reader does not contain a valid install manifest
nit:
if the XPI file does not contain a valid install manifest
or
if the zip reader cannot find a valid install manifest

>@@ -590,30 +597,60 @@ function loadManifestFromZipReader(aZipR
>   }
>   finally {
>     bis.close();
>     zis.close();
>   }
> }
> 
> /**
>+ * Loads an AddonInternal object from an add-on in an XPI file.
>+ *
>+ * @param  aZipFile
>+ *         An nsIFile pointing to the add-on's XPI file
nit: seems weird to interchange Zip and XPI here and elsewhere. Not everyone will immediately make the connection that an XPI is a Zip.

>+ * @return an AddonInternal object
>+ * @throws if the XPI file does not contain a valid install manifest
>+ */
>+function loadManifestFromZipFile(aZipFile) {
>+  let zipReader = Cc["@mozilla.org/libjar/zip-reader;1"].
>+                  createInstance(Ci.nsIZipReader);
>+  try {
>+    zipReader.open(aZipFile);
>+
>+    return loadManifestFromZipReader(zipReader);
>+  }
>+  finally {
>+    zipReader.close();
>+  }
>+}
Doesn't always return a value

>+
>+/**
>  * Creates a jar: URI for a file inside a ZIP file.
>  *
>  * @param  aJarfile
>  *         The ZIP file as an nsIFile
>  * @param  aPath
>  *         The path inside the ZIP file
>  * @return an nsIURI for the file
>  */
> function buildJarURI(aJarfile, aPath) {
>   let uri = Services.io.newFileURI(aJarfile);
>   uri = "jar:" + uri.spec + "!/" + aPath;
>   return NetUtil.newURI(uri);
> }
> 
>+function getTemporaryFile() {
Please add a function comment header

>+  let file = FileUtils.getDir(KEY_TEMPDIR, []);
>+  let random = Math.random().toString(36).replace(/0./, '').substr(-3);
>+  file.append("tmp-" + random + ".xpi");
>+  file.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE);
>+
>+  return file;
>+}
>+

Comment 16

[Robert Strong (they/them - no direct email)]

Comment on attachment 460746 [details] [diff] [review]
patch rev 1

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>...
>@@ -3864,53 +3901,53 @@ function AddonInstall(aCallback, aInstal
>         this.state = AddonManager.STATE_DOWNLOAD_FAILED;
>         this.error = AddonManager.ERROR_INCORRECT_HASH;
>         aCallback(this);
>         return;
>       }
>     }
> 
>     try {
>-      this.loadManifest();
>+      let self = this;
>+      this.loadManifest(function() {
>+        XPIDatabase.getVisibleAddonForID(self.addon.id, function(aAddon) {
>+          self.existingAddon = aAddon;
>+
>+          if (!self.addon.isCompatible) {
>+            // TODO Should we send some event here?
>+            self.state = AddonManager.STATE_CHECKING;
>+            new UpdateChecker(self.addon, {
>+              onUpdateFinished: function(aAddon) {
>+                self.state = AddonManager.STATE_DOWNLOADED;
>+                XPIProvider.installs.push(self);
>+                AddonManagerPrivate.callInstallListeners("onNewInstall",
>+                                                         self.listeners,
>+                                                         self.wrapper);
>+
>+                aCallback(self);
>+              }
>+            }, AddonManager.UPDATE_WHEN_ADDON_INSTALLED);
>+          }
>+          else {
>+            XPIProvider.installs.push(self);
>+            AddonManagerPrivate.callInstallListeners("onNewInstall", self.listeners,
>+                                                     self.wrapper);
nit: might as well wrap this to keep it under 80

>+
>+            aCallback(self);
>+          }
>+        });
>+      });
>     }
>...
>@@ -3935,19 +3972,21 @@ AddonInstall.prototype = {
> 
>   name: null,
>   type: null,
>   version: null,
>   iconURL: null,
>   releaseNotesURI: null,
>   sourceURI: null,
>   file: null,
>+  isOwnFile: false,
nit: how about ownsTempFile? If not, maybe ownsFile?

>@@ -4039,69 +4078,183 @@ AddonInstall.prototype = {
>    *         The InstallListener to remove
>    */
>   removeListener: function AI_removeListener(aListener) {
>     this.listeners = this.listeners.filter(function(i) {
>       return i != aListener;
>     });
>   },
> 
>+  removeTemporaryFile: function AI_removeTemporaryFile() {
Please add a function comment header

>+    // Only proceed if this AddonInstall owns its XPI file
>+    if (!this.isOwnFile)
>+      return;
>+
>+    try {
>+      this.file.remove(true);
>+      this.isOwnFile = false;
>+    }
>+    catch (e) {
>+      WARN("Failed to remove temporary file " + this.file.path + ": " + e);
>+    }
>+  },

>...
>+      if (files.length == 0) {
>+        throw new Error("Multi-package XPI does not contain any packages " +
>+                        "to install");
>       }
> 
>-      this.addon = loadManifestFromZipReader(zipreader);
>-      this.addon.sourceURI = this.sourceURI.spec;
>-      if (this.releaseNotesURI)
>-        this.addon.releaseNotesURI = this.releaseNotesURI.spec;
>+      let addon = null;
>+
>+      // Find the first file that has a valid install manifest and use it
Please add a little more detail especially about how it is used.

>+      while (files.length > 0) {
>+        this.removeTemporaryFile();
>+        this.file = files.shift();
>+        this.isOwnFile = true;
>+        try {
>+          addon = loadManifestFromZipFile(this.file);
>+          break;
>+        }
>+        catch (e) {
>+          WARN(this.file.leafName + " in multi-package XPI was invalid: " + e);
nit: I think 'was' should be 'is' but I think this should be 
this.file.leafName + " cannot be installed from multi-package XPI: " + e

>+        }
>+      }
>+
>+      if (!addon) {
>+        // No valid add-on was found
>+        aCallback();
>+        return;
>+      }
>+
>+      this.addon = addon;
>+
>+      updateURIs(this);
>+
>       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 objects for every subsequent file
nit: perhaps "Create an AddonInstall object for each remaining file"?

>...
>+    else {
>+      updateURIs(this);
>+
>+      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;
>+
>+      aCallback();
I'm tempted to ask that the normal use case (e.g. not multipackage) come first so that case is easier to read since the multipackage section would be visible when reading the code. I'm ok either way.

>@@ -4316,36 +4467,39 @@ AddonInstall.prototype = {
>    */
>   downloadFailed: function(aReason, aError) {
>     WARN("Download failed: " + aError);
>     this.state = AddonManager.STATE_DOWNLOAD_FAILED;
>     this.error = aReason;
>     XPIProvider.removeActiveInstall(this);
>     AddonManagerPrivate.callInstallListeners("onDownloadFailed", this.listeners,
>                                              this.wrapper);
>-    try {
>-      this.file.remove(true);
>-    }
>-    catch (e) {
>-      WARN("Failed to remove temporary file " + this.file.path + ": " + e);
>-    }
>+
nitty nit: don't think this empty line adds any value / is needed.

>+    this.removeTemporaryFile();
>   },

Still need to review the tests but when I ran the mochitest-browser-chrome an XPI was left behind in my temp dir.

Comment 17

[Robert Strong (they/them - no direct email)]

Here is the error

*** WARN addons.xpi: Failed to remove temporary file C:\Users\rstrong\AppData\Local\Temp\tmp-ade.xpi: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.remove]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame :: resource://gre/modules/XPIProvider.jsm :: AI_removeTemporaryFile :: line 4056"  data: no]

Comment 18

[Robert Strong (they/them - no direct email)]

You probably already know this but just in case the same happens with the xpcshell test

Comment 19

[Robert Strong (they/them - no direct email)]

(In reply to comment #18)
> You probably already know this but just in case the same happens with the
> xpcshell test
there are 10 xpi's left behind after running the test_install.js xpcshell test

Comment 20

[Robert Strong (they/them - no direct email)]

Comment on attachment 460746 [details] [diff] [review]
patch rev 1

> >+ * @return an AddonInternal object
> >+ * @throws if the XPI file does not contain a valid install manifest
> >+ */
> >+function loadManifestFromZipFile(aZipFile) {
> >+  let zipReader = Cc["@mozilla.org/libjar/zip-reader;1"].
> >+                  createInstance(Ci.nsIZipReader);
> >+  try {
> >+    zipReader.open(aZipFile);
> >+
> >+    return loadManifestFromZipReader(zipReader);
> >+  }
> >+  finally {
> >+    zipReader.close();
> >+  }
> >+}
> Doesn't always return a value
ignore that one

Would like to see an updated patch and a fix for the xpi's being left behind in the tmp dir before r+'ing

Comment 21

[Robert Strong (they/them - no direct email)]

Comment on attachment 460746 [details] [diff] [review]
patch rev 1

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>...
>   /**
>    * Called after the add-on is a local file and the signature and install
>    * manifest can be read.
>    *
>    * @throws if the add-on does not contain a valid install manifest or the
>    *         XPI is incorrectly signed
>    */
>-  loadManifest: function AI_loadManifest() {
>+  loadManifest: function AI_loadManifest(aCallback) {
>     let zipreader = Cc["@mozilla.org/libjar/zip-reader;1"].
>                     createInstance(Ci.nsIZipReader);
>     zipreader.open(this.file);
This can throw and the zip reader is no longer closed as it was previously in the finally block.

> 
>+    let principal = zipreader.getCertificatePrincipal(null);
>...
>+      else {
>+        aCallback();
>+      }
>     }
>-    finally {
>-      zipreader.close();
>+    else {
>+      updateURIs(this);
>+
>+      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;
>+
>+      aCallback();
>     }
With the removal of the finally block above where the zip reader is closed the non multi-item package no longer closes the zip reader.

Comment 22

[Robert Strong (they/them - no direct email)]

I think it would be a good thing to register a custom TmpD for this (probably others as well) xpcshell test and test that all xpi's have been removed.

Comment 23

[Dave Townsend [:mossop]]

> >+ * @return an AddonInternal object
> >+ * @throws if the XPI file does not contain a valid install manifest
> >+ */
> >+function loadManifestFromZipFile(aZipFile) {
> >+  let zipReader = Cc["@mozilla.org/libjar/zip-reader;1"].
> >+                  createInstance(Ci.nsIZipReader);
> >+  try {
> >+    zipReader.open(aZipFile);
> >+
> >+    return loadManifestFromZipReader(zipReader);
> >+  }
> >+  finally {
> >+    zipReader.close();
> >+  }
> >+}
> Doesn't always return a value

It will always either return the manifest or throw the exception that open/loadManifestFromZipReader generates.

Comment 24

[Dave Townsend [:mossop]]

(In reply to comment #16)
> >...
> >+    else {
> >+      updateURIs(this);
> >+
> >+      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;
> >+
> >+      aCallback();
> I'm tempted to ask that the normal use case (e.g. not multipackage) come first
> so that case is easier to read since the multipackage section would be visible
> when reading the code. I'm ok either way.

I've broken the multipackage handling bits out into a separate function, I think it makes more sense that will.

Comment 25

[Dave Townsend [:mossop]]

(In reply to comment #21)
> Comment on attachment 460746 [details] [diff] [review]
> patch rev 1
> 
> >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
> >--- a/toolkit/mozapps/extensions/XPIProvider.jsm
> >+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
> >...
> >   /**
> >    * Called after the add-on is a local file and the signature and install
> >    * manifest can be read.
> >    *
> >    * @throws if the add-on does not contain a valid install manifest or the
> >    *         XPI is incorrectly signed
> >    */
> >-  loadManifest: function AI_loadManifest() {
> >+  loadManifest: function AI_loadManifest(aCallback) {
> >     let zipreader = Cc["@mozilla.org/libjar/zip-reader;1"].
> >                     createInstance(Ci.nsIZipReader);
> >     zipreader.open(this.file);
> This can throw and the zip reader is no longer closed as it was previously in
> the finally block.
> 
> > 
> >+    let principal = zipreader.getCertificatePrincipal(null);
> >...
> >+      else {
> >+        aCallback();
> >+      }
> >     }
> >-    finally {
> >-      zipreader.close();
> >+    else {
> >+      updateURIs(this);
> >+
> >+      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;
> >+
> >+      aCallback();
> >     }
> With the removal of the finally block above where the zip reader is closed the
> non multi-item package no longer closes the zip reader.

Good catches, missed it because OSX doesn't have a problem with deleting files in use. I've fixed those and added the custom temp dir to the tests to verify nothing gets left behind.

Comment 26

[Dave Townsend [:mossop]]

(In reply to comment #25)
> (In reply to comment #21)
> > Comment on attachment 460746 [details] [diff] [review] [details]
> > patch rev 1
> > 
> > >diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
> > >--- a/toolkit/mozapps/extensions/XPIProvider.jsm
> > >+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
> > >...
> > >   /**
> > >    * Called after the add-on is a local file and the signature and install
> > >    * manifest can be read.
> > >    *
> > >    * @throws if the add-on does not contain a valid install manifest or the
> > >    *         XPI is incorrectly signed
> > >    */
> > >-  loadManifest: function AI_loadManifest() {
> > >+  loadManifest: function AI_loadManifest(aCallback) {
> > >     let zipreader = Cc["@mozilla.org/libjar/zip-reader;1"].
> > >                     createInstance(Ci.nsIZipReader);
> > >     zipreader.open(this.file);
> > This can throw and the zip reader is no longer closed as it was previously in
> > the finally block.

Actually this is kind of strange. I had expected that if zipreader.open threw then it would have closed the file and dropped any locks, but it seems that it doesn't. That sounds like a bug in zipreader. Workaroundable but maybe I'll file something on it anyway.

Comment 27

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Addresses the comments and passes all tests on try server

Comment 28

[Robert Strong (they/them - no direct email)]

(In reply to comment #23)
>...
> > Doesn't always return a value
> 
> It will always either return the manifest or throw the exception that
> open/loadManifestFromZipReader generates.
Yep...
(In reply to comment #20)
>...
> > Doesn't always return a value
> ignore that one

(In reply to comment #26)
>...
> > > This can throw and the zip reader is no longer closed as it was previously in
> > > the finally block.
> 
> Actually this is kind of strange. I had expected that if zipreader.open threw
> then it would have closed the file and dropped any locks, but it seems that it
> doesn't. That sounds like a bug in zipreader. Workaroundable but maybe I'll
> file something on it anyway.
Agreed... I expected it to drop the locks as well. Please file a new bug for that.

Reviewing the patch now so it can make the next beta

Comment 29

[Robert Strong (they/them - no direct email)]

Comment on attachment 461827 [details] [diff] [review]
patch rev 2

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>@@ -558,22 +565,22 @@ function loadManifestFromDir(aDir) {
>   }
>   finally {
>     bis.close();
>     fis.close();
>   }
> }
> 
> /**
>- * Loads an AddonInternal object from an add-on in a zip file.
>+ * Loads an AddonInternal object from an nsIZipReader for an add-on.
>  *
>  * @param  aZipReader
>- *         An nsIZipReader open reading from the add-on XPI file
>+ *         An nsIZipReader open with the add-on's XPI file
previous nit: could you reword the "An nsIZipReader open reading from" part as well? It doesn't seem all that well formed.

>@@ -590,31 +597,65 @@ function loadManifestFromZipReader(aZipR
>...

> /**
>+ * Creates a unique temporary file. The caller should delete the file when
>+ * it is no longer needed.
s/Creates/Returns/

and add a @return

>+ */
>+function getTemporaryFile() {
>+  let file = FileUtils.getDir(KEY_TEMPDIR, []);
>+  let random = Math.random().toString(36).replace(/0./, '').substr(-3);
>+  file.append("tmp-" + random + ".xpi");
>+  file.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE);
>+
>+  return file;
>+}

>@@ -4040,69 +4084,205 @@ AddonInstall.prototype = {
>...
>+  loadMultipackageManifests: function AI_loadMultipackageManifests(aZipReader,
Please add a function comment headr

>+  /**
>    * Called after the add-on is a local file and the signature and install
>    * manifest can be read.
>    *
>    * @throws if the add-on does not contain a valid install manifest or the
>    *         XPI is incorrectly signed
>    */
>-  loadManifest: function AI_loadManifest() {
>+  loadManifest: function AI_loadManifest(aCallback) {
Please add a @param for aCallback to the function comment header

Comment 30

[Robert Strong (they/them - no direct email)]

Comment on attachment 461827 [details] [diff] [review]
patch rev 2

r=me with comments addressed.

Comment 31

[Robert Strong (they/them - no direct email)]

(In reply to comment #29)
>...
> >@@ -590,31 +597,65 @@ function loadManifestFromZipReader(aZipR
> >...
> 
> > /**
> >+ * Creates a unique temporary file. The caller should delete the file when
> >+ * it is no longer needed.
> s/Creates/Returns/
or something similar

Comment 32

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/ff1ecd74f827

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #2)
> It is a way of installing multiple extensions out of a single XPI file. Some
> extensions on AMO use it, I'll find some once I get round to looking at this.

Dave, have you had a chance to check for an example we could use for our litmus test and manual verification?

Comment 34

[Dave Townsend [:mossop]]

(In reply to comment #33)
> (In reply to comment #2)
> > It is a way of installing multiple extensions out of a single XPI file. Some
> > extensions on AMO use it, I'll find some once I get round to looking at this.
> 
> Dave, have you had a chance to check for an example we could use for our litmus
> test and manual verification?

The Charamel and Silvermel themes do it.

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks Dave. Verified fixed with builds on OS X and Windows like Mozilla/5.0 (Windows NT 5.1; rv:2.0b5pre) Gecko/20100826 Minefield/4.0b5pre

For the Litmus and Mozmill test I would like to create a simplified multi-package XPI which will hopefully be compatible all the time and doesn't mess-up the UI like the themes above.

Comment 36

[Pardal Freudenthal (:ShareBird)]

(In reply to comment #35)
> For the Litmus and Mozmill test I would like to create a simplified
> multi-package XPI which will hopefully be compatible all the time and doesn't
> mess-up the UI like the themes above.

Hehe, I wouldn't expect something other since those themes are not updated to work with the current nightlies.
Anyway, I've wrote a little theme using the "light-weight" approach described here: http://www.tudobom.de/articles/yatt/#light_weight
This theme is supposed to don't ruin the UI, even on future versions from Firefox, since it doesn't register skin providers for the existent packages.
I've bundled it with the Switch Themes extension, but you can substitute it to another extension from your choice.
www.silvermel.net/dev/xpi/firebird_bundle_1.0.0pre_src_r39.xpi

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Wow, thanks for that XPI! I will definitely check it soon. If it fits our needs I will use it for our tests.

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Manual test is covered by:
https://litmus.mozilla.org/show_test.cgi?id=15221

Pardal, I have checked-in this extension into our litmus-data repository, which is shadowed to mozqa.com. So if you want you can remove the file from your server. Thanks!

http://hg.mozilla.org/qa/litmus-data/rev/94df20eb5ab8
http://www.mozqa.com/data/firefox/addons/multipackage/