Backed out 8 changesets (bug 1631451, bug 1698842) for xpcshell failures on test_breakpoint-25.js. CLOSED TREE

Backed out changeset 0d9035ed5c50 (bug 1698842)
Backed out changeset 55166c719cd2 (bug 1698842)
Backed out changeset 8cc10a259de1 (bug 1631451)
Backed out changeset bc9072114706 (bug 1631451)
Backed out changeset b5cb78fd1f61 (bug 1631451)
Backed out changeset da765754e3a8 (bug 1631451)
Backed out changeset d7a1c44ea37c (bug 1631451)
Backed out changeset 810bdfa0d899 (bug 1631451)

devtools/client/devtools-client.js

@@ -113,26 +113,33 @@ DevToolsClient.prototype = {
    *         Resolves after the underlying transport is closed.
    */
   close() {
-    if (this._closed) {
-      return Promise.resolve();
-    }
-    if (this._closePromise) {
-      return this._closePromise;
-    }
-    // Immediately set the destroy promise,
-    // as the following code is fully synchronous and can be reentrant.
-    this._closePromise = this.once("closed");
+    const promise = new Promise(resolve => {
+      // Disable detach event notifications, because event handlers will be in a
+      // cleared scope by the time they run.
+      this._eventsEnabled = false;
 
-    // Disable detach event notifications, because event handlers will be in a
-    // cleared scope by the time they run.
-    this._eventsEnabled = false;
+      const cleanup = () => {
+        if (this._transport) {
+          this._transport.close();
+        }
+        this._transport = null;
+      };
 
-    if (this._transport) {
-      this._transport.close();
-      this._transport = null;
-    }
+      // If the connection is already closed,
+      // there is no need to detach client
+      // as we won't be able to send any message.
+      if (this._closed) {
+        cleanup();
+        resolve();
+        return;
+      }
 
-    return this._closePromise;
+      this.once("closed", resolve);
+
+      cleanup();
+    });
+
+    return promise;
   },
 
   /**

devtools/client/framework/browser-toolbox/test/helpers-browser-toolbox.js

@@ -167,13 +167,7 @@ async function initBrowserToolboxTask({
 
   async function destroy() {
     const closePromise = process._dbgProcess.wait();
-    consoleFront.evaluateJSAsync("gToolbox.destroy()").catch(e => {
-      // Ignore connection close as the toolbox destroy may destroy
-      // everything quickly enough so that evaluate request is still pending
-      if (!e.message.includes("Connection closed")) {
-        throw e;
-      }
-    });
+    consoleFront.evaluateJSAsync("gToolbox.destroy()");
 
     const { exitCode } = await closePromise;
     ok(true, "Browser toolbox process closed");

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

@@ -66,13 +66,6 @@ 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_target_server_compartment.js

@@ -64,7 +64,7 @@ async function testChromeTab() {
     Services.obs.addObserver(observe, "devtools:loader:destroy");
   });
 
-  await target.descriptorFront.destroy();
+  await target.destroy();
 
   // Wait for the dedicated loader used for DevToolsServer to be destroyed
   // in order to prevent leak reports on try

devtools/client/framework/test/browser_toolbox_target.js

@@ -35,7 +35,6 @@ 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,
@@ -54,9 +53,5 @@ 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,9 +108,5 @@ 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,6 +126,14 @@ 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);
@@ -169,7 +177,9 @@ 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.
-  await descriptor.getTarget();
+  const target = await descriptor.getTarget();
+  // Instruct the Target to automatically close the client on destruction.
+  target.shouldCloseClient = true;
 
   return descriptor;
 }

devtools/client/framework/toolbox.js

@@ -3800,7 +3800,7 @@ Toolbox.prototype = {
 
     // Finish all outstanding tasks (which means finish destroying panels and
     // then destroying the host, successfully or not) before destroying the
-    // target descriptor.
+    // target.
     const onceDestroyed = new Promise(resolve => {
       resolve(
         settleAll(outstanding)
@@ -3836,7 +3836,7 @@ Toolbox.prototype = {
             // will lead to destroy frame targets which can temporarily make
             // some fronts unresponsive and block the cleanup.
             this.commands.targetCommand.destroy();
-            return this.descriptorFront.destroy();
+            return this.target.destroy();
           }, console.error)
           .then(() => {
             this.emit("destroyed");

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

@@ -28,15 +28,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() {
@@ -49,37 +40,6 @@ function DescriptorMixin(parentClass) {
       }
       return this._commands;
     }
-
-    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

@@ -86,12 +86,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 isLocalTab() {
@@ -144,6 +138,10 @@ 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;
@@ -164,12 +162,7 @@ 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.
-    // Also destroy descriptor of web extension as they expect the client to be closed immediately
-    if (
-      !this.traits.emitDescriptorDestroyed ||
-      !this.isLocalTab ||
-      this.isDevToolsExtensionContext
-    ) {
+    if (!this.traits.emitDescriptorDestroyed || !this.isLocalTab) {
       this.destroy();
     }
   }
@@ -202,26 +195,19 @@ class TabDescriptorFront extends DescriptorMixin(
     }
 
     this._targetFrontPromise = (async () => {
-      let newTargetFront = null;
+      let targetFront = null;
       try {
         const targetForm = await super.getTarget();
-        newTargetFront = this._createTabTarget(targetForm);
-        await newTargetFront.attach();
+        targetFront = this._createTabTarget(targetForm);
+        await targetFront.attach();
       } catch (e) {
         console.log(
           `Request to connect to TabDescriptor "${this.id}" failed: ${e}`
         );
       }
-
-      // Completely ignore the previous target.
-      // We might nullify the _targetFront unexpectely due to previous target
-      // being destroyed after the new is created
-      if (this._targetFront) {
-        this._targetFront.off("target-destroyed", this._onTargetDestroyed);
-      }
-      this._targetFront = newTargetFront;
+      this._targetFront = targetFront;
       this._targetFrontPromise = null;
-      return newTargetFront;
+      return targetFront;
     })();
     return this._targetFrontPromise;
   }

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

@@ -127,16 +127,6 @@ class BrowsingContextTargetFront extends TargetMixin(
     // 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()) {
-      await this.destroy();
-    }
   }
 
   destroy() {

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

@@ -45,6 +45,12 @@ 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
@@ -559,6 +565,14 @@ function TargetMixin(parentClass) {
      */
     _addListeners() {
       this.client.on("closed", this.destroy);
+
+      // `tabDetached` is sent by all target targets types: frame, process and workers.
+      // This is sent when the target is destroyed:
+      // * the target context destroys itself (the tab closes for ex, or the worker shuts down)
+      //   in this case, it may be the connector that send this event in the name of the target actor
+      // * the target actor is destroyed, but the target context stays up and running (for ex, when we call Watcher.unwatchTargets)
+      // * the DevToolsServerConnection closes (client closes the connection)
+      this.on("tabDetached", this.destroy);
     }
 
     /**
@@ -569,6 +583,7 @@ function TargetMixin(parentClass) {
       if (this.client) {
         this.client.off("closed", this.destroy);
       }
+      this.off("tabDetached", this.destroy);
 
       // Remove listeners set in attachConsole
       if (this.removeOnInspectObjectListener) {
@@ -630,13 +645,18 @@ function TargetMixin(parentClass) {
 
       this.threadFront = null;
 
-      // 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");
+      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);
+        }
 
-      // 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()) {
+        // 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()) {
         // 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
@@ -648,6 +668,10 @@ 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.isDestroyed(),
+    workerDescriptorFront1.actorID,
     "front for worker in tab 1 has been attached"
   );
 
@@ -47,7 +47,7 @@ add_task(async function() {
   });
   await waitForWorkerClose(workerDescriptorFront1);
   ok(
-    workerDescriptorFront1.isDestroyed(),
+    !!workerDescriptorFront1.actorID,
     "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.isDestroyed(),
+    workerDescriptorFront2.actorID,
     "front for worker in tab 2 has been attached"
   );
 
@@ -65,7 +65,7 @@ add_task(async function() {
   });
   await waitForWorkerClose(workerDescriptorFront2);
   ok(
-    workerDescriptorFront2.isDestroyed(),
+    !!workerDescriptorFront2.actorID,
     "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.isDestroyed(),
+    workerDescriptorFront1.actorID,
     "front for worker in tab 1 has been attached"
   );
 

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

@@ -24,8 +24,10 @@ add_task(async function test() {
 
   await assertListTabs(client.mainRoot);
 
-  // To run last as it will close the client and remove the tab
+  // To run last as it will close the client
   await assertGetTab(client, client.mainRoot, tab);
+
+  await removeTab(tab);
 });
 
 async function assertListTabs(rootFront) {
@@ -41,8 +43,8 @@ async function assertListTabs(rootFront) {
 
   const tabTarget = await tabDescriptor.getTarget();
   ok(
-    !tabDescriptor.shouldCloseClient,
-    "Tab descriptors from listTabs shouldn't auto-close their client"
+    !tabTarget.shouldCloseClient,
+    "Tab targets from listTabs shouldn't auto-close their client"
   );
   ok(isTargetAttached(tabTarget), "The tab target should be attached");
 
@@ -54,11 +56,6 @@ 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;
 
@@ -102,48 +99,36 @@ async function assertGetTab(client, rootFront, tab) {
 
   const tabTarget = await tabDescriptor.getTarget();
   ok(
-    tabDescriptor.shouldCloseClient,
-    "Tab descriptor from getTab should close their client"
+    tabTarget.shouldCloseClient,
+    "Tab targets from getTab shoul 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 closing the tab"
+    "The tab target should be destroyed after detach"
   );
   ok(
     tabDescriptor.isDestroyed(),
-    "The tab descriptor is also always destroyed after tab closing"
+    "The tab descriptor kept running after detach"
   );
 
-  // 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");
+  info("Wait for client being auto-closed by the target");
   await onClientClosed;
 }
 

devtools/client/storage/test/browser_storage_webext_storage_local.js

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

devtools/server/tests/browser/browser_debugger_server.js

@@ -30,7 +30,8 @@ async function testDevToolsServerInitialized() {
     "By default, the DevToolsServer isn't initialized not in content process"
   );
 
-  const descriptor = await createDescriptorForTabAndInitTarget(tab);
+  const descriptor = await TabDescriptorFactory.createDescriptorForTab(tab);
+  const target = await descriptor.getTarget();
 
   ok(
     DevToolsServer.initialized,
@@ -42,19 +43,19 @@ async function testDevToolsServerInitialized() {
     "Creating the target will initialize the DevToolsServer in content process"
   );
 
-  await descriptor.destroy();
+  await target.destroy();
 
   // Disconnecting the client will remove all connections from both server,
   // in parent and content process. But only the one in the content process will be
   // destroyed.
   ok(
     DevToolsServer.initialized,
-    "Destroying the descriptor doesn't destroy the DevToolsServer in the parent process"
+    "Destroying the target doesn't destroy the DevToolsServer in the parent process"
   );
   await assertServerInitialized(
     browser,
     false,
-    "But destroying the descriptor ends up destroying the DevToolsServer in the content" +
+    "But destroying the target ends up destroying the DevToolsServer in the content" +
       " process"
   );
 
@@ -72,7 +73,8 @@ async function testDevToolsServerKeepAlive() {
     "Server not started in content process"
   );
 
-  const descriptor = await createDescriptorForTabAndInitTarget(tab);
+  const descriptor = await TabDescriptorFactory.createDescriptorForTab(tab);
+  const target = await descriptor.getTarget();
 
   await assertServerInitialized(
     browser,
@@ -83,8 +85,8 @@ async function testDevToolsServerKeepAlive() {
   info("Set DevToolsServer.keepAlive to true in the content process");
   await setContentServerKeepAlive(browser, true);
 
-  info("Destroy the descriptor, the content server should be kept alive");
-  await descriptor.destroy();
+  info("Destroy the target, the content server should be kept alive");
+  await target.destroy();
 
   await assertServerInitialized(
     browser,
@@ -95,10 +97,11 @@ async function testDevToolsServerKeepAlive() {
   info("Set DevToolsServer.keepAlive back to false");
   await setContentServerKeepAlive(browser, false);
 
-  info("Create and destroy a descriptor again");
-  const newDescriptor = await createDescriptorForTabAndInitTarget(tab);
+  info("Create and destroy a target again");
+  const newDescriptor = await TabDescriptorFactory.createDescriptorForTab(tab);
+  const newTarget = await newDescriptor.getTarget();
 
-  await newDescriptor.destroy();
+  await newTarget.destroy();
 
   await assertServerInitialized(
     browser,
@@ -130,11 +133,3 @@ async function setContentServerKeepAlive(browser, keepAlive, message) {
     DevToolsServer.keepAlive = _keepAlive;
   });
 }
-
-async function createDescriptorForTabAndInitTarget(tab) {
-  const descriptor = await TabDescriptorFactory.createDescriptorForTab(tab);
-  // Force spawning the target as that's what creates a DevToolsServer in content processes.
-  // Only creating the descriptor will only spawn actors in the parent process.
-  await descriptor.getTarget();
-  return descriptor;
-}

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

@@ -144,9 +144,7 @@ class TargetCommand extends EventEmitter {
       for (const target of this._targets) {
         // We only consider the top level target to be switched
         const isDestroyedTargetSwitching = target == this.targetFront;
-        this._onTargetDestroyed(target, {
-          isTargetSwitching: isDestroyedTargetSwitching,
-        });
+        this._onTargetDestroyed(target, isDestroyedTargetSwitching);
       }
       // Stop listening to legacy listeners as we now have to listen
       // on the new target.
@@ -176,10 +174,6 @@ class TargetCommand extends EventEmitter {
       targetFrontsSet.delete(targetFront);
     }
 
-    if (this.isDestroyed() || targetFront.isDestroyedOrBeingDestroyed()) {
-      return;
-    }
-
     // Then, once the target is attached, notify the target front creation listeners
     await this._createListeners.emitAsync(targetType, {
       targetFront,
@@ -196,39 +190,7 @@ class TargetCommand extends EventEmitter {
     this.emitForTests("processed-available-target", targetFront);
   }
 
-  /**
-   * Function fired everytime a target is destroyed.
-   *
-   * This is called either:
-   * - via target-destroyed event fired by the WatcherFront,
-   *   event which is a simple translation of the target-destroyed-form emitted by the WatcherActor.
-   *   Watcher Actor emits this is various condition when the debugged target is meant to be destroyed:
-   *   - the related target context is destroyed (tab closed, worker shut down, content process destroyed, ...),
-   *   - when the DevToolsServerConnection used on the server side to communicate to the client is closed.
-
-   * - by TargetCommand._onTargetAvailable, when a top level target switching happens and all previously
-   *   registered target fronts should be destroyed.
-
-   * - by the legacy Targets listeners, calling this method directly.
-   *   This usecase is meant to be removed someday when all target targets are supported by the Watcher.
-   *   (bug 1687459)
-   *
-   * @param {TargetFront} targetFront
-   *        The target that just got destroyed.
-   * @param Object options
-   *        Dictionary object with:
-   *        - `isTargetSwitching` optional boolean. To be set to true when this
-   *           is about the top level target which is being replaced by a new one.
-   *           The passed target should be still the one store in TargetCommand.targetFront
-   *           and will be replaced via a call to onTargetAvailable with a new target front.
-   *        - `shouldDestroyTargetFront` optional boolean. By default, the passed target
-   *           front will be destroyed. But in some cases like legacy listeners for service workers
-   *           we want to keep the front alive.
-   */
-  _onTargetDestroyed(
-    targetFront,
-    { isTargetSwitching = false, shouldDestroyTargetFront = true } = {}
-  ) {
+  _onTargetDestroyed(targetFront, isTargetSwitching = false) {
     // The watcher actor may notify us about the destruction of the top level target.
     // But second argument to this method, isTargetSwitching is only passed from the frontend.
     // So automatically toggle the isTargetSwitching flag for server side destructions
@@ -241,10 +203,6 @@ class TargetCommand extends EventEmitter {
       isTargetSwitching,
     });
     this._targets.delete(targetFront);
-
-    if (shouldDestroyTargetFront) {
-      targetFront.destroy();
-    }
   }
 
   _setListening(type, value) {
@@ -308,8 +266,6 @@ class TargetCommand extends EventEmitter {
       const supportsWatcher = this.descriptorFront.traits?.watcher;
       if (supportsWatcher) {
         this.watcherFront = await this.descriptorFront.getWatcher();
-        this.watcherFront.on("target-available", this._onTargetAvailable);
-        this.watcherFront.on("target-destroyed", this._onTargetDestroyed);
       }
     }
 
@@ -356,10 +312,18 @@ class TargetCommand extends EventEmitter {
         // When we switch to a new top level target, we don't have to stop and restart
         // Watcher listener as it is independant from the top level target.
         // This isn't the case for some Legacy Listeners, which fetch targets from the top level target
-        if (!onlyLegacy) {
-          await this.watcherFront.watchTargets(type);
+        if (onlyLegacy) {
+          continue;
         }
-      } else if (this.legacyImplementation[type]) {
+        if (!this._startedListeningToWatcher) {
+          this._startedListeningToWatcher = true;
+          this.watcherFront.on("target-available", this._onTargetAvailable);
+          this.watcherFront.on("target-destroyed", this._onTargetDestroyed);
+        }
+        await this.watcherFront.watchTargets(type);
+        continue;
+      }
+      if (this.legacyImplementation[type]) {
         await this.legacyImplementation[type].listen();
       } else {
         throw new Error(`Unsupported target type '${type}'`);
@@ -391,7 +355,9 @@ class TargetCommand extends EventEmitter {
         if (!onlyLegacy) {
           this.watcherFront.unwatchTargets(type);
         }
-      } else if (this.legacyImplementation[type]) {
+        continue;
+      }
+      if (this.legacyImplementation[type]) {
         this.legacyImplementation[type].unlisten();
       } else {
         throw new Error(`Unsupported target type '${type}'`);
@@ -580,12 +546,17 @@ class TargetCommand extends EventEmitter {
    *        The BrowsingContextTargetFront instance that navigated to another process
    */
   async onLocalTabRemotenessChange(targetFront) {
-    // Wait for the target to be destroyed so that TabDescriptorFactory clears its memoized descriptor for this tab
-    // TabDescriptor may emit the event with a null targetFront, interpret that as if the previous target
-    // has already been destroyed
-    if (targetFront) {
-      await targetFront.once("target-destroyed");
-    }
+    // 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");
 
     // Fetch the new target from the descriptor.
     const newTarget = await this.descriptorFront.getTarget();

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

@@ -6,7 +6,6 @@
 // Test the TargetCommand API with all possible descriptors
 
 const TEST_URL = "https://example.org/document-builder.sjs?html=org";
-const SECOND_TEST_URL = "https://example.com/document-builder.sjs?html=org";
 const CHROME_WORKER_URL = CHROME_URL_ROOT + "test_worker.js";
 
 add_task(async function() {
@@ -21,8 +20,7 @@ add_task(async function() {
   const client = await createLocalClient();
   const mainRoot = client.mainRoot;
 
-  await testLocalTab(mainRoot);
-  await testRemoteTab(mainRoot);
+  await testTab(mainRoot);
   await testParentProcess(mainRoot);
   await testContentProcess(mainRoot);
   await testWorker(mainRoot);
@@ -57,18 +55,16 @@ async function testParentProcess(rootFront) {
   await waitForAllTargetsToBeAttached(targetList);
 }
 
-async function testLocalTab(rootFront) {
-  info("Test TargetCommand against local tab descriptor (via getTab({ tab }))");
+async function testTab(rootFront) {
+  info("Test TargetCommand against tab descriptor");
 
   const tab = await addTab(TEST_URL);
   const descriptor = await rootFront.getTab({ 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.
-  descriptor.shouldCloseClient = false;
   const commands = await descriptor.getCommands();
   const targetList = commands.targetCommand;
   await targetList.startListening();
+  // Avoid the target to close the client when we destroy the target list and the target
+  targetList.targetFront.shouldCloseClient = false;
 
   const targets = await targetList.getAllTargets(targetList.ALL_TYPES);
   is(targets.length, 1, "Got a unique target");
@@ -86,56 +82,6 @@ async function testLocalTab(rootFront) {
   BrowserTestUtils.removeTab(tab);
 }
 
-async function testRemoteTab(rootFront) {
-  info(
-    "Test TargetCommand against remote tab descriptor (via getTab({ outerWindowID }))"
-  );
-
-  const tab = await addTab(TEST_URL);
-  const descriptor = await rootFront.getTab({
-    outerWindowID: tab.linkedBrowser.outerWindowID,
-  });
-  const commands = await descriptor.getCommands();
-  const targetList = commands.targetCommand;
-  await targetList.startListening();
-
-  const targets = await targetList.getAllTargets(targetList.ALL_TYPES);
-  is(targets.length, 1, "Got a unique target");
-  const targetFront = targets[0];
-  is(
-    targetFront,
-    targetList.targetFront,
-    "TargetCommand top target is the same as the first target"
-  );
-  is(
-    targetFront.targetType,
-    targetList.TYPES.FRAME,
-    "the tab target is of frame type"
-  );
-  is(targetFront.isTopLevel, true, "This is flagged as top level");
-
-  const browser = tab.linkedBrowser;
-  const onLoaded = BrowserTestUtils.browserLoaded(browser);
-  await BrowserTestUtils.loadURI(browser, SECOND_TEST_URL);
-  await onLoaded;
-
-  if (isFissionEnabled()) {
-    info("With fission, cross process switching destroy everything");
-    ok(targetFront.isDestroyed(), "Top level target is destroyed");
-    ok(descriptor.isDestroyed(), "Descriptor is also destroyed");
-  } else {
-    is(
-      targetList.targetFront,
-      targetFront,
-      "Without fission, the top target stays the same"
-    );
-  }
-
-  targetList.destroy();
-
-  BrowserTestUtils.removeTab(tab);
-}
-
 async function testWebExtension(rootFront) {
   info("Test TargetCommand against webextension descriptor");
 

devtools/shared/resources/legacy-target-watchers/legacy-serviceworkers-watcher.js

@@ -147,10 +147,7 @@ class LegacyServiceWorkersWatcher extends LegacyWorkersWatcher {
     for (const target of allServiceWorkerTargets) {
       const isRegisteredBefore = this.targetList.isTargetRegistered(target);
       if (shouldDestroy && isRegisteredBefore) {
-        // Instruct the target command to notify about the worker target destruction
-        // but do not destroy the front as we want to keep using it.
-        // We will notify about it again via onTargetAvailable.
-        this.onTargetDestroyed(target, { shouldDestroyTargetFront: false });
+        this.onTargetDestroyed(target);
       }
 
       // Note: we call isTargetRegistered again because calls to

devtools/shared/transport/websocket-transport.js

@@ -38,9 +38,6 @@ WebSocketDebuggerTransport.prototype = {
   },
 
   close() {
-    if (!this.socket) {
-      return;
-    }
     this.emit("close");
     this.active = false;