693899 - Support detecting binary components, and enable strict compatibility checking when found

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

We're making addons compatible by default, but binary components have much stricter compatibility requirements. So addons with binary components can't be compatible by default - which requires detecting binary components, and enabling strict compatibility checks when any are found.

Detecting based on found filenames in the package seems error-prone. They can be in any directory (they don't have to be in a /components/ subdir these days), and not all native libraries are guaranteed to be XPCOM components.

A better solution is to parse chrome.manifest, which is how binary components are registered these days. Will need to also parse sub-manifests.

Should be able to get rid of some additional false-positives by also parsing the OS/ABI/app/version flags, since addons might use js-ctypes but fallback to binary components for older platform versions (for example). That may be followup fodder.

Comment 1

[Dave Townsend [:mossop]]

(In reply to Blair McBride (:Unfocused) from comment #0)
> Should be able to get rid of some additional false-positives by also parsing
> the OS/ABI/app/version flags, since addons might use js-ctypes but fallback
> to binary components for older platform versions (for example). That may be
> followup fodder.

To a point I care less about these cases. I guess it depends how much extra work it looks like involving

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

Comment 3

[Dave Townsend [:mossop]]

Comment on attachment 569002 [details] [diff] [review]
Patch v1

Review of attachment 569002 [details] [diff] [review]:
-----------------------------------------------------------------

Tests look good, would like some follow-up on these comments before reviewing the implementation much more.

::: toolkit/mozapps/extensions/ChromeManifestParser.jsm
@@ +72,5 @@
> +    return NetUtil.newURI(resource);
> +  }
> +
> +  return buildJarURI(aFile, aPath);
> +}

Seems to be unused

@@ +87,5 @@
> +function buildJarURI(aJarfile, aPath) {
> +  let uri = Services.io.newFileURI(aJarfile);
> +  uri = "jar:" + uri.spec + "!/" + aPath;
> +  return NetUtil.newURI(uri);
> +}

Seems to be unused

