Bug 1631451 - [devtools] Control DevToolsClient closure from Descriptor instead of Target. r=nchevobbe,jdescottes draft
authorAlexandre Poirot <poirot.alex@gmail.com>
Thu, 01 Apr 2021 15:16:13 +0000
changeset 3637646 bdec7185a2b5dd3505ef4ce3ea347a7d10589dac
parent 3637645 720055f165acd54505d7eede27ea70a817de5e16
child 3637647 333f6cf849695c5f69f5614f2286ddc5e67cc688
push id675936
push userreviewbot
push dateThu, 01 Apr 2021 15:17:18 +0000
treeherdertry@4d67ff902f3d [default view] [failures only]
reviewersnchevobbe, jdescottes
bugs1631451
milestone89.0a1
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 Differential Diff: PHID-DIFF-66cdwyswmj7m73pszlnd
devtools/client/framework/descriptor-from-url.js
devtools/client/framework/test/browser_toolbox_target.js
devtools/client/framework/test/browser_two_tabs.js
devtools/client/framework/toolbox-init.js
devtools/client/fronts/descriptors/descriptor-mixin.js
devtools/client/fronts/descriptors/tab.js
devtools/client/fronts/targets/browsing-context.js
devtools/client/fronts/targets/target-mixin.js
devtools/client/shared/test/browser_dbg_WorkerTargetActor.attach.js
devtools/client/shared/test/browser_dbg_listtabs-03.js
devtools/client/storage/test/browser_storage_webext_storage_local.js
devtools/shared/commands/target/target-command.js
devtools/shared/commands/target/tests/browser_target_list_various_descriptors.js
--- a/devtools/client/framework/descriptor-from-url.js
+++ b/devtools/client/framework/descriptor-from-url.js
@@ -61,16 +61,23 @@ exports.descriptorFromURL = async functi
     if (!isCachedClient) {
       // If the client was not cached, then the client was created here. If the target
       // creation failed, we should close the client.
       await client.close();
     }
     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;
 };
 
 async function _descriptorFromURL(client, id, type) {
   if (!type) {
     throw new Error("descriptorFromURL, missing type parameter");
   }
 
--- a/devtools/client/framework/test/browser_toolbox_target.js
+++ b/devtools/client/framework/test/browser_toolbox_target.js
@@ -30,16 +30,17 @@ add_task(async function() {
   onLoad = once(toolboxIframe, "load", true);
   toolboxIframe.setAttribute("src", "about:devtools-toolbox?target");
   await onLoad;
 
   // Also wait for toolbox-ready, as toolbox document load isn't enough, there
   // is plenty of asynchronous steps during toolbox load
   info("Waiting for toolbox-ready");
   const toolbox = await onToolboxReady;
+  const { client } = toolbox.descriptorFront;
 
   is(
     toolbox.hostType,
     Toolbox.HostType.PAGE,
     "Host type of this toolbox shuld be Toolbox.HostType.PAGE"
   );
 
   const onToolboxDestroyed = gDevTools.once("toolbox-destroyed");
@@ -48,10 +49,14 @@ add_task(async function() {
   toolboxIframe.remove();
 
   // And wait for toolbox-destroyed as toolbox unload is also full of
   // asynchronous operation that outlast unload event
   info("Waiting for toolbox-destroyed");
   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();
 });
--- a/devtools/client/framework/test/browser_two_tabs.js
+++ b/devtools/client/framework/test/browser_two_tabs.js
@@ -103,10 +103,14 @@ async function checkFirstTargetActor(tar
   ok(
     "startedListeners" in response,
     "Actor from the first tab should still respond."
   );
 }
 
 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();
 }
--- a/devtools/client/framework/toolbox-init.js
+++ b/devtools/client/framework/toolbox-init.js
@@ -121,24 +121,16 @@ async function initToolbox(url, host) {
     const { descriptorFront } = target;
     descriptorFront.once("descriptor-destroyed", function() {
       // Prevent trying to display the error page if the toolbox tab is being destroyed
       if (host.contentDocument) {
         const error = new Error("Debug target was disconnected");
         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);
     showErrorPage(host.contentDocument, `${error}`);
   }
 }
 
 /**
@@ -172,19 +164,17 @@ async function _createTestOnlyDescriptor
 
   await client.connect();
   // Creates a target for a given browser iframe.
   const descriptor = await client.mainRoot.getTab({ tab });
 
   // 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;
 }
 
 // Only use this method to attach the toolbox if some query parameters are given
 if (url.search.length > 1) {
   initToolbox(url, host);
 }
--- a/devtools/client/fronts/descriptors/descriptor-mixin.js
+++ b/devtools/client/fronts/descriptors/descriptor-mixin.js
@@ -21,17 +21,57 @@ function DescriptorMixin(parentClass) {
       this._client = client;
 
       // Pass a true value in order to distinguish this event reception
       // from any manual destroy caused by the frontend
       this.on(
         "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;
 }
 exports.DescriptorMixin = DescriptorMixin;
--- a/devtools/client/fronts/descriptors/tab.js
+++ b/devtools/client/fronts/descriptors/tab.js
@@ -77,16 +77,22 @@ class TabDescriptorFront extends Descrip
       this._teardownLocalTabListeners();
     }
     super.destroy();
   }
 
   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() {
     return !!this._localTab;
   }
 
   get localTab() {
     return this._localTab;
@@ -129,20 +135,16 @@ class TabDescriptorFront extends Descrip
     // Note: the favicon is not part of the default form() payload, it will be
     // added in `retrieveFavicon`.
     return this._form.favicon;
   }
 
   _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;
     front.form(form);
     this.manage(front);
     front.on("target-destroyed", this._onTargetDestroyed);
     return front;
   }
@@ -153,17 +155,22 @@ class TabDescriptorFront extends Descrip
     // in getTarget, this acts as an additional security to avoid races.
     this._targetFront = null;
 
     // @backward-compat { version 88 } Descriptor actors now emit descriptor-destroyed.
     // But about:debugging / remote debugging tabs doesn't support top level target switching
     // 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();
     }
   }
 
   /**
    * Safely retrieves the favicon via getFavicon() and populates this._form.favicon.
    *
    * We could let callers explicitly retrieve the favicon instead of inserting it in the
--- a/devtools/client/fronts/targets/browsing-context.js
+++ b/devtools/client/fronts/targets/browsing-context.js
@@ -113,25 +113,44 @@ class BrowsingContextTargetFront extends
     if (typeof options.javascriptEnabled != "undefined") {
       this._javascriptEnabled = options.javascriptEnabled;
     }
 
     return response;
   }
 
   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() {
     const promise = super.destroy();
 
     // As detach isn't necessarily called on target's destroy
     // (it isn't for local tabs), ensure removing listeners set in attach.
     this.off("tabNavigated", this._onTabNavigated);
--- a/devtools/client/fronts/targets/target-mixin.js
+++ b/devtools/client/fronts/targets/target-mixin.js
@@ -40,22 +40,16 @@ function TargetMixin(parentClass) {
       super(client, targetFront, parentFront);
 
       this._forceChrome = false;
 
       this.destroy = this.destroy.bind(this);
 
       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
       // [typeName:string => Front instance]
       this.fronts = new Map();
 
       // `resource-available-form` events can be emitted by target actors before the
       // ResourceWatcher could add event listeners. The target front will cache those
@@ -631,43 +625,34 @@ function TargetMixin(parentClass) {
           console.warn("Error while destroying front:", name, e);
         }
       }
 
       this._removeListeners();
 
       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
         // cleanup directly after this.
         try {
           await this.detach();
         } catch (e) {
           this.logDetachError(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");
-
       // Do that very last in order to let a chance to dispatch `detach` requests.
       super.destroy();
 
       this._cleanup();
     }
 
     /**
      * Detach can fail under regular circumstances, if the target was already
--- a/devtools/client/shared/test/browser_dbg_WorkerTargetActor.attach.js
+++ b/devtools/client/shared/test/browser_dbg_WorkerTargetActor.attach.js
@@ -33,51 +33,51 @@ add_task(async function() {
   // because the worker is not guaranteed to have finished loading when it is
   // registered. Instead, we have to wait for the promise returned by
   // createWorker in the tab to be resolved.
   await createWorkerInTab(tab, WORKER1_URL);
   let { workers } = await listWorkers(target);
   let workerDescriptorFront1 = findWorker(workers, WORKER1_URL);
   await workerDescriptorFront1.attach();
   ok(
-    workerDescriptorFront1.actorID,
+    !workerDescriptorFront1.isDestroyed(),
     "front for worker in tab 1 has been attached"
   );
 
   executeSoon(() => {
     BrowserTestUtils.loadURI(tab.linkedBrowser, TAB2_URL);
   });
   await waitForWorkerClose(workerDescriptorFront1);
   ok(
-    !!workerDescriptorFront1.actorID,
+    workerDescriptorFront1.isDestroyed(),
     "front for worker in tab 1 has been closed"
   );
 
   await createWorkerInTab(tab, WORKER2_URL);
   ({ workers } = await listWorkers(target));
   const workerDescriptorFront2 = findWorker(workers, WORKER2_URL);
   await workerDescriptorFront2.attach();
   ok(
-    workerDescriptorFront2.actorID,
+    !workerDescriptorFront2.isDestroyed(),
     "front for worker in tab 2 has been attached"
   );
 
   executeSoon(() => {
     tab.linkedBrowser.goBack();
   });
   await waitForWorkerClose(workerDescriptorFront2);
   ok(
-    !!workerDescriptorFront2.actorID,
+    workerDescriptorFront2.isDestroyed(),
     "front for worker in tab 2 has been closed"
   );
 
   ({ workers } = await listWorkers(target));
   workerDescriptorFront1 = findWorker(workers, WORKER1_URL);
   await workerDescriptorFront1.attach();
   ok(
-    workerDescriptorFront1.actorID,
+    !workerDescriptorFront1.isDestroyed(),
     "front for worker in tab 1 has been attached"
   );
 
   await target.destroy();
   SpecialPowers.setIntPref(MAX_TOTAL_VIEWERS, oldMaxTotalViewers);
   finish();
 });
--- a/devtools/client/shared/test/browser_dbg_listtabs-03.js
+++ b/devtools/client/shared/test/browser_dbg_listtabs-03.js
@@ -19,48 +19,51 @@ add_task(async function test() {
   const transport = DevToolsServer.connectPipe();
   const client = new DevToolsClient(transport);
   const [type] = await client.connect();
   is(type, "browser", "Root actor should identify itself as a browser.");
   const tab = await addTab(TAB1_URL);
 
   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) {
   const tabDescriptors = await rootFront.listTabs();
   is(tabDescriptors.length, 2, "Should be two tabs");
 
   const tabDescriptor = tabDescriptors.find(d => d.url == TAB1_URL);
   ok(tabDescriptor, "Should have a descriptor actor for the tab");
   ok(
     !tabDescriptor.isLocalTab,
     "listTabs's tab descriptor aren't considered as local tabs"
   );
 
   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");
 
   info("Detach the tab target");
   const onTargetDestroyed = tabTarget.once("target-destroyed");
   const onDescriptorDestroyed = tabDescriptor.once("descriptor-destroyed");
   await tabTarget.detach();
 
   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;
 
   // Tab Descriptors returned by listTabs are not considered as local tabs
   // and follow the lifecycle of their target. So both target and descriptor are destroyed.
   ok(
     tabTarget.isDestroyed(),
     "The tab target should be destroyed after detach"
@@ -94,44 +97,56 @@ async function assertGetTab(client, root
   ok(
     !tabDescriptor2.isLocalTab,
     "getTab's tab descriptor aren't considered as local tabs when we pass an outerWindowID"
   );
   await removeTab(tab2);
 
   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;
 }
 
 function isTargetAttached(targetFront) {
   return !!targetFront?.targetForm?.threadActor;
 }
--- a/devtools/client/storage/test/browser_storage_webext_storage_local.js
+++ b/devtools/client/storage/test/browser_storage_webext_storage_local.js
@@ -41,24 +41,26 @@ async function setupLocalDevToolsServerA
  * @param {String} id
  *        The id for the extension to be targeted by the toolbox.
  * @return {Promise} Resolves with a web extension actor target object and the
  *         toolbox and storage objects when the toolbox has been setup
  */
 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,
     hostType: Toolbox.HostType.WINDOW,
   });
 
   const target = toolbox.target;
-  target.shouldCloseClient = true;
 
   return { target, toolbox, storage };
 }
 
 add_task(async function set_enable_extensionStorage_pref() {
   await SpecialPowers.pushPrefEnv({
     set: [["devtools.storage.extensionStorage.enabled", true]],
   });
--- a/devtools/shared/commands/target/target-command.js
+++ b/devtools/shared/commands/target/target-command.js
@@ -578,25 +578,16 @@ class TargetCommand extends EventEmitter
    *
    * @param TargetFront targetFront
    *        The BrowsingContextTargetFront instance that navigated to another process
    */
   async onLocalTabRemotenessChange(targetFront) {
     // 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");
     }
 
     // Fetch the new target from the descriptor.
     const newTarget = await this.descriptorFront.getTarget();
 
     this.switchToTarget(newTarget);
--- a/devtools/shared/commands/target/tests/browser_target_list_various_descriptors.js
+++ b/devtools/shared/commands/target/tests/browser_target_list_various_descriptors.js
@@ -53,20 +53,22 @@ async function testParentProcess() {
   await commands.destroy();
 }
 
 async function testLocalTab() {
   info("Test TargetCommand against local tab descriptor (via getTab({ tab }))");
 
   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");
   const targetFront = targets[0];
   is(targetFront, targetCommand.targetFront, "The first is the top level one");
   is(
     targetFront.targetType,
     targetCommand.TYPES.FRAME,