714698 - AMO not sending compatiblity updates for addons when compatMode=normal

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: API, defect)

Description

[:glob ✱]

Attachment: extensions.sqlite

"mass password reset" addon is identified as being incompatible with firefox 12.0a1 (nightly 2012-01-02), and i'm unable to enable or install it.

my extensions.log file doesn't contain anything interesting from today:

> 2011-10-01 11:07:03 ERROR addons.repository: Outstanding transaction, rolling back. at resource://gre/modules/AddonRepository.jsm:1325
> 2011-12-28 14:18:46 ERROR addons.xpi: Failed to open database (1st attempt): [Exception... "Component returned failure code: 0x80630001 (NS_ERROR_STORAGE_BUSY) [mozIStorageService.openUnsharedDatabase]"  nsresult: "0x80630001 (NS_ERROR_STORAGE_BUSY)"  location: "JS frame :: resource:///modules/XPIProvider.jsm :: XPIDB_openDatabaseFile :: line 4216"  data: no] at resource:///modules/XPIProvider.jsm:4216
> 2011-12-28 14:18:46 ERROR addons.xpi: Error processing file changes: TypeError: this.installLocations is null at resource:///modules/XPIProvider.jsm:1779

Comment 1

[Dave Townsend [:mossop]]

Related bugs with the same storage error: bug 671894 bug 705205.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

This seems to be caused by this extension having a maxVersion of 3.6a1pre in it's install.rdf - which is less than the minimum compatible version (4.0). AMO has updated compatibility data which makes the maxVersion 11.0a1, which makes it compatible. However, it seems AMO is not sending updated compatibility data on the update ping, when compatMode=normal. Presumably, a regression from bug 703781.

The requested URL for the update ping:
https://versioncheck.addons.mozilla.org/update/VersionCheck.php?reqVersion=2&id=[e-mail]&version=1.05&maxAppVersion=3.6a1pre&status=userEnabled,incompatible&appID={ec8030f7-c20a-464f-9b0e-13a3a9e97384}&appVersion=12.0a1&appOS=WINNT&appABI=x86-msvc&locale=en-US&currentAppVersion=12.0a1&updateType=49&compatMode=normal

Currently returns nothing, whereas it should be returning data on the current version, so that the Add-ons Manager gets the updated compatibility data.

A similar update ping request with compatMode=ignore correctly includes the relevant data:
https://versioncheck.addons.mozilla.org/update/VersionCheck.php?reqVersion=2&id=[e-mail]&version=1.05&maxAppVersion=3.6a1pre&status=userEnabled,incompatible&appID={ec8030f7-c20a-464f-9b0e-13a3a9e97384}&appVersion=12.0a1&appOS=WINNT&appABI=x86-msvc&locale=en-US&currentAppVersion=12.0a1&updateType=49&compatMode=ignore

Comment 3

[Rob Hudson [:robhudson]]

Looking at the code history, I can't see that we've ever returned the latest version if no compatible versions were found.  Would Firefox look at the RDF to tell whether the version we're supplying is either (1) a compatible version or (2) not a compatible version but the latest version fallback?

Also, isn't the GUID search the place to get compatibility information about an add-on?

Comment 4

[Dave Townsend [:mossop]]

(In reply to Rob Hudson [:robhudson] from comment #3)
> Looking at the code history, I can't see that we've ever returned the latest
> version if no compatible versions were found.  Would Firefox look at the RDF
> to tell whether the version we're supplying is either (1) a compatible
> version or (2) not a compatible version but the latest version fallback?

When Firefox installs an add-on that appears incompatible it pings versioncheck to ask AMO to send the latest compatibility information about the add-on. In the past if the latest compatibility information didn't include the user's current Firefox then not sending it wouldn't have been a problem. Now that we have DTC in some cases it would be required to achieve the effect we want (identifying add-ons that are compatible with at least Firefox 4 even if they aren't compatible with the current Firefox).

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

For future reference:
This is testable by attempting to install Mass Password Reset v1.04. It's maxVersion according to AMO is 10.*, but according to it's install.rdf it's some version below 4.0 - which is the minimum cut-off. Note that the newer v1.05 doesn't exhibit this behaviour, because it's install.rdf was recently updated in order to work around this bug.
https://addons.mozilla.org/en-US/firefox/addon/mass-password-reset/versions/

Comment 7

[Rob Hudson [:robhudson]]

