Bug 1363886 - Part 1: Check API function results against schema r=kmag
☠☠ backed out by 07a91e554ec5 ☠ ☠
authorTomislav Jovanovic <tomica@gmail.com>
Mon, 24 Jul 2017 00:03:20 +0200
changeset 370679 8955980312a6fa1996b541998885f1f06926bb76
parent 370678 52b49a63629c78042d7683691fc46fb072a81fcb
child 370680 0bb1d7f7feb09facfac44d0a1d9e3c8737c06a38
push id32231
push usercbook@mozilla.com
push dateTue, 25 Jul 2017 12:20:10 +0000
treeherdermozilla-central@80394cbcae0f [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);