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

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;
 };

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: {

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();
 }

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;

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() {

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) {

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,

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, () => {

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();