Add preference to globally (re)enable strict compatibility checks

VERIFIED FIXED in mozilla10

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: Unfocused, Assigned: Unfocused)

Tracking

(Depends on: 1 bug, {dev-doc-complete})

unspecified
mozilla10
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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
My patch for this breaks a third of all Addon Manager xpcshell tests. This may take awhile...
Created attachment 567683 [details] [diff] [review]
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.
Attachment #567683 - Flags: review?(dtownsend)
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.
Attachment #567683 - Flags: review?(dtownsend) → review-
(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.
Created attachment 568299 [details] [diff] [review]
Patch v2
Attachment #567683 - Attachment is obsolete: true
Attachment #568299 - Flags: review?(dtownsend)
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
Attachment #568299 - Flags: review?(dtownsend) → review+
(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)
Created attachment 570629 [details] [diff] [review]
Patch v2.1

Just some minor test fixes.
Attachment #568299 - Attachment is obsolete: true
(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
(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.
https://hg.mozilla.org/integration/fx-team/rev/41c52a9e1337
Flags: in-testsuite+
Flags: in-litmus-
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/1fb831781a15
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Keywords: dev-doc-needed
Does this not affect compatibility checking for themes?
(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.
Depends on: 702920
Depends on: 704483
Depends on: 704165
No longer depends on: 704483
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.
Status: RESOLVED → VERIFIED
Documented:

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

And mentioned on Firefox 10 for developers.
Keywords: dev-doc-needed → dev-doc-complete

Updated

5 years ago
Depends on: 779471
You need to log in before you can comment on or make changes to this bug.