From a6ca7867b418ceec38bbe7128b604b4d8d56371e Mon Sep 17 00:00:00 2001 From: Alexandre Poirot Date: Tue, 11 Oct 2022 20:33:52 +0000 Subject: [PATCH] Bug 1700909 - [devtools] Migrate DescriptorMixin.shouldCloseClient to Commands. r=jdescottes Note that this depends on the following patch, in order to destroy commands from the toolbox. Commands have always been destroying the related client. This was a significant simplification in test. So that I had to reverse the default value of shouldCloseClient and only toggle it to false where it is strictly needed. Ideally only for about:debugging toolboxes (and two tests). Differential Revision: https://phabricator.services.mozilla.com/D158206 --- .../framework/browser-toolbox/window.js | 6 +++ .../client/framework/commands-from-url.js | 13 +++--- devtools/client/framework/devtools.js | 6 --- .../client/framework/test/browser_two_tabs.js | 16 ++++--- .../fronts/descriptors/descriptor-mixin.js | 17 ------- devtools/client/fronts/descriptors/tab.js | 6 --- .../shared/test/browser_dbg_listtabs-03.js | 14 +----- .../browser_storage_webext_storage_local.js | 3 -- devtools/shared/commands/index.js | 45 +++++++++++++++++-- ...wser_target_command_various_descriptors.js | 5 --- 10 files changed, 67 insertions(+), 64 deletions(-) diff --git a/devtools/client/framework/browser-toolbox/window.js b/devtools/client/framework/browser-toolbox/window.js index 85a3eb4071c4..0561941ad57d 100644 --- a/devtools/client/framework/browser-toolbox/window.js +++ b/devtools/client/framework/browser-toolbox/window.js @@ -117,6 +117,12 @@ var connect = async function() { appendStatusMessage("Get root form for toolbox"); gCommands = await CommandsFactory.forMainProcess({ client }); + + // Bug 1794607: for some unexpected reason, closing the DevToolsClient + // when the commands is destroyed by the toolbox would introduce leaks + // when running the browser-toolbox mochitests. + gCommands.shouldCloseClient = false; + await openToolbox(gCommands); }; diff --git a/devtools/client/framework/commands-from-url.js b/devtools/client/framework/commands-from-url.js index e7d94894beac..16569fea90ae 100644 --- a/devtools/client/framework/commands-from-url.js +++ b/devtools/client/framework/commands-from-url.js @@ -73,12 +73,13 @@ exports.commandsFromURL = async function commandsFromURL(url) { throw e; } - // If this isn't a cached client, it means that we just created a new client - // in `clientFromURL` and we have to destroy it at some point. - // In such case, force the Descriptor to destroy the client as soon as it gets - // destroyed. This typically happens only for about:debugging toolboxes - // opened for local Firefox's targets. - commands.shouldCloseClient = !isCachedClient; + // When opening about:debugging's toolboxes for remote runtimes, + // we create a new commands using a shared and cached client. + // Prevent closing the DevToolsClient on toolbox close and Commands destruction + // as this can be used by about:debugging and other toolboxes. + if (isCachedClient) { + commands.shouldCloseClient = false; + } return commands; }; diff --git a/devtools/client/framework/devtools.js b/devtools/client/framework/devtools.js index 1432a2901ca5..6ae76536f891 100644 --- a/devtools/client/framework/devtools.js +++ b/devtools/client/framework/devtools.js @@ -655,12 +655,6 @@ DevTools.prototype = { this._commandsPromiseByWebExtId.delete(extensionId); }); - // CommandsFactory.forAddon will spawn a new DevToolsClient. - // And by default, the WebExtensionDescriptor won't close the DevToolsClient - // when the toolbox closes and fronts are destroyed. - // Ensure we do close it, similarly to local tab debugging. - commands.descriptorFront.shouldCloseClient = true; - return this.showToolbox(commands.descriptorFront, { hostType: Toolbox.HostType.WINDOW, hostOptions: { diff --git a/devtools/client/framework/test/browser_two_tabs.js b/devtools/client/framework/test/browser_two_tabs.js index aeca6297ccf6..b9b9550db62e 100644 --- a/devtools/client/framework/test/browser_two_tabs.js +++ b/devtools/client/framework/test/browser_two_tabs.js @@ -101,10 +101,16 @@ async function checkFirstTargetActor(targetFront1) { } async function getTabTarget(client, filter) { - const descriptor = await client.mainRoot.getTab(filter); - // By default, descriptor returned by getTab will close the client - // when the tab is closed. Disable this default behavior for this test. + let commands; + if (filter.tab) { + commands = await CommandsFactory.forTab(filter.tab, { client }); + } else if (filter.browserId) { + commands = await CommandsFactory.forRemoteTab(filter.browserId, { client }); + } + await commands.targetCommand.startListening(); + // By default, commands will close the client when the tab is closed. + // Disable this default behavior for this test. // Bug 1698890: The test should probably stop assuming this. - descriptor.shouldCloseClient = false; - return descriptor.getTarget(); + commands.shouldCloseClient = false; + return commands.descriptorFront.getTarget(); } diff --git a/devtools/client/fronts/descriptors/descriptor-mixin.js b/devtools/client/fronts/descriptors/descriptor-mixin.js index 8b49ae7e99c0..ac38e104b5cf 100644 --- a/devtools/client/fronts/descriptors/descriptor-mixin.js +++ b/devtools/client/fronts/descriptors/descriptor-mixin.js @@ -26,15 +26,6 @@ function DescriptorMixin(parentClass) { "descriptor-destroyed", this.destroy.bind(this, { isServerDestroyEvent: true }) ); - - // Boolean flag to know if the DevtoolsClient should be closed - // when this descriptor happens to be destroyed. - // This is set by: - // * target-from-url in case we are opening a toolbox - // with a dedicated DevToolsClient (mostly from about:debugging, when the client isn't "cached"). - // * TabDescriptor, when we are connecting to a local tab and expect - // the client, toolbox and descriptor to all follow the same lifecycle. - this.shouldCloseClient = false; } get client() { @@ -45,9 +36,6 @@ function DescriptorMixin(parentClass) { if (this.isDestroyed()) { return; } - // Cache the client attribute as in case of workers, TargetMixin class may nullify `_client` - const { client } = this; - // This workaround is mostly done for Workers, as WorkerDescriptor // extends the Target class, which causes some issue down the road: // In Target.destroy, we call WorkerDescriptorActor.detach *before* calling super.destroy(), @@ -65,11 +53,6 @@ function DescriptorMixin(parentClass) { } await super.destroy(); - - // See comment in DescriptorMixin constructor - if (this.shouldCloseClient) { - await client.close(); - } } } return Descriptor; diff --git a/devtools/client/fronts/descriptors/tab.js b/devtools/client/fronts/descriptors/tab.js index f4433f96ce89..e9b6c2946614 100644 --- a/devtools/client/fronts/descriptors/tab.js +++ b/devtools/client/fronts/descriptors/tab.js @@ -115,12 +115,6 @@ class TabDescriptorFront extends DescriptorMixin( setLocalTab(localTab) { this._localTab = localTab; this._setupLocalTabListeners(); - - // This is pure legacy. We always assumed closing the DevToolsClient - // when the tab was closed. It is mostly important for tests, - // but also ensure cleaning up the client and everything on tab closing. - // (this flag is handled by DescriptorMixin) - this.shouldCloseClient = true; } get isTabDescriptor() { diff --git a/devtools/client/shared/test/browser_dbg_listtabs-03.js b/devtools/client/shared/test/browser_dbg_listtabs-03.js index 7a4c257a1c9b..3306605707eb 100644 --- a/devtools/client/shared/test/browser_dbg_listtabs-03.js +++ b/devtools/client/shared/test/browser_dbg_listtabs-03.js @@ -50,10 +50,6 @@ async function assertListTabs(tab, rootFront) { ); const tabTarget = await tabDescriptor.getTarget(); - ok( - !tabDescriptor.shouldCloseClient, - "Tab descriptors from listTabs shouldn't auto-close their client" - ); ok(isTargetAttached(tabTarget), "The tab target should be attached"); info("Detach the tab target"); @@ -116,10 +112,6 @@ async function assertGetTab(client, rootFront, tab) { await removeTab(tab2); const tabTarget = await tabDescriptor.getTarget(); - ok( - tabDescriptor.shouldCloseClient, - "Tab descriptor from getTab should close their client" - ); ok(isTargetAttached(tabTarget), "The tab target should be attached"); info("Detach the tab target"); @@ -141,7 +133,6 @@ async function assertGetTab(client, rootFront, tab) { info("Close the descriptor's tab"); const onDescriptorDestroyed = tabDescriptor.once("descriptor-destroyed"); - const onClientClosed = client.once("closed"); await removeTab(tab); info("Wait for descriptor destruction"); @@ -156,10 +147,7 @@ async function assertGetTab(client, rootFront, tab) { "The tab descriptor is also always destroyed after tab closing" ); - // Tab Descriptors returned by getTab({ tab }) are considered as local tabs - // and auto-close their client. - info("Wait for client being auto-closed by the descriptor"); - await onClientClosed; + await client.close(); } function isTargetAttached(targetFront) { diff --git a/devtools/client/storage/test/browser_storage_webext_storage_local.js b/devtools/client/storage/test/browser_storage_webext_storage_local.js index 110b6d2ffb05..0dc06e3442e0 100644 --- a/devtools/client/storage/test/browser_storage_webext_storage_local.js +++ b/devtools/client/storage/test/browser_storage_webext_storage_local.js @@ -19,9 +19,6 @@ const { Toolbox } = require("resource://devtools/client/framework/toolbox.js"); async function setupExtensionDebuggingToolbox(id) { const commands = await CommandsFactory.forAddon(id); const descriptor = commands.descriptorFront; - // As this mimic about:debugging toolbox, by default, the toolbox won't close - // the client on shutdown. So request it to do that here, via the descriptor. - descriptor.shouldCloseClient = true; const { toolbox, storage } = await openStoragePanel({ descriptor, diff --git a/devtools/shared/commands/index.js b/devtools/shared/commands/index.js index ada7c28e7d22..01bd52895a7b 100644 --- a/devtools/shared/commands/index.js +++ b/devtools/shared/commands/index.js @@ -52,18 +52,57 @@ async function createCommandsDictionary(descriptorFront) { return descriptorFront.client.waitForRequestsToSettle(); }, - // We want to keep destroy being defined last + // Boolean flag to know if the DevtoolsClient should be closed + // when this commands happens to be destroyed. + // This is set by: + // * commands-from-url in case we are opening a toolbox + // with a dedicated DevToolsClient (mostly from about:debugging, when the client isn't "cached"). + // * CommandsFactory, when we are connecting to a local tab and expect + // the client, toolbox and descriptor to all follow the same lifecycle. + shouldCloseClient: true, + + /** + * Destroy the commands which will destroy: + * - all inner commands, + * - the related descriptor, + * - the related DevToolsClient (not always) + */ async destroy() { + descriptorFront.off("descriptor-destroyed", this.destroy); + + // Destroy all inner command modules for (const command of allInstantiatedCommands) { if (typeof command.destroy == "function") { command.destroy(); } } allInstantiatedCommands.clear(); - await descriptorFront.destroy(); - await client.close(); + + // Destroy the descriptor front, and all its children fronts. + // Watcher, targets,... + // + // Note that DescriptorFront.destroy will be null because of Pool.destroy + // when this function is called while the descriptor front itself is being + // destroyed. + if (!descriptorFront.isDestroyed()) { + await descriptorFront.destroy(); + } + + // Close the DevToolsClient. Shutting down the connection + // to the debuggable context and its DevToolsServer. + // + // See shouldCloseClient jsdoc about this condition. + if (this.shouldCloseClient) { + await client.close(); + } }, }; + dictionary.destroy = dictionary.destroy.bind(dictionary); + + // Automatically destroy the commands object if the descriptor + // happens to be destroyed. Which means that the debuggable context + // is no longer debuggable. + descriptorFront.on("descriptor-destroyed", dictionary.destroy); for (const name in Commands) { loader.lazyGetter(dictionary, name, () => { diff --git a/devtools/shared/commands/target/tests/browser_target_command_various_descriptors.js b/devtools/shared/commands/target/tests/browser_target_command_various_descriptors.js index 4951dbe2574d..7883ade03fc4 100644 --- a/devtools/shared/commands/target/tests/browser_target_command_various_descriptors.js +++ b/devtools/shared/commands/target/tests/browser_target_command_various_descriptors.js @@ -91,11 +91,6 @@ async function testLocalTab() { "Descriptor front isTabDescriptor is correct" ); - // By default, tab descriptor will close the client when destroying the client - // Disable this behavior via this boolean - // Bug 1698890: The test should probably stop assuming this. - descriptorFront.shouldCloseClient = false; - const targetCommand = commands.targetCommand; await targetCommand.startListening();