1297752 - Issue a warning when "*" is used as part of strict_min_version in a WebExtension manifest.json

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

The behavior of the compatibility checks when there is a "*" in the `strict_min_version` compatibility field of a WebExtension manifest is currently confusing to the addon developer, e.g. a WebExtension with a strict_min_version set to "48.*" is not compatible with Firefox 48.0 (nor with 48.1 etc., but it is compatible with alpha version of the next version e.g. 49.0a2).

After a brief discussion on IRC, it seems that the most reasonable change can be to prevent the "*" to be used in the script_min_version at all.

A similar issue should be filed for web-ext lint (and the addons-linter), so that we can warn the addon developer that the strict_min_version have to be fixed, way before that the addon reaches any real user (and fails to install because we raise an error for the invalid strict_min_version format in the new Firefox versions, and because of the issue describe above on the older Firefox versions).

Comment 1

[Andy McKay]

Added https://github.com/mozilla/addons-linter/issues/914

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi Jorge,
how it looks to you the above plan?

Feel free to add any additonal info/suggestions, this is still in a brainstorming phase, but we are pretty sure that it is something that we need to fix, and, at the same time, to be very very careful about how we are going to release the fix.

Comment 3

[Andreas Wagner [:TheOne] [use NI]]

Another idea would be to fix the comparator to return the expected output when you compare 48.0 vs 48.*. Although it can have a bit more impact, I think it's reasonable to do. Especially because we will run into the issue again for temporary add-ons due to bug 1281884.

Comment 4

[Kris Maglione [:kmag]]

I don't think '.*' makes sense in min versions, so I'd rather forbid it than try to reverse the way that it currently works.

Comment 5

[Andreas Wagner [:TheOne] [use NI]]

OK, but how are we going to solve the problem for temporary add-ons?

Comment 6

[Kris Maglione [:kmag]]

(In reply to Andreas Wagner [:TheOne] from comment #5)
> OK, but how are we going to solve the problem for temporary add-ons?

What problem, exactly?

Comment 7

[Andreas Wagner [:TheOne] [use NI]]

(In reply to Kris Maglione [:kmag] from comment #6)
> (In reply to Andreas Wagner [:TheOne] from comment #5)
> > OK, but how are we going to solve the problem for temporary add-ons?
> 
> What problem, exactly?

I suspect that with bug 1281884 fixed, an add-on having a strict_min_version of 48.* for example will not be able to get temporarily loaded in 48.

Comment 8

[Kris Maglione [:kmag]]

(In reply to Andreas Wagner [:TheOne] from comment #7)
> I suspect that with bug 1281884 fixed, an add-on having a strict_min_version
> of 48.* for example will not be able to get temporarily loaded in 48.

Well, given that neither of those changes is going to be uplifted to release, I don't see how it could possibly be an issue. Even if they were, I don't see why it would be an issue for temporary add-ons in particular.

Comment 9

[Andreas Wagner [:TheOne] [use NI]]

The numbers were just an example. I think we're going to have that problem in whatever release both bug 1281884 and this bug a re fixed.

So, when loading temporary add-ons is going to respect 'strict_min_version' and (example number) 55.* will be considered a higher version than 55.0, an add-on with 'strict_min_version' : '55.*', will refuse to load in 55.0 because Firefox says it's incompatible.

Where am I wrong?

Comment 10

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Luca Greco [:rpl] from comment #2)
> how it looks to you the above plan?

That looks good to me, to keep consistency with how we treat other add-ons.

If about:debugging is somehow bypassing the compatibility metadata, like Andreas is suggesting, there should be some visible error so developers know the add-on won't work correctly when installed normally.

Comment 11

[Kris Maglione [:kmag]]

(In reply to Andreas Wagner [:TheOne] from comment #9)
> So, when loading temporary add-ons is going to respect 'strict_min_version'
> and (example number) 55.* will be considered a higher version than 55.0, an
> add-on with 'strict_min_version' : '55.*', will refuse to load in 55.0
> because Firefox says it's incompatible.

It will refuse to load anywhere where we reject .* in min versions. But I don't see what it has to do with temporary add-ons in particular. If anything, I'd be less worried about that case than about the permanent install case.

Comment 12

[:shell escalante]

2 parts: firefox warning uplift to beta, rejection that ride trains 

andy will file the github issue (that's the need info)

Comment 13

[Andy McKay]

Done in comment 1

Comment 14

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1297752 - Issue a warning when "*" is used as part of strict_min_version in a WebExtension manifest.json,

Review commit: https://reviewboard.mozilla.org/r/75024/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75024/

Comment 15

[Kris Maglione [:kmag]]

Comment on attachment 8786062 [details]
Bug 1297752 - Issue a warning when "*" is used as part of strict_min_version in a WebExtension manifest.json,

https://reviewboard.mozilla.org/r/75024/#review73294

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:369
(Diff revision 1)
> +    newId = "[e-mail]";
> +    apps = {

`let`

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:378
(Diff revision 1)
> +          id: newId,
> +          strict_min_version: version,
> +        },
> +      },
> +    }
> +    testManifest = Object.assign(apps, MANIFEST);

`let`

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:380
(Diff revision 1)
> +        },
> +      },
> +    }
> +    testManifest = Object.assign(apps, MANIFEST);
> +
> +    addonDir = yield promiseWriteWebManifestForExtension(testManifest, gTmpD,

`let`

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:389
(Diff revision 1)
> +      yield AddonManager.installTemporaryAddon(addonDir);
> +    });
> +    ok(messages.some(msg => msg.message.includes("The use of '*' in strict_min_version is deprecated")),
> +       "Deprecation warning for strict_min_version with '*' was generated");
> +
> +    yield AddonManager.installTemporaryAddon(addonDir);

