1244474 - Unknown properties and permissions in WebExtension manifests should be warnings rather than errors

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Kris Maglione [:kmag]]

+++ This bug was initially created as a clone of Bug #1225715 +++

I want to wait until the type checking has been in nightlies for a week or so before landing this, to see what kinds of issues it turns up.

Comment 1

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1244474: [webext] Part 1 - Add "deprecated" property support to schema validator. r?billm

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

Comment 2

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1244474: [webext] Part 2 - Make extra manifest properties/permissions warnings instead of errors. r?billm

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

Comment 3

[Kris Maglione [:kmag]]

There's no rush on these. I still want to leave the stricter checks enabled to a few more days to see what bugs they turn up.

Comment 4

[Blake Winton (:bwinton) (:☕️)]

I'm seeing more and more people hit these errors, and it doesn't really leave them with any workaround, so I'ld like to suggest that there's a small rush on these…  ;)

How about we add Telemetry or UITelemetry to find out which invalid keys people are hitting instead of throwing errors and asking people to file bugs?  Or if that wasn't your intention, how about making them errors only when we're running the test suite?  (Actually, adding some UITelemetry is probably a pretty good idea whether or not that was your intention…)

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8714539 [details]
MozReview Request: Bug 1244474: [webext] Part 1 - Add "deprecated" property support to schema validator. r?billm

https://reviewboard.mozilla.org/r/33137/#review30433

::: toolkit/components/extensions/Schemas.jsm:103
(Diff revision 1)
> +    return this.params.cloneScope;

Perhaps I'm getting confused by all the overriding, but do you want to add a cloneScope property to the wrapper object in Extension.jsm?

::: toolkit/components/extensions/Schemas.jsm:148
(Diff revision 1)
> +    let { error } = this.error(message);

I think most of the destructuring we do in WebExtensions looks like |let {error} = ...|.

::: toolkit/components/extensions/Schemas.jsm:290
(Diff revision 1)
> +    if (context.logError) {

Is |context| always a |Context|? If so, won't we always have this property?

::: toolkit/components/extensions/Schemas.jsm:823
(Diff revision 1)
>    checkParameters(args, global, context) {

I think we can drop the |global| parameter now.

::: toolkit/components/extensions/Schemas.jsm:922
(Diff revision 1)
>    checkListener(global, listener, context) {

Same here about removing |global|.

::: toolkit/components/extensions/Schemas.jsm:1160
(Diff revision 1)
> -      this.register(namespaceName, name, new TypeProperty(namespaceName, name, type),
> +      this.register(namespaceName, name, new TypeProperty(prop, namespaceName, name, type, prop.writable));

Can we do prop.writable || false? I think I saw a strict mode warning about this.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8714540 [details]
MozReview Request: Bug 1244474: [webext] Part 2 - Make extra manifest properties/permissions warnings instead of errors. r?billm

https://reviewboard.mozilla.org/r/33139/#review30783

Thanks very much. I like this mechanism.

::: toolkit/components/extensions/Extension.jsm:361
(Diff revision 1)
> +        get cloneScope() {

Heh. Ignore that comment for the previous patch.

Comment 8

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/33137/#review30433

> Is |context| always a |Context|? If so, won't we always have this property?

Yeah. I think I added the fallback logError method after I wrote this bit.

> Can we do prop.writable || false? I think I saw a strict mode warning about this.

Yeah, I think I noticed that too. Does adding `|| false` hide those warnings now?

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Kris Maglione [:kmag] from comment #8)
> Yeah, I think I noticed that too. Does adding `|| false` hide those warnings
> now?

I think this is one of the strict warnings issued only by SpiderMonkey. If that's true, then using || false will suppress it.

Comment 10

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/b8c718e8b0ff4743aab61a1df10570cb1eade5e2
Bug 1244474: [webext] Part 1 - Add "deprecated" property support to schema validator. r=billm

https://hg.mozilla.org/integration/fx-team/rev/6f9582d410311502c1219aaf3371d0a0f2105a09
Bug 1244474: [webext] Part 2 - Make extra manifest properties/permissions warnings instead of errors. r=billm

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b8c718e8b0ff
https://hg.mozilla.org/mozilla-central/rev/6f9582d41031

Comment 13

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce the initial issue on Firefox 47.0a1 (2016-02-04) under Windows 10 64-bit.

Verified fixed using https://addons.mozilla.org/en-US/firefox/addon/salesforce-inspector/ on Firefox 48.0a1 (2016-04-04) and Firefox 46.0a2 (2016-04-04) under Windows 10 64-bit, Mac OS X 10.11 and Ubuntu 12.04 32-bit. The webextension is successfully installed and no errors are thrown in Browser Console.

Comment 15

[Jan Biniok]

> status-firefox47: fixed → verified
As a side note: this issue is not fixed at Firefox 47.0 Stable from 2016-06-07 or bug 1274649 was not a duplicate.

To reproduce just try to install Tampermonkey 4.1.5240 with the latest official stable build: https://addons.mozilla.org/de/firefox/addon/tampermonkey/versions/?page=1#version-4.1.5240

Comment 16

[CosminB]

I tried to install https://addons-dev.allizom.org/en-US/firefox/addon/tampermonkey/ on Firefox 50.0a1 (2016-06-22) under Windows 7 x64 and I receive this error in browser console:
  
1466675085784	addons.webextension.<unknown>	ERROR	Loading extension 'null': Reading manifest: Error processing content_scripts.0.matches.0: Value must either: be one of ["<all_urls>"], match the pattern /^(https?|file|ftp|app|\*):\/\/(\*|\*\.[^*/]+|[^*/]+)\/.*$/, or match the pattern /^file:\/\/\/.*$/
Log.jsm:753


This issue might be related to Comment 15.

Any thoughts about this error?

Comment 17

[Kris Maglione [:kmag]]

The extension is missing a / in its file: pattern. In any case, the error is not related. That manifest doesn't contain an unknown property or permission, just an invalid match pattern.

Comment 18

[CosminB]

Attachment: Tampermonkey.zip

I modified the manifest file and now I receive this error http://screencast.com/t/QWq1ae9F 

Is this error valid or not?

Comment 19

[Kris Maglione [:kmag]]

Yes. We don't support split incognito mode.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df6b25262c65
cut off path from URLs passed to PAC scripts, r=mcmanus

Comment 21

[Carsten Book [:Tomcat]]

had to back this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=9ef5260af8f32989b2a5e1d0bd9a4f9103b566c6 - because with landing of this checkin we had timeouts like https://treeherder.mozilla.org/logviewer.html#?job_id=31056460&repo=mozilla-inbound 

could you take a look at this and if its not related to your push, please set checkin-needed again. 

Sorry for the inconvenience

Comment 22

[Kris Maglione [:kmag]]

It looks like the commits in question have the wrong bug number.

Comment 23

[Daniel Stenberg [:bagder]]

Ugh, how silly of me. Yes, the "cut off path from URLs passed" commit was is for bug 1255474!