Bug 1456526 - Add Error Checking to Protocol.js WIP draft
authoryulia <ystartsev@mozilla.com>
Tue, 24 Apr 2018 17:57:18 +0200
changeset 787242 e7d050293644995b86f26d89d331245e5bea68d4
parent 786478 dfb15917c057f17e5143f7d7c6e1972ba53efc49
push id107692
push userbmo:ystartsev@mozilla.com
push dateTue, 24 Apr 2018 15:57:55 +0000
bugs1456526
milestone61.0a1
Bug 1456526 - Add Error Checking to Protocol.js WIP MozReview-Commit-ID: LXayvFcSd3u
devtools/server/main.js
devtools/shared/DevToolsUtils.js
devtools/shared/protocol.js
--- a/devtools/server/main.js
+++ b/devtools/server/main.js
@@ -10,17 +10,17 @@
  */
 var { Ci, Cc } = require("chrome");
 var Services = require("Services");
 var { ActorPool, OriginalLocation, RegisteredActorFactory,
       ObservedActorFactory } = require("devtools/server/actors/common");
 var { LocalDebuggerTransport, ChildDebuggerTransport, WorkerDebuggerTransport } =
   require("devtools/shared/transport/transport");
 var DevToolsUtils = require("devtools/shared/DevToolsUtils");
-var { dumpn } = DevToolsUtils;
+var { dumpn, formatTypeSignature } = DevToolsUtils;
 
 DevToolsUtils.defineLazyGetter(this, "DebuggerSocket", () => {
   let { DebuggerSocket } = require("devtools/shared/security/socket");
   return DebuggerSocket;
 });
 DevToolsUtils.defineLazyGetter(this, "Authentication", () => {
   return require("devtools/shared/security/auth");
 });
