Closed Bug 1196283 Opened 9 years ago Closed 8 years ago

Support comments in manifest.json file

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox43 affected, firefox48 verified)

VERIFIED FIXED
mozilla48
Tracking Status
firefox43 --- affected
firefox48 --- verified

People

(Reporter: mossop, Assigned: kmag)

References

Details

Attachments

(1 file)

Apparently chrome supports comments in the manifest.json file, something that isn't part of the JSON spec and so our JSON parser rejects the file.
I think comment support is intentional.

Chrome documentation[1] contains an example manifest with comments.

[1] https://developer.chrome.com/extensions/manifest
I'm going to WONTFIX this. We're not going to be bug-for-bug compatible with Chrome.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
You call it bug, I call it feature.
It looks like this is going to be standardized across browsers, so we're going to have to support it.
Assignee: nobody → kmaglione+bmo
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
We will need amo-validator or amo-linter support for this as well.
Comment on attachment 8742989 [details]
MozReview Request: Bug 1196283: [webext] Support comments in JSON files. r?aswan

https://reviewboard.mozilla.org/r/47511/#review44313

::: toolkit/components/extensions/Extension.jsm:107
(Diff revision 1)
>    flushJarCache,
>  } = ExtensionUtils;
>  
>  const LOGGER_ID_BASE = "addons.webextension.";
>  
> +const COMMENT_REGEXP = new RegExp(String.raw`

Oh the perils of doing parsing with regular expressions.  I think this technique fails on:
```
{
  "foo": "tricky\" // ",
  ...
}
```
That does seem awfully obscure, and living with it sound preferrable to actually writing a parser.
Attachment #8742989 - Flags: review?(aswan)
https://reviewboard.mozilla.org/r/47511/#review44313

> Oh the perils of doing parsing with regular expressions.  I think this technique fails on:
> ```
> {
>   "foo": "tricky\" // ",
>   ...
> }
> ```
> That does seem awfully obscure, and living with it sound preferrable to actually writing a parser.

Nope, it works as expected there. I was actually considering updating the patch so that it explicitly tested for it.
(In reply to Andy McKay [:andym] from comment #7)
> We will need amo-validator or amo-linter support for this as well.

This is dated, I don't know if they've fixed it but Chrome didn't let lack of support in their store stop them from shipping this originally: http://stackoverflow.com/questions/14151058/manifest-file-is-invalid-when-uploading-my-chrome-extension-to-the-chrome-web
https://reviewboard.mozilla.org/r/47511/#review44321

::: toolkit/components/extensions/Extension.jsm:107
(Diff revision 1)
>    flushJarCache,
>  } = ExtensionUtils;
>  
>  const LOGGER_ID_BASE = "addons.webextension.";
>  
> +const COMMENT_REGEXP = new RegExp(String.raw`

Eh, I added that line to the test and it threw a SyntaxError in JSON.parse().  And this is from node but ought to be the same here:
```
> const COMMENT_REGEXP = new RegExp(String.raw`
...     ^
...     (
.....       (?:
.......         [^"] |
.......         " (?:[^"\\] | \\.)* "
.......       )*?
.....     )
...     // .*
...   `.replace(/\s+/g, ""), "gm");
undefined
> let s = '"foo": "tricky\" // "';
undefined
> s.replace(COMMENT_REGEXP, "$1")
'"foo": "tricky" '
```
(In reply to Andrew Swan [:aswan] from comment #10)
> This is dated, I don't know if they've fixed it but Chrome didn't let lack
> of support in their store stop them from shipping this originally:
> http://stackoverflow.com/questions/14151058/manifest-file-is-invalid-when-
> uploading-my-chrome-extension-to-the-chrome-web

I'm not suggesting it block, but if we don't file the relevant bugs, it will never happen :)
https://reviewboard.mozilla.org/r/47511/#review44321

> Eh, I added that line to the test and it threw a SyntaxError in JSON.parse().  And this is from node but ought to be the same here:
> ```
> > const COMMENT_REGEXP = new RegExp(String.raw`
> ...     ^
> ...     (
> .....       (?:
> .......         [^"] |
> .......         " (?:[^"\\] | \\.)* "
> .......       )*?
> .....     )
> ...     // .*
> ...   `.replace(/\s+/g, ""), "gm");
> undefined
> > let s = '"foo": "tricky\" // "';
> undefined
> > s.replace(COMMENT_REGEXP, "$1")
> '"foo": "tricky" '
> ```

That's because the backslash in your string literal isn't escaped.

    const COMMENT_REGEXP = new RegExp(String.raw`
        ^
        (
          (?:
            [^"] |
            " (?:[^"\\] | \\.)* "
          )*?
        )
        // .*
      `.replace(/\s+/g, ""), "gm");
    
    print((String.raw`"foo \" // bar"`).replace(COMMENT_REGEXP, "$1"));

Result: "foo \" // bar"
Comment on attachment 8742989 [details]
MozReview Request: Bug 1196283: [webext] Support comments in JSON files. r?aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47511/diff/1-2/
Attachment #8742989 - Flags: review?(aswan)
Comment on attachment 8742989 [details]
MozReview Request: Bug 1196283: [webext] Support comments in JSON files. r?aswan

https://reviewboard.mozilla.org/r/47511/#review44329

I think a test that includes an escaped quote inside a string would be a decent addition but looks good.

::: toolkit/components/extensions/Extension.jsm:115
(Diff revision 1)
> +      (?:
> +        [^"] |
> +        " (?:[^"\\] | \\.)* "
> +      )*?
> +    )
> +    // .*

The space here threw me before I got to the `replace()` below.  Maybe remove it for clarity?
Attachment #8742989 - Flags: review?(aswan) → review+
From a quick&dirty test it looks like Chrome also supports /* */ comments.
Looking forward to seeing what sort of regexp Kris comes up with for those :-)
(In reply to Andrew Swan [:aswan] from comment #16)
> From a quick&dirty test it looks like Chrome also supports /* */ comments.
> Looking forward to seeing what sort of regexp Kris comes up with for those
> :-)

I could, but the consensus so far seems to be that the webextension spec will only allow for // comments.
(In reply to Andy McKay [:andym] from comment #7)
> We will need amo-validator or amo-linter support for this as well.

Since writing a custom parser is not a particularly good idea I wanted to share a simple esprima hack I used here:
https://github.com/abarreir/crx2ff/blob/d2b882056f902d751ad05e329efda7eddcb9d268/libs/ext-converter.js#L19-L37
https://hg.mozilla.org/mozilla-central/rev/036c763dedca
Status: REOPENED → RESOLVED
Closed: 9 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
FYI: The linter has landed support for comments, but only line comments; block-level comments are an error that will fail validation on AMO.

https://github.com/mozilla/addons-linter/pull/724
I was able to reproduce this issue using the following webextension https://addons-dev.allizom.org/en-US/firefox/addon/tab-wrangler-stage1/ on Firefox 47 (20160604131506) under Windows 10 64-bit.

The webextension which contains comments in manifest.json file is successfully installed on Firefox 48 beta 3 (20160623122823) using Windows 10 64-bit and Ubuntu 16.04 32-bit.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.