1192437 - Support custom min/maxVersion Web Extensions

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[Dave Townsend [:mossop]]

The applications.gecko object can have a strict_min_version and strict_max_version property for setting application compatibility. We should use this and maybe something for actual app selection instead of defaulting to toolkit >= 42.

Comment 1

[Kris Maglione [:kmag]]

Looks like I accidentally fixed this already.

Comment 2

[Will Bamberg [:wbamberg]]

This does not seem to be fixed: https://github.com/mdn/webextensions-examples/issues/48.

Comment 3

[Kris Maglione [:kmag]]

I just tested on the latest Aurora, and it works as expected for me.

Comment 4

[Will Bamberg [:wbamberg]]

Oh, I just tried with a built XPI and it also worked as expected. If I use "Load Temporary Add-on", though, it doesn't seem to respect strict_min_version. Should it?

Comment 5

[Andrew Swan [:aswan]]

(In reply to Will Bamberg [:wbamberg] from comment #4)
> Oh, I just tried with a built XPI and it also worked as expected. If I use
> "Load Temporary Add-on", though, it doesn't seem to respect
> strict_min_version. Should it?

I can't see any good reason why it shouldn't.
I just created an extension with strict_min_version set to 49.0.0 and was able to install it temporarily in nightly (which is currently 48.0a1).  I opened the browser toolbox and ran:
AddonManager.getAddonByID(myid, addon => console.log(addon.appDisabled))
which displayed true, even though the addon was active!

From some quick reading of the code, it looks like the checking of appDisabled at install time actually happens from the front-end code (!!)  Specifically, the check here:
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/amWebInstallListener.js#114
which leads to canceling the install here:
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/amWebInstallListener.js#148

My assumption is that we should enforce this check for temporary add-ons and that the proper fix would be to check within the AddonManager itself rather than in the UI.  But now I've chained together a few assumptions along with potentially misunderstanding the current implementation so double-checking with a higher authority.

Comment 6

[Andrew Swan [:aswan]]

Also, separate from the issue with temporary add-ons, I think the fix for the original bug referenced in comment 1 landed here:
http://hg.mozilla.org/mozilla-central/rev/43d3300fa0a9
which, if I'm reading correctly, means it first appears in 47.  Which means that in the next release cycle when Aurora is 48 and we announce that web extensions are ready for widespread use there, users on the release channel will have 46 which still has the original bug.  Is this worthy of uplift?

Comment 7

[Kris Maglione [:kmag]]

We're not going to be able to uplift the change that fixed this. We'd have to create another patch and land it directly on beta. It would also need tests, which we'd have to land to m-c and Aurora as well.

Comment 8

[Andrew Swan [:aswan]]

Right, to be clear I would expect the uplift would just be a correction of the application/applications typo.

Comment 9

[Dave Townsend [:mossop]]

(In reply to Andrew Swan [:aswan] from comment #5)
> (In reply to Will Bamberg [:wbamberg] from comment #4)
> > Oh, I just tried with a built XPI and it also worked as expected. If I use
> > "Load Temporary Add-on", though, it doesn't seem to respect
> > strict_min_version. Should it?
> 
> I can't see any good reason why it shouldn't.
> I just created an extension with strict_min_version set to 49.0.0 and was
> able to install it temporarily in nightly (which is currently 48.0a1).  I
> opened the browser toolbox and ran:
> AddonManager.getAddonByID(myid, addon => console.log(addon.appDisabled))
> which displayed true, even though the addon was active!
> 
> From some quick reading of the code, it looks like the checking of
> appDisabled at install time actually happens from the front-end code (!!) 
> Specifically, the check here:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> amWebInstallListener.js#114
> which leads to canceling the install here:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> amWebInstallListener.js#148
> 
> My assumption is that we should enforce this check for temporary add-ons and
> that the proper fix would be to check within the AddonManager itself rather
> than in the UI.  But now I've chained together a few assumptions along with
> potentially misunderstanding the current implementation so double-checking
> with a higher authority.

The goal has always been that the internal AddonManager APIs are pretty unrestricted allowing any add-ons to be installed regardless of compatibility. That way the front-end gets to control the experience for the user, it may choose to allow incompatible add-ons to install so that they can automatically update to working versions later for example.

However you're right in that when an add-on is appDisabled it shouldn't be active so that is definitely broken here.

I could see two ways we might want to go here. On the one hand we might want to reject incompatible add-ons, on the other this is for testing so maybe we want to allow them and make it impossible for a temporary add-on to be appDisabled. It can still be incompatible and would show a warning about that in the add-ons manager.

Fixing the former would be a front-end fix I think. The latter we could do in XPIProvider.isUsableAddon.

Comment 10

[Andrew Swan [:aswan]]

(In reply to Dave Townsend [:mossop] from comment #9)
> The goal has always been that the internal AddonManager APIs are pretty
> unrestricted allowing any add-ons to be installed regardless of
> compatibility. That way the front-end gets to control the experience for the
> user, it may choose to allow incompatible add-ons to install so that they
> can automatically update to working versions later for example.

Okay, I'm not trying to second-guess that decision but I'm concerned that there are actually a substantial number of users of the AddonManager that will all have to do these checks (including off the top of my head: at least two different install paths in the browser front-end, add-ons that import AddonManager, the new APIs coming via bug 1255039).

> I could see two ways we might want to go here. On the one hand we might want
> to reject incompatible add-ons, on the other this is for testing so maybe we
> want to allow them and make it impossible for a temporary add-on to be
> appDisabled. It can still be incompatible and would show a warning about
> that in the add-ons manager.
> 
> Fixing the former would be a front-end fix I think. The latter we could do
> in XPIProvider.isUsableAddon.

For testing purposes, I think there's already a preference to disable compatibility checks [1] or am I misunderstanding what that is?

[1] http://kb.mozillazine.org/Extensions.checkCompatibility

This discussion is focused on temporary extensions, there's also the question about uplifting the original bug (which applied to all extensions).  Comments 7 and 8 got into mechanics, does that mean we have consensus that it would be a good thing to do if we can do so in a straightforward and non-risky way?

Comment 11

[Will Bamberg [:wbamberg]]

(In reply to Andrew Swan [:aswan] from comment #5)
> (In reply to Will Bamberg [:wbamberg] from comment #4)
> > Oh, I just tried with a built XPI and it also worked as expected. If I use
> > "Load Temporary Add-on", though, it doesn't seem to respect
> > strict_min_version. Should it?
> 
> I can't see any good reason why it shouldn't.


I just filed bug 1281884 for this.