Bug 1569859 - [devtools] Spawn WindowGlobal targets for popup opened by the currently debugged tab r=nchevobbe,devtools-backward-compat-reviewers

For now, we only do that when "devtools.popups.debug" is manually set to true.

This is introducing some complexity in the way we filter out the WindowGlobal
we should consider or not. Before this patch it was quite straightforward.
We accepted all WindowGlobal's matching the tab's `browserId`.
Now we also accept the WindowGlobal whose `opener`'s `browserId` matches.

With this patch only, popups start appearing in the iframe dropdown.
You still have to manually switch to the popup via the dropdown to debug it in the inspector or console.
In the debugger, you will already start seeing the popup source and break on it.

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

browser/app/profile/firefox.js

@@ -2193,6 +2193,10 @@ pref("devtools.target-switching.server.enabled", true);
 // remote frames).
 pref("devtools.every-frame-target.enabled", true);
 
+// Controls the hability to debug popups from the same DevTools
+// of the original tab the popups are coming from
+pref("devtools.popups.debug", false);
+
 // Toolbox Button preferences
 pref("devtools.command-button-pick.enabled", true);
 pref("devtools.command-button-frames.enabled", true);

devtools/client/fronts/descriptors/tab.js

@@ -28,6 +28,7 @@ const {
 
 const SERVER_TARGET_SWITCHING_ENABLED_PREF =
   "devtools.target-switching.server.enabled";
+const POPUP_DEBUG_PREF = "devtools.popups.debug";
 
 /**
  * DescriptorFront for tab targets.
@@ -97,8 +98,13 @@ class TabDescriptorFront extends DescriptorMixin(
   }
 
   getWatcher() {
+    const isPopupDebuggingEnabled = Services.prefs.getBoolPref(
+      POPUP_DEBUG_PREF,
+      false
+    );
     return super.getWatcher({
       isServerTargetSwitchingEnabled: this.isServerTargetSwitchingEnabled(),
+      isPopupDebuggingEnabled,
     });
   }
 

devtools/server/actors/descriptors/tab.js

@@ -188,6 +188,7 @@ const TabDescriptorActor = ActorClassWithSpec(tabDescriptorSpec, {
         this.conn,
         createBrowserElementSessionContext(this._browser, {
           isServerTargetSwitchingEnabled: config.isServerTargetSwitchingEnabled,
+          isPopupDebuggingEnabled: config.isPopupDebuggingEnabled,
         })
       );
       this.manage(this.watcher);

devtools/server/actors/targets/target-actor-registry.jsm

@@ -91,7 +91,8 @@ var TargetActorRegistry = {
         (sessionContext.type == "all" &&
           actor.typeName === "parentProcessTarget") ||
         (sessionContext.type == "browser-element" &&
-          actor.browserId == sessionContext.browserId) ||
+          (actor.browserId == sessionContext.browserId ||
+            actor.openerBrowserId == sessionContext.browserId)) ||
         (sessionContext.type == "webextension" &&
           actor.addonId == sessionContext.addonId);
       if (isMatchingPrefix && isMatchingContext) {

devtools/server/actors/targets/window-global.js

@@ -445,6 +445,10 @@ const windowGlobalTargetPrototype = {
     return this.browsingContext?.browserId;
   },
 
+  get openerBrowserId() {
+    return this.browsingContext?.opener?.browserId;
+  },
+
   /**
    * Getter for the WebExtensions ContentScript globals related to the
    * window global's current DOM window.
@@ -581,25 +585,16 @@ const windowGlobalTargetPrototype = {
     // created by DevTools, which always exists and help better connect resources to the target
     // in the frontend. Otherwise all other <browser> element of webext may be reloaded or go away
     // and then we would have troubles matching targets for resources.
-    const browsingContextID = this.devtoolsSpawnedBrowsingContextForWebExtension
-      ? this.devtoolsSpawnedBrowsingContextForWebExtension.id
-      : this.originalDocShell.browsingContext.id;
-    const originalInnerWindowId = this._originalWindow
-      ? getInnerId(this._originalWindow)
-      : null;
-    const innerWindowId = this.devtoolsSpawnedBrowsingContextForWebExtension
-      ? this.devtoolsSpawnedBrowsingContextForWebExtension.currentWindowGlobal
-          .innerWindowId
-      : originalInnerWindowId;
-    const originalParentInnerWindowId = this._originalWindow
-      ? this._originalWindow.docShell.browsingContext.parent
-          ?.currentWindowContext.innerWindowId
-      : null;
-    const parentInnerWindowId = this
+    const originalBrowsingContext = this
       .devtoolsSpawnedBrowsingContextForWebExtension
-      ? this.devtoolsSpawnedBrowsingContextForWebExtension.parent
-          .currentWindowGlobal.innerWindowId
-      : originalParentInnerWindowId;
+      ? this.devtoolsSpawnedBrowsingContextForWebExtension
+      : this.originalDocShell.browsingContext;
+    const browsingContextID = originalBrowsingContext.id;
+    const innerWindowId =
+      originalBrowsingContext.currentWindowContext.innerWindowId;
+    const parentInnerWindowId =
+      originalBrowsingContext.parent?.currentWindowContext.innerWindowId;
+    const isPopup = !!originalBrowsingContext.opener;
 
     const response = {
       actor: this.actorID,
@@ -611,6 +606,7 @@ const windowGlobalTargetPrototype = {
       topInnerWindowId: this.browsingContext.topWindowContext.innerWindowId,
       isTopLevelTarget: this.isTopLevelTarget,
       ignoreSubFrames: this.ignoreSubFrames,
+      isPopup,
       traits: {
         // @backward-compat { version 64 } Exposes a new trait to help identify
         // BrowsingContextActor's inherited actors from the client side.

devtools/server/actors/watcher/browsing-context-helpers.jsm

@@ -60,6 +60,9 @@ const isEveryFrameTargetEnabled = Services.prefs.getBoolPref(
  *        Also, there is some race conditions where browsingContext.currentWindowGlobal
  *        is null, while the callsite may have a reference to the WindowGlobal.
  */
+// The goal of this method is to gather all checks done against BrowsingContext and WindowGlobal interfaces
+// which leads it to be a lengthy method. So disable the complexity rule which is counter productive here.
+// eslint-disable-next-line complexity
 function isBrowsingContextPartOfContext(
   browsingContext,
   sessionContext,
@@ -122,9 +125,18 @@ function isBrowsingContextPartOfContext(
     return true;
   }
   if (sessionContext.type == "browser-element") {
-    if (browsingContext.browserId != sessionContext.browserId) {
+    // Check if the document is:
+    // - part of the Browser element, or,
+    // - a popup originating from the browser element (the popup being loaded in a distinct browser element)
+    const isMatchingTheBrowserElement =
+      browsingContext.browserId == sessionContext.browserId;
+    if (
+      !isMatchingTheBrowserElement &&
+      !isPopupToDebug(browsingContext, sessionContext)
+    ) {
       return false;
     }
+
     // For client-side target switching, only mention the "remote frames".
     // i.e. the frames which are in a distinct process compared to their parent document
     // If there is no parent, this is most likely the top level document which we want to ignore.
@@ -162,6 +174,33 @@ function isBrowsingContextPartOfContext(
   throw new Error("Unsupported session context type: " + sessionContext.type);
 }
 
+/**
+ * Return true for popups to debug when debugging a browser-element.
+ *
+ * @param {BrowsingContext} browsingContext
+ *        The browsing context we want to check if it is part of debugged context
+ * @param {Object} sessionContext
+ *        WatcherActor's session context. This helps know what is the overall debugged scope.
+ *        See watcher actor constructor for more info.
+ */
+function isPopupToDebug(browsingContext, sessionContext) {
+  // If enabled, create targets for popups (i.e. window.open() calls).
+  // If the opener is the tab we are currently debugging, accept the WindowGlobal and create a target for it.
+  //
+  // Note that it is important to do this check *after* the isInitialDocument one.
+  // Popups end up involving three WindowGlobals:
+  // - a first WindowGlobal loading an initial about:blank document (so isInitialDocument is true)
+  // - a second WindowGlobal which looks exactly as the first one
+  // - a final WindowGlobal which loads the URL passed to window.open() (so isInitialDocument is false)
+  //
+  // For now, we only instantiate a target for the last WindowGlobal.
+  return (
+    sessionContext.isPopupDebuggingEnabled &&
+    browsingContext.opener &&
+    browsingContext.opener.browserId == sessionContext.browserId
+  );
+}
+
 /**
  * Helper function of isBrowsingContextPartOfContext to execute all checks
  * against WindowGlobal interface which aren't specific to a given SessionContext type

devtools/server/actors/watcher/session-context.js

@@ -56,6 +56,9 @@ function createBrowserElementSessionContext(browserElement, config) {
     // Nowaday, it should always be enabled except for WebExtension special
     // codepath and some tests.
     isServerTargetSwitchingEnabled: config.isServerTargetSwitchingEnabled,
+    // Should we instantiate targets for popups opened in distinct tabs/windows?
+    // Driven by devtools.popups.debug=true preference.
+    isPopupDebuggingEnabled: config.isPopupDebuggingEnabled,
   };
 }
 

devtools/server/connectors/js-window-actor/WindowGlobalLogger.jsm

@@ -54,6 +54,7 @@ const WindowGlobalLogger = {
       "BrowsingContext.browserId: " + browsingContext.browserId,
       "BrowsingContext.id: " + browsingContext.id,
       "innerWindowId: " + windowGlobal.innerWindowId,
+      "opener.id: " + browsingContext.opener?.id,
       "pid: " + windowGlobal.osPid,
       "isClosed: " + windowGlobal.isClosed,
       "isInProcess: " + windowGlobal.isInProcess,

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

@@ -238,10 +238,16 @@ class TargetCommand extends EventEmitter {
       // We only consider the top level target to be switched
       const isDestroyedTargetSwitching = target == this.targetFront;
       const isServiceWorker = target.targetType === this.TYPES.SERVICE_WORKER;
+      const isPopup = target.targetForm.isPopup;
 
-      // Only notify about service worker targets if this.destroyServiceWorkersOnNavigation
-      // is true
-      if (!isServiceWorker || this.destroyServiceWorkersOnNavigation) {
+      // Never destroy the popup targets when the top level target is destroyed
+      // as the popup follow a different lifecycle.
+      // Also avoid destroying service worker targets for similar reason,
+      // unless this.destroyServiceWorkersOnNavigation is true.
+      if (
+        !isPopup &&
+        (!isServiceWorker || this.destroyServiceWorkersOnNavigation)
+      ) {
         this._onTargetDestroyed(target, {
           isTargetSwitching: isDestroyedTargetSwitching,
           // Do not destroy service worker front as we may want to keep using it.

devtools/shared/commands/target/tests/browser.ini

@@ -18,6 +18,7 @@ support-files =
 [browser_target_command_bfcache.js]
 [browser_target_command_browser_workers.js]
 [browser_target_command_detach.js]
+[browser_target_command_frames_popups.js]
 [browser_target_command_frames_reload_server_side_targets.js]
 skip-if = !fission
 [browser_target_command_frames.js]

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

@@ -0,0 +1,153 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Test that we create targets for popups
+
+const TEST_URL = "https://example.org/document-builder.sjs?html=main page";
+const POPUP_URL = "https://example.com/document-builder.sjs?html=popup";
+const POPUP_SECOND_URL =
+  "https://example.com/document-builder.sjs?html=popup-navigated";
+
+add_task(async function() {
+  await pushPref("devtools.popups.debug", true);
+
+  // Create a TargetCommand for a given test tab
+  const tab = await addTab(TEST_URL);
+  const commands = await CommandsFactory.forTab(tab);
+  const targetCommand = commands.targetCommand;
+  const { TYPES } = targetCommand;
+
+  await targetCommand.startListening();
+
+  // Assert that watchTargets will call the create callback for all existing frames
+  const targets = [];
+  const destroyedTargets = [];
+  const onAvailable = ({ targetFront }) => {
+    targets.push(targetFront);
+  };
+  const onDestroyed = ({ targetFront }) => {
+    destroyedTargets.push(targetFront);
+  };
+  await targetCommand.watchTargets({
+    types: [TYPES.FRAME],
+    onAvailable,
+    onDestroyed,
+  });
+
+  is(targets.length, 1, "At first, we only get one target");
+  is(
+    targets[0],
+    targetCommand.targetFront,
+    "And this target is the top level one"
+  );
+
+  info("Open a popup");
+  const firstPopupBrowsingContext = await SpecialPowers.spawn(
+    tab.linkedBrowser,
+    [POPUP_URL],
+    url => {
+      const win = content.open(url);
+      return win.browsingContext;
+    }
+  );
+
+  await waitFor(() => targets.length === 2);
+  ok(true, "We are notified about the first popup's target");
+
+  is(
+    targets[1].browsingContextID,
+    firstPopupBrowsingContext.id,
+    "the new target is for the popup"
+  );
+  is(targets[1].url, POPUP_URL, "the new target has the right url");
+
+  info("Navigate the popup to a second location");
+  await SpecialPowers.spawn(
+    firstPopupBrowsingContext,
+    [POPUP_SECOND_URL],
+    url => {
+      content.location.href = url;
+    }
+  );
+
+  await waitFor(() => targets.length === 3);
+  ok(true, "We are notified about the new location popup's target");
+
+  await waitFor(() => destroyedTargets.length === 1);
+  ok(true, "The first popup's target is destroyed");
+  is(
+    destroyedTargets[0],
+    targets[1],
+    "The destroyed target is the popup's one"
+  );
+
+  is(
+    targets[2].browsingContextID,
+    firstPopupBrowsingContext.id,
+    "the new location target is for the popup"
+  );
+  is(
+    targets[2].url,
+    POPUP_SECOND_URL,
+    "the new location target has the right url"
+  );
+
+  info("Close the popup");
+  await SpecialPowers.spawn(firstPopupBrowsingContext, [], () => {
+    content.close();
+  });
+
+  await waitFor(() => destroyedTargets.length === 2);
+  ok(true, "The popup's target is destroyed");
+  is(
+    destroyedTargets[1],
+    targets[2],
+    "The destroyed target is the popup's one"
+  );
+
+  info("Open a about:blank popup");
+  const aboutBlankPopupBrowsingContext = await SpecialPowers.spawn(
+    tab.linkedBrowser,
+    [],
+    () => {
+      const win = content.open("about:blank");
+      return win.browsingContext;
+    }
+  );
+
+  await waitFor(() => targets.length === 4);
+  ok(true, "We are notified about the about:blank popup's target");
+
+  is(
+    targets[3].browsingContextID,
+    aboutBlankPopupBrowsingContext.id,
+    "the new target is for the popup"
+  );
+  is(targets[3].url, "about:blank", "the new target has the right url");
+
+  info("Select the original tab and reload it");
+  gBrowser.selectedTab = tab;
+  const onBrowserLoaded = BrowserTestUtils.browserLoaded(
+    gBrowser.selectedBrowser
+  );
+  gBrowser.reloadTab(tab);
+  await onBrowserLoaded;
+
+  await waitFor(() => targets.length === 5);
+  is(targets[4], targetCommand.targetFront, "We get a new top level target");
+  ok(!targets[3].isDestroyed(), "The about:blank popup target is still alive");
+
+  info("Call about:blank popup method to ensure it really is functional");
+  await targets[3].focus();
+
+  targetCommand.unwatchTargets({
+    types: [TYPES.FRAME],
+    onAvailable,
+    onDestroyed,
+  });
+  targetCommand.destroy();
+  BrowserTestUtils.removeTab(tab);
+  await commands.destroy();
+});

devtools/shared/specs/descriptors/tab.js

@@ -28,6 +28,7 @@ const tabDescriptorSpec = generateActorSpec({
     getWatcher: {
       request: {
         isServerTargetSwitchingEnabled: Option(0, "boolean"),
+        isPopupDebuggingEnabled: Option(0, "boolean"),
       },
       response: RetVal("watcher"),
     },