597255 - add-on validator: check if <em:unpack>true</em:unpack> is needed in install.rdf, else show error

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Developer Pages, defect, P3)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

The add-on validator should check if <em:unpack>true</em:unpack> is needed in the install.rdf, else it should show an error.

Source: http://blog.mozilla.com/mwu/2010/09/10/extensions-now-installed-packed/
"Extensions with dictionaries, binary components, or window icons are most likely to be incompatible with packed installation."
Packed installation is new the default install method for gecko > 2.0b7 (assuming that 2.0b6 doesn't have it because of the chemspill release).

Dictionaries can be detected, e.g. by file extensions.
Binary components can be detected, e.g. by file extensions.
Window icons can be detected by their path: https://developer.mozilla.org/en/Window_icons

Comment 1

[Benedikt Pfeifer]

I wanted to file a bug about unpacking as well but suggesting a different approach: why should Firefox be told things that it can figure out itself?

Suggested solution: keep unpack behaviour as it is (ie always unpack if the tag is saying to do so) but let it do the checks mentioned above itself during install. That is: it would install extensions with binary components, dictionaries and window icons (is there a possible check for search engines?) always as unpacked and doesn't need to rely on the extension developer to set a 'sort of obvious flag' (and is therefore better in my opinion).

Comment 2

[Benedikt Pfeifer]

I filed an enhancement request (bug 599307) for the installation behaviour as described in comment 1. I guess either of these two bugs might make the other unnecessary?

Comment 3

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to comment #2)
> I filed an enhancement request (bug 599307) for the installation behaviour as
> described in comment 1. I guess either of these two bugs might make the other
> unnecessary?

Well, your bug is a Firefox bug, but it's likely a WONTFIX, since it will be the developer's responsibility to take care of this flag, and the automatic detection is likely to be a hit or miss.

Making this update to the validator is a good idea, but I think it is something that will be better implemented in the new version.

Comment 4

[Wil Clouser [:clouserw]]

Jorge, can you clarify what we need to look for, when, and what to do with it?  What's the message we are telling the user?

Comment 5

[Benedikt Pfeifer]

(In reply to comment #3)

> Well, your bug is a Firefox bug, but it's likely a WONTFIX, since it will be
> the developer's responsibility to take care of this flag, 

Why should it be their responsibility if there are characteristics that require an extension to be unpacked?

> and the automatic detection is likely to be a hit or miss.


If the rules are not reliable why would it make sense to include them in the add-ons validator? If they don't work during installation, they shouldn't give any better results here either?

Comment 6

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to comment #4)
> Jorge, can you clarify what we need to look for, when, and what to do with it? 
> What's the message we are telling the user?

Show warning when one or more of the following happen:
1) The add-on contains a binary file in the components directory.
2) The add-on type is dictionary.
3) The add-on contains a .ico file.

The warning should say something like "This add-on contains code that possibly doesn't work on Firefox 4 without setting <em:unpack> to true in install.rdf".

(In reply to comment #5)
> (In reply to comment #3)
> > and the automatic detection is likely to be a hit or miss.
> 
> 
> If the rules are not reliable why would it make sense to include them in the
> add-ons validator? If they don't work during installation, they shouldn't give
> any better results here either?

The validator doesn't need to be perfect. All it does is show warnings that developers can follow or ignore. There's an additional human review that can help figure out what it required and what isn't. False positives or false negatives during add-on installation in Firefox is more problematic because it can lead to performance problems or developers incorrectly relying on Firefox dealing with it instead of using the flag, as they should.

(In reply to comment #5)
> (In reply to comment #3)
> 
> > Well, your bug is a Firefox bug, but it's likely a WONTFIX, since it will be
> > the developer's responsibility to take care of this flag, 
> 
> Why should it be their responsibility if there are characteristics that require
> an extension to be unpacked?
> 

There's a need for the flag because automatic detection would be unreliable. Given the need for the flag, it's better to train developers to use it rather than combining the flag and automatic detection, which just complicates the process.

Comment 7

[Matt Basta [:basta]]

Hey Jorge, can you define for this what an executable would be? .exe? .sh? A little list would be fine. That's the only thing that I need to finish this up.

I just need to write unit tests and add the executable extensions and this will be good to close. The changes are in f95b910:

https://github.com/mattbasta/amo-validator/commit/f95b9108bf5d26a76bb45c7b7fece007f6e4b876

Comment 8

[Jorge Villalobos [:jorgev] (he/him)]

Extensions can include binaries in the components, plugins and platform directories. Those would normally be .dll, .so or .dylib, but can also be .exe or .bin. I think those should cover most cases.
If you need testcases, here are a few add-ons that you can use:
https://addons.mozilla.org/en-US/firefox/addon/249328/
https://addons.mozilla.org/en-US/firefox/addon/219/
https://addons.mozilla.org/en-US/firefox/addon/64247/

Comment 9

[Matt Basta [:basta]]

This is as good as done; I just need to build some tests for it.

Comment 10

[Wil Clouser [:clouserw]]

(In reply to comment #9)
> This is as good as done; I just need to build some tests for it.

Leaving it in 5.12.3 then.

Comment 11

[Matt Basta [:basta]]

Tests were added in c30ce88.

https://github.com/mattbasta/amo-validator/commit/ce0ce883bdce595d5c5fc3457072f3a53b772f7a