Bug 1363886 - Part 3: Check async callback arguments against schema r=kmag
authorTomislav Jovanovic <tomica@gmail.com>
Sun, 21 May 2017 04:19:46 +0200
changeset 419646 4ca0f86e67cbd017aeff60eddc65440b273ccc13
parent 419645 c43af73943bb1ed6dcd5b6f1b029e758fc8f4974
child 419647 f12698197df524a0258435f21286a259c2a0df8a
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 3: Check async callback arguments against schema r=kmag MozReview-Commit-ID: E0yp9SdJrv6
toolkit/components/extensions/Schemas.jsm
toolkit/components/extensions/test/xpcshell/test_ext_schemas_async.js
--- a/toolkit/components/extensions/Schemas.jsm
+++ b/toolkit/components/extensions/Schemas.jsm
@@ -2103,16 +2103,17 @@ class CallEntry extends Entry {
 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,
+        name: "result",
       };
     }
 
     return new this(schema, path, schema.name,
                     Schemas.parseSchema(schema, path,
                       ["name", "unsupported", "returns",
                        "permissions",
                        "allowAmbiguousOptionalArguments"]),
@@ -2127,28 +2128,35 @@ FunctionEntry = class FunctionEntry exte
     this.unsupported = unsupported;
     this.returns = returns;
     this.permissions = permissions;
 
     this.isAsync = type.isAsync;
     this.hasAsyncCallback = type.hasAsyncCallback;
   }
 
-  checkValue({type, optional}, value, context) {
+  checkValue({type, optional, name}, 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})`);
+      this.throwError(context, `Type error for ${name} value (${error})`);
+    }
+  }
+
+  checkCallback(args, context) {
+    const callback = this.parameters[this.parameters.length - 1];
+    for (const [i, param] of callback.type.parameters.entries()) {
+      this.checkValue(param, args[i], context);
     }
   }
 
   getDescriptor(path, context) {
     let apiImpl = context.getImplementation(path.join("."), this.name);
 
     let stub;
     if (this.isAsync) {
@@ -2160,17 +2168,31 @@ FunctionEntry = class FunctionEntry exte
           callback = actuals.pop();
         }
         if (callback === null && context.isChromeCompat) {
           // We pass an empty stub function as a default callback for
           // the `chrome` API, so promise objects are not returned,
           // and lastError values are reported immediately.
           callback = () => {};
         }
-        return apiImpl.callAsyncFunction(actuals, callback);
+        if (DEBUG && this.hasAsyncCallback && callback) {
+          let original = callback;
+          callback = (...args) => {
+            this.checkCallback(args, context);
+            original(...args);
+          };
+        }
+        let result = apiImpl.callAsyncFunction(actuals, callback);
+        if (DEBUG && this.hasAsyncCallback && !callback) {
+          return result.then(result => {
+            this.checkCallback([result], context);
+            return result;
+          });
+        }
+        return result;
       };
     } else if (!this.returns) {
       stub = (...args) => {
         this.checkDeprecated(context);
         let actuals = this.checkParameters(args, context);
         return apiImpl.callFunctionNoReturn(actuals);
       };
     } else {
--- a/toolkit/components/extensions/test/xpcshell/test_ext_schemas_async.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_schemas_async.js
@@ -3,16 +3,24 @@
 Components.utils.import("resource://gre/modules/ExtensionCommon.jsm");
 Components.utils.import("resource://gre/modules/Schemas.jsm");
 
 let {BaseContext, LocalAPIImplementation} = ExtensionCommon;
 
 let schemaJson = [
   {
     namespace: "testnamespace",
+    types: [{
+      id: "Widget",
+      type: "object",
+      properties: {
+        size: {type: "integer"},
+        colour: {type: "string", optional: true},
+      },
+    }],
     functions: [{
       name: "one_required",
       type: "function",
       parameters: [{
         name: "first",
         type: "function",
         parameters: [],
       }],
@@ -39,16 +47,28 @@ let schemaJson = [
       type: "function",
       async: "first",
       parameters: [{
         name: "first",
         type: "function",
         parameters: [],
         optional: true,
       }],
+    }, {
+      name: "async_result",
+      type: "function",
+      async: "callback",
+      parameters: [{
+        name: "callback",
+        type: "function",
+        parameters: [{
+          name: "widget",
+          $ref: "Widget",
+        }],
+      }],
     }],
   },
 ];
 
 const global = this;
 class StubContext extends BaseContext {
   constructor() {
     let fakeExtension = {id: "test@web.extension"};
@@ -142,16 +162,46 @@ add_task(async function testParameterVal
     assertNoThrows("async_optional");
     assertNoThrows("async_optional", null);
     assertNoThrows("async_optional", cb);
     assertThrows("async_optional", cb, null);
     assertThrows("async_optional", cb, cb);
   }
 });
 
+add_task(async function testCheckAsyncResults() {
+  await Schemas.load("data:," + JSON.stringify(schemaJson));
+
+  const complete = generateAPIs({}, {
+    async_result: async () => ({size: 5, colour: "green"}),
+  });
+
+  const optional = generateAPIs({}, {
+    async_result: async () => ({size: 6}),
+  });
+
+  const invalid = generateAPIs({}, {
+    async_result: async () => ({}),
+  });
+
+  deepEqual(await complete.async_result(), {size: 5, colour: "green"});
+
+  deepEqual(await optional.async_result(), {size: 6},
+            "Missing optional properties is allowed");
+
+  if (AppConstants.DEBUG) {
+    await Assert.rejects(invalid.async_result(),
+          `Type error for widget value (Property "size" is required)`,
+          "Should throw for invalid callback argument in DEBUG builds");
+  } else {
+    deepEqual(await invalid.async_result(), {},
+              "Invalid callback argument doesn't throw in release builds");
+  }
+});
+
 add_task(async function testAsyncResults() {
   await Schemas.load("data:," + JSON.stringify(schemaJson));
   function runWithCallback(func) {
     do_print(`Calling testnamespace.${func.name}, expecting callback with result`);
     return new Promise(resolve => {
       let result = "uninitialized value";
       let returnValue = func(reply => {
         result = reply;