Bug 1631451 - [devtools] Control DevToolsClient closure from Descriptor instead of Target. r=nchevobbe,jdescottes

Still use a shouldCloseClient flag, but instead of closing the client from the target's destruction,
from which we should ignore cross process target switching,
we now close it from the descriptor destruction.
Descriptor destruction should only happen when the toolbox is meant to be closed.

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

devtools/client/framework/descriptor-from-url.js

@@ -66,6 +66,13 @@ exports.descriptorFromURL = async function descriptorFromURL(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.
+  descriptorFront.shouldCloseClient = !isCachedClient;
+
   return descriptorFront;
 };
 

devtools/client/framework/test/browser_toolbox_target.js

@@ -35,6 +35,7 @@ add_task(async function() {
   // is plenty of asynchronous steps during toolbox load
   info("Waiting for toolbox-ready");
   const toolbox = await onToolboxReady;
+  const { client } = toolbox.descriptorFront;
 
   is(
     toolbox.hostType,
@@ -53,5 +54,9 @@ add_task(async function() {
   await onToolboxDestroyed;
   info("Toolbox destroyed");
 
+  // The descriptor involved with this special case won't close
+  // the client, so we have to do it manually from this test.
+  await client.close();
+
   iframe.remove();
 });

devtools/client/framework/test/browser_two_tabs.js

@@ -108,5 +108,9 @@ 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.
+  // Bug 1698890: The test should probably stop assuming this.
+  descriptor.shouldCloseClient = false;
   return descriptor.getTarget();
 }

devtools/client/framework/toolbox-init.js

@@ -126,14 +126,6 @@ async function initToolbox(url, host) {
         showErrorPage(host.contentDocument, `${error}`);
       }
     });
-
-    // 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 Target to destroy the client as soon as it gets
-    // destroyed. This typically happens only for about:debugging toolboxes
-    // opened for local Firefox's targets.
-    const isCachedClient = url.searchParams.get("remoteId");
-    target.shouldCloseClient = !isCachedClient;
   } catch (error) {
     // When an error occurs, show error page with message.
     console.error("Exception while loading the toolbox", error);
@@ -177,9 +169,7 @@ async function _createTestOnlyDescriptor(host) {
   // XXX: Normally we don't need to fetch the target anymore, but the test
   // listens to an early event `toolbox-ready` which will kick in before
   // the rest of `initToolbox` can be done.
-  const target = await descriptor.getTarget();
-  // Instruct the Target to automatically close the client on destruction.
-  target.shouldCloseClient = true;
+  await descriptor.getTarget();
 
   return descriptor;
 }

devtools/client/fronts/descriptors/descriptor-mixin.js

@@ -26,11 +26,51 @@ 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() {
       return this._client;
     }
+
+    async destroy({ isServerDestroyEvent } = {}) {
+      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(),
+      // and so hold on before calling Front.destroy() which would reject all pending requests, including detach().
+      // When calling detach, the server will emit "descriptor-destroyed", which will call Target.destroy again,
+      // but will still be blocked on detach resolution and won't call Front.destroy, and won't reject pending requests either.
+      //
+      // So call Front.baseFrontClassDestroyed manually from here, so that we ensure rejecting the pending detach request
+      // and unblock Target.destroy resolution.
+      //
+      // Here is the inheritance chain for WorkerDescriptor:
+      // WorkerDescriptor -> Descriptor (from descriptor-mixin.js) -> Target (from target-mixin.js) -> Front (protocol.js) -> Pool (protocol.js) -> EventEmitter
+      if (isServerDestroyEvent) {
+        this.baseFrontClassDestroy();
+      }
+
+      await super.destroy();
+
+      // See comment in DescriptorMixin constructor
+      if (this.shouldCloseClient) {
+        await client.close();
+      }
+    }
   }
   return Descriptor;
 }

devtools/client/fronts/descriptors/tab.js