No need to do this twice.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:390
(Diff revision 1)
> +    });
> +    ok(messages.some(msg => msg.message.includes("The use of '*' in strict_min_version is deprecated")),
> +       "Deprecation warning for strict_min_version with '*' was generated");
> +
> +    yield AddonManager.installTemporaryAddon(addonDir);
> +    addon = yield promiseAddonByID(newId);

`let`

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:392
(Diff revision 1)
> +    do_check_neq(addon, null);
> +    do_check_eq(addon.id, newId);

Please use `equal` and `notEqual` in new code, and always include an assertion message.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:395
(Diff revision 1)
> +    addon = yield promiseAddonByID(newId);
> +
> +    do_check_neq(addon, null);
> +    do_check_eq(addon.id, newId);
> +
> +    addon.uninstall();

Hum. There should really be a `promiseExtensionStartup` any time we install an extension, but it doesn't look like any of the other tests in this file do it either...

Comment 16

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8786062 [details]
Bug 1297752 - Issue a warning when "*" is used as part of strict_min_version in a WebExtension manifest.json,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75024/diff/1-2/

Comment 17

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8786062 [details]
Bug 1297752 - Issue a warning when "*" is used as part of strict_min_version in a WebExtension manifest.json,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75024/diff/2-3/

Comment 18

[Bob Silverberg [:bsilverberg]]

Try looks good. Requesting checkin.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fffb67d17a59
Issue a warning when "*" is used as part of strict_min_version in a WebExtension manifest.json. r=kmag

Comment 20

[Andy McKay]

Will also be blocked in the validator in the next AMO push.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fffb67d17a59

Comment 22

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f9a4af16ace

Comment 23

[Bob Silverberg [:bsilverberg]]

Attachment: Patch for uplift to beta

Comment 24

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=194cdc6974c9

Comment 25

[Bob Silverberg [:bsilverberg]]

Attachment: Patch for uplift to beta

Comment 26

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8787722 [details] [diff] [review]
Patch for uplift to beta

