Bug 1363886 - Part 1: Check API function results against schema r=kmag
authorTomislav Jovanovic <tomica@gmail.com>
Mon, 24 Jul 2017 00:03:20 +0200
changeset 419644 d2d722422f8762666f5167ccf996a1cdf0872512
parent 419643 ba130cb1f4270176a013f69c5f754a65ae81d108
child 419645 c43af73943bb1ed6dcd5b6f1b029e758fc8f4974
push id7566
push usermtabara@mozilla.com
push dateWed, 02 Aug 2017 08:25:16 +0000
treeherdermozilla-beta@86913f512c3c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskmag
bugs1363886
milestone56.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1363886 - Part 1: Check API function results against schema r=kmag MozReview-Commit-ID: E2mGR03zUSf
toolkit/components/extensions/Schemas.jsm
toolkit/components/extensions/schemas/extension.json
toolkit/components/extensions/schemas/identity.json
toolkit/components/extensions/test/xpcshell/test_ext_schemas.js
toolkit/components/extensions/test/xpcshell/test_ext_schemas_revoke.js
--- a/toolkit/components/extensions/Schemas.jsm
+++ b/toolkit/components/extensions/Schemas.jsm
@@ -2097,37 +2097,61 @@ class CallEntry extends Entry {
 
     return fixedArgs;
   }
 }
 
 // Represents a "function" defined in a schema namespace.
 FunctionEntry = class FunctionEntry extends CallEntry {
   static parseSchema(schema, path) {
+    // When not in DEBUG mode, we just need to know *if* this returns.
+    let returns = !!schema.returns;
+    if (DEBUG && "returns" in schema) {
+      returns = {
+        type: Schemas.parseSchema(schema.returns, path, ["optional", "name"]),
+        optional: schema.returns.optional || false,
+      };
+    }
+
     return new this(schema, path, schema.name,
                     Schemas.parseSchema(schema, path,
                       ["name", "unsupported", "returns",
                        "permissions",
                        "allowAmbiguousOptionalArguments"]),
                     schema.unsupported || false,
                     schema.allowAmbiguousOptionalArguments || false,
-                    schema.returns || null,
+                    returns,
                     schema.permissions || null);
   }
 
   constructor(schema, path, name, type, unsupported, allowAmbiguousOptionalArguments, returns, permissions) {
     super(schema, path, name, type.parameters, allowAmbiguousOptionalArguments);
     this.unsupported = unsupported;
     this.returns = returns;
     this.permissions = permissions;
 
     this.isAsync = type.isAsync;
     this.hasAsyncCallback = type.hasAsyncCallback;
   }
 
+  checkValue({type, optional}, value, context) {
+    if (optional && value == null) {
+      return;
+    }
+    if (type.reference === "ExtensionPanel" || type.reference === "Port") {
+      // TODO: We currently treat objects with functions as SubModuleType,
+      // which is just wrong, and a bigger yak.  Skipping for now.
+      return;
+    }
+    const {error} = type.normalize(value, context);
+    if (error) {
+      this.throwError(context, `Type error for result value (${error})`);
+    }
+  }
+
   getDescriptor(path, context) {
     let apiImpl = context.getImplementation(path.join("."), this.name);
 
     let stub;
     if (this.isAsync) {
       stub = (...args) => {
         this.checkDeprecated(context);
         let actuals = this.checkParameters(args, context);
@@ -2148,17 +2172,21 @@ FunctionEntry = class FunctionEntry exte
         this.checkDeprecated(context);
         let actuals = this.checkParameters(args, context);
         return apiImpl.callFunctionNoReturn(actuals);
       };
     } else {
       stub = (...args) => {
         this.checkDeprecated(context);
         let actuals = this.checkParameters(args, context);
-        return apiImpl.callFunction(actuals);
+        let result = apiImpl.callFunction(actuals);
+        if (DEBUG && this.returns) {
+          this.checkValue(this.returns, result, context);
+        }
+        return result;
       };
     }
 
     return {
       descriptor: {value: Cu.exportFunction(stub, context.cloneScope)},
       revoke() {
         apiImpl.revoke();
         apiImpl = null;
--- a/toolkit/components/extensions/schemas/extension.json
+++ b/toolkit/components/extensions/schemas/extension.json
@@ -80,32 +80,30 @@
               }
             }
           }
         ],
         "returns": {
           "type": "array",
           "description": "Array of global objects",
           "items": {
-            "name": "viewGlobals",
             "type": "object",
             "isInstanceOf": "Window",
             "additionalProperties": { "type": "any" }
           }
         }
       },
       {
         "name": "getBackgroundPage",
         "type": "function",
         "description": "Returns the JavaScript 'window' object for the background page running inside the current extension. Returns null if the extension has no background page.",
         "parameters": [],
         "returns": {
             "type": "object",
             "optional": true,
-            "name": "backgroundPageGlobal",
             "isInstanceOf": "Window",
             "additionalProperties": { "type": "any" }
          }
       },
       {
         "name": "isAllowedIncognitoAccess",
         "type": "function",
         "description": "Retrieves the state of the extension's access to Incognito-mode (as determined by the user-controlled 'Allowed in Incognito' checkbox.",
--- a/toolkit/components/extensions/schemas/identity.json
+++ b/toolkit/components/extensions/schemas/identity.json
@@ -188,17 +188,17 @@
             "name": " path",
             "type": "string",
             "default": "",
             "optional": true,
             "description": "The path appended to the end of the generated URL. "
           }
         ],
         "returns": {
-          "string": "path"
+          "type": "string"
         }
       }
     ],
     "events": [
       {
         "name": "onSignInChanged",
         "unsupported": true,
         "type": "function",
--- a/toolkit/components/extensions/test/xpcshell/test_ext_schemas.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_schemas.js
@@ -71,17 +71,17 @@ let json = [
      {
        id: "submodule",
        type: "object",
        functions: [
          {
            name: "sub_foo",
            type: "function",
            parameters: [],
-           returns: "integer",
+           returns: {type: "integer"},
          },
        ],
      },
    ],
 
    functions: [
      {
        name: "foo",
@@ -399,16 +399,19 @@ class TallyingAPIImplementation extends 
   constructor(namespace, name) {
     super();
     this.namespace = namespace;
     this.name = name;
   }
 
   callFunction(args) {
     tally("call", this.namespace, this.name, args);
+    if (this.name === "sub_foo") {
+      return 13;
+    }
   }
 
   callFunctionNoReturn(args) {
     tally("call", this.namespace, this.name, args);
   }
 
   getProperty() {
     tally("get", this.namespace, this.name);
@@ -1495,16 +1498,17 @@ let defaultsJson = [
        type: "function",
        parameters: [
          {name: "arg", type: "object", optional: true, properties: {
            prop1: {type: "integer", optional: true},
          }, default: {prop1: 1}},
        ],
        returns: {
          type: "object",
+         additionalProperties: true,
        },
      },
    ]},
 ];
 
 add_task(async function testDefaults() {
   let url = "data:," + JSON.stringify(defaultsJson);
   await Schemas.load(url);
@@ -1531,8 +1535,85 @@ add_task(async function testDefaults() {
 
   let root = {};
   Schemas.inject(root, localWrapper);
 
   deepEqual(root.defaultsJson.defaultFoo(), {prop1: 1, newProp: 1});
   deepEqual(root.defaultsJson.defaultFoo({prop1: 2}), {prop1: 2, newProp: 1});
   deepEqual(root.defaultsJson.defaultFoo(), {prop1: 1, newProp: 1});
 });
+
+let returnsJson = [{
+  namespace: "returns",
+  types: [
+    {
+      id: "Widget",
+      type: "object",
+      properties: {
+        size: {type: "integer"},
+        colour: {type: "string", optional: true},
+      },
+    },
+  ],
+  functions: [
+    {
+      name: "complete",
+      type: "function",
+      returns: {$ref: "Widget"},
+      parameters: [],
+    },
+    {
+      name: "optional",
+      type: "function",
+      returns: {$ref: "Widget"},
+      parameters: [],
+    },
+    {
+      name: "invalid",
+      type: "function",
+      returns: {$ref: "Widget"},
+      parameters: [],
+    },
+  ],
+}];
+
+add_task(async function testReturns() {
+  const url = "data:," + JSON.stringify(returnsJson);
+  await Schemas.load(url);
+
+  const apiObject = {
+    complete() {
+      return {size: 3, colour: "orange"};
+    },
+    optional() {
+      return {size: 4};
+    },
+    invalid() {
+      return {};
+    },
+  };
+
+  const localWrapper = {
+    cloneScope: global,
+    shouldInject(ns) {
+      return true;
+    },
+    getImplementation(ns, name) {
+      return new LocalAPIImplementation(apiObject, name, null);
+    },
+  };
+
+  const root = {};
+  Schemas.inject(root, localWrapper);
+
+  deepEqual(root.returns.complete(), {size: 3, colour: "orange"});
+  deepEqual(root.returns.optional(), {size: 4},
+            "Missing optional properties is allowed");
+
+  if (AppConstants.DEBUG) {
+    Assert.throws(() => root.returns.invalid(),
+                  `Type error for result value (Property "size" is required)`,
+                  "Should throw for invalid result in DEBUG builds");
+  } else {
+    deepEqual(root.returns.invalid(), {},
+              "Doesn't throw for invalid result value in release builds");
+  }
+});
--- a/toolkit/components/extensions/test/xpcshell/test_ext_schemas_revoke.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_schemas_revoke.js
@@ -41,17 +41,17 @@ let json = [
       {
         id: "submodule",
         type: "object",
         functions: [
           {
             name: "sub_foo",
             type: "function",
             parameters: [],
-            returns: "integer",
+            returns: {type: "integer"},
           },
         ],
       },
     ],
 
     functions: [
       {
         name: "func",
@@ -116,16 +116,19 @@ class APIImplementation extends SchemaAP
   }
 
   revoke(...args) {
     this.record("revoke", args);
   }
 
   callFunction(...args) {
     this.record("callFunction", args);
+    if (this.name === "sub_foo") {
+      return 13;
+    }
   }
 
   callFunctionNoReturn(...args) {
     this.record("callFunctionNoReturn", args);
   }
 
   getProperty(...args) {
     this.record("getProperty", args);