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

VERIFIED FIXED in Firefox 47

Status

()

Toolkit
WebExtensions: Untriaged
VERIFIED FIXED
2 years ago
6 months ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

({regression})

Trunk
mozilla47
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 verified, firefox48 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
+++ 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.
(Assignee)

Comment 1

2 years ago
Created attachment 8714539 [details]
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/
Attachment #8714539 - Flags: review?(wmccloskey)
(Assignee)

Comment 2

2 years ago
Created attachment 8714540 [details]
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/
Attachment #8714540 - Flags: review?(wmccloskey)
(Assignee)

Comment 3

2 years ago
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.
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…)
Duplicate of this bug: 1245812
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.
Attachment #8714539 - Flags: review?(wmccloskey) → review+
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.
Attachment #8714540 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 8

2 years ago
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?
(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.
(Assignee)

Comment 10

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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b8c718e8b0ff
https://hg.mozilla.org/mozilla-central/rev/6f9582d41031
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1245166
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.
Status: RESOLVED → VERIFIED
status-firefox47: fixed → verified
status-firefox48: --- → verified
(Assignee)

Updated

a year ago
Duplicate of this bug: 1274649

Comment 15

a year ago
> 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
(Assignee)

Updated

a year ago
See Also: → bug 1278815

Comment 16

a year ago
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?
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 17

a year ago
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.
Flags: needinfo?(kmaglione+bmo)

Comment 18

a year ago
Created attachment 8764558 [details]
Tampermonkey.zip

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

Is this error valid or not?
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 19

a year ago
Yes. We don't support split incognito mode.
Flags: needinfo?(kmaglione+bmo)

Comment 20

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df6b25262c65
cut off path from URLs passed to PAC scripts, r=mcmanus
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
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 22

a year ago
It looks like the commits in question have the wrong bug number.
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(daniel)
Flags: needinfo?(cbook)
Ugh, how silly of me. Yes, the "cut off path from URLs passed" commit was is for bug 1255474!
Flags: needinfo?(daniel)
(Assignee)

Updated

a year ago
Flags: needinfo?(cbook)
Keywords: regression
Version: unspecified → Trunk
(Assignee)

Updated

6 months ago
See Also: → bug 1339131
You need to log in before you can comment on or make changes to this bug.