@@ -1758,16 +1758,23 @@ DebuggerServerConnection.prototype = {
         ret = actor.requestTypes[packet.type].bind(actor)(packet, this);
       } catch (error) {
         let prefix = "error occurred while processing '" + packet.type;
         this.transport.send(this._unknownError(actor.actorID, prefix, error));
       } finally {
         this.currentPacket = undefined;
       }
     } else {
+      dump(
+        `Unrecognized packet with type "${packet.type}" and following request type` +
+        ` signiture = \n${formatTypeSignature(packet)}\n`
+      );
+      dump(`known types for this actor are: \n ${
+        Object.keys(actor.requestTypes).join("\n ")
+      }\n`);
       ret = { error: "unrecognizedPacketType",
               message: ("Actor " + actor.actorID +
                         " does not recognize the packet type " +
                         packet.type) };
     }
 
     // There will not be a return value if a bulk reply is sent.
     if (ret) {
--- a/devtools/shared/DevToolsUtils.js
+++ b/devtools/shared/DevToolsUtils.js
@@ -376,16 +376,51 @@ exports.dumpn = function(str) {
  */
 exports.dumpv = function(msg) {
   if (flags.wantVerbose) {
     exports.dumpn(msg);
   }
 };
 
 /**
+ * Formats the types of a request in an easy to read way.
+ *
+ * @param object
+ *        The object being traversed.
+ * @param depth
+ *        The indentation of this segment of string.
+ */
+function formatTypeSignature(object, depth = 0) {
+  function typeSignatureToString(string, key) {
+    if (key === "to" || key === "type") {
+      return string;
+    }
+    const padding = " ".repeat(depth);
+    const value = object[key];
+    const type = typeof value;
+    let newSegment;
+    if (type === "object") {
+      const stringOutput = formatTypeSignature(value, depth + 1);
+      newSegment = `${padding}${key}: {\n${stringOutput}${padding}}\n`;
+    } else {
+      newSegment = `${padding}${key}: ${type}\n`;
+    }
+    return string + newSegment;
+  }
+
+  if (!object) {
+    return "null";
+  }
+
+  return Object.keys(object).reduce(typeSignatureToString, "");
+}
+
+exports.formatTypeSignature = formatTypeSignature;
+
+/**
  * Defines a getter on a specified object that will be created upon first use.
  *
  * @param object
  *        The object to define the lazy getter on.
  * @param name
  *        The name of the getter to define on object.
  * @param lambda
  *        A function that returns what the getter should return.  This will
--- a/devtools/shared/protocol.js
+++ b/devtools/shared/protocol.js
@@ -4,16 +4,17 @@
 
 "use strict";
 
 const { extend } = require("devtools/shared/extend");
 var EventEmitter = require("devtools/shared/event-emitter");
 var {getStack, callFunctionWithAsyncStack} = require("devtools/shared/platform/stack");
 var {settleAll} = require("devtools/shared/DevToolsUtils");
 var {lazyLoadSpec, lazyLoadFront} = require("devtools/shared/specs/index");
+var { formatTypeSignature } = require("devtools/shared/DevToolsUtils");
 
 // Bug 1454373: devtools/shared/defer still uses Promise.jsm which is slower
 // than DOM Promises. So implement our own copy of `defer` based on DOM Promises.
 function defer() {
   let resolve, reject;
   let promise = new Promise(function() {
     resolve = arguments[0];
     reject = arguments[1];
@@ -510,17 +511,17 @@ Arg.prototype = {
 
   read: function(v, ctx, outArgs) {
     outArgs[this.index] = this.type.read(v, ctx);
   },
 
   describe: function() {
     return {
       _arg: this.index,
-      type: this.type.name,
+     type: this.type.name,
     };
   }
 };
 
 // Outside of protocol.js, Arg is called as factory method, without the new keyword.
 exports.Arg = function(index, type) {
   return new Arg(index, type);
 };
@@ -703,27 +704,105 @@ Request.prototype = {
    * @returns an arguments array
    */
   read: function(packet, ctx) {
     let fnArgs = [];
     for (let templateArg of this.args) {
       let arg = templateArg.placeholder;
       let path = templateArg.path;
       let name = path[path.length - 1];
+      typeCheck(packet, arg.type.name, name);
       arg.read(getPath(packet, path), ctx, fnArgs, name);
     }
     return fnArgs;
   },
 
   describe: function() {
     return describeTemplate(this.template);
   }
 };
 
 /**
+ * Checks only the primitive types on a function, bails if it finds a custom type.
+ * Returns true if typecheck passes, false if it fails and emitts an error message.
+ *
+ * @param object packet
+ *    The request packet.
+ * @param string typeName
+ *    The name of the type being checked.
+ * @param string name
+ *    The name of the type being checked.
+ */
+function typeCheck(packet, typeName, argName) {
+  const arg = packet[argName];
+  const type = types.getType(typeName);
+
+  if (type.primitive) {
+    if (typeName.includes(":")) {
+      const isArray = Array.isArray(arg);
+      const subtype = typeName.split(":").pop();
+      if (typeName.includes("array") && !isArray) {
+        console.error(new TypeError(
+          `Expected an array for the argument "${argName}" in packet of ` +
+          `type ${packet.type}\n`
+        ));
+        return false;
+      }
+      if (isArray && typeof arg[0] !== subtype) {
+        console.error(new TypeError(
+          `Expected an array populated with subtype "${subtype}" for ${argName} in ` +
+          `packet ${packet.type}. Instead found an array of ${typeof arg[0]}\n`
+        ));
+        return false;
+      }
+      if (typeName.includes("nullable") && !arg) {
+        return true;
+      }
+    }
+    if (typeof arg !== typeName) {
+      console.error(new TypeError(
+        `Expected the type ${typeName} as the argument for ${packet.type}, received ` +
+         `an argument with ${typeof arg} instead\n`
+      ));
+      return false;
+    }
+    return true;
+  }
+
+  return typeCheckDict(packet, typeName, arg);
+}
+
+function typeCheckDict(packet, typeName) {
+  const { from, to, ...arg } = packet;
+  if (typeof arg !== "object") {
+    console.error(new TypeError(
+      `Expected a DictType "${typeName}" as the argument for ${packet.type}, received ` +
+       `an argument with ${typeof arg} instead\n`
+    ));
+    return false;
+  }
+  const type = types.getType(typeName);
+
+  if (!type.specializations) {
+    // we have no way of knowing more about this type at this point.
+    return true;
+  }
+
+  for (const key in Object.values(type.specializations)) {
+  dump(`%%%%%%%%%%%%%%%%%%%% ${key}\n`);
+    if (arg[key]) {
+    dump(`%%%%%%%%%%%%%%%%%%%% hi\n`);
+
+    } else {
+    }
+  }
+}
+
+
+/**
  * Manages a response template.
  *
  * @param object template
  *    The response template.
  * @construcor
  */
 var Response = function(template = {}) {
   this.template = template;
@@ -1081,16 +1160,23 @@ var generateActorSpec = function(actorDe
       actorSpec.methods.push(spec);
     }
   }
 
   // Find additional method specifications
   if (actorDesc.methods) {
     for (let name in actorDesc.methods) {
       let methodSpec = actorDesc.methods[name];
+      if (methodSpec.request && methodSpec.request.type) {
+        console.warn(
+          `Request object for protocol method "${name}" has a "type" field and this ` +
+          "will override the spec name.  Have you use an Option or Arg without a " +
+          "wrapping it in request object?\n"
+        );
+      }
       let spec = {};
 
       spec.name = methodSpec.name || name;
       spec.request = new Request(Object.assign({type: spec.name},
                                           methodSpec.request || undefined));
       spec.response = new Response(methodSpec.response || undefined);
       spec.release = methodSpec.release;
       spec.oneway = methodSpec.oneway;
@@ -1119,17 +1205,17 @@ var generateActorSpec = function(actorDe
 exports.generateActorSpec = generateActorSpec;
 
 /**
  * Generates request handlers as described by the given actor specification on
  * the given actor prototype. Returns the actor prototype.
  */
 var generateRequestHandlers = function(actorSpec, actorProto) {
   if (actorProto._actorSpec) {
-    throw new Error("actorProto called twice on the same actor prototype!");
+    // throw new Error("actorProto called twice on the same actor prototype!");
   }
 
   actorProto.typeName = actorSpec.typeName;
 
   // Generate request handlers for each method definition
   actorProto.requestTypes = Object.create(null);
   actorSpec.methods.forEach(spec => {
     let handler = function(packet, conn) {
@@ -1137,16 +1223,24 @@ var generateRequestHandlers = function(a
         let args;
         try {
           args = spec.request.read(packet, this);
         } catch (ex) {
           console.error("Error reading request: " + packet.type);
           throw ex;
         }
 
+        if (args.length !== this[spec.name].length) {
+          console.error(new TypeError(
+            `Actor type "${actorSpec.typeName}" has a spec which expects ` +
+            `${args.length} arguments to be passed to the "${spec.name}" method, ` +
+            `but "${spec.name}" expects ${this[spec.name].length}`
+          ));
+        }
+
         let ret = this[spec.name].apply(this, args);
 
         let sendReturn = (retToSend) => {
           if (spec.oneway) {
             // No need to send a response.
             return;
           }
 
@@ -1207,16 +1301,17 @@ var ActorClassWithSpec = function(actorS
   }
 
   // Existing Actors are relying on the initialize instead of constructor methods.
   let cls = function() {
     let instance = Object.create(cls.prototype);
     instance.initialize.apply(instance, arguments);
     return instance;
   };
+
   cls.prototype = extend(Actor.prototype, generateRequestHandlers(actorSpec, actorProto));
 
   return cls;
 };
 exports.ActorClassWithSpec = ActorClassWithSpec;
 
 /**
  * Base class for client-side actor fronts.