I spoke with Blair on IRC. I was misunderstanding this bug to mean send the latest version even if there are no compatible versions found. But this was incorrect.

So I'm looking at the details of the parameters passed and what we have in AMO to return the correct result in these cases. The example posted above no longer works. Is there another example I can look at?

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Rob Hudson [:robhudson] from comment #7)
> I spoke with Blair on IRC. I was misunderstanding this bug to mean send the
> latest version even if there are no compatible versions found. But this was
> incorrect.
> 
> So I'm looking at the details of the parameters passed and what we have in
> AMO to return the correct result in these cases. The example posted above no
> longer works. Is there another example I can look at?

Mass Password Reset 1.0.4 (as Blair mentioned in comment 6) still triggers this for me on the latest Nightly build.

Comment 9

[Rob Hudson [:robhudson]]

According to AMO, this add-on has binary components, which triggers version check to switch to strict compatibility mode.

Is it true this add-on has binary components?  If not, that would be a bug in our validator.

If it is true and version check is doing the wrong thing, please let me know.

Comment 10

[Dave Townsend [:mossop]]

(In reply to Rob Hudson [:robhudson] from comment #9)
> According to AMO, this add-on has binary components, which triggers version
> check to switch to strict compatibility mode.
> 
> Is it true this add-on has binary components?  If not, that would be a bug
> in our validator.

No, mass password reset contains no binary components

Comment 11

[Rob Hudson [:robhudson]]

In this particular case it looks like the validator is tripping up on the .sh files:
https://github.com/mozilla/amo-validator/blob/master/validator/testcases/packagelayout.py#L8

The verbose output from the validator is as follows...

Warning: Flagged file extension found

The file "build.sh" has a flagged file extension.
The extension of this file is flagged because it usually identifies binary components. Please see http://addons.mozilla.org/developers/docs/policies/reviews#section-binary for more information on the binary content review process.
	Tier:	1
	File:	build.sh

Warning: Flagged file extension found

The file "config_build.sh" has a flagged file extension.
The extension of this file is flagged because it usually identifies binary components. Please see http://addons.mozilla.org/developers/docs/policies/reviews#section-binary for more information on the binary content review process.
	Tier:	1
	File:	config_build.sh

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Oh boy... then I think AMO and the Add-ons Manager may have wildly differing definitions of "binary component". For the purposes of compatible-by-default, a binary component is one that is registered via a binary-component line in the addon's chrome.manifest file. Anything else has no relevance to compatibility, as far as the Add-ons Manager is concerned.

Comment 13

[Dave Townsend [:mossop]]

Odd, .sh files are never binary to my knowledge. Perhaps the validator is thinking the add-on might run them and then run OS stuff perhaps? Either way AMO really should be using the same mechanism for detecting binary components as Firefox does, looking for binary-component lines in chrome.manifest.

Comment 14

[Wil Clouser [:clouserw]]

You can embed binary content in .sh scripts.  Some installers have their logic at the top and then attach the binary content at the bottom in order to distribute their files in a single runnable blob.

I agree the binary detection mechanisms between AMO and Firefox should match.  CCing Jorge and Andrew for what the right thing to do here is.

Comment 15

[Andrew Williamson [:eviljeff]]

(In reply to Wil Clouser [:clouserw] from comment #14)
> I agree the binary detection mechanisms between AMO and Firefox should
> match.  CCing Jorge and Andrew for what the right thing to do here is.

For review purposes, its best to have all possible executable files flagged in the validator so they're not missed.  For compatibility reasons it only really makes sense to flag binary XPCOM components as they're the only files that definitely need recompiling each gecko version.

IMO a good solution would be the introduction of a new validator check to do the same as Firefox and check for the binary-component* line in chrome.manifest and use that as the basis of the DTC check/fail.  Keep the other checks for review purposes though.

* I'm assuming we don't need to bother about Firefox3.6 and prior add-ons as apparently this instruction was only introduced in Firefox4.

Comment 16

[Wil Clouser [:clouserw]]

Bug 718694 adjusts the validator for the additional binary detection

Comment 17

[Rob Hudson [:robhudson]]

https://github.com/mozilla/zamboni/commit/1fca769

This updates AMO to use the binary_components field in versioncheck. Note: Before this will work we need to have AMO scan all files for a validator check to update this flag.

Comment 18

[Rob Hudson [:robhudson]]

Going to close. We'll run the re-validation soon when https://bugzilla.mozilla.org/show_bug.cgi?id=719545 is fixed.