Approval Request Comment
[Feature/regressing bug #]: 1297752
[User impact if declined]: This is a new warning being issued for a deprecated feature. Our plan is to implement this as a warning in beta, and promote it to an error in nightly. Without this uplift extension developers will not have an opportunity to see this warning before it becomes an error in a later version of Firefox.
[Describe test coverage new/current, TreeHerder]: This patch comes with its own test, and seems to have passed on try against beta: https://treeherder.mozilla.org/#/jobs?repo=try&revision=194cdc6974c9 
[Risks and why]: This is a simple change in a single file, which only introduces a warning. It also comes with a test. It therefore can be considered to be low risk.
[String/UUID change made/needed]: None

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

I think you mean for 50 (still aurora right now).

Comment 28

[Bob Silverberg [:bsilverberg]]

Answering Liz's questions from IRC:

Q. Why do we need this?
A. Firefox's behaviour when one specifies an asterisk as part of a strict_min_version is counter-intuitive and confusing, and it doesn't really make sense to use an asterisk when specifying a min version. We want to eliminate the confusion by forbidding the use of an asterisk in strict_min_version. The long term solution is to forbid it and generate an error, but to give developers a bit of lead time to deal with it, we want to start by just issuing a warning. This patch adds the warning and we'd like it to land on beta to get the warning out to users. Another patch will land on nightly with the error, so that will give users a couple of releases before the warning turns into an error.

Q. How do we test it?
A. Create a web extension with a manifest in which strict_min_version [1] includes an asterisk (e.g., 1.0.* or 1.*.1). When you load the web extension you should see the warning in the console.

Please let me know if you have any other questions. 

[1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/applications

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

We are about to release 49. Do you need this in the 49 release that should happen next Tuesday? My question is because in comments you mention wanting this on beta. We are already building the release candidate.

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

My point here is that I don't want to uplift anything else for 49 at the last minute. Users on beta would only see this for about 2 days. So it seems pointless to add more risk to the actual release for a couple of days of possible warning. I'm thinking devs can get the warning in beta 50.  Seem reasonable?

Comment 31

[Bob Silverberg [:bsilverberg]]

Kris, can you reply to Liz please? I think we do want this to go out for 49, but I'm not totally confident in that assertion.

Comment 32

[Kris Maglione [:kmag]]

I'd still like to see it in 49, since it will get more attention from developers on release than on other channels. The patch is extremely simple, so I don't think it's a serious risk. But if we can't get it into 49, we should probably push the hard error back a release

Comment 33

[Kris Maglione [:kmag]]

If we're already building the RC, this probably isn't urgent enough to uplift unless we do a second RC.

Comment 34

[Liz Henry (:lizzard) (relman/hg->git project)]

Let's give it a shot then, I can push back the build to a bit later this afternoon.

Comment 35

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8787722 [details] [diff] [review]
Patch for uplift to beta

[Triage Comment]

Add a warning for webextensions developers. 
Andrei, can your team test once we have RC2 builds? 
Or, Andy, is this something for the Addons team to test?

Comment 36

[Kris Maglione [:kmag]]

Add-ons QA should test this.

Krupa, can you or someone from QA test this in the next RC?

The following should trigger warnings in applications.gecko.strict_min_version, but not strict_max_version:

 - "*"
 - "48.*"
 - "48.*.0"

The following should *not* trigger warnings:

 - "48"
 - "48.*b2"
 - "48.0*"

Missing or null strict_min_version keys should also not trigger any warnings or errors.

Comment 37

[krupa raj [:krupa--use this to needinfo]]

Yes, on it.

Comment 38

[krupa raj [:krupa--use this to needinfo]]

When do you guys need this tested by?

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/b8ea232286b1
https://hg.mozilla.org/releases/mozilla-release/rev/b8ea232286b1

Comment 40

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4b4cc630c399

Comment 41

[Bob Silverberg [:bsilverberg]]

Thanks everyone for your help getting this landed and uplifted. :)

Comment 42

[Vasilica Tamas, QA [:vtamas]]

We confirm that the following warning appears on Firefox 49 RC unbranded build (https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-release-win64-add-on-devel/1473287416/) and Firefox 51.0a1 (2016-09-07/08) under Windows 10 64-bit, Ubuntu 12.04/14.04 64-bit and Mac OS X 10.10.4: “1473342800550	addons.xpi	WARN	The use of '*' in strict_min_version is deprecated”. Tomorrow we will also cover Firefox 50.0a2.

We tested as Kris suggested in Comment 36 and the testing results are available via: http://goo.gl/VEaXG3

We also noticed that the following invalid strict_min_version value is accepted on install: “abc.*” . Any thoughts about this?

Comment 43

[Kris Maglione [:kmag]]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #42)
> We also noticed that the following invalid strict_min_version value is
> accepted on install: “abc.*” . Any thoughts about this?

I tested with this value as a minimum version, and it emitted a warning as expected. Can you provide more details?

Comment 44

[Vasilica Tamas, QA [:vtamas]]

We also confirm that this issue is verified as fixed on FIrefox 50.0a1 (2016-09-08) under Windows 10 64-bit, Ubuntu 14.04 64-bit and Mac OS X 10.10.4. Testing details available via: http://goo.gl/VmavWb


(In reply to Kris Maglione [:kmag] from comment #43)
> I tested with this value as a minimum version, and it emitted a warning as
> expected. Can you provide more details?

The warning is triggered also for this strict_min_version = “abc.*”, but I was referring to its actually invalid value, because Firefox doesn’t have an “abc” version. 

Also there is no way to submit such an add-on that contains this strict_min_version because the platforms checkboxes are automatically disabled on amo(even an error would be more useful for a beginner - for which I’ve already file a Github bug - https://github.com/mozilla/addons-linter/issues/931). 


So the question is, should this invalid value be accepted at install or the installation should fail (same as for these values: “*”, “*.*”)?

Comment 45

[Kris Maglione [:kmag]]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #44)
> (In reply to Kris Maglione [:kmag] from comment #43)
> > I tested with this value as a minimum version, and it emitted a warning as
> > expected. Can you provide more details?
> 
> The warning is triggered also for this strict_min_version = “abc.*”, but I
> was referring to its actually invalid value, because Firefox doesn’t have an
> “abc” version. 

That's expected. It's technically a valid version string, and Firefox doesn't
keep a list of known releases to check it against. AMO should reject it as an
unknown version, though.