@@ -82,6 +82,12 @@ 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 isLocalTab() {
@@ -134,10 +140,6 @@ class TabDescriptorFront extends DescriptorMixin(
   _createTabTarget(form) {
     const front = new BrowsingContextTargetFront(this._client, null, this);
 
-    if (this.isLocalTab) {
-      front.shouldCloseClient = true;
-    }
-
     // As these fronts aren't instantiated by protocol.js, we have to set their actor ID
     // manually like that:
     front.actorID = form.actor;
@@ -158,7 +160,12 @@ class TabDescriptorFront extends DescriptorMixin(
     // so that we also have to remove the descriptor when the target is destroyed.
     // Should be kept until about:debugging supports target switching and we remove the
     // !isLocalTab check.
-    if (!this.traits.emitDescriptorDestroyed || !this.isLocalTab) {
+    // Also destroy descriptor of web extension as they expect the client to be closed immediately
+    if (
+      !this.traits.emitDescriptorDestroyed ||
+      !this.isLocalTab ||
+      this.isDevToolsExtensionContext
+    ) {
       this.destroy();
     }
   }

devtools/client/fronts/targets/browsing-context.js

@@ -118,15 +118,34 @@ class BrowsingContextTargetFront extends TargetMixin(
   }
 
   async detach() {
+    // When calling this.destroy() at the end of this method,
+    // we will end up calling detach again from TargetMixin.destroy.
+    // Avoid invalid loops and do not try to resolve only once the previous call to detach
+    // is done as it would do async infinite loop that never resolves.
+    if (this._isDetaching) {
+      return;
+    }
+    this._isDetaching = true;
+
+    // Remove listeners set in attach
+    this.off("tabNavigated", this._onTabNavigated);
+    this.off("frameUpdate", this._onFrameUpdate);
+
     try {
       await super.detach();
     } catch (e) {
       this.logDetachError(e, "browsing context");
     }
 
-    // Remove listeners set in attach
-    this.off("tabNavigated", this._onTabNavigated);
-    this.off("frameUpdate", this._onFrameUpdate);
+    // Detach will destroy the target actor, but the server won't emit any
+    // target-destroyed-form in such manual, client side destruction.
+    // So that we have to manually destroy the associated front on the client
+    //
+    // If detach was called by TargetFrontMixin.destroy, avoid recalling it from it
+    // as it would do an async infinite loop which would never resolve.
+    if (!this.isDestroyedOrBeingDestroyed()) {
+      this.destroy();
+    }
   }
 
   destroy() {

devtools/client/fronts/targets/target-mixin.js

@@ -45,12 +45,6 @@ function TargetMixin(parentClass) {
 
       this.threadFront = null;
 
-      // This flag will be set to true from:
-      // - TabDescriptorFront getTarget(), for local tab targets
-      // - descriptorFromURL(), for local targets (about:debugging)
-      // - initToolbox(), for some test-only targets
-      this.shouldCloseClient = false;
-
       this._client = client;
 
       // Cache of already created targed-scoped fronts
@@ -636,18 +630,13 @@ function TargetMixin(parentClass) {
 
       this.threadFront = null;
 
-      if (this.shouldCloseClient) {
-        try {
-          await this._client.close();
-        } catch (e) {
-          // Ignore any errors while closing, since there is not much that can be done
-          // at this point.
-          console.warn("Error while closing client:", e);
-        }
+      // This event should be emitted before calling super.destroy(), because
+      // super.destroy() will remove all event listeners attached to this front.
+      this.emit("target-destroyed");
 
-        // Not all targets supports attach/detach. For example content process doesn't.
-        // Also ensure that the front is still active before trying to do the request.
-      } else if (this.detach && !this.isDestroyed()) {
+      // Not all targets supports attach/detach. For example content process doesn't.
+      // Also ensure that the front is still active before trying to do the request.
+      if (this.detach && !this.isDestroyed()) {
         // The client was handed to us, so we are not responsible for closing
         // it. We just need to detach from the tab, if already attached.
         // |detach| may fail if the connection is already dead, so proceed with
@@ -659,10 +648,6 @@ function TargetMixin(parentClass) {
         }
       }
 
-      // This event should be emitted before calling super.destroy(), because
-      // super.destroy() will remove all event listeners attached to this front.
-      this.emit("target-destroyed");
-
       // Do that very last in order to let a chance to dispatch `detach` requests.
       super.destroy();
 

devtools/client/shared/test/browser_dbg_WorkerTargetActor.attach.js

@@ -38,7 +38,7 @@ add_task(async function() {
   let workerDescriptorFront1 = findWorker(workers, WORKER1_URL);
   await workerDescriptorFront1.attach();
   ok(
-    workerDescriptorFront1.actorID,
+    !workerDescriptorFront1.isDestroyed(),
     "front for worker in tab 1 has been attached"
   );
 
@@ -47,7 +47,7 @@ add_task(async function() {
   });
   await waitForWorkerClose(workerDescriptorFront1);
   ok(
-    !!workerDescriptorFront1.actorID,
+    workerDescriptorFront1.isDestroyed(),
     "front for worker in tab 1 has been closed"
   );
 
@@ -56,7 +56,7 @@ add_task(async function() {
   const workerDescriptorFront2 = findWorker(workers, WORKER2_URL);
   await workerDescriptorFront2.attach();
   ok(
-    workerDescriptorFront2.actorID,
+    !workerDescriptorFront2.isDestroyed(),
     "front for worker in tab 2 has been attached"
   );
 
@@ -65,7 +65,7 @@ add_task(async function() {
   });
   await waitForWorkerClose(workerDescriptorFront2);
   ok(
-    !!workerDescriptorFront2.actorID,
+    workerDescriptorFront2.isDestroyed(),
     "front for worker in tab 2 has been closed"
   );
 
@@ -73,7 +73,7 @@ add_task(async function() {
   workerDescriptorFront1 = findWorker(workers, WORKER1_URL);
   await workerDescriptorFront1.attach();
   ok(
-    workerDescriptorFront1.actorID,
+    !workerDescriptorFront1.isDestroyed(),
     "front for worker in tab 1 has been attached"
   );
 

devtools/client/shared/test/browser_dbg_listtabs-03.js

@@ -24,10 +24,8 @@ add_task(async function test() {
 
   await assertListTabs(client.mainRoot);
 
-  // To run last as it will close the client
+  // To run last as it will close the client and remove the tab
   await assertGetTab(client, client.mainRoot, tab);
-
-  await removeTab(tab);
 });
 
 async function assertListTabs(rootFront) {
@@ -43,8 +41,8 @@ async function assertListTabs(rootFront) {
 
   const tabTarget = await tabDescriptor.getTarget();
   ok(
-    !tabTarget.shouldCloseClient,
-    "Tab targets from listTabs shouldn't auto-close their client"
+    !tabDescriptor.shouldCloseClient,
+    "Tab descriptors from listTabs shouldn't auto-close their client"
   );
   ok(isTargetAttached(tabTarget), "The tab target should be attached");
 
@@ -56,6 +54,11 @@ async function assertListTabs(rootFront) {
   info("Wait for target destruction");
   await onTargetDestroyed;
 
+  // listTabs returns non-local tabs, which we suppose are remote debugging.
+  // And because of that, the TabDescriptor self-destroy itself on target being destroyed.
+  // That's because we don't yet support top level target switching in case
+  // of remote debugging. So we prefer destroy the descriptor as well as the toolbox.
+  // Bug 1698891 should revisit that.
   info("Wait for descriptor destruction");
   await onDescriptorDestroyed;
 
@@ -99,36 +102,48 @@ async function assertGetTab(client, rootFront, tab) {
 
   const tabTarget = await tabDescriptor.getTarget();
   ok(
-    tabTarget.shouldCloseClient,
-    "Tab targets from getTab shoul close their client"
+    tabDescriptor.shouldCloseClient,
+    "Tab descriptor from getTab should close their client"
   );
   ok(isTargetAttached(tabTarget), "The tab target should be attached");
 
   info("Detach the tab target");
   const onTargetDestroyed = tabTarget.once("target-destroyed");
-  const onDescriptorDestroyed = tabDescriptor.once("descriptor-destroyed");
-  const onClientClosed = client.once("closed");
   await tabTarget.detach();
 
   info("Wait for target destruction");
   await onTargetDestroyed;
 
+  // When the target is detached and destroyed,
+  // the descriptor stays alive, because the tab is still opened.
+  // And compared to listTabs, getTab's descriptor are considered local tabs.
+  // And as we support top level target switching, the descriptor can be kept alive
+  // when the target is destroyed.
+  ok(
+    !tabDescriptor.isDestroyed(),
+    "The tab descriptor isn't destroyed on target detach"
+  );
+
+  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");
   await onDescriptorDestroyed;
 
-  // Tab Descriptors returned by getTab are considered as local tabs.
-  // So that when the target is destroyed, the descriptor stays alive.
-  // But as their target have shouldCloseClient set to true... everything ends up being destroyed anyway.
   ok(
     tabTarget.isDestroyed(),
-    "The tab target should be destroyed after detach"
+    "The tab target should be destroyed after closing the tab"
   );
   ok(
     tabDescriptor.isDestroyed(),
-    "The tab descriptor kept running after detach"
+    "The tab descriptor is also always destroyed after tab closing"
   );
 
-  info("Wait for client being auto-closed by the target");
+  // 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;
 }
 

devtools/client/storage/test/browser_storage_webext_storage_local.js

@@ -46,6 +46,9 @@ async function setupLocalDevToolsServerAndClient() {
 async function setupExtensionDebuggingToolbox(id) {
   const client = await setupLocalDevToolsServerAndClient();
   const descriptor = await client.mainRoot.getAddon({ id });
+  // 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,
@@ -53,7 +56,6 @@ async function setupExtensionDebuggingToolbox(id) {
   });
 
   const target = toolbox.target;
-  target.shouldCloseClient = true;
 
   return { target, toolbox, storage };
 }

devtools/shared/commands/target/target-command.js

@@ -583,15 +583,6 @@ class TargetCommand extends EventEmitter {
     // TabDescriptor may emit the event with a null targetFront, interpret that as if the previous target
     // has already been destroyed
     if (targetFront) {
-      // By default, we do close the DevToolsClient when the target is destroyed.
-      // This happens when we close the toolbox (Toolbox.destroy calls Target.destroy),
-      // or when the tab is closes, the server emits tabDetached and the target
-      // destroy itself.
-      // Here, in the context of the process switch, the current target will be destroyed
-      // due to a tabDetached event and a we will create a new one. But we want to reuse
-      // the same client.
-      targetFront.shouldCloseClient = false;
-
       // Wait for the target to be destroyed so that TabDescriptorFactory clears its memoized target for this tab
       await targetFront.once("target-destroyed");
     }

devtools/shared/commands/target/tests/browser_target_list_various_descriptors.js

@@ -58,10 +58,12 @@ async function testLocalTab() {
 
   const tab = await addTab(TEST_URL);
   const commands = await CommandsFactory.forTab(tab);
+  // 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.
+  commands.descriptorFront.shouldCloseClient = false;
   const targetCommand = commands.targetCommand;
   await targetCommand.startListening();
-  // Avoid the target to close the client when we destroy the target list and the target
-  targetCommand.targetFront.shouldCloseClient = false;
 
   const targets = await targetCommand.getAllTargets(targetCommand.ALL_TYPES);
   is(targets.length, 1, "Got a unique target");