diff --git a/.eslintignore b/.eslintignore index 6422ad56831a..bb4d9af75cd2 100644 --- a/.eslintignore +++ b/.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 diff --git a/remote/shared/messagehandler/MessageHandler.jsm b/remote/shared/messagehandler/MessageHandler.jsm index 8dc6e02ef3a7..b70fec028939 100644 --- a/remote/shared/messagehandler/MessageHandler.jsm +++ b/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. * diff --git a/remote/shared/messagehandler/Module.jsm b/remote/shared/messagehandler/Module.jsm index c420bada3b81..f6791393d62e 100644 --- a/remote/shared/messagehandler/Module.jsm +++ b/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"; + } } diff --git a/remote/shared/messagehandler/ModuleCache.jsm b/remote/shared/messagehandler/ModuleCache.jsm index c0026cbd5ea1..9953043b380e 100644 --- a/remote/shared/messagehandler/ModuleCache.jsm +++ b/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; - // 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; - } + this._useTestModules = Services.prefs.getBoolPref( + "remote.messagehandler.modulecache.useBrowserTestRoot", + false + ); // 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.=>} + * 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)) { - try { - const ModuleClass = ChromeUtils.import(moduleFullPath)[moduleName]; - this._modules.set(moduleFullPath, new ModuleClass(this.messageHandler)); - 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}`); - } + if (this._modules.has(key)) { + // If there is already a cached instance (potentially null) for the + // module name + destination type pair, return it. + return this._modules.get(key); } - 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) { - let moduleFolder; - if (this.messageHandler.constructor.type === destination.type) { - // 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}`; + _getModuleClass(moduleName, moduleFolder) { + if (this._useTestModules) { + return getTestModuleClass(moduleName, moduleFolder); } - 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}`; } } diff --git a/remote/shared/messagehandler/test/browser/browser_handle_command_errors.js b/remote/shared/messagehandler/test/browser/browser_handle_command_errors.js index 4ae6a04e9117..a689607e1b21 100644 --- a/remote/shared/messagehandler/test/browser/browser_handle_command_errors.js +++ b/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"); + const fakeBrowsingContextId = -1; + ok( + !BrowsingContext.get(fakeBrowsingContextId), + "No browsing context matches fakeBrowsingContextId" + ); + 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({ - moduleName: "commandwindowglobalonly", - commandName: "testOnlyInWindowGlobal", - destination: { - type: WindowGlobalMessageHandler.type, - id: fakeBrowsingContextId, - }, - }); - ok(false, "Incorrect destination error was not caught"); - } catch (e) { - ok(true, "Incorrect destination error was caught"); - } + Assert.throws( + () => + rootMessageHandler.handleCommand({ + moduleName: "commandwindowglobalonly", + commandName: "testOnlyInWindowGlobal", + destination: { + type: WindowGlobalMessageHandler.type, + id: fakeBrowsingContextId, + }, + }), + err => err.message == `Unable to find a BrowsingContext for id -1` + ); + + 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(); }); diff --git a/remote/shared/messagehandler/test/browser/resources/modules/ModuleRegistry.jsm b/remote/shared/messagehandler/test/browser/resources/modules/ModuleRegistry.jsm new file mode 100644 index 000000000000..1fd158617e70 --- /dev/null +++ b/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; + } +}; diff --git a/remote/shared/messagehandler/test/browser/resources/modules/root/invalid.jsm b/remote/shared/messagehandler/test/browser/resources/modules/root/invalid.jsm new file mode 100644 index 000000000000..3b74769d0658 --- /dev/null +++ b/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(; diff --git a/remote/shared/messagehandler/test/browser/resources/modules/windowglobal/commandwindowglobalonly.jsm b/remote/shared/messagehandler/test/browser/resources/modules/windowglobal/commandwindowglobalonly.jsm index 22550002b736..3a7517f689ba 100644 --- a/remote/shared/messagehandler/test/browser/resources/modules/windowglobal/commandwindowglobalonly.jsm +++ b/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; diff --git a/remote/shared/messagehandler/transports/js-window-actors/MessageHandlerFrameChild.jsm b/remote/shared/messagehandler/transports/js-window-actors/MessageHandlerFrameChild.jsm index c3e8a82b7611..828615bf728e 100644 --- a/remote/shared/messagehandler/transports/js-window-actors/MessageHandlerFrameChild.jsm +++ b/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; diff --git a/remote/shared/messagehandler/transports/js-window-actors/MessageHandlerFrameParent.jsm b/remote/shared/messagehandler/transports/js-window-actors/MessageHandlerFrameParent.jsm index 140d442e6299..f6022946de0d 100644 --- a/remote/shared/messagehandler/transports/js-window-actors/MessageHandlerFrameParent.jsm +++ b/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) { - return this.sendQuery("MessageHandlerFrameParent:sendCommand", { - command, - sessionId, - }); + async sendCommand(command, sessionId) { + const result = await this.sendQuery( + "MessageHandlerFrameParent:sendCommand", + { + command, + sessionId, + } + ); + + if (result?.error) { + throw error.MessageHandlerError.fromJSON(result.error); + } + + return result; } } diff --git a/remote/webdriver-bidi/modules/ModuleRegistry.jsm b/remote/webdriver-bidi/modules/ModuleRegistry.jsm index 47e80946a72d..31af49fd9ab9 100644 --- a/remote/webdriver-bidi/modules/ModuleRegistry.jsm +++ b/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]; +};