Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Support custom min/maxVersion Web Extensions

RESOLVED FIXED

Status

()

Toolkit
WebExtensions: Untriaged
P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: mossop, Assigned: kmag)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox42 affected)

Details

(Reporter)

Description

2 years ago
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.

Updated

2 years ago
Blocks: 1214433
Priority: -- → P3

Updated

2 years ago
Flags: blocking-webextensions+
(Assignee)

Comment 1

2 years ago
Looks like I accidentally fixed this already.
Assignee: nobody → kmaglione+bmo
Depends on: 1192435
(Assignee)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
This does not seem to be fixed: https://github.com/mdn/webextensions-examples/issues/48.
(Assignee)

Comment 3

a year ago
I just tested on the latest Aurora, and it works as expected for me.
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?
(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.
Flags: needinfo?(dtownsend)
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?
(Assignee)

Comment 7

a year ago
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.
Right, to be clear I would expect the uplift would just be a correction of the application/applications typo.
(Reporter)

Comment 9

a year ago
(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.
Flags: needinfo?(dtownsend)
(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?
(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.
You need to log in before you can comment on or make changes to this bug.