Bug 1709267 - [devtools] Add support for target switching in about:debugging. r=jdescottes

The challenges here are:
* xpcshell tests still don't support the watcher actor and server side targets. So we have to ensure still using client side target fetched via Descriptor.getTarget RDP request. (We still also need that for WebExtension)
* some tests weren't spawning the TargetCommand while querying TabDescriptor.getTarget. I tuned them to call TargetCommand.startListening so that we start instantiating server side targets, including the top level one retrieved via TabDescriptor.getTarget.

Otherwise, thanks to this patch a few check can now be moved from `if (isLocalTab)` to `if (isTabDescriptor)`.

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

devtools/client/aboutdebugging/test/browser/browser_aboutdebugging_devtoolstoolbox_navigate_back_forward.js

@@ -3,13 +3,8 @@
 
 "use strict";
 
-// FIXME(bug 1709267): This test used to test navigations between `about:home`
-// and `about:blank`. Some process switch changes during Fission (bug 1650089)
-// meant that this navigation now leads to a process switch. Unfortunately,
-// about:debugging is not resillient to process switches, so the URLs were
-// changed to both load within the same content process.
 const ORIGINAL_URL = "https://example.com/document-builder.sjs?html=page1";
-const OTHER_URL = "https://example.com/document-builder.sjs?html=page2";
+const OTHER_URL = "https://example.org/document-builder.sjs?html=page2";
 
 async function waitForUrl(url, toolbox, browserTab, win) {
   const {

devtools/client/framework/test/browser_target_from_url.js

@@ -7,6 +7,7 @@ const TEST_URI =
 const { DevToolsLoader } = ChromeUtils.import(
   "resource://devtools/shared/loader/Loader.jsm"
 );
+const { createCommandsDictionary } = require("devtools/shared/commands/index");
 const {
   descriptorFromURL,
 } = require("devtools/client/framework/descriptor-from-url");
@@ -45,6 +46,9 @@ add_task(async function() {
   descriptor = await descriptorFromURL(
     new URL("http://foo?type=tab&id=" + windowId)
   );
+  const commands = await createCommandsDictionary(descriptor);
+  // Descriptor's getTarget will only work if the TargetCommand watches for the first top target
+  await commands.targetCommand.startListening();
   target = await descriptor.getTarget();
   assertTarget(target, TEST_URI);
   await descriptor.client.close();

devtools/client/framework/test/browser_target_parents.js

@@ -7,6 +7,7 @@
 
 const { DevToolsClient } = require("devtools/client/devtools-client");
 const { DevToolsServer } = require("devtools/server/devtools-server");
+const { createCommandsDictionary } = require("devtools/shared/commands/index");
 
 const TEST_URL = `data:text/html;charset=utf-8,<div id="test"></div>`;
 
@@ -19,6 +20,19 @@ add_task(async function() {
 
   const tabDescriptors = await mainRoot.listTabs();
 
+  const concurrentCommands = [];
+  for (const descriptor of tabDescriptors) {
+    concurrentCommands.push(
+      (async () => {
+        const commands = await createCommandsDictionary(descriptor);
+        // Descriptor's getTarget will only work if the TargetCommand watches for the first top target
+        await commands.targetCommand.startListening();
+      })()
+    );
+  }
+  info("Instantiate all tab's commands and initialize their TargetCommand");
+  await Promise.all(concurrentCommands);
+
   await testGetTargetWithConcurrentCalls(tabDescriptors, tabTarget => {
     // We only call BrowsingContextTargetFront.attach and not TargetMixin.attachAndInitThread.
     // So very few things are done.

devtools/client/framework/test/browser_two_tabs.js

@@ -7,6 +7,7 @@
 
 var { DevToolsServer } = require("devtools/server/devtools-server");
 var { DevToolsClient } = require("devtools/client/devtools-client");
+const { createCommandsDictionary } = require("devtools/shared/commands/index");
 
 const TAB_URL_1 = "data:text/html;charset=utf-8,foo";
 const TAB_URL_2 = "data:text/html;charset=utf-8,bar";
@@ -23,6 +24,13 @@ add_task(async () => {
   await client.connect();
 
   const tabDescriptors = await client.mainRoot.listTabs();
+  await Promise.all(
+    tabDescriptors.map(async descriptor => {
+      const commands = await createCommandsDictionary(descriptor);
+      // Descriptor's getTarget will only work if the TargetCommand watches for the first top target
+      await commands.targetCommand.startListening();
+    })
+  );
   const tabs = await Promise.all(tabDescriptors.map(d => d.getTarget()));
   const targetFront1 = tabs.find(a => a.url === TAB_URL_1);
   const targetFront2 = tabs.find(a => a.url === TAB_URL_2);

devtools/client/fronts/descriptors/tab.js

@@ -47,10 +47,22 @@ class TabDescriptorFront extends DescriptorMixin(
     // (eg, regular tab toolbox) or browsing context targets (eg tab remote
     // debugging).
     this._localTab = null;
-    this._isForWebExtension = false;
+
+    // Flag to prevent the server from trying to spawn targets by the watcher actor.
+    this._disableTargetSwitching = false;
 
     this._onTargetDestroyed = this._onTargetDestroyed.bind(this);
     this._handleTabEvent = this._handleTabEvent.bind(this);
+
+    // When the target is created from the server side,
+    // it is not created via TabDescriptor.getTarget.
+    // Instead, it is retrieved by the TargetCommand which
+    // will call TabDescriptor.setTarget from TargetCommand.onTargetAvailable
+    if (this.isServerTargetSwitchingEnabled()) {
+      this._targetFrontPromise = new Promise(
+        r => (this._resolveTargetFrontPromise = r)
+      );
+    }
   }
 
   form(json) {
@@ -99,16 +111,6 @@ class TabDescriptorFront extends DescriptorMixin(
     // but also ensure cleaning up the client and everything on tab closing.
     // (this flag is handled by DescriptorMixin)
     this.shouldCloseClient = true;
-
-    // When the target is created from the server side,
-    // it is not created via TabDescriptor.getTarget.
-    // Instead, it is retrieved by the TargetCommand which
-    // will call TabDescriptor.setTarget from TargetCommand.onTargetAvailable
-    if (this.isServerTargetSwitchingEnabled()) {
-      this._targetFrontPromise = new Promise(
-        r => (this._resolveTargetFrontPromise = r)
-      );
-    }
   }
 
   get isTabDescriptor() {
@@ -141,9 +143,7 @@ class TabDescriptorFront extends DescriptorMixin(
       SERVER_TARGET_SWITCHING_ENABLED_PREF,
       false
     );
-    // We explicitely disable server targets for remote tabs (i.e. about:debugging)
+    const enabled = isEnabled && !this._disableTargetSwitching;
-    // and WebExtension codebase (see setIsForWebExtension)
-    const enabled = isEnabled && this.isLocalTab && !this._isForWebExtension;
     return enabled;
   }
 
@@ -154,7 +154,19 @@ class TabDescriptorFront extends DescriptorMixin(
    * introduce target switching for all navigations and reloads
    */
   setIsForWebExtension() {
-    this._isForWebExtension = true;
+    this.disableTargetSwitching();
+  }
+
+  /**
+   * Method used by the WebExtension which still need to disable server side targets,
+   * and also a few xpcshell tests which are using legacy API and don't support watcher actor.
+   */
+  disableTargetSwitching() {
+    this._disableTargetSwitching = true;
+    // Delete these two attributes which have to be set early from the constructor,
+    // but we don't know yet if target switch should be disabled.
+    delete this._targetFrontPromise;
+    delete this._resolveTargetFrontPromise;
   }
 
   get isZombieTab() {
@@ -199,14 +211,6 @@ class TabDescriptorFront extends DescriptorMixin(
     // Note that we are also checking that _targetFront has a valid actorID
     // in getTarget, this acts as an additional security to avoid races.
     this._targetFront = null;
-
-    // about:debugging / remote debugging tabs don't support top level
-    // target-switching so we have to remove the descriptor when the target is
-    // destroyed. When about:debugging supports target switching, we can remove
-    // the !isLocalTab check. See Bug 1709267.
-    if (!this.isLocalTab) {
-      this.destroy();
-    }
   }
 
   /**

devtools/client/fronts/root.js

@@ -245,7 +245,6 @@ class RootFront extends FrontClassWithSpec(rootSpec) {
 
     const descriptorFront = await super.getTab(packet);
 
-    // Should be called before setLocalTab.
     // Will flag TabDescriptor used by WebExtension codebase.
     if (filter?.isWebExtension) {
       descriptorFront.setIsForWebExtension(true);

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

@@ -25,15 +25,16 @@ add_task(async function test() {
   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);
+  const tab1 = await addTab(TAB1_URL);
 
-  await assertListTabs(client.mainRoot);
+  await assertListTabs(tab1, client.mainRoot);
 
-  // To run last as it will close the client and remove the tab
+  const tab2 = await addTab(TAB1_URL);
-  await assertGetTab(client, client.mainRoot, tab);
+  // To run last as it will close the client
+  await assertGetTab(client, client.mainRoot, tab2);
 });
 
-async function assertListTabs(rootFront) {
+async function assertListTabs(tab, rootFront) {
   const tabDescriptors = await rootFront.listTabs();
   is(tabDescriptors.length, 2, "Should be two tabs");
 
@@ -53,29 +54,34 @@ async function assertListTabs(rootFront) {
 
   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.
+  // When the target is detached and destroyed,
-  // And because of that, the TabDescriptor self-destroy itself on target being destroyed.
+  // the descriptor stays alive, because the tab is still opened.
-  // That's because we don't yet support top level target switching in case
+  // As we support top level target switching, the descriptor can be kept alive
-  // of remote debugging. So we prefer destroy the descriptor as well as the toolbox.
+  // when the target is destroyed.
-  // Bug 1698891 should revisit that.
+  ok(
+    !tabDescriptor.isDestroyed(),
+    "The tab descriptor isn't destroyed on target detach"
+  );
+
+  info("Close the descriptor's tab");
+  const onDescriptorDestroyed = tabDescriptor.once("descriptor-destroyed");
+  await removeTab(tab);
+
   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"
+    "The tab target should be destroyed after closing the tab"
   );
   ok(
     tabDescriptor.isDestroyed(),
-    "The tab descriptor should be destroyed after detach"
+    "The tab descriptor is also always destroyed after tab closing"
   );
 
   // Compared to getTab, the client should be kept alive.
@@ -122,7 +128,7 @@ async function assertGetTab(client, rootFront, tab) {
   // 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
+  // As we support top level target switching, the descriptor can be kept alive
   // when the target is destroyed.
   ok(
     !tabDescriptor.isDestroyed(),

devtools/server/tests/xpcshell/head_dbg.js

@@ -87,6 +87,12 @@ async function createTargetForFakeTab(title) {
 
   const tabs = await listTabs(client);
   const tabDescriptor = findTab(tabs, title);
+
+  // These xpcshell tests use mocked actors (xpcshell-test/testactors)
+  // which still don't support watcher actor.
+  // Because of that we still can't enable server side targets and target swiching.
+  tabDescriptor.disableTargetSwitching();
+
   return tabDescriptor.getTarget();
 }
 
@@ -378,6 +384,12 @@ async function getTestTab(client, title) {
  */
 async function attachTestTab(client, title) {
   const descriptorFront = await getTestTab(client, title);
+
+  // These xpcshell tests use mocked actors (xpcshell-test/testactors)
+  // which still don't support watcher actor.
+  // Because of that we still can't enable server side targets and target swiching.
+  descriptorFront.disableTargetSwitching();
+
   const commands = await createCommandsDictionary(descriptorFront);
   await commands.targetCommand.startListening();
   return commands;
@@ -810,6 +822,12 @@ async function setupTestFromUrl(url) {
 
   const tabs = await listTabs(devToolsClient);
   const descriptorFront = findTab(tabs, "test");
+
+  // These xpcshell tests use mocked actors (xpcshell-test/testactors)
+  // which still don't support watcher actor.
+  // Because of that we still can't enable server side targets and target swiching.
+  descriptorFront.disableTargetSwitching();
+
   const targetFront = await descriptorFront.getTarget();
 
   const threadFront = await attachThread(targetFront);

devtools/server/tests/xpcshell/test_add_actors.js

@@ -48,7 +48,15 @@ add_task(async function() {
 
   let actors = await client.mainRoot.rootForm;
   const tabs = await client.mainRoot.listTabs();
-  const tabTarget = await tabs[0].getTarget();
+  const tabDescriptor = tabs[0];
+
+  // These xpcshell tests use mocked actors (xpcshell-test/testactors)
+  // which still don't support watcher actor.
+  // Because of that we still can't enable server side targets and target swiching.
+  tabDescriptor.disableTargetSwitching();
+
+  const tabTarget = await tabDescriptor.getTarget();
+
   Assert.equal(tabs.length, 1);
 
   let reply = await client.request({

devtools/shared/commands/resource/legacy-listeners/console-messages.js

@@ -11,7 +11,7 @@ module.exports = async function({ targetCommand, targetFront, onAvailable }) {
   // Also allow frame, but only in content toolbox, i.e. still ignore them in
   // the context of the browser toolbox as we inspect messages via the process
   // targets
-  const listenForFrames = targetCommand.descriptorFront.isLocalTab;
+  const listenForFrames = targetCommand.descriptorFront.isTabDescriptor;
 
   // Allow workers when messages aren't dispatched to the main thread.
   const listenForWorkers = !targetCommand.rootFront.traits

devtools/shared/commands/resource/legacy-listeners/error-messages.js

@@ -13,7 +13,7 @@ module.exports = async function({ targetCommand, targetFront, onAvailable }) {
   // the context of the browser toolbox as we inspect messages via the process
   // targets
   // Also ignore workers as they are not supported yet. (see bug 1592584)
-  const listenForFrames = targetCommand.descriptorFront.isLocalTab;
+  const listenForFrames = targetCommand.descriptorFront.isTabDescriptor;
   const isAllowed =
     targetFront.isTopLevel ||
     targetFront.targetType === targetCommand.TYPES.PROCESS ||

devtools/shared/commands/resource/legacy-listeners/network-events.js

@@ -17,7 +17,7 @@ module.exports = async function({
   // the context of the browser toolbox as we inspect messages via the process
   // targets
   // Also ignore workers as they are not supported yet. (see bug 1592584)
-  const listenForFrames = targetCommand.descriptorFront.isLocalTab;
+  const listenForFrames = targetCommand.descriptorFront.isTabDescriptor;
   const isAllowed =
     targetFront.isTopLevel ||
     targetFront.targetType === targetCommand.TYPES.PROCESS ||

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

@@ -79,7 +79,7 @@ class LegacyServiceWorkersWatcher extends LegacyWorkersWatcher {
     // Listen to the current target front.
     this.target = this.targetCommand.targetFront;
 
-    if (this.targetCommand.descriptorFront.isLocalTab) {
+    if (this.targetCommand.descriptorFront.isTabDescriptor) {
       this.#currentTargetURL = new URL(this.targetCommand.targetFront.url);
     }
 
@@ -90,7 +90,7 @@ class LegacyServiceWorkersWatcher extends LegacyWorkersWatcher {
     // registrations.
     await this._onRegistrationListChanged();
 
-    if (this.targetCommand.descriptorFront.isLocalTab) {
+    if (this.targetCommand.descriptorFront.isTabDescriptor) {
       await this.commands.resourceCommand.watchResources(
         [this.commands.resourceCommand.TYPES.DOCUMENT_EVENT],
         {
@@ -107,7 +107,7 @@ class LegacyServiceWorkersWatcher extends LegacyWorkersWatcher {
   unlisten(...args) {
     this._workersListener.removeListener(this._onRegistrationListChanged);
 
-    if (this.targetCommand.descriptorFront.isLocalTab) {
+    if (this.targetCommand.descriptorFront.isTabDescriptor) {
       this.commands.resourceCommand.unwatchResources(
         [this.commands.resourceCommand.TYPES.DOCUMENT_EVENT],
         {
@@ -121,7 +121,7 @@ class LegacyServiceWorkersWatcher extends LegacyWorkersWatcher {
 
   // Override from LegacyWorkersWatcher.
   async _onProcessAvailable({ targetFront }) {
-    if (this.targetCommand.descriptorFront.isLocalTab) {
+    if (this.targetCommand.descriptorFront.isTabDescriptor) {
       // XXX: This has been ported straight from the current debugger
       // implementation. Since pauseMatchingServiceWorkers expects an origin
       // to filter matching workers, it only makes sense when we are debugging
@@ -298,8 +298,8 @@ class LegacyServiceWorkersWatcher extends LegacyWorkersWatcher {
       return true;
     }
 
-    if (!this.targetCommand.descriptorFront.isLocalTab) {
+    if (!this.targetCommand.descriptorFront.isTabDescriptor) {
-      // No support for service worker targets outside of main process & local
+      // No support for service worker targets outside of main process &
       // tab debugging.
       return false;
     }

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

@@ -56,7 +56,7 @@ class TargetCommand extends EventEmitter {
     this.onLocalTabRemotenessChange = this.onLocalTabRemotenessChange.bind(
       this
     );
-    if (this.descriptorFront.isLocalTab) {
+    if (this.descriptorFront.isTabDescriptor) {
       this.descriptorFront.on(
         "remoteness-change",
         this.onLocalTabRemotenessChange
@@ -506,7 +506,11 @@ class TargetCommand extends EventEmitter {
   _computeTargetTypes() {
     let types = [];
 
-    if (this.descriptorFront.isLocalTab) {
+    // We also check for watcher support as some xpcshell tests uses legacy APIs and don't support frames.
+    if (
+      this.descriptorFront.isTabDescriptor &&
+      this.hasTargetWatcherSupport(TargetCommand.TYPES.FRAME)
+    ) {
       types = [TargetCommand.TYPES.FRAME];
     } else if (this.descriptorFront.isBrowserProcessDescriptor) {
       const fissionBrowserToolboxEnabled = Services.prefs.getBoolPref(

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

@@ -142,17 +142,18 @@ async function testRemoteTab() {
   await BrowserTestUtils.loadURI(browser, SECOND_TEST_URL);
   await onLoaded;
 
-  if (isFissionEnabled()) {
+  info("Wait for the new target");
-    info("With fission, cross process switching destroy everything");
+  await waitFor(() => targetCommand.targetFront != targetFront);
-    ok(targetFront.isDestroyed(), "Top level target is destroyed");
+  isnot(
-    ok(commands.descriptorFront.isDestroyed(), "Descriptor is also destroyed");
+    targetCommand.targetFront,
-  } else {
+    targetFront,
-    is(
+    "The top level target changes on navigation"
-      targetCommand.targetFront,
+  );
-      targetFront,
+  ok(
-      "Without fission, the top target stays the same"
+    !targetCommand.targetFront.isDestroyed(),
-    );
+    "The new target isn't destroyed"
-  }
+  );
+  ok(targetFront.isDestroyed(), "While the previous target is destroyed");
 
   targetCommand.destroy();
 

devtools/shared/webconsole/test/chrome/common.js

@@ -69,6 +69,14 @@ var _attachConsole = async function(listeners, attachToTab, attachToWorker) {
       target = await targetDescriptor.getTarget();
     } else {
       const targetDescriptor = await client.mainRoot.getTab();
+      // As there is no real tab in mochitest chrome, we can use CommandsFactory
+      // and fallback to create the commands manually
+      const {
+        createCommandsDictionary,
+      } = require("devtools/shared/commands/index");
+      const commands = await createCommandsDictionary(targetDescriptor);
+      // Descriptor's getTarget will only work if the TargetCommand watches for the first top target
+      await commands.targetCommand.startListening();
       target = await targetDescriptor.getTarget();
       if (attachToWorker) {
         const workerName = "console-test-worker.js#" + new Date().getTime();