Bug 1726800 - [remote] Verify commands as early as possible in MessageHandler r=webdriver-reviewers,whimboo

Depends on D123655
With this patch, the MessageHandler can immediately check if a command is implemented by the modules, and therefore reject as early as possible.
This is implemented via a checkCommand method on MessageHandler.

Other required changes:
- ModuleRegistry now owns the logic to import BiDi modules.
- ModuleCache exposes a `getAllModuleClasses` to get all the relevant modules for a moduleName+destination pair.

Error messages have been improved and are verified with a dedicated test

Differential Revision: https://phabricator.services.mozilla.com/D123655

.eslintignore

@@ -160,6 +160,9 @@ remote/cdp/Protocol.jsm
 remote/cdp/test/browser/chrome-remote-interface.js
 remote/marionette/atom.js
 
+# This file explicitly has a syntax error and cannot be parsed by eslint.
+remote/shared/messagehandler/test/browser/resources/modules/root/invalid.jsm
+
 # services/ exclusions
 
 # Third party services

remote/shared/messagehandler/MessageHandler.jsm

@@ -13,6 +13,7 @@ const { XPCOMUtils } = ChromeUtils.import(
 XPCOMUtils.defineLazyModuleGetters(this, {
   EventEmitter: "resource://gre/modules/EventEmitter.jsm",
 
+  error: "chrome://remote/content/shared/messagehandler/Errors.jsm",
   Log: "chrome://remote/content/shared/Log.jsm",
   ModuleCache: "chrome://remote/content/shared/messagehandler/ModuleCache.jsm",
 });
@@ -73,6 +74,32 @@ class MessageHandler extends EventEmitter {
     return this._sessionId;
   }
 
+  /**
+   * Check if the command can be handled from this MessageHandler node.
+   *
+   * @param {Command} command
+   *     The command to check. See type definition in MessageHandler.js
+   * @throws {Error}
+   *     Throws if there is no module supporting the provided command on the
+   *     path to the command's destination
+   */
+  checkCommand(command) {
+    const { commandName, destination, moduleName } = command;
+
+    // Retrieve all the modules classes which can be used to reach the
+    // command's destination.
+    const moduleClasses = this._moduleCache.getAllModuleClasses(
+      moduleName,
+      destination
+    );
+
+    if (!moduleClasses.some(cls => cls.supportsMethod(commandName))) {
+      throw new error.UnsupportedCommandError(
+        `${moduleName}.${commandName} not supported for destination ${destination?.type}`
+      );
+    }
+  }
+
   destroy() {
     logger.trace(
       `MessageHandler ${this.constructor.type} for session ${this.sessionId} is being destroyed`
@@ -139,8 +166,10 @@ class MessageHandler extends EventEmitter {
       `Received command ${moduleName}.${commandName} for destination ${destination.type}`
     );
 
+    this.checkCommand(command);
+
     const module = this._moduleCache.getModuleInstance(moduleName, destination);
-    if (this._isCommandSupportedByModule(commandName, module)) {
+    if (module && module.supportsMethod(commandName)) {
       return module[commandName](params, destination);
     }
 
@@ -151,14 +180,6 @@ class MessageHandler extends EventEmitter {
     return `[object ${this.constructor.name} ${this.name}]`;
   }
 
-  _isCommandSupportedByModule(commandName, module) {
-    // TODO: With the current implementation, all functions of a given module
-    // are considered as valid commands.
-    // This should probably be replaced by a more explicit declaration, via a
-    // manifest for instance.
-    return module && typeof module[commandName] === "function";
-  }
-
   /**
    * Returns the module path corresponding to this MessageHandler class.
    *

remote/shared/messagehandler/Module.jsm

@@ -26,7 +26,19 @@ class Module {
     throw new Error("Not implemented");
   }
 
+  /**
+   * Instance shortcut for supportsMethod to avoid reaching the constructor for
+   * consumers which directly deal with an instance.
+   */
+  supportsMethod(methodName) {
+    return this.constructor.supportsMethod(methodName);
+  }
+
   get messageHandler() {
     return this._messageHandler;
   }
+
+  static supportsMethod(methodName) {
+    return typeof this.prototype[methodName] === "function";
+  }
 }

remote/shared/messagehandler/ModuleCache.jsm

@@ -10,29 +10,28 @@ const { XPCOMUtils } = ChromeUtils.import(
   "resource://gre/modules/XPCOMUtils.jsm"
 );
 
-// Import the ModuleRegistry so that all modules are properly referenced.
-ChromeUtils.import(
-  "chrome://remote/content/webdriver-bidi/modules/ModuleRegistry.jsm"
-);
-
 XPCOMUtils.defineLazyModuleGetters(this, {
   Services: "resource://gre/modules/Services.jsm",
 
   getMessageHandlerClass:
     "chrome://remote/content/shared/messagehandler/MessageHandlerRegistry.jsm",
+  // Additional protocols might use a different registry for their modules,
+  // in which case this will no longer be a constant but will instead depend on
+  // the protocol owning the MessageHandler. See Bug 1722464.
+  getModuleClass:
+    "chrome://remote/content/webdriver-bidi/modules/ModuleRegistry.jsm",
   Log: "chrome://remote/content/shared/Log.jsm",
 });
 
+XPCOMUtils.defineLazyModuleGetter(
+  this,
+  "getTestModuleClass",
+  "chrome://mochitests/content/browser/remote/shared/messagehandler/test/browser/resources/modules/ModuleRegistry.jsm",
+  "getModuleClass"
+);
+
 XPCOMUtils.defineLazyGetter(this, "logger", () => Log.get());
 
-// Additional protocols might use a different root folder for their modules,
-// in which case this will no longer be a constant but will instead depend on
-// the protocol owning the MessageHandler. See Bug 1722464.
-const MODULES_FOLDER = "chrome://remote/content/webdriver-bidi/modules";
-
-const TEST_MODULES_FOLDER =
-  "chrome://mochitests/content/browser/remote/shared/messagehandler/test/browser/resources/modules";
-
 /**
  * ModuleCache instances are dedicated to lazily create and cache the instances
  * of all the modules related to a specific MessageHandler instance.
@@ -71,20 +70,12 @@ class ModuleCache {
    */
   constructor(messageHandler) {
     this.messageHandler = messageHandler;
-    this._messageHandlerPath = messageHandler.constructor.modulePath;
+    this._messageHandlerType = messageHandler.constructor.type;
 
-    this._modulesRootFolder = MODULES_FOLDER;
+    this._useTestModules = Services.prefs.getBoolPref(
-    // TODO: Temporary workaround to use a different folder for tests.
-    // After Bug 1722464 lands, we should be able to set custom root folders
-    // per session and we can remove this workaround.
-    if (
-      Services.prefs.getBoolPref(
       "remote.messagehandler.modulecache.useBrowserTestRoot",
       false
-      )
+    );
-    ) {
-      this._modulesRootFolder = TEST_MODULES_FOLDER;
-    }
 
     // Map of absolute module paths to module instances.
     this._modules = new Map();
@@ -97,6 +88,41 @@ class ModuleCache {
     this._modules.forEach(module => module?.destroy());
   }
 
+  /**
+   * Retrieve all module classes matching the provided module name to reach the
+   * provided destination, from the current context.
+   *
+   * This corresponds to the path a command can take to reach its destination.
+   * A command's method must be implemented in one of the classes returned by
+   * getAllModuleClasses in order to be successfully handled.
+   *
+   * @param {String} moduleName
+   *     The name of the module.
+   * @param {Destination} destination
+   *     The destination.
+   * @return {Array.<class<Module>=>}
+   *     An array of Module classes. Will contain `null` items if the module
+   *     name is not implemented in a given layer.
+   */
+  getAllModuleClasses(moduleName, destination) {
+    const destinationType = destination.type;
+    const folders = [
+      this._getModuleFolder(this._messageHandlerType, destinationType),
+    ];
+
+    // Bug 1733242: Extend the implementation of this method to handle workers.
+    // It assumes layers have at most one level of nesting, for instance
+    // "root -> windowglobal", but it wouldn't work for something such as
+    // "root -> windowglobal -> worker".
+    if (destinationType !== this._messageHandlerType) {
+      folders.push(this._getModuleFolder(destinationType, destinationType));
+    }
+
+    return folders
+      .map(folder => this._getModuleClass(moduleName, folder))
+      .filter(cls => !!cls);
+  }
+
   /**
    * Get a module instance corresponding to the provided moduleName and
    * destination. If no existing module can be found in the cache, ModuleCache
@@ -113,43 +139,56 @@ class ModuleCache {
    *     destination, or null if it could not be instantiated.
    */
   getModuleInstance(moduleName, destination) {
-    const moduleFullPath = this._getModuleFullPath(moduleName, destination);
+    const key = `${moduleName}-${destination.type}`;
 
-    if (!this._modules.has(moduleFullPath)) {
+    if (this._modules.has(key)) {
-      try {
+      // If there is already a cached instance (potentially null) for the
-        const ModuleClass = ChromeUtils.import(moduleFullPath)[moduleName];
+      // module name + destination type pair, return it.
-        this._modules.set(moduleFullPath, new ModuleClass(this.messageHandler));
+      return this._modules.get(key);
-        logger.trace(`Module ${moduleName} created for ${moduleFullPath}`);
-      } catch (e) {
-        // If the module could not be imported, set null in the module cache
-        // so that the root MessageHandler can forward the message to the next
-        // layer.
-        this._modules.set(moduleFullPath, null);
-        logger.trace(`No module ${moduleName} found for ${moduleFullPath}`);
-      }
     }
 
-    return this._modules.get(moduleFullPath);
+    const moduleFolder = this._getModuleFolder(
+      this._messageHandlerType,
+      destination.type
+    );
+    const ModuleClass = this._getModuleClass(moduleName, moduleFolder);
+
+    let module = null;
+    if (ModuleClass) {
+      module = new ModuleClass(this.messageHandler);
+      logger.trace(`Module ${moduleName} created for ${destination.type}`);
+    } else {
+      logger.trace(`No module ${moduleName} found for ${destination.type}`);
+    }
+
+    this._modules.set(key, module);
+    return module;
   }
 
   toString() {
     return `[object ${this.constructor.name} ${this.messageHandler.name}]`;
   }
 
-  _getModuleFullPath(moduleName, destination) {
+  _getModuleClass(moduleName, moduleFolder) {
-    let moduleFolder;
+    if (this._useTestModules) {
-    if (this.messageHandler.constructor.type === destination.type) {
+      return getTestModuleClass(moduleName, moduleFolder);
-      // If the command is targeting the current type, the module is expected to
-      // be in eg "windowglobal/${moduleName}.jsm".
-      moduleFolder = this._messageHandlerPath;
-    } else {
-      // If the command is targeting another type, the module is expected to
-      // be in a composed folder eg "windowglobal-in-root/${moduleName}.jsm".
-      const MessageHandlerClass = getMessageHandlerClass(destination.type);
-      const destinationPath = MessageHandlerClass.modulePath;
-      moduleFolder = `${destinationPath}-in-${this._messageHandlerPath}`;
     }
 
-    return `${this._modulesRootFolder}/${moduleFolder}/${moduleName}.jsm`;
+    // Retrieve the module class from the WebDriverBiDi ModuleRegistry if we
+    // are not using test modules.
+    return getModuleClass(moduleName, moduleFolder);
+  }
+
+  _getModuleFolder(originType, destinationType) {
+    const originPath = getMessageHandlerClass(originType).modulePath;
+    if (originType === destinationType) {
+      // If the command is targeting the current type, the module is expected to
+      // be in eg "windowglobal/${moduleName}.jsm".
+      return originPath;
+    }
+    // If the command is targeting another type, the module is expected to
+    // be in a composed folder eg "windowglobal-in-root/${moduleName}.jsm".
+    const destinationPath = getMessageHandlerClass(destinationType).modulePath;
+    return `${destinationPath}-in-${originPath}`;
   }
 }

remote/shared/messagehandler/test/browser/browser_handle_command_errors.js

@@ -3,6 +3,9 @@
 
 "use strict";
 
+const { RootMessageHandler } = ChromeUtils.import(
+  "chrome://remote/content/shared/messagehandler/RootMessageHandler.jsm"
+);
 const { WindowGlobalMessageHandler } = ChromeUtils.import(
   "chrome://remote/content/shared/messagehandler/WindowGlobalMessageHandler.jsm"
 );
@@ -37,25 +40,182 @@ add_task(async function test_module_error() {
 add_task(async function test_destination_error() {
   const rootMessageHandler = createRootMessageHandler("session-id-error");
 
-  info("Call a valid module method, but on a non-existent browsing context id");
-  try {
   const fakeBrowsingContextId = -1;
   ok(
     !BrowsingContext.get(fakeBrowsingContextId),
     "No browsing context matches fakeBrowsingContextId"
   );
-    await rootMessageHandler.handleCommand({
+
+  info("Call a valid module method, but on a non-existent browsing context id");
+  Assert.throws(
+    () =>
+      rootMessageHandler.handleCommand({
         moduleName: "commandwindowglobalonly",
         commandName: "testOnlyInWindowGlobal",
         destination: {
           type: WindowGlobalMessageHandler.type,
           id: fakeBrowsingContextId,
         },
-    });
+      }),
-    ok(false, "Incorrect destination error was not caught");
+    err => err.message == `Unable to find a BrowsingContext for id -1`
-  } catch (e) {
+  );
-    ok(true, "Incorrect destination error was caught");
+
-  }
+  rootMessageHandler.destroy();
+});
+
+add_task(async function test_invalid_module_error() {
+  const rootMessageHandler = createRootMessageHandler(
+    "session-id-missing_module"
+  );
+
+  info("Attempt to call a Root module which has a syntax error");
+  Assert.throws(
+    () =>
+      rootMessageHandler.handleCommand({
+        moduleName: "invalid",
+        commandName: "someMethod",
+        destination: {
+          type: RootMessageHandler.type,
+        },
+      }),
+    err =>
+      err.name === "SyntaxError" &&
+      err.message == "expected expression, got ';'"
+  );
+
+  rootMessageHandler.destroy();
+});
+
+add_task(async function test_missing_root_module_error() {
+  const rootMessageHandler = createRootMessageHandler(
+    "session-id-missing_module"
+  );
+
+  info("Attempt to call a Root module which doesn't exist");
+  Assert.throws(
+    () =>
+      rootMessageHandler.handleCommand({
+        moduleName: "missingmodule",
+        commandName: "someMethod",
+        destination: {
+          type: RootMessageHandler.type,
+        },
+      }),
+    err =>
+      err.name == "UnsupportedCommandError" &&
+      err.message ==
+        `missingmodule.someMethod not supported for destination ROOT`
+  );
+
+  rootMessageHandler.destroy();
+});
+
+add_task(async function test_missing_windowglobal_module_error() {
+  const browsingContextId = gBrowser.selectedBrowser.browsingContext.id;
+  const rootMessageHandler = createRootMessageHandler(
+    "session-id-missing_windowglobal_module"
+  );
+
+  info("Attempt to call a WindowGlobal module which doesn't exist");
+  Assert.throws(
+    () =>
+      rootMessageHandler.handleCommand({
+        moduleName: "missingmodule",
+        commandName: "someMethod",
+        destination: {
+          type: WindowGlobalMessageHandler.type,
+          id: browsingContextId,
+        },
+      }),
+    err =>
+      err.name == "UnsupportedCommandError" &&
+      err.message ==
+        `missingmodule.someMethod not supported for destination WINDOW_GLOBAL`
+  );
+
+  rootMessageHandler.destroy();
+});
+
+add_task(async function test_missing_root_method_error() {
+  const rootMessageHandler = createRootMessageHandler(
+    "session-id-missing_root_method"
+  );
+
+  info("Attempt to call an invalid method on a Root module");
+  Assert.throws(
+    () =>
+      rootMessageHandler.handleCommand({
+        moduleName: "command",
+        commandName: "wrongMethod",
+        destination: {
+          type: RootMessageHandler.type,
+        },
+      }),
+    err =>
+      err.name == "UnsupportedCommandError" &&
+      err.message == `command.wrongMethod not supported for destination ROOT`
+  );
+
+  rootMessageHandler.destroy();
+});
+
+add_task(async function test_missing_windowglobal_method_error() {
+  const browsingContextId = gBrowser.selectedBrowser.browsingContext.id;
+  const rootMessageHandler = createRootMessageHandler(
+    "session-id-missing_windowglobal_method"
+  );
+
+  info("Attempt to call an invalid method on a WindowGlobal module");
+  Assert.throws(
+    () =>
+      rootMessageHandler.handleCommand({
+        moduleName: "commandwindowglobalonly",
+        commandName: "wrongMethod",
+        destination: {
+          type: WindowGlobalMessageHandler.type,
+          id: browsingContextId,
+        },
+      }),
+    err =>
+      err.name == "UnsupportedCommandError" &&
+      err.message ==
+        `commandwindowglobalonly.wrongMethod not supported for destination WINDOW_GLOBAL`
+  );
+
+  rootMessageHandler.destroy();
+});
+
+/**
+ * This test checks that even if a command is rerouted to another command after
+ * the RootMessageHandler, we still check the new command and log a useful
+ * error message.
+ *
+ * This illustrates why it is important to perform the command check at each
+ * layer of the MessageHandler network.
+ */
+add_task(async function test_missing_intermediary_method_error() {
+  const browsingContextId = gBrowser.selectedBrowser.browsingContext.id;
+  const rootMessageHandler = createRootMessageHandler(
+    "session-id-missing_intermediary_method"
+  );
+
+  info(
+    "Call a (valid) command that relies on another (missing) command on a WindowGlobal module"
+  );
+  await Assert.rejects(
+    rootMessageHandler.handleCommand({
+      moduleName: "commandwindowglobalonly",
+      commandName: "testMissingIntermediaryMethod",
+      destination: {
+        type: WindowGlobalMessageHandler.type,
+        id: browsingContextId,
+      },
+    }),
+    err =>
+      err.name == "UnsupportedCommandError" &&
+      err.message ==
+        `commandwindowglobalonly.missingMethod not supported for destination WINDOW_GLOBAL`
+  );
 
   rootMessageHandler.destroy();
 });

remote/shared/messagehandler/test/browser/resources/modules/ModuleRegistry.jsm

@@ -0,0 +1,33 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+var EXPORTED_SYMBOLS = ["getModuleClass"];
+
+/**
+ * Retrieve the WebDriver BiDi module class matching the provided module name
+ * and folder.
+ *
+ * @param {String} moduleName
+ *     The name of the module to get the class for.
+ * @param {String} moduleFolder
+ *     A valid folder name for modules.
+ * @return {Class=}
+ *     The class corresponding to the module name and folder, null if no match
+ *     was found.
+ * @throws {Error}
+ *     If the provided module folder is unexpected.
+ **/
+const getModuleClass = function(moduleName, moduleFolder) {
+  const path = `chrome://mochitests/content/browser/remote/shared/messagehandler/test/browser/resources/modules/${moduleFolder}/${moduleName}.jsm`;
+  try {
+    return ChromeUtils.import(path)[moduleName];
+  } catch (e) {
+    if (e.result == Cr.NS_ERROR_FILE_NOT_FOUND) {
+      return null;
+    }
+    throw e;
+  }
+};

remote/shared/messagehandler/test/browser/resources/modules/root/invalid.jsm

@@ -0,0 +1,4 @@
+// This module is meant to check error reporting when importing a module fails
+// due to an actual issue (syntax error etc...).
+
+SyntaxError(;

remote/shared/messagehandler/test/browser/resources/modules/windowglobal/commandwindowglobalonly.jsm

@@ -32,6 +32,16 @@ class CommandWindowGlobalOnly extends Module {
   testError() {
     throw new Error("error-from-module");
   }
+
+  testMissingIntermediaryMethod(params, destination) {
+    // Spawn a new internal command, but with a commandName which doesn't match
+    // any method.
+    return this.messageHandler.handleCommand({
+      moduleName: "commandwindowglobalonly",
+      commandName: "missingMethod",
+      destination,
+    });
+  }
 }
 
 const commandwindowglobalonly = CommandWindowGlobalOnly;

remote/shared/messagehandler/transports/js-window-actors/MessageHandlerFrameChild.jsm

@@ -11,6 +11,7 @@ const { XPCOMUtils } = ChromeUtils.import(
 );
 
 XPCOMUtils.defineLazyModuleGetters(this, {
+  error: "chrome://remote/content/shared/messagehandler/Errors.jsm",
   MessageHandlerRegistry:
     "chrome://remote/content/shared/messagehandler/MessageHandlerRegistry.jsm",
   WindowGlobalMessageHandler:
@@ -37,13 +38,22 @@ class MessageHandlerFrameChild extends JSWindowActorChild {
     this._registry.on("message-handler-registry-event", this._onRegistryEvent);
   }
 
-  receiveMessage(message) {
+  async receiveMessage(message) {
     if (message.name === "MessageHandlerFrameParent:sendCommand") {
       const { sessionId, command } = message.data;
       const messageHandler = this._registry.getOrCreateMessageHandler(
         sessionId
       );
-      return messageHandler.handleCommand(command);
+      try {
+        return await messageHandler.handleCommand(command);
+      } catch (e) {
+        if (e instanceof error.MessageHandlerError) {
+          return {
+            error: e.toJSON(),
+          };
+        }
+        throw e;
+      }
     }
 
     return null;

remote/shared/messagehandler/transports/js-window-actors/MessageHandlerFrameParent.jsm

@@ -11,6 +11,7 @@ const { XPCOMUtils } = ChromeUtils.import(
 );
 
 XPCOMUtils.defineLazyModuleGetters(this, {
+  error: "chrome://remote/content/shared/messagehandler/Errors.jsm",
   RootMessageHandlerRegistry:
     "chrome://remote/content/shared/messagehandler/RootMessageHandlerRegistry.jsm",
 });
@@ -51,10 +52,19 @@ class MessageHandlerFrameParent extends JSWindowActorParent {
    *     Promise that will resolve with the result of query sent to the
    *     MessageHandlerFrameChild actor.
    */
-  sendCommand(command, sessionId) {
+  async sendCommand(command, sessionId) {
-    return this.sendQuery("MessageHandlerFrameParent:sendCommand", {
+    const result = await this.sendQuery(
+      "MessageHandlerFrameParent:sendCommand",
+      {
         command,
         sessionId,
-    });
+      }
+    );
+
+    if (result?.error) {
+      throw error.MessageHandlerError.fromJSON(result.error);
+    }
+
+    return result;
   }
 }

remote/webdriver-bidi/modules/ModuleRegistry.jsm

@@ -4,7 +4,7 @@
 
 "use strict";
 
-var EXPORTED_SYMBOLS = [];
+var EXPORTED_SYMBOLS = ["getModuleClass"];
 
 const { XPCOMUtils } = ChromeUtils.import(
   "resource://gre/modules/XPCOMUtils.jsm"
@@ -16,8 +16,6 @@ const modules = {
   windowglobal: {},
 };
 
-// Modules are not exported here and the lazy getters are only here to avoid
-// errors in browser_all_files_referenced.js
 XPCOMUtils.defineLazyModuleGetters(modules.root, {
   log: "chrome://remote/content/webdriver-bidi/modules/root/log.jsm",
   session: "chrome://remote/content/webdriver-bidi/modules/root/session.jsm",
@@ -31,3 +29,33 @@ XPCOMUtils.defineLazyModuleGetters(modules["windowglobal-in-root"], {
 XPCOMUtils.defineLazyModuleGetters(modules.windowglobal, {
   log: "chrome://remote/content/webdriver-bidi/modules/windowglobal/log.jsm",
 });
+
+/**
+ * Retrieve the WebDriver BiDi module class matching the provided module name
+ * and folder.
+ *
+ * @param {String} moduleName
+ *     The name of the module to get the class for.
+ * @param {String} moduleFolder
+ *     A valid folder name for modules.
+ * @return {Class=}
+ *     The class corresponding to the module name and folder, null if no match
+ *     was found.
+ * @throws {Error}
+ *     If the provided module folder is unexpected.
+ **/
+const getModuleClass = function(moduleName, moduleFolder) {
+  if (!modules[moduleFolder]) {
+    throw new Error(
+      `Invalid module folder "${moduleFolder}", expected one of "${Object.keys(
+        modules
+      )}"`
+    );
+  }
+
+  if (!modules[moduleFolder][moduleName]) {
+    return null;
+  }
+
+  return modules[moduleFolder][moduleName];
+};