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

VERIFIED FIXED in 6.3.9

Status

addons.mozilla.org Graveyard
Add-on Validation
VERIFIED FIXED
7 years ago
2 years ago

People

(Reporter: robhudson, Assigned: basta)

Tracking

unspecified
6.3.9

Details

(Reporter)

Description

7 years ago
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?
Assignee: nobody → mattbasta
Target Milestone: --- → 6.3.9
Blocks: 694658
(Assignee)

Comment 1

7 years ago
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.
Status: NEW → UNCONFIRMED
Ever confirmed: false
(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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 3

7 years ago
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.
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)
(Reporter)

Comment 5

7 years ago
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.
(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!
(Reporter)

Comment 7

7 years ago
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)?
(Assignee)

Comment 8

7 years ago
(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?
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
(Reporter)

Comment 10

7 years ago
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.
(Assignee)

Comment 11

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

Comment 12

7 years ago
Merged. Thanks basta

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

I'll confirm on -dev.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Reporter)

Comment 13

7 years ago
Confirmed detection of binary-components in add-ons reference in comment 0.
Status: RESOLVED → VERIFIED
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.