693906 - Parse and use new compatibility ranges in AMO metadata ping

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Bug 692971 will add a new section to the metadata ping, that specifies compatibility info for addon version ranges for an app version range.  This will allow AMO to override compatible-by-default, to mark an addon version as explicitly incompatible with a given app version.

Note: For the moment, this is in addition to the information supplied in the versioncheck ping. Might eventually remove that redundancy.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Note to self: will need to take into account bug 581065.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

This ended up being bigger than I expected.

Also end up fixing at least one other bug. Peviously, when AddonRepository refetched all it's data from the server due to a background update check, that data wasn't used until next restart - fixed that so that compatibility overrides can be applied immediately.

I needed to put the compatiblityOverrides property directly on the AddonInternal object, rather than just referencing _repositoryAddon, so that it would work with importMetadata().

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Pushed this through Try, looks like I broke browser_searching.js. Will need some minor (I hope) fixup for that.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1.1

Only minor changes:
* Test fix for browser_searching.js (added an additional test there too)
* Removed handling of compatibility overrides in AddonRepository when it's not a GUID search (since only GUID searches get them anyway)

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 576701 [details] [diff] [review]
Patch v1.1

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

Some comments here and I'd like to spin over it one last time before r+.

What about the upgrade scenario, are we going to repopulate the cache when we switch to a new version of the app?

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +261,5 @@
> +  if (aMinVersion) this.minVersion = aMinVersion;
> +  if (aMaxVersion) this.maxVersion = aMaxVersion;
> +  if (aAppID) this.appID = aAppID;
> +  if (aAppMinVersion) this.appMinVersion = aAppMinVersion;
> +  if (aAppMaxVersion) this.appMaxVersion = aAppMaxVersion;

Seems like there is no reason for the ifs here

@@ +1256,5 @@
> +
> +      if (Services.vc.compare(override.minVersion, aAddonVersion) <= 0 &&
> +          Services.vc.compare(aAddonVersion, override.maxVersion) <= 0 &&
> +          Services.vc.compare(override.appMinVersion, appVersion) <= 0 &&
> +          Services.vc.compare(appVersion, override.appMaxVersion) <= 0) {

Above it appears that you allow minVersion, maxVersion etc. to be null yet you still do comparisons here. What is the expected behaviour?

::: toolkit/mozapps/extensions/AddonRepository.jsm
@@ +1176,5 @@
>  
>          let currentVersion = Services.appinfo.version;
>          return (Services.vc.compare(minVersion, currentVersion) <= 0 &&
> +                ((!strictCompatibility) ||
> +                 Services.vc.compare(currentVersion, maxVersion) <= 0));

We don't know here if the add-on has strictCompatibility in its install.rdf so this would cause us to show add-ons in the search results that the user can't actually install. Is there a bug on file to add that to the API?

@@ +1268,5 @@
> +          continue;
> +
> +        return { appID: appID,
> +                 appMinVersion: minVersion,
> +                 appMaxVersion: maxVersion };

This should return the app ID range in preference to the toolkit ID range

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +3349,5 @@
>    /**
> +   * Update the repositoryAddon property for all add-ons.
> +   */
> +  updateAddonRepositoryData: function XPI_updateAddonRepositoryData() {
> +    let addons = XPIDatabase.getAddons();

Can we use an async query here please

::: toolkit/mozapps/extensions/test/browser/browser_searching.js
@@ +621,5 @@
>      run_next_test();
>    });
>  });
>  
> +// Tests that compatibly-by-default addons are shown if strict compatibility checking is disabled

*compatible

::: toolkit/mozapps/extensions/test/xpcshell/data/test_AddonRepository_cache.xml
@@ +117,5 @@
>    </addon>
> +
> +  <addon_compatibility hosted="true" id="123">
> +    <guid>[e-mail]</guid>
> +    <name>PASS</name>

We never actually parse this name right?

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Dave Townsend (:Mossop) from comment #5)
> What about the upgrade scenario, are we going to repopulate the cache when
> we switch to a new version of the app?

Hmm, guess we need to.


> @@ +1256,5 @@
> > +
> > +      if (Services.vc.compare(override.minVersion, aAddonVersion) <= 0 &&
> > +          Services.vc.compare(aAddonVersion, override.maxVersion) <= 0 &&
> > +          Services.vc.compare(override.appMinVersion, appVersion) <= 0 &&
> > +          Services.vc.compare(appVersion, override.appMaxVersion) <= 0) {
> 
> Above it appears that you allow minVersion, maxVersion etc. to be null yet
> you still do comparisons here. What is the expected behaviour?

You mean override.minVersion, override.maxVersion, etc? They'll never be null - that's checked in AddonRepository._parseAddonCompatElement().


> ::: toolkit/mozapps/extensions/AddonRepository.jsm
> @@ +1176,5 @@
> >  
> >          let currentVersion = Services.appinfo.version;
> >          return (Services.vc.compare(minVersion, currentVersion) <= 0 &&
> > +                ((!strictCompatibility) ||
> > +                 Services.vc.compare(currentVersion, maxVersion) <= 0));
> 
> We don't know here if the add-on has strictCompatibility in its install.rdf
> so this would cause us to show add-ons in the search results that the user
> can't actually install. Is there a bug on file to add that to the API?

Bah, I forgot about search results. The Search API actually filters server-side, so having the strictCompatility option in the returned XML won't help much. Filed bug 706385 (AMO side) and bug 706387 (client side) to make sure the Search API filters correctly.


> :::
> toolkit/mozapps/extensions/test/xpcshell/data/test_AddonRepository_cache.xml
> @@ +117,5 @@
> >    </addon>
> > +
> > +  <addon_compatibility hosted="true" id="123">
> > +    <guid>[e-mail]</guid>
> > +    <name>PASS</name>
> 
> We never actually parse this name right?

Nope, but the API does include it. So some of the test data has it, some if it doesn't - just to be doubly sure nothing breaks.

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Blair McBride (:Unfocused) from comment #6)
> (In reply to Dave Townsend (:Mossop) from comment #5)
> > What about the upgrade scenario, are we going to repopulate the cache when
> > we switch to a new version of the app?
> 
> Hmm, guess we need to.

This turned out to be a lot trickier than I anticipated, due to only opening addons.sqlite when data is first requested, and only supporting one query to AMO at a time :\ If you don't mind, I'd like to tackle that separately, in bug 706777.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v2

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Oh, and I moved findMatchingCompatibilityOverride in this patch rather than in bug 527141 to avoid unneeded code churn.

Comment 10

[Dave Townsend [:mossop]]

(In reply to Blair McBride (:Unfocused) from comment #7)
> (In reply to Blair McBride (:Unfocused) from comment #6)
> > (In reply to Dave Townsend (:Mossop) from comment #5)
> > > What about the upgrade scenario, are we going to repopulate the cache when
> > > we switch to a new version of the app?
> > 
> > Hmm, guess we need to.
> 
> This turned out to be a lot trickier than I anticipated, due to only opening
> addons.sqlite when data is first requested, and only supporting one query to
> AMO at a time :\ If you don't mind, I'd like to tackle that separately, in
> bug 706777.

Sounds fine, I think it's a must have for first release though.

Comment 11

[Dave Townsend [:mossop]]

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

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

::: toolkit/mozapps/extensions/AddonRepository.jsm
@@ +1276,5 @@
> +        // Only use Toolkit app ranges if no ranges match the application ID.
> +        if (appID == TOOLKIT_ID && !toolkitAppRange)
> +          toolkitAppRange = appRange;
> +        else
> +          return appRange;

This will return the toolkit range if for some reason there are two in the XML before the app range. I assume that can't happen normally in which case testing !toolkitAppRange is unnecessary.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/integration/fx-team/rev/29879af41d58

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Pe-emptively requesting approval (it's not merged into central yet). Important piece of the compatible-by-default story, required for enabling it on a release version.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/29879af41d58

Not marking as fixed because it's waiting for Aurora approval.

Comment 15

[Dave Townsend [:mossop]]

Bugs get marked as fixed when they land in m-c

Comment 16

[Alex Keybl [:akeybl]]

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

[Triage Comment]
Approving low-risk add-ons default to compatible bugs for Aurora with the understanding that we're going to come up with explicit methods of tracking any regressions in aurora/beta related to this and stay on top of them.

Comment 17

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Note: This can't land on Aurora until bug 707328 also has approval and can land, as it fixes an issue with the patch here.

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/30420d5f77cf

Comment 19

[u279076]

Is this something QA can verify?

Comment 20

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(Pasting from email)

You won't be able to fully test this yet, as AMO only has one compatibility override so far. It's for Addon Compatibility Reporter (ACR), from versions 0 to 1.0, for versions of Firefox from 10.0 onward. Unfortunately, the addon doesn't work correctly when compatible-by-default is enabled - it will automatically disable all compatibility checks, including compatibility overrides. v1.0.1 was released to as a temporary fix: that version will automatically re-enable compatibility checking when compatibile-by-default is enabled, but that version isn't included in the compatibility override.

Still, you can test some things with an old version from here:
https://addons.mozilla.org/en-US/firefox/addon/add-on-compatibility-reporter/versions/

1. With compatible-by-default enabled, v1.0 should not installed - instead, you should get a warning saying it's incompatible.
2. With strict compatibility enabled, v1.0 should install fine.

It may be worth testing that ACR correctly upgrades to v1.0.1 and allows compatibly-by-default to work:
1. Manually install ACR v1.0 into your Firefox profile.
2. Start Firefox 10 (or 11).
3. ACR should be upgraded to v1.0.1
4. Compatibility checking should not be disabled. There should not be a warning at the top of the Add-ons Manager. And the extensions.checkCompatibility.VERSION prefs should not be set.

When AMO has a compatibility override for another addon, you'll be able to test more:

1. Enable strict compatibility.
2. Install an addon with a compatibility override.
3. Then disable strict compatibility.
4. The addon should then be marked as incompatible and be disabled.

And:
1. In Forefox 9, install an addon that has a compatibility override that matches Firefox 10
2. Upgrade to Firefox 10
3. The addon should be marked as incompatible


Unfortunately, a lot of this depends on AMO having compatibility overrides for the different scenarios. Krupa has been testing the AMO bugs, and she's had some test data inserted into the database on the development site (https://addons-dev.allizom.org/) - you could ask to have some additional test data added for you. If you want to do that, you'll need to change some preferences to point to the development site. It should be enough to change the domain from addons.mozilla.org to addons-dev.allizom.org in the following preferences:
* extensions.getAddons.get.url
* extensions.getAddons.search.url
* extensions.update.url

Comment 21

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (X11; Linux x86_64; rv:12.0a1) Gecko/20120117 Firefox/12.0a1
Mozilla/5.0 (X11; Linux x86_64; rv:11.0a2) Gecko/20120116 Firefox/11.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:10.0) Gecko/20100101 Firefox/10.0

Tested the following scenario with Zotero and After the Deadline - Spell and Grammar Checker:
1. In Firefox 9, install an addon that has a compatibility override that matches Firefox 10
2. Upgrade to Firefox 10
3. The addon should be marked as incompatible

This works correctly, except for the case in which the user skips the compatibility check at startup. Filed bug 718630 for that.

Got in contact with Krupa for the last scenario test (the first scenario from comment 20 has been verified in bug 713815)

Will mark this as verified as soon as all scenarios are tested.

Comment 22

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (X11; Linux x86_64; rv:10.0) Gecko/20100101 Firefox/10.0

Filed new bug 720662 (now that Tab utilities add-on has an override) for the following scenario:

1. Enable strict compatibility.
2. Install an addon with a compatibility override.
3. Then disable strict compatibility.
4. The addon should then be marked as incompatible and be disabled.

The add-on is enabled in step 4 if the Add-ons manager is not opened before in the session. 

Went through all scenarios required for this issue (comment 20)-see comment 21 for the rest of the results. Adding bug 720662 and bug 718630 to dependencies so that they can be tracked separately.

Setting resolution to verified for Firefox 10.