From 216a21b4b6bfe1dd050414c7476ae411136186f4 Mon Sep 17 00:00:00 2001 From: Julian Descottes Date: Fri, 26 Nov 2021 13:42:43 +0000 Subject: [PATCH] Bug 1743083 - [remote] Instantiate MessageHandler modules after constructor has returned r=webdriver-reviewers,whimboo Depends on D132064 Differential Revision: https://phabricator.services.mozilla.com/D132229 --- .../shared/messagehandler/MessageHandler.jsm | 15 ++--- .../messagehandler/MessageHandlerRegistry.jsm | 6 +- .../WindowGlobalMessageHandler.jsm | 24 ++++---- .../test/browser/broadcast/head.js | 8 --- .../messagehandler/test/browser/browser.ini | 1 + .../test/browser/browser_session_data.js | 4 -- .../browser_session_data_constructor_race.js | 56 +++++++++++++++++++ .../messagehandler/test/browser/head.js | 8 +++ .../modules/windowglobal/command.jsm | 17 ++++++ 9 files changed, 102 insertions(+), 37 deletions(-) create mode 100644 remote/shared/messagehandler/test/browser/browser_session_data_constructor_race.js diff --git a/remote/shared/messagehandler/MessageHandler.jsm b/remote/shared/messagehandler/MessageHandler.jsm index 10ba112bf4de..346fd4cbab1d 100644 --- a/remote/shared/messagehandler/MessageHandler.jsm +++ b/remote/shared/messagehandler/MessageHandler.jsm @@ -79,10 +79,8 @@ class MessageHandler extends EventEmitter { * ID of the session the handler is used for. * @param {Object} context * The context linked to this MessageHandler instance. - * @param {Array} sessionDataItems - * Initial session data items for this MessageHandler. */ - constructor(sessionId, context, sessionDataItems) { + constructor(sessionId, context) { super(); this._moduleCache = new ModuleCache(this); @@ -90,12 +88,6 @@ class MessageHandler extends EventEmitter { this._sessionId = sessionId; this._context = context; this._contextId = this.constructor.getIdFromContext(context); - - // If any session data item was provided to the constructor, apply it for - // this MessageHandler. - if (Array.isArray(sessionDataItems)) { - this._applyInitialSessionDataItems(sessionDataItems); - } } get contextId() { @@ -226,8 +218,11 @@ class MessageHandler extends EventEmitter { * startup. Implementation is specific to each MessageHandler class. * * By default the implementation is a no-op. + * + * @param {Array} sessionDataItems + * Initial session data items for this MessageHandler. */ - async _applyInitialSessionDataItems(sessionDataItems) {} + async applyInitialSessionDataItems(sessionDataItems) {} /** * Returns the module path corresponding to this MessageHandler class. diff --git a/remote/shared/messagehandler/MessageHandlerRegistry.jsm b/remote/shared/messagehandler/MessageHandlerRegistry.jsm index aac1e31b4ab0..0e76a9b65acf 100644 --- a/remote/shared/messagehandler/MessageHandlerRegistry.jsm +++ b/remote/shared/messagehandler/MessageHandlerRegistry.jsm @@ -193,9 +193,11 @@ class MessageHandlerRegistry extends EventEmitter { _createMessageHandler(sessionId, sessionDataItems) { const messageHandler = new this._messageHandlerClass( sessionId, - this._context, - sessionDataItems + this._context ); + + messageHandler.applyInitialSessionDataItems(sessionDataItems); + this._messageHandlersMap.set(sessionId, messageHandler); logger.trace( diff --git a/remote/shared/messagehandler/WindowGlobalMessageHandler.jsm b/remote/shared/messagehandler/WindowGlobalMessageHandler.jsm index 3764c22093cb..393faa93d4e9 100644 --- a/remote/shared/messagehandler/WindowGlobalMessageHandler.jsm +++ b/remote/shared/messagehandler/WindowGlobalMessageHandler.jsm @@ -28,9 +28,7 @@ class WindowGlobalMessageHandler extends MessageHandler { constructor() { super(...arguments); - // Bug 1743083: Temporarily delay caching of the inner window id until - // the session data is no longer applied before super() returns. - this._innerWindowId; + this._innerWindowId = this._context.window.windowGlobalChild.innerWindowId; } /** @@ -66,20 +64,14 @@ class WindowGlobalMessageHandler extends MessageHandler { } get innerWindowId() { - if (this._innerWindowId === undefined) { - this._innerWindowId = this._context.window.windowGlobalChild.innerWindowId; - } - return this._innerWindowId; } - forwardCommand(command) { - throw new Error( - `Cannot forward commands from a "WINDOW_GLOBAL" MessageHandler` - ); - } + async applyInitialSessionDataItems(sessionDataItems) { + if (!Array.isArray(sessionDataItems)) { + return; + } - async _applyInitialSessionDataItems(sessionDataItems) { for (const sessionDataItem of sessionDataItems) { const { moduleName, @@ -107,6 +99,12 @@ class WindowGlobalMessageHandler extends MessageHandler { } } + forwardCommand(command) { + throw new Error( + `Cannot forward commands from a "WINDOW_GLOBAL" MessageHandler` + ); + } + _isRelevantContext(contextDescriptor) { // Once we allow to filter on browsing contexts, the contextDescriptor would // for instance contain a browserId on top of a type. For instance: diff --git a/remote/shared/messagehandler/test/browser/broadcast/head.js b/remote/shared/messagehandler/test/browser/broadcast/head.js index 1cc3aab77151..7c52eb4782ec 100644 --- a/remote/shared/messagehandler/test/browser/broadcast/head.js +++ b/remote/shared/messagehandler/test/browser/broadcast/head.js @@ -10,14 +10,6 @@ Services.scriptloader.loadSubScript( this ); -var { CONTEXT_DESCRIPTOR_TYPES } = ChromeUtils.import( - "chrome://remote/content/shared/messagehandler/MessageHandler.jsm" -); - -var contextDescriptorAll = { - type: CONTEXT_DESCRIPTOR_TYPES.ALL, -}; - /** * Broadcast the provided method to WindowGlobal contexts on a MessageHandler * network. diff --git a/remote/shared/messagehandler/test/browser/browser.ini b/remote/shared/messagehandler/test/browser/browser.ini index 03dde4425902..31047cf4ca13 100644 --- a/remote/shared/messagehandler/test/browser/browser.ini +++ b/remote/shared/messagehandler/test/browser/browser.ini @@ -13,3 +13,4 @@ prefs = [browser_registry.js] [browser_session_data.js] [browser_session_data_broadcast.js] +[browser_session_data_constructor_race.js] diff --git a/remote/shared/messagehandler/test/browser/browser_session_data.js b/remote/shared/messagehandler/test/browser/browser_session_data.js index fbd22977b274..18c6ff110f05 100644 --- a/remote/shared/messagehandler/test/browser/browser_session_data.js +++ b/remote/shared/messagehandler/test/browser/browser_session_data.js @@ -6,9 +6,6 @@ const { MessageHandlerRegistry } = ChromeUtils.import( "chrome://remote/content/shared/messagehandler/MessageHandlerRegistry.jsm" ); -const { CONTEXT_DESCRIPTOR_TYPES } = ChromeUtils.import( - "chrome://remote/content/shared/messagehandler/MessageHandler.jsm" -); const { RootMessageHandler } = ChromeUtils.import( "chrome://remote/content/shared/messagehandler/RootMessageHandler.jsm" ); @@ -24,7 +21,6 @@ add_task(async function test_sessionData() { await loadURL(tab1.linkedBrowser, TEST_PAGE); const sessionId = "sessionData-test"; - const contextDescriptorAll = { type: CONTEXT_DESCRIPTOR_TYPES.ALL }; const rootMessageHandlerRegistry = new MessageHandlerRegistry( RootMessageHandler.type diff --git a/remote/shared/messagehandler/test/browser/browser_session_data_constructor_race.js b/remote/shared/messagehandler/test/browser/browser_session_data_constructor_race.js new file mode 100644 index 000000000000..9b1bdf7a74e5 --- /dev/null +++ b/remote/shared/messagehandler/test/browser/browser_session_data_constructor_race.js @@ -0,0 +1,56 @@ +/* Any copyright is dedicated to the Public Domain. + * http://creativecommons.org/publicdomain/zero/1.0/ */ + +"use strict"; + +const { WindowGlobalMessageHandler } = ChromeUtils.import( + "chrome://remote/content/shared/messagehandler/WindowGlobalMessageHandler.jsm" +); + +const TEST_PAGE = "https://example.com/document-builder.sjs?html=tab"; + +/** + * Check that modules created early for session data are still created with a + * fully initialized MessageHandler. See Bug 1743083. + */ +add_task(async function() { + const tab = BrowserTestUtils.addTab(gBrowser, TEST_PAGE); + await BrowserTestUtils.browserLoaded(tab.linkedBrowser); + const browsingContext = tab.linkedBrowser.browsingContext; + + const root = createRootMessageHandler("session-id-event"); + + info("Add some session data for the command module"); + await root.addSessionData({ + moduleName: "command", + category: "testCategory", + contextDescriptor: contextDescriptorAll, + values: ["some-value"], + }); + + info("Reload the current tab to create new message handlers and modules"); + const finished = BrowserTestUtils.browserLoaded(tab.linkedBrowser); + gBrowser.reloadTab(tab); + await finished; + + info( + "Check if the command module was created by the MessageHandler constructor" + ); + const isCreatedByMessageHandlerConstructor = await root.handleCommand({ + moduleName: "command", + commandName: "testIsCreatedByMessageHandlerConstructor", + destination: { + type: WindowGlobalMessageHandler.type, + id: browsingContext.id, + }, + }); + + is( + isCreatedByMessageHandlerConstructor, + false, + "The command module from session data should not be created by the MessageHandler constructor" + ); + root.destroy(); + + gBrowser.removeTab(tab); +}); diff --git a/remote/shared/messagehandler/test/browser/head.js b/remote/shared/messagehandler/test/browser/head.js index 65c1477e4643..aa56b079c811 100644 --- a/remote/shared/messagehandler/test/browser/head.js +++ b/remote/shared/messagehandler/test/browser/head.js @@ -4,6 +4,14 @@ "use strict"; +var { CONTEXT_DESCRIPTOR_TYPES } = ChromeUtils.import( + "chrome://remote/content/shared/messagehandler/MessageHandler.jsm" +); + +var contextDescriptorAll = { + type: CONTEXT_DESCRIPTOR_TYPES.ALL, +}; + function createRootMessageHandler(sessionId) { const { RootMessageHandlerRegistry } = ChromeUtils.import( "chrome://remote/content/shared/messagehandler/RootMessageHandlerRegistry.jsm" diff --git a/remote/shared/messagehandler/test/browser/resources/modules/windowglobal/command.jsm b/remote/shared/messagehandler/test/browser/resources/modules/windowglobal/command.jsm index c4c4e8ca6b2a..d757e8f5f7ef 100644 --- a/remote/shared/messagehandler/test/browser/resources/modules/windowglobal/command.jsm +++ b/remote/shared/messagehandler/test/browser/resources/modules/windowglobal/command.jsm @@ -14,6 +14,8 @@ class Command extends Module { constructor(messageHandler) { super(messageHandler); this._testCategorySessionData = []; + + this._createdByMessageHandlerConstructor = this._isCreatedByMessageHandlerConstructor(); } destroy() {} @@ -53,6 +55,21 @@ class Command extends Module { testForwardToWindowGlobal() { return "forward-to-windowglobal-value"; } + + testIsCreatedByMessageHandlerConstructor() { + return this._createdByMessageHandlerConstructor; + } + + _isCreatedByMessageHandlerConstructor() { + let caller = Components.stack.caller; + while (caller) { + if (caller.name === this.messageHandler.constructor.name) { + return true; + } + caller = caller.caller; + } + return false; + } } const command = Command;