Bug 1271785: [webext] Better error messages for "choices" schema types. r=aswan

The generated messages are still a bit rough in some instances, but they're at
least much better than what we have now.

MozReview-Commit-ID: gTS0RvDnwk

--HG--
rename : toolkit/components/extensions/test/xpcshell/.eslintrc => browser/components/extensions/test/xpcshell/.eslintrc
rename : toolkit/components/extensions/test/xpcshell/head.js => browser/components/extensions/test/xpcshell/head.js
rename : toolkit/components/extensions/test/xpcshell/test_ext_manifest_content_security_policy.js => browser/components/extensions/test/xpcshell/test_ext_manifest_commands.js
rename : toolkit/components/extensions/test/xpcshell/xpcshell.ini => browser/components/extensions/test/xpcshell/xpcshell.ini
extra : rebase_source : 058603a2ab0efa90d3626b82be6d5e2cf1436fc3
extra : amend_source : 3a7924ef237cb392dd2f4c1f696b8858daad43ab

browser/components/extensions/moz.build

@@ -9,3 +9,4 @@ JAR_MANIFESTS += ['jar.mn']
 DIRS += ['schemas']
 
 BROWSER_CHROME_MANIFESTS += ['test/browser/browser.ini']
+XPCSHELL_TESTS_MANIFESTS += ['test/xpcshell/xpcshell.ini']

browser/components/extensions/test/browser/browser_ext_browserAction_pageAction_icon.js