@@ +109,5 @@
> +   * @return Array of objects describing each manifest instruction, in the form:
> +   *         { type: instruction-type, args: [arguments] }
> +   * @thorws If not passed a nsILocalFile or nsIZipReader.
> +   **/
> +  parseSync: function CMP_parseSync(aContainer, aFilename) {

Not a big fan of this API. How about just passing in a URI then just creating a channel from it then opening an input stream from that channel rather than needing the two code paths? You can then just resolve a new URI against that for recursive manifests.

@@ +206,5 @@
> +  
> +  _parseContents: function CMP_parseContents(aContents, aContextPath, aRecursiveCallback) {
> +    function parseLine(aLine) {
> +      let line = aLine.trim();
> +      if (line.length == 0 || line.charAt(0) == '#')

Add some tests for comment handling

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v2

Using URIs. Bit of a pain to munge the URIs into submission, since the nsIURI is useless when it comes to file: and jar: URIs. But it does mean it can load manifests from jars within jars, which the old approach couldn't. Still need to add a test for that.

Unfortunately, there's a bug I can't figure out (and spent far too long today trying to), hoping you'll have some insight. When running test_migrate1.js and test_migrate3.js, I get NS_ERROR_FILE_ACCESS_DENIED when it tries to remove the .xpi files from the xpi-stage directory. It's definitely something to do with the channel/inputstream - it happens only if channel.open() is called (although it throws, since the file doesn't exist in that xpi). Any ideas?

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

I can't seem to find a solution to the open file handle - spent most of today reading through & debugging modules/libjar/, but didn't come up with anything useful. Going to try a hybrid approach tomorrow, to keep using URIs but do the file IO manually. If that ends up being too much work (or too fragile), I'll have to go back to the first approach.

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 569338 [details] [diff] [review]
Patch v2

Review of attachment 569338 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/ChromeManifestParser.jsm
@@ +69,5 @@
> +
> +    try {
> +      var channel = Services.io.newChannelFromURI(aURI);
> +      var iStream = channel.open();
> +      iStream.close();

Uhh really?

@@ +105,5 @@
> +          stream.close();
> +      } catch(e) {
> +        // Do nothing. close() will throw if init() didn't get called.
> +      }
> +    }

Any reason not to use this version?

@@ +132,5 @@
> +      var result = "jar:" + aBaseURI + "!/" + aPartialURI.substr(4);
> +    } else {
> +      var result = aBaseURI + aPartialURI;
> +    }
> +    return NetUtil.newURI(result);

Any reason not to just do NetUtil.newURI(aPartialURI, null, aBaseURI) ?

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Dave Townsend (:Mossop) from comment #6)
> > +    try {
> > +      var channel = Services.io.newChannelFromURI(aURI);
> > +      var iStream = channel.open();
> > +      iStream.close();
> 
> Uhh really?

Heh, ignore that - was testing. 

> Any reason not to use this version?

The commented out code reads it as a string representation of binary data. The non-commented code is friendlier to non-ASCII text, as it makes sure it's UTF-8. Think I need to rip out that stream code anyway, thanks to this file access bug :\

> Any reason not to just do NetUtil.newURI(aPartialURI, null, aBaseURI) ?

Because I thought it that didn't work for file: or jar: URIs... testing again today suggests it does *sigh*

Comment 8

[Dave Townsend [:mossop]]

(In reply to Blair McBride (:Unfocused) from comment #7)
> (In reply to Dave Townsend (:Mossop) from comment #6)
> > > +    try {
> > > +      var channel = Services.io.newChannelFromURI(aURI);
> > > +      var iStream = channel.open();
> > > +      iStream.close();
> > 
> > Uhh really?
> 
> Heh, ignore that - was testing. 
> 
> > Any reason not to use this version?
> 
> The commented out code reads it as a string representation of binary data.
> The non-commented code is friendlier to non-ASCII text, as it makes sure
> it's UTF-8. Think I need to rip out that stream code anyway, thanks to this
> file access bug :\

The actual manifest parsing code just reads the data as a stream of chars, no character decoding at all. I think we want the same here.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Quoting from bug 533038:
>> I'm not sure I understand the jars-within-jars bit. We already support nested
>> JARs using URIs:
>> 
>> jar:jar:some.jar!/other.jar!/inner.path
>> 
>> Why can't we just use URIs?
>
> Nested jars are only supported when doing non-blocking IO.

So... yea.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v3

This was a bit of a pain, but I think it was worth it. It's much less fragile than manually figuring out paths like the first patch did. And it doesn't have the downsides of the second patch (namely, this allows us to have control over file handles).

Also added additional tests for comments, and nested jars. And it's smarter about knowing that binary components need the addon to be unpacked.

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 570594 [details] [diff] [review]
Patch v3

Review of attachment 570594 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty much there, I think there are a few ways to clean this up though

::: toolkit/mozapps/extensions/ChromeManifestParser.jsm
@@ +107,5 @@
> +      while (true) {
> +        if (uri instanceof Ci.nsIJARURI) {
> +          entries.push(uri.JAREntry);
> +          uri = uri.JARFile;
> +        } else {

How about instead:

while (uri instanceof Ci.nsIJARURI) {
  entries.push(uri.JAREntry);
  uri = uri.JARFile
}

... open the reader

@@ +123,5 @@
> +        let innerReader = Cc["@mozilla.org/libjar/zip-reader;1"].
> +                          createInstance(Ci.nsIZipReader);
> +        innerReader.openInner(outerReader, entries[i]);
> +        readers.push(innerReader);
> +      }

Then outerReader is just reader

@@ +169,5 @@
> +  findBinaryComponents: function CMP_findBinaryComponents(aManifest) {
> +    return aManifest.some(function(aEntry) {
> +      return aEntry.type == "binary-component";
> +    });
> +  },

Feels slightly weird to have this in this module, it's rather specific to the needs here but I'm not terribly concerned by it

@@ +172,5 @@
> +    });
> +  },
> +
> +  _parseContents: function CMP_parseContents(aContents, aURI,
> +                                             aRecursiveCallback) {

aRecursiveCallback is always this.parseSync so it is probably unnecessary to pass it, in fact this entire function could just be rolled into parseSync now

@@ +183,5 @@
> +      if (type == "manifest") {
> +        let uri = NetUtil.newURI(tokens.shift(), null, aURI);
> +        data = data.concat(aRecursiveCallback(uri));
> +      } else {
> +        data.push({type: type, baseURI: baseURI, args: tokens});

It might be worth storing the line number here too

@@ +187,5 @@
> +        data.push({type: type, baseURI: baseURI, args: tokens});
> +      }
> +    }
> +
> +    let baseURI = NetUtil.newURI(".", null, aURI).spec;

What is this doing exactly?

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +897,5 @@
> +      let chromeManifest = ChromeManifestParser.parseSync(uri);
> +      addon.hasBinaryComponents = ChromeManifestParser.findBinaryComponents(chromeManifest);
> +    } else {
> +      addon.hasBinaryComponents = false;
> +    }

I wonder if instead we should test for binary components and then force addon.unpack to true in that case?

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Dave Townsend (:Mossop) from comment #11)
> It might be worth storing the line number here too

I'm having trouble coming up with any uses of that. Also, if that were included, it would also need to include the manifest filename (baseURI is only the containing directory).


> @@ +187,5 @@
> > +    let baseURI = NetUtil.newURI(".", null, aURI).spec;
> 
> What is this doing exactly?

It removes the filename from the URI, so it ends up being the containing directory of the manifest file. It's not used yet, but it's needed due to relative paths in chrome manifests (which could be resolved when we parse, but the parser has no knowledge of specific instructions and which arguments are expected to be paths).

> I wonder if instead we should test for binary components and then force
> addon.unpack to true in that case?

Filed bug 698641, to avoid scope creep here.

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Dave Townsend (:Mossop) from comment #11)
> > +  findBinaryComponents: function CMP_findBinaryComponents(aManifest) {
> > +    return aManifest.some(function(aEntry) {
> > +      return aEntry.type == "binary-component";
> > +    });
> > +  },
> 
> Feels slightly weird to have this in this module, it's rather specific to
> the needs here but I'm not terribly concerned by it

I've made it a more generic API:
    ChromeManifestParser.hasType(manifest, "binary-component");

I want to keep it in ChromeManifestParser so that additional logic can be added in  bug 696701 (and to avoid yet another random global function in XPIProvider).

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v3.1

Comment 15

[Dave Townsend [:mossop]]

(In reply to Blair McBride (:Unfocused) from comment #13)
> (In reply to Dave Townsend (:Mossop) from comment #11)
> > > +  findBinaryComponents: function CMP_findBinaryComponents(aManifest) {
> > > +    return aManifest.some(function(aEntry) {
> > > +      return aEntry.type == "binary-component";
> > > +    });
> > > +  },
> > 
> > Feels slightly weird to have this in this module, it's rather specific to
> > the needs here but I'm not terribly concerned by it
> 
> I've made it a more generic API:
>     ChromeManifestParser.hasType(manifest, "binary-component");
> 
> I want to keep it in ChromeManifestParser so that additional logic can be
> added in  bug 696701 (and to avoid yet another random global function in
> XPIProvider).

I was assuming that would just ignore those lines and so they wouldn't be in the manifests, but the API you've come up with here feels nicer now anyway.

Comment 16

[Dave Townsend [:mossop]]

(In reply to Blair McBride (:Unfocused) from comment #12)
> (In reply to Dave Townsend (:Mossop) from comment #11)
> > It might be worth storing the line number here too
> 
> I'm having trouble coming up with any uses of that. Also, if that were
> included, it would also need to include the manifest filename (baseURI is
> only the containing directory).

Right I was misreading and thinking baseURI was the manifest URI. My only thought would be that some other extension or something could use it for chrome manifest validation or maybe some other app wants to support new manifest types and would use that info for error reporting. Not a bug deal though.

Comment 17

[Dave Townsend [:mossop]]

Comment on attachment 570893 [details] [diff] [review]
Patch v3.1

Review of attachment 570893 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/ChromeManifestParser.jsm
@@ +105,5 @@
> +
> +    if (!contents)
> +      return [];
> +
> +

No need for two newlines

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/integration/fx-team/rev/6b7874f5b777

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/6b7874f5b777

Comment 20

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (Windows NT 5.1; rv:11.0a1) Gecko/20111120 Firefox/11.0a1
Mozilla/5.0 (Windows NT 5.1; rv:10.0a2) Gecko/20111120 Firefox/10.0a2

Verified on Aurora 10 and Nightly Beta on Windows XP, Windows 7, Ubuntu 11.10 and MacOS 10.6. Incompatible binary add-ons are displayed as incompatible when the add-on default to compatible pref is enabled.

1. Start Firefox 9 or lower with a clean profile.
2. Switch the extensions.strictCompatibility pref to false if updating to F10.
3. Install a binary add-on which is incompatible with the Firefox version to which upgrading-F10/11 (e.g. FoxyTunes 4.3.3)
4. Manually upgrade to Firefox 10/11.

The add-on should be listed as incompatible after the upgrade. Also checked with Grafx bot, evernote web clipper, cooliris, as well as binary third party add-ons specific for Windows OSs-real player, avg safe search, div x player.

Comment 21

[Dave Vasilevsky]

My extension Dock Progress uses binary components only for Firefox 3.x and lower: https://github.com/vasi/firefox-dock-progress/blob/master/chrome.manifest . For later versions of Firefox the extension uses js-ctypes instead, but it still gets flagged as incompatible with each new version.

Any chance this code could be modified to only flag binary components that apply to the current version of Firefox?

Comment 22

[Dave Townsend [:mossop]]

(In reply to Dave Vasilevsky from comment #21)
> My extension Dock Progress uses binary components only for Firefox 3.x and
> lower:
> https://github.com/vasi/firefox-dock-progress/blob/master/chrome.manifest .
> For later versions of Firefox the extension uses js-ctypes instead, but it
> still gets flagged as incompatible with each new version.
> 
> Any chance this code could be modified to only flag binary components that
> apply to the current version of Firefox?

That would be bug 696701 however in your case I'm not sure what the problem is. We detect binary components using the binary-component lines in chrome.manifest, these are only used for Firefox 4 and later so I'm assuming you don't have them in your extension, so I don't know what would be the problem here. Maybe file a new bug for investigation?

Comment 23

[Dave Vasilevsky]

Thanks for the link to the new bug. You may be right that I can simply remove the binary-component field, I'll give that a try.