719545 - validator fails to detect binary-components in some add-ons

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Add-on Validation, defect)

Description

[Rob Hudson [:robhudson]]

After some testing this morning, I discovered a bug for detecting binary-components…

It failed to detect binary-components in 2 add-ons we tested with known use of binary-components in the chrome.manifest.  Those add-ons are:

https://addons.mozilla.org/en-US/firefox/addon/dock-progress/
https://addons.mozilla.org/en-US/firefox/addon/pricegong/

I'm curious what reason the validator doesn't detect these correctly?

Comment 1

[Matt Basta [:basta]]

The validator is behaving correctly, from what I can see. The max version in each of those add-ons falls below the compatibility threshold for the test to be run. The test is defined for any add-on supporting 4.2a1pre or higher.

I don't know what this *should* be, but it's basically defined for everything above FX4.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Matt Basta from comment #1)
> The test is defined for any add-on supporting 4.2a1pre or higher.

4.2a1pre? 4.0 supported registering binary components this way, so it should be 4.0b1 (or one of the 3.7 alphas, but 4.0b1 seems good enough IMO). The PriceGong addon should indeed be flagged as having binary components.

The Dock Progress addon has a binary-component line too, but it's got a flag restricting it to Firefox 3, which didn't support registering binary components that way - so that line serves no purpose.

Comment 3

[Matt Basta [:basta]]

Done:

https://github.com/mozilla/amo-validator/pull/110

There's no test(s) since it's just a change in which value we're using for the version and there shouldn't be any side effects.

Comment 4

[Andrew Williamson [:eviljeff]]

Can we test this is picked up:

https://addons.mozilla.org/en-US/thunderbird/addon/new-account-types/ 
(as the instruction isn't in the root chrome.manifest, but in a linked one)

And this one isn't:

https://addons.mozilla.org/en-US/firefox/addon/conductory/ 
(uses ctypes to load dll's so isn't affected by the check, but does have a 'binary-component' line in a file in the /components folder)

Comment 5

[Rob Hudson [:robhudson]]

Locally I tested both of these...

The "new-account-types" add-on failed to find the beta.manifest file with the binary-component line in it, "Notice:	Linked manifest could not be found." so didn't get flagged for binary-components. It did find the binary extensions.

The "conductory" add-on looked found the binary extensions but not binary-components. If I follow correctly, this is good.

Comment 6

[Andrew Williamson [:eviljeff]]

(In reply to Rob Hudson [:robhudson] from comment #5)
> The "new-account-types" add-on failed to find the beta.manifest file with
> the binary-component line in it, "Notice:	Linked manifest could not be
> found." so didn't get flagged for binary-components. It did find the binary
> extensions.

Ah. The validator should read the "manifest components/components.manifest" in chrome.manifest, then read that file where there is a binary-components line.  beta.manifest is then linked from components.manifest but I suppose there is a limit to how much spidering is practical.  Does it do any at all currently?

> The "conductory" add-on looked found the binary extensions but not
> binary-components. If I follow correctly, this is good.

yes!

Comment 7

[Rob Hudson [:robhudson]]

It finds the "components/components.manifest", but then fails to find the "components/beta.manifest". I'm curious if it gets confused by the paths. Is that correct that if the file is in the "components/" folder, it can reference the file "beta.manifest" without a path from the root (i.e. it doesn't need the "components/" part of the path)?

Comment 8

[Matt Basta [:basta]]

(In reply to Andrew Williamson [:eviljeff] from comment #6)
> Ah. The validator should read the "manifest components/components.manifest"
> in chrome.manifest, then read that file where there is a binary-components
> line.  beta.manifest is then linked from components.manifest but I suppose
> there is a limit to how much spidering is practical.  Does it do any at all
> currently?

Yes, it will recursively visit manifest files, and will also report when the manifest instructions create a cycle.

I think where the problem might stem is with the paths that the manifest instruction accepts. I believe the validator assumes all paths are absolute, though this might not be the case. MDN does not go into detail. Are these paths relative? Or perhaps both relative and absolute?

Comment 9

[Andrew Williamson [:eviljeff]]

I'm not 100% if manifest paths are(In reply to Matt Basta from comment #8)
> I think where the problem might stem is with the paths that the manifest
> instruction accepts. I believe the validator assumes all paths are absolute,
> though this might not be the case. MDN does not go into detail. Are these
> paths relative? Or perhaps both relative and absolute?

Not sure but I'm guessing relative works (given the developer).  The MDN page mentions URIs for the content package being absolute or relative so its probably the same for the manifest instruction also.

According to an MXR search [*] this is the only add-on (on AMO) that does these nested manifests and the binary-component instruction should be picked up from components.manifest anyway so I'm not sure its worth putting any extra in for this edge case.

[*]https://mxr.mozilla.org/addons/search?string=^manifest&regexp=on&find=\.manifest&findi=\.xul%24&filter=^[^\0]*%24&hitlimit=&tree=addons

Comment 10

[Rob Hudson [:robhudson]]

Is there anything more to do here? I know there's a pull request on the amo-validator to fix the Fx4 min version. Once this is done I can merge it in and re-test the validator on -dev.

Comment 11

[Matt Basta [:basta]]

It should be good once the pull is merged in. I'll file a separate bug for the sub-manifest path issue(s).

Comment 12

[Rob Hudson [:robhudson]]

Merged. Thanks basta

https://github.com/mozilla/amo-validator/commit/0f9882d

I'll confirm on -dev.

Comment 13

[Rob Hudson [:robhudson]]

Confirmed detection of binary-components in add-ons reference in comment 0.