Bug 1363886 - Part 1: Check API function results against schema draft
authorTomislav Jovanovic <tomica@gmail.com>
Mon, 24 Jul 2017 00:03:20 +0200
changeset 614046 8bf9a703c3a61446a5d21762cc627fca256e2b8f
parent 614015 5928d905c0bc0b28f5488b236444c7d7991cf8d4
child 614047 0775472c6bc101cae133b18e16ff909777e91b12
child 614571 9ed1a62c9e008e9597547732f9952fa90a131a4f
child 615307 5e447851a8202d5c0e5d4f41f1c263bfcb4f6109
push id69897
push userbmo:tomica@gmail.com
push dateSun, 23 Jul 2017 22:05:14 +0000
bugs1363886
milestone56.0a1
Bug 1363886 - Part 1: Check API function results against schema 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);