Some validation messages for manifest syntax errors could be more accurate

[AlexandraMoga]

Describe the problem and steps to reproduce it:

Run addons-linter on an add-on that has the following syntax errors in manifest.json:

1. /browser_specific_settings/gecko/id has the below non-string format:

• first example - only letters:

"browser_specific_settings": {
    "gecko": {
      "id": foobar

• second example - numbers+letters:

"browser_specific_settings": {
    "gecko": {
      "id": 123foobar

2. /applications.gecko.strict_min_version not in string format:

"applications": {
    "gecko": {
      "strict_min_version": 55.0
    }

What happened?

The following error messages are received:

• for /browser_specific_settings/gecko/id only letters:

ERRORS:

Code           Message            Description                                  File            Line   Column
JSON_INVALID   Your JSON is not   Unexpected token o in JSON at position 463
               valid.
JSON_INVALID   Your JSON is not   Unexpected token o in JSON at position 463   manifest.json
               valid.

• for /browser_specific_settings/gecko/id numbers+letters:

ERRORS:

Code           Message            Description                        File            Line   Column
JSON_INVALID   Your JSON is not   Line 8: Unexpected token ILLEGAL
               valid.
JSON_INVALID   Your JSON is not   Line 8: Unexpected token ILLEGAL   manifest.json
               valid.

• /applications.gecko.strict_min_version not in string format: manifestJson.applications.gecko.strict_min_version.split is not a function

What did you expect to happen?

For the gecko/id errors we should probably return the '/browser_specific_settings/gecko/id" should be string' error message. For the /applications.gecko.strict_min_version error we should also have a validation message stating that this value should be in string format.

[willdurand]

I personally think the messages are good enough, it is "standard JSON error"-like. @rpl WDYT?

[rpl]

@willdurand I agree on the first two, I expect the extension developers to be able to find where the error pretty quickly based on the JSON_INVALID error reported to them.

But we should fix the last one (manifestJson.applications.gecko.strict_min_version.split is not a function) because at the moment we are throwing:

manifestJson.applications.gecko.strict_min_version.split is not a function
(node:2850623) UnhandledPromiseRejectionWarning: TypeError: manifestJson.applications.gecko.strict_min_version.split is not a function
    at firefoxStrictMinVersion (/.../addons-linter/dist/webpack:/addons-linter/src/utils.js:404:58)
    at ManifestJSONParser._validate (/.../addons-linter/dist/webpack:/addons-linter/src/parsers/manifestjson.js:550:24)
    at new ManifestJSONParser (/.../addons-linter/dist/webpack:/addons-linter/src/parsers/manifestjson.js:122:12)
    at Linter.getAddonMetadata (/.../addons-linter/dist/webpack:/addons-linter/src/linter.js:280:30)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
    at Linter.extractMetadata (/.../addons-linter/dist/webpack:/addons-linter/src/linter.js:474:25)
    at Linter.scan (/.../addons-linter/dist/webpack:/addons-linter/src/linter.js:521:7)
    at Linter.run (/.../addons-linter/dist/webpack:/addons-linter/src/linter.js:586:5)
    at /.../addons-linter/bin/addons-linter:22:5
(node:2850623) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:2850623) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

(@AlexandraMoga would you mind to collect the stack trace of the exception being raised, as in the code block right above, for the scenario that triggered one during a QA verification? that would highlight these unexpected conditions, and make them more visible in the middle of the ones reported with an actual linting error, and to get an idea of which condition did trigger the unexpected exception).

I think we should be able to prevent this exception by just adding an additional check and early return in firefoxStrictMinVersion, something like the following:

diff --git a/src/utils.js b/src/utils.js
index 219c35bd..d47dbd41 100644
--- a/src/utils.js
+++ b/src/utils.js
@@ -400,6 +400,9 @@ export function firefoxStrictMinVersion(manifestJson) {
     manifestJson.applications.gecko &&
     manifestJson.applications.gecko.strict_min_version
   ) {
+    if (typeof manifestJson.applications.gecko.strict_min_version !== "string") {
+      return null;
+    }
     return parseInt(
       manifestJson.applications.gecko.strict_min_version.split('.')[0],
       10

[AlexandraMoga]

@rpl this is the full stack trace I'm getting when I'm running addons-linter with --stack:

TypeError: manifestJson.applications.gecko.strict_min_version.split is not a function
    at firefoxStrictMinVersion (~\AppData\Roaming\npm\node_modules\addons-linter\dist\webpack:\addons-linter\src\utils.js:404:58)
    at ManifestJSONParser._validate (~\AppData\Roaming\npm\node_modules\addons-linter\dist\webpack:\addons-linter\src\parsers\manifestjson.js:550:24)
    at new ManifestJSONParser (~\AppData\Roaming\npm\node_modules\addons-linter\dist\webpack:\addons-linter\src\parsers\manifestjson.js:122:12)
    at Linter.getAddonMetadata (~\AppData\Roaming\npm\node_modules\addons-linter\dist\webpack:\addons-linter\src\linter.js:280:30)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at Linter.extractMetadata (~\AppData\Roaming\npm\node_modules\addons-linter\dist\webpack:\addons-linter\src\linter.js:474:25)
    at Linter.scan (~\AppData\Roaming\npm\node_modules\addons-linter\dist\webpack:\addons-linter\src\linter.js:521:7)
    at Linter.run (~\AppData\Roaming\npm\node_modules\addons-linter\dist\webpack:\addons-linter\src\linter.js:586:5)
    at ~\AppData\Roaming\npm\node_modules\addons-linter\bin\addons-linter:22:5
(node:14512) UnhandledPromiseRejectionWarning: TypeError: manifestJson.applications.gecko.strict_min_version.split is not a function
    at firefoxStrictMinVersion (~\AppData\Roaming\npm\node_modules\addons-linter\dist\webpack:\addons-linter\src\utils.js:404:58)
    at ManifestJSONParser._validate (~\AppData\Roaming\npm\node_modules\addons-linter\dist\webpack:\addons-linter\src\parsers\manifestjson.js:550:24)
    at new ManifestJSONParser (~\AppData\Roaming\npm\node_modules\addons-linter\dist\webpack:\addons-linter\src\parsers\manifestjson.js:122:12)
    at Linter.getAddonMetadata (~\AppData\Roaming\npm\node_modules\addons-linter\dist\webpack:\addons-linter\src\linter.js:280:30)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at Linter.extractMetadata (~\AppData\Roaming\npm\node_modules\addons-linter\dist\webpack:\addons-linter\src\linter.js:474:25)
    at Linter.scan (~\AppData\Roaming\npm\node_modules\addons-linter\dist\webpack:\addons-linter\src\linter.js:521:7)
    at Linter.run (~\AppData\Roaming\npm\node_modules\addons-linter\dist\webpack:\addons-linter\src\linter.js:586:5)
    at ~\AppData\Roaming\npm\node_modules\addons-linter\bin\addons-linter:22:5
(node:14512) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:14512) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

[AlexandraMoga]

The following error is now raised if strict_min_version is not a string:

ERRORS:

Code                     Message                 Description                                 File            Line   Column
MANIFEST_FIELD_INVALID   "/applications/gecko…   See https://mzl.la/1ZOhoEN (MDN Docs) for   manifest.json
                         /strict_min_version"    more information.
                         should be string

This issue is verified fixed with linter version 3.16.0