@@ -393,7 +393,7 @@ add_task(function* testSecureManifestURLsDenied() {
       info(`TEST ${api} icon url: ${url}`);
 
       let matchURLForbidden = url => ({
-        message: new RegExp(`String "${url}" must be a relative URL`),
+        message: new RegExp(`match the format "strictRelativeUrl"`),
       });
 
       let messages = [matchURLForbidden(url)];

browser/components/extensions/test/xpcshell/.eslintrc

@@ -0,0 +1,3 @@
+{
+  "extends": "../../../../../testing/xpcshell/xpcshell.eslintrc",
+}

browser/components/extensions/test/xpcshell/head.js

@@ -0,0 +1,55 @@
+"use strict";
+
+const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
+
+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
+
+XPCOMUtils.defineLazyModuleGetter(this, "Extension",
+                                  "resource://gre/modules/Extension.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "ExtensionData",
+                                  "resource://gre/modules/Extension.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
+                                  "resource://gre/modules/NetUtil.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "Schemas",
+                                  "resource://gre/modules/Schemas.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "Services",
+                                  "resource://gre/modules/Services.jsm");
+
+Cu.import("resource://gre/modules/ExtensionManagement.jsm");
+
+/* exported normalizeManifest */
+
+let BASE_MANIFEST = {
+  "applications": {"gecko": {"id": "test@web.ext"}},
+
+  "manifest_version": 2,
+
+  "name": "name",
+  "version": "0",
+};
+
+ExtensionManagement.registerSchema("chrome://browser/content/schemas/commands.json");
+
+function* normalizeManifest(manifest, baseManifest = BASE_MANIFEST) {
+  const {Management} = Cu.import("resource://gre/modules/Extension.jsm", {});
+
+  yield Management.lazyInit();
+
+  let errors = [];
+  let context = {
+    url: null,
+
+    logError: error => {
+      errors.push(error);
+    },
+
+    preprocessors: {},
+  };
+
+  manifest = Object.assign({}, baseManifest, manifest);
+
+  let normalized = Schemas.normalize(manifest, "manifest.WebExtensionManifest", context);
+  normalized.errors = errors;
+
+  return normalized;
+}

browser/components/extensions/test/xpcshell/test_ext_manifest_commands.js

@@ -0,0 +1,24 @@
+/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
+/* vim: set sts=2 sw=2 et tw=80: */
+"use strict";
+
+
+add_task(function* test_manifest_commands() {
+  let normalized = yield normalizeManifest({
+    "commands": {
+      "toggle-feature": {
+        "suggested_key": {"default": "Shifty+Y"},
+        "description": "Send a 'toggle-feature' event to the extension",
+      },
+    },
+  });
+
+  let expectedError = (
+    String.raw`commands.toggle-feature.suggested_key.default: Value must either: ` +
+    String.raw`match the pattern /^\s*(Alt|Ctrl|Command|MacCtrl)\s*\+\s*(Shift\s*\+\s*)?([A-Z0-9]|Comma|Period|Home|End|PageUp|PageDown|Space|Insert|Delete|Up|Down|Left|Right)\s*$/, or ` +
+    String.raw`match the pattern /^(MediaNextTrack|MediaPlayPause|MediaPrevTrack|MediaStop)$/`
+  );
+
+  ok(normalized.error.includes(expectedError),
+     `The manifest error ${JSON.stringify(normalized.error)} must contain ${JSON.stringify(expectedError)}`);
+});

browser/components/extensions/test/xpcshell/xpcshell.ini

@@ -0,0 +1,6 @@
+[DEFAULT]
+head = head.js
+tail =
+firefox-appdir = browser
+
+[test_ext_manifest_commands.js]

toolkit/components/extensions/Schemas.jsm

@@ -93,6 +93,9 @@ class Context {
       },
     };
 
+    this.currentChoices = new Set();
+    this.choicePathIndex = 0;
+
     let methods = ["addListener", "callFunction",
                    "callFunctionNoReturn", "callAsyncFunction",
                    "hasListener", "removeListener",
@@ -116,6 +119,11 @@ class Context {
     }
   }
 
+  get choicePath() {
+    let path = this.path.slice(this.choicePathIndex);
+    return path.join(".");
+  }
+
   get cloneScope() {
     return this.params.cloneScope;
   }
@@ -146,14 +154,33 @@ class Context {
    * If the context has a `currentTarget` value, this is prepended to
    * the message to indicate the location of the error.
    *
-   * @param {string} message
+   * @param {string} errorMessage
+   *        The error message which will be displayed when this is the
+   *        only possible matching schema.
+   * @param {string} choicesMessage
+   *        The message describing the valid what constitutes a valid
+   *        value for this schema, which will be displayed when multiple
+   *        schema choices are available and none match.
+   *
+   *        A caller may pass `null` to prevent a choice from being
+   *        added, but this should *only* be done from code processing a
+   *        choices type.
    * @returns {object}
    */
-  error(message) {
-    if (this.currentTarget) {
-      return {error: `Error processing ${this.currentTarget}: ${message}`};
+  error(errorMessage, choicesMessage = undefined) {
+    if (choicesMessage !== null) {
+      let {choicePath} = this;
+      if (choicePath) {
+        choicesMessage = `.${choicePath} must ${choicesMessage}`;
+      }
+
+      this.currentChoices.add(choicesMessage);
     }
-    return {error: message};
+
+    if (this.currentTarget) {
+      return {error: `Error processing ${this.currentTarget}: ${errorMessage}`};
+    }
+    return {error: errorMessage};
   }
 
   /**
@@ -199,6 +226,43 @@ class Context {
     return this.path.join(".");
   }
 
+  /**
+   * Executes the given callback, and returns an array of choice strings
+   * passed to {@see #error} during its execution.
+   *
+   * @param {function} callback
+   * @returns {object}
+   *          An object with a `result` property containing the return
+   *          value of the callback, and a `choice` property containing
+   *          an array of choices.
+   */
+  withChoices(callback) {
+    let {currentChoices, choicePathIndex} = this;
+
+    let choices = new Set();
+    this.currentChoices = choices;
+    this.choicePathIndex = this.path.length;
+
+    try {
+      let result = callback();
+
+      return {result, choices: Array.from(choices)};
+    } finally {
+      this.currentChoices = currentChoices;
+      this.choicePathIndex = choicePathIndex;
+
+      choices = Array.from(choices);
+      if (choices.length == 1) {
+        currentChoices.add(choices[0]);
+      } else if (choices.length) {
+        let n = choices.length - 1;
+        choices[n] = `or ${choices[n]}`;
+
+        this.error(null, `must either [${choices.join(", ")}]`);
+      }
+    }
+  }
+
   /**
    * Appends the given component to the `currentTarget` path to indicate
    * that it is being processed, calls the given callback function, and
@@ -418,7 +482,16 @@ class Type extends Entry {
       this.checkDeprecated(context, value);
       return {value: this.preprocess(value, context)};
     }
-    return context.error(`Expected ${type} instead of ${JSON.stringify(value)}`);
+
+    let choice;
+    if (/^[aeiou]/.test(type)) {
+      choice = `be an ${type} value`;
+    } else {
+      choice = `be a ${type} value`;
+    }
+
+    return context.error(`Expected ${type} instead of ${JSON.stringify(value)}`,
+                         choice);
   }
 }
 
@@ -451,19 +524,30 @@ class ChoiceType extends Type {
     this.checkDeprecated(context, value);
 
     let error;
-
-    let baseType = getValueBaseType(value);
-    for (let choice of this.choices) {
-      if (choice.checkBaseType(baseType)) {
+    let {choices, result} = context.withChoices(() => {
+      for (let choice of this.choices) {
         let r = choice.normalize(value, context);
         if (!r.error) {
           return r;
         }
-        error = r.error;
+
+        error = r;
       }
+    });
+
+    if (result) {
+      return result;
+    }
+    if (choices.length <= 1) {
+      return error;
     }
 
-    return context.error(error || `Unexpected value ${JSON.stringify(value)}`);
+    let n = choices.length - 1;
+    choices[n] = `or ${choices[n]}`;
+
+    let message = `Value must either: ${choices.join(", ")}`;
+
+    return context.error(message, null);
   }
 
   checkBaseType(baseType) {
@@ -521,25 +605,32 @@ class StringType extends Type {
       if (this.enumeration.includes(value)) {
         return {value};
       }
-      return context.error(`Invalid enumeration value ${JSON.stringify(value)}`);
+
+      let choices = this.enumeration.map(JSON.stringify).join(", ");
+
+      return context.error(`Invalid enumeration value ${JSON.stringify(value)}`,
+                           `be one of [${choices}]`);
     }
 
     if (value.length < this.minLength) {
-      return context.error(`String ${JSON.stringify(value)} is too short (must be ${this.minLength})`);
+      return context.error(`String ${JSON.stringify(value)} is too short (must be ${this.minLength})`,
+                           `be longer than ${this.minLength}`);
     }
     if (value.length > this.maxLength) {
-      return context.error(`String ${JSON.stringify(value)} is too long (must be ${this.maxLength})`);
+      return context.error(`String ${JSON.stringify(value)} is too long (must be ${this.maxLength})`,
+                           `be shorter than ${this.maxLength}`);
     }
 
     if (this.pattern && !this.pattern.test(value)) {
-      return context.error(`String ${JSON.stringify(value)} must match ${this.pattern}`);
+      return context.error(`String ${JSON.stringify(value)} must match ${this.pattern}`,
+                           `match the pattern ${this.pattern.toSource()}`);
     }
 
     if (this.format) {
       try {
         r.value = this.format(r.value, context);
       } catch (e) {
-        return context.error(String(e));
+        return context.error(String(e), `match the format "${this.format.name}"`);
       }
     }
 
@@ -603,7 +694,8 @@ class ObjectType extends Type {
       }
 
       if (!instanceOf(value, this.isInstanceOf)) {
-        return context.error(`Object must be an instance of ${this.isInstanceOf}`);
+        return context.error(`Object must be an instance of ${this.isInstanceOf}`,
+                             `be an instance of ${this.isInstanceOf}`);
       }
 
       // This is kind of a hack, but we can't normalize things that
@@ -621,7 +713,8 @@ class ObjectType extends Type {
 
     let klass = Cu.getClassName(value, true);
     if (klass != "Object") {
-      return context.error(`Expected a plain JavaScript object, got a ${klass}`);
+      return context.error(`Expected a plain JavaScript object, got a ${klass}`,
+                           `be a plain JavaScript object`);
     }
 
     let properties = Object.create(null);
@@ -632,7 +725,8 @@ class ObjectType extends Type {
       for (let prop of Object.getOwnPropertyNames(waived)) {
         let desc = Object.getOwnPropertyDescriptor(waived, prop);
         if (desc.get || desc.set) {
-          return context.error("Objects cannot have getters or setters on properties");
+          return context.error("Objects cannot have getters or setters on properties",
+                               "contain no getter or setter properties");
         }
         if (!desc.enumerable) {
           // Chrome ignores non-enumerable properties.
@@ -648,7 +742,8 @@ class ObjectType extends Type {
       let {type, optional, unsupported} = propType;
       if (unsupported) {
         if (prop in properties) {
-          return context.error(`Property "${prop}" is unsupported by Firefox`);
+          return context.error(`Property "${prop}" is unsupported by Firefox`,
+                               `not contain an unsupported "${prop}" property`);
         }
       } else if (prop in properties) {
         if (optional && (properties[prop] === null || properties[prop] === undefined)) {
@@ -663,7 +758,8 @@ class ObjectType extends Type {
         }
         remainingProps.delete(prop);
       } else if (!optional) {
-        return context.error(`Property "${prop}" is required`);
+        return context.error(`Property "${prop}" is required`,
+                             `contain the required "${prop}" property`);
       } else {
         result[prop] = null;
       }
@@ -706,9 +802,12 @@ class ObjectType extends Type {
         result[prop] = r.value;
       }
     } else if (remainingProps.size == 1) {
-      return context.error(`Unexpected property "${[...remainingProps]}"`);
+      return context.error(`Unexpected property "${[...remainingProps]}"`,
+                           `not contain an unexpected "${[...remainingProps]}" property`);
     } else if (remainingProps.size) {
-      return context.error(`Unexpected properties: ${[...remainingProps]}`);
+      let props = [...remainingProps].sort().join(", ");
+      return context.error(`Unexpected properties: ${props}`,
+                           `not contain the unexpected properties [${props}]`);
     }
 
     return {value: result};
@@ -732,7 +831,8 @@ class NumberType extends Type {
     }
 
     if (isNaN(r.value) || !Number.isFinite(r.value)) {
-      return context.error("NaN or infinity are not valid");
+      return context.error("NaN and infinity are not valid",
+                           "be a finite number");
     }
 
     return r;
@@ -759,14 +859,17 @@ class IntegerType extends Type {
 
     // Ensure it's between -2**31 and 2**31-1
     if (!Number.isSafeInteger(value)) {
-      return context.error("Integer is out of range");
+      return context.error("Integer is out of range",
+                           "be a valid 32 bit signed integer");
     }
 
     if (value < this.minimum) {
-      return context.error(`Integer ${value} is too small (must be at least ${this.minimum})`);
+      return context.error(`Integer ${value} is too small (must be at least ${this.minimum})`,
+                           `be at least ${this.minimum}`);
     }
     if (value > this.maximum) {
-      return context.error(`Integer ${value} is too big (must be at most ${this.maximum})`);
+      return context.error(`Integer ${value} is too big (must be at most ${this.maximum})`,
+                           `be no greater than ${this.maximum}`);
     }
 
     return r;
@@ -812,11 +915,13 @@ class ArrayType extends Type {
     }
 
     if (result.length < this.minItems) {
-      return context.error(`Array requires at least ${this.minItems} items; you have ${result.length}`);
+      return context.error(`Array requires at least ${this.minItems} items; you have ${result.length}`,
+                           `have at least ${this.minItems} items`);
     }
 
     if (result.length > this.maxItems) {
-      return context.error(`Array requires at most ${this.maxItems} items; you have ${result.length}`);
+      return context.error(`Array requires at most ${this.maxItems} items; you have ${result.length}`,
+                           `have at most ${this.maxItems} items`);
     }
 
     return {value: result};

toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html

@@ -239,19 +239,19 @@ function backgroundScript() {
     Promise.resolve().then(() => {
       return browser.bookmarks.search({banana: "banana"});
     }).then(expectedError, error => {
+      let substr = `an unexpected "banana" property`;
       browser.test.assertTrue(
-        error.message.includes(`Unexpected property "banana"`),
-        "Expected error thrown when trying to search with an invalid property as an argument"
-      );
+        error.message.includes(substr),
+        `Expected error ${JSON.stringify(error.message)} to contain ${JSON.stringify(substr)}`);
     });
 
     Promise.resolve().then(() => {
       return browser.bookmarks.search({url: "spider-man vs. batman"});
     }).then(expectedError, error => {
+      let substr = 'must match the format "url"';
       browser.test.assertTrue(
-        error.message.includes("spider-man vs. batman is not a valid URL"),
-        "Expected error thrown when trying to search with an invalid url as an argument"
-      );
+        error.message.includes(substr),
+        `Expected error ${JSON.stringify(error.message)} to contain ${JSON.stringify(substr)}`);
     });
 
     // queries the full url

toolkit/components/extensions/test/xpcshell/test_ext_schemas.js

@@ -942,3 +942,142 @@ add_task(function* testDeprecation() {
   root.deprecated.onDeprecated.hasListener(() => {});
   checkErrors(["This event does not work"]);
 });
+
+
+let choicesJson = [
+  {namespace: "choices",
+
+   types: [
+   ],
+
+   functions: [
+     {
+       name: "meh",
+       type: "function",
+       parameters: [
+         {
+           name: "arg",
+           choices: [
+             {
+               type: "string",
+               enum: ["foo", "bar", "baz"],
+             },
+             {
+               type: "string",
+               pattern: "florg.*meh",
+             },
+             {
+               type: "integer",
+               minimum: 12,
+               maximum: 42,
+             },
+           ],
+         },
+       ],
+     },
+
+     {
+       name: "foo",
+       type: "function",
+       parameters: [
+         {
+           name: "arg",
+           choices: [
+             {
+               type: "object",
+               properties: {
+                 blurg: {
+                   type: "string",
+                   unsupported: true,
+                   optional: true,
+                 },
+               },
+               additionalProperties: {
+                 type: "string",
+               },
+             },
+             {
+               type: "string",
+             },
+             {
+               type: "array",
+               minItems: 2,
+               maxItems: 3,
+               items: {
+                 type: "integer",
+               },
+             },
+           ],
+         },
+       ],
+     },
+
+     {
+       name: "bar",
+       type: "function",
+       parameters: [
+         {
+           name: "arg",
+           choices: [
+             {
+               type: "object",
+               properties: {
+                 baz: {
+                   type: "string",
+                 },
+               },
+             },
+             {
+               type: "array",
+               items: {
+                 type: "integer",
+               },
+             },
+           ],
+         },
+       ],
+     },
+   ]},
+];
+
+add_task(function* testChoices() {
+  let url = "data:," + JSON.stringify(choicesJson);
+  yield Schemas.load(url);
+
+  let root = {};
+  Schemas.inject(root, wrapper);
+
+  talliedErrors.length = 0;
+
+  Assert.throws(() => root.choices.meh("frog"),
+                /Value must either: be one of \["foo", "bar", "baz"\], match the pattern \/florg\.\*meh\/, or be an integer value/);
+
+  Assert.throws(() => root.choices.meh(4),
+                /be a string value, or be at least 12/);
+
+  Assert.throws(() => root.choices.meh(43),
+                /be a string value, or be no greater than 42/);
+
+
+  Assert.throws(() => root.choices.foo([]),
+                /be an object value, be a string value, or have at least 2 items/);
+
+  Assert.throws(() => root.choices.foo([1, 2, 3, 4]),
+                /be an object value, be a string value, or have at most 3 items/);
+
+  Assert.throws(() => root.choices.foo({foo: 12}),
+                /.foo must be a string value, be a string value, or be an array value/);
+
+  Assert.throws(() => root.choices.foo({blurg: "foo"}),
+                /not contain an unsupported "blurg" property, be a string value, or be an array value/);
+
+
+  Assert.throws(() => root.choices.bar({}),
+                /contain the required "baz" property, or be an array value/);
+
+  Assert.throws(() => root.choices.bar({baz: "x", quux: "y"}),
+                /not contain an unexpected "quux" property, or be an array value/);
+
+  Assert.throws(() => root.choices.bar({baz: "x", quux: "y", foo: "z"}),
+                /not contain the unexpected properties \[foo, quux\], or be an array value/);
+});