693901 - Add preference to globally (re)enable strict compatibility checks

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Bug 693897 will allow individual addons to opt-in to strict compatibility checking, but there are cases where strict compatibility checking should be able to be enabled globally (ie, fall back to old not-compatibile-by-default behaviour):

* Other toolkit apps may want to have strict compatibility checking
* Mission critical situations. If you're sending Firefox to the aliens on Europa as a peace offering, you don't want it breaking because of an incompatible addon. They might not take kindly to that.
* Will act as a kill-switch for this functionality if something breaks (saves a backout of the feature).

Suggested name: extensions.strictCompatibilityChecking

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

My patch for this breaks a third of all Addon Manager xpcshell tests. This may take awhile...

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

My strategy for fixing the (OMG SO MANY) tests was:
* If it tests something that determines compatibility, make a copy - one for compatible-by-default, one for strict compatibility. This was much easier than testing both in one test.
* Otherwise, the test just wants an incompatible addon and doesn't care why it's incompatible - so enable strict compatibility for that test.

I also made the updater tests run with strict compatiblity, as some of them expect certain addons to be incompatible. Same with one of the browser tests.

The tests I made copies of already test everything to do with actually determining whats compatible and what's not, and how that affects appDisabled (etc). That just left testing the pref changes.

Comment 3

[Dave Townsend [:mossop]]

Comment on attachment 567683 [details] [diff] [review]
Patch v1

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

I'm surprised to see no changes to AddonUpdateChecker.jsm here, or will those come elsewhere?

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +378,5 @@
>  
> +    try {
> +      gStrictCompatibility = Services.prefs.getBoolPref(PREF_EM_STRICT_COMPATIBILITY);
> +    } catch (e) {}
> +    Services.prefs.addObserver(PREF_EM_STRICT_COMPATIBILITY, this, false);

I think the default should be true so apps have to explicitly opt into this behaviour

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +6857,5 @@
> +    // Only extensions can be compatible by default; themes always use strict
> +    // compatibility checking.
> +    if (this.type == "extension" && !AddonManager.strictCompatibility)
> +      return true;
> +

I'm not sure we remembered to add it to the spec but we still want to test against the minVersion, i.e. an extension for Firefox 21 shouldn't install in Firefox 10.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Dave Townsend (:Mossop) from comment #3)
> I'm surprised to see no changes to AddonUpdateChecker.jsm here, or will
> those come elsewhere?

Was going to do that in bug 527141.

> I think the default should be true so apps have to explicitly opt into this
> behaviour

Hm, yea, makes sense.

> I'm not sure we remembered to add it to the spec but we still want to test
> against the minVersion, i.e. an extension for Firefox 21 shouldn't install
> in Firefox 10.

Almost forgot about that (it wasn't in the spec). Filed bug 695977 and amended the spec.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v2

Comment 6

[Dave Townsend [:mossop]]

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

Would it be worth grabbing one of hte project branches for this work? Looks like larch might be free

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Dave Townsend (:Mossop) from comment #6)
> Would it be worth grabbing one of hte project branches for this work? Looks
> like larch might be free

Instead of that, where I'd still need to deal with bitrot, what do you say to having this land with the pref toggled? (ie, land with the entire feature disabled)

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v2.1

Just some minor test fixes.

Comment 9

[Dave Townsend [:mossop]]

(In reply to Blair McBride (:Unfocused) from comment #7)
> (In reply to Dave Townsend (:Mossop) from comment #6)
> > Would it be worth grabbing one of hte project branches for this work? Looks
> > like larch might be free
> 
> Instead of that, where I'd still need to deal with bitrot, what do you say
> to having this land with the pref toggled? (ie, land with the entire feature
> disabled)

I am in full support of this plan

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Dave Townsend (:Mossop) from comment #9)
> > Instead of that, where I'd still need to deal with bitrot, what do you say
> > to having this land with the pref toggled? (ie, land with the entire feature
> > disabled)
> 
> I am in full support of this plan

Filed bug 698653 to flip the pref when we're ready.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/integration/fx-team/rev/41c52a9e1337

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/1fb831781a15

Comment 13

[Siddhartha Dugar [:sdrocking]]

Does this not affect compatibility checking for themes?

Comment 14

[Dão Gottwald [:dao]]

(In reply to sdrocking from comment #13)
> Does this not affect compatibility checking for themes?

Correct. Themes created for a particular Firefox version are almost certainly broken in the subsequent release.

Comment 15

[Virgil Dicu [:virgil] [QA]]

Setting this to verified.

The pref is present and functional in about:config. Currently enabled on Nightly and disabled on Aurora.

Two dependencies remaining for this.

Comment 16

[Eric Shepherd [:sheppy]]

Documented:

https://developer.mozilla.org/en/Install_Manifests#strictCompatibility

And mentioned on Firefox 10 for developers.