Bug 1691585 - [devtools] Enable browser_target_list_service_workers_navigation.js on Fission. r=jdescottes.

There were a few issues when Fission was being enabled:

1. the sw legacy listener was throwing when trying to compute the origin of the
  "current" target in _onProcessAvailable. That's because the function might be
  called while the "old" target was being destroyed, in which case its `url` property
  is nullified. We can't simply use `targetCommand.target` though, as we might
  be notified about the new process being created before about the new frame
  document. The fix is to store a `currentTargetURL` property in the sw legacy listener,
  and to update it when we receive `will-navigate` events.

2. A few functions were ignoring the `targetCommand.destroyServiceWorkersOnNavigation`
   flag and clearing caches when doing a target switch. This meant that we might
   not be notified about sw targets being unregistered after multiple navigations.

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

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

@@ -12,6 +12,9 @@ const {
 } = require("devtools/shared/commands/target/legacy-target-watchers/legacy-workers-watcher");
 
 class LegacyServiceWorkersWatcher extends LegacyWorkersWatcher {
+  // Holds the current target URL object
+  #currentTargetURL;
+
   constructor(targetCommand, onTargetAvailable, onTargetDestroyed, commands) {
     super(targetCommand, onTargetAvailable, onTargetDestroyed);
     this._registrations = [];
@@ -76,6 +79,10 @@ class LegacyServiceWorkersWatcher extends LegacyWorkersWatcher {
     // Listen to the current target front.
     this.target = this.targetCommand.targetFront;
 
+    if (this.targetCommand.descriptorFront.isLocalTab) {
+      this.#currentTargetURL = new URL(this.targetCommand.targetFront.url);
+    }
+
     this._workersListener.addListener(this._onRegistrationListChanged);
 
     // Fetch the registrations before calling listen, since service workers
@@ -97,7 +104,7 @@ class LegacyServiceWorkersWatcher extends LegacyWorkersWatcher {
   }
 
   // Override from LegacyWorkersWatcher.
-  unlisten() {
+  unlisten(...args) {
     this._workersListener.removeListener(this._onRegistrationListChanged);
 
     if (this.targetCommand.descriptorFront.isLocalTab) {
@@ -109,7 +116,7 @@ class LegacyServiceWorkersWatcher extends LegacyWorkersWatcher {
       );
     }
 
-    super.unlisten();
+    super.unlisten(...args);
   }
 
   // Override from LegacyWorkersWatcher.
@@ -120,11 +127,12 @@ class LegacyServiceWorkersWatcher extends LegacyWorkersWatcher {
       // to filter matching workers, it only makes sense when we are debugging
       // a tab. However in theory, parent process debugging could pause all
       // service workers without matching anything.
-      const origin = new URL(this.target.url).origin;
       try {
         // To support early breakpoint we need to setup the
         // `pauseMatchingServiceWorkers` mechanism in each process.
-        await targetFront.pauseMatchingServiceWorkers({ origin });
+        await targetFront.pauseMatchingServiceWorkers({
+          origin: this.#currentTargetURL.origin,
+        });
       } catch (e) {
         if (targetFront.actorID) {
           throw e;
@@ -151,6 +159,21 @@ class LegacyServiceWorkersWatcher extends LegacyWorkersWatcher {
 
   _onDocumentEvent(resources) {
     for (const resource of resources) {
+      if (
+        resource.resourceType !==
+        this.commands.resourceCommand.TYPES.DOCUMENT_EVENT
+      ) {
+        continue;
+      }
+
+      if (resource.name === "will-navigate") {
+        // We rely on will-navigate as the onTargetAvailable for the top-level frame can
+        // happen after the onTargetAvailable for processes (handled in _onProcessAvailable),
+        // where we need the origin we navigate to.
+        this.#currentTargetURL = new URL(resource.newURI);
+        continue;
+      }
+
       // Note that we rely on "dom-loading" rather than "will-navigate" because the
       // destroyed/available callbacks should be triggered after the Debugger
       // has cleaned up its reducers, which happens on "will-navigate".
@@ -159,36 +182,32 @@ class LegacyServiceWorkersWatcher extends LegacyWorkersWatcher {
       // in test (like browser_target_list_service_workers_navigation.js), as the new worker
       // target would already be registered at this point, and seen as something that would
       // need to be destroyed.
-      if (
-        resource.resourceType !==
-          this.commands.resourceCommand.TYPES.DOCUMENT_EVENT ||
-        resource.name !== "dom-loading"
-      ) {
-        continue;
-      }
+      if (resource.name === "dom-loading") {
+        const allServiceWorkerTargets = this._getAllServiceWorkerTargets();
+        const shouldDestroy = this._shouldDestroyTargetsOnNavigation();
 
-      const allServiceWorkerTargets = this._getAllServiceWorkerTargets();
-      const shouldDestroy = this._shouldDestroyTargetsOnNavigation();
+        for (const target of allServiceWorkerTargets) {
+          const isRegisteredBefore = this.targetCommand.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 });
+          }
 
-      for (const target of allServiceWorkerTargets) {
-        const isRegisteredBefore = this.targetCommand.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 });
-        }
-
-        // Note: we call isTargetRegistered again because calls to
-        // onTargetDestroyed might have modified the list of registered targets.
-        const isRegisteredAfter = this.targetCommand.isTargetRegistered(target);
-        const isValidTarget = this._supportWorkerTarget(target);
-        if (isValidTarget && !isRegisteredAfter) {
-          // If the target is still valid for the current top target, call
-          // onTargetAvailable as well.
-          this.onTargetAvailable(target);
+          // Note: we call isTargetRegistered again because calls to
+          // onTargetDestroyed might have modified the list of registered targets.
+          const isRegisteredAfter = this.targetCommand.isTargetRegistered(
+            target
+          );
+          const isValidTarget = this._supportWorkerTarget(target);
+          if (isValidTarget && !isRegisteredAfter) {
+            // If the target is still valid for the current top target, call
+            // onTargetAvailable as well.
+            this.onTargetAvailable(target);
+          }
         }
       }
     }
@@ -287,7 +306,7 @@ class LegacyServiceWorkersWatcher extends LegacyWorkersWatcher {
 
     // For local tabs, we match ServiceWorkerRegistrations and the target
     // if they share the same hostname for their "url" properties.
-    const targetDomain = new URL(this.target.url).hostname;
+    const targetDomain = this.#currentTargetURL.hostname;
     try {
       const registrationDomain = new URL(registration.url).hostname;
       return registrationDomain === targetDomain;

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

@@ -180,7 +180,10 @@ class LegacyWorkersWatcher {
     }
 
     // Here, we're handling Dedicated Workers in content toolbox.
-    this.targetsByProcess.set(this.target, new Set());
+    this.targetsByProcess.set(
+      this.target,
+      this.targetsByProcess.get(this.target) || new Set()
+    );
     this._workerListChangedListener = this._workerListChanged.bind(
       this,
       this.target
@@ -193,7 +196,7 @@ class LegacyWorkersWatcher {
     return this.targetCommand.getAllTargets([this.targetCommand.TYPES.PROCESS]);
   }
 
-  unlisten() {
+  unlisten({ isTargetSwitching } = {}) {
     // Stop listening for new process targets.
     if (this.target.isParentProcess) {
       this.targetCommand.unwatchTargets(
@@ -213,7 +216,16 @@ class LegacyWorkersWatcher {
       for (const targetFront of this._getProcessTargets()) {
         const listener = this.targetsListeners.get(targetFront);
         targetFront.off("workerListChanged", listener);
-        this.targetsByProcess.delete(targetFront);
+
+        // When unlisten is called from a target switch and service workers targets are not
+        // destroyed on navigation, we don't want to remove the targets from targetsByProcess
+        if (
+          !isTargetSwitching ||
+          !this._isServiceWorkerWatcher ||
+          this.targetCommand.destroyServiceWorkersOnNavigation
+        ) {
+          this.targetsByProcess.delete(targetFront);
+        }
         this.targetsListeners.delete(targetFront);
       }
     } else {

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

@@ -165,19 +165,7 @@ class TargetCommand extends EventEmitter {
     if (targetFront.isTopLevel) {
       // First report that all existing targets are destroyed
       if (!isFirstTarget) {
-        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,
-          });
-        }
-        // Stop listening to legacy listeners as we now have to listen
-        // on the new target.
-        this.stopListening({ onlyLegacy: true });
-
-        // Clear the cached target list
-        this._targets.clear();
+        this._destroyExistingTargetsOnTargetSwitching();
       }
 
       // Update the reference to the memoized top level target
@@ -220,7 +208,7 @@ class TargetCommand extends EventEmitter {
     // Re-register the listeners as the top level target changed
     // and some targets are fetched from it
     if (targetFront.isTopLevel && !isFirstTarget) {
-      await this.startListening({ onlyLegacy: true });
+      await this.startListening({ isTargetSwitching: true });
     }
 
     // To be consumed by tests triggering frame navigations, spawning workers...
@@ -230,6 +218,36 @@ class TargetCommand extends EventEmitter {
     }
   }
 
+  _destroyExistingTargetsOnTargetSwitching() {
+    const destroyedTargets = [];
+    for (const target of this._targets) {
+      // We only consider the top level target to be switched
+      const isDestroyedTargetSwitching = target == this.targetFront;
+
+      // Only destroy service worker targets if this.destroyServiceWorkersOnNavigation is true
+      if (
+        target.targetType !== this.TYPES.SERVICE_WORKER ||
+        this.destroyServiceWorkersOnNavigation
+      ) {
+        this._onTargetDestroyed(target, {
+          isTargetSwitching: isDestroyedTargetSwitching,
+        });
+        destroyedTargets.push(target);
+      }
+    }
+
+    // Stop listening to legacy listeners as we now have to listen
+    // on the new target.
+    this.stopListening({ isTargetSwitching: true });
+
+    // Remove destroyed target from the cached target list. We don't simply clear the
+    // Map as SW targets might not have been destroyed (i.e. when destroyServiceWorkersOnNavigation
+    // is set to false).
+    for (const target of destroyedTargets) {
+      this._targets.delete(target);
+    }
+  }
+
   /**
    * Function fired everytime a target is destroyed.
    *
@@ -349,11 +367,12 @@ class TargetCommand extends EventEmitter {
    * reported to _onTargetAvailable.
    *
    * @param Object options
-   *        Dictionary object with `onlyLegacy` optional boolean.
-   *        If true, we wouldn't register listener set on the Watcher Actor,
-   *        but still register listeners set via Legacy Listeners.
+   * @param Boolean options.isTargetSwitching
+   *        Set to true when this is called while a target switching happens. In such case,
+   *        we won't register listener set on the Watcher Actor, but still register listeners
+   *        set via Legacy Listeners.
    */
-  async startListening({ onlyLegacy = false } = {}) {
+  async startListening({ isTargetSwitching = false } = {}) {
     // The first time we call this method, we pull the current top level target from the descriptor
     if (
       !this.isServerTargetSwitchingEnabled() &&
@@ -391,7 +410,7 @@ 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) {
+        if (!isTargetSwitching) {
           await this.watcherFront.watchTargets(type);
         }
       } else if (this.legacyImplementation[type]) {
@@ -469,16 +488,15 @@ class TargetCommand extends EventEmitter {
    * Stop listening for targets from the server
    *
    * @param Object options
-   *        Dictionary object with `onlyLegacy` optional boolean.
-   *        If true, we wouldn't unregister listener set on the Watcher Actor,
-   *        but still unregister listeners set via Legacy Listeners.
+   * @param Boolean options.isTargetSwitching
+   *        Set to true when this is called while a target switching happens. In such case,
+   *        we won't unregister listener set on the Watcher Actor, but still unregister
+   *        listeners set via Legacy Listeners.
    */
-  stopListening({ onlyLegacy = false } = {}) {
+  stopListening({ isTargetSwitching = false } = {}) {
     // As DOCUMENT_EVENT isn't using legacy listener,
     // there is no need to stop and restart it in case of target switching.
-    // (We typically set onlyLegacy=true when we stop and restart legacy listeners
-    // during a target switch)
-    if (this._watchingDocumentEvent && !onlyLegacy) {
+    if (this._watchingDocumentEvent && !isTargetSwitching) {
       this.commands.resourceCommand.unwatchResources(
         [this.commands.resourceCommand.TYPES.DOCUMENT_EVENT],
         {
@@ -500,11 +518,11 @@ 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) {
+        if (!isTargetSwitching) {
           this.watcherFront.unwatchTargets(type);
         }
       } else if (this.legacyImplementation[type]) {
-        this.legacyImplementation[type].unlisten();
+        this.legacyImplementation[type].unlisten({ isTargetSwitching });
       } else {
         throw new Error(`Unsupported target type '${type}'`);
       }

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

@@ -25,12 +25,6 @@ support-files =
 [browser_target_list_processes.js]
 [browser_target_list_service_workers.js]
 [browser_target_list_service_workers_navigation.js]
-skip-if = fission
-# There are several issues to test Targets navigation scenarios with fission.
-# Without a toolbox linked to the target-list, the target list cannot switch
-# targets. The legacy worker watchers are also not designed to support target
-# switching, since they set this.target = targetCommand.targetFront just once in
-# their constructor.
 [browser_target_list_switchToTarget.js]
 [browser_target_list_tab_workers.js]
 [browser_target_list_tab_workers_bfcache_navigation.js]

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

@@ -86,8 +86,12 @@ add_task(async function test_NavigationBetweenTwoDomains_NoDestroy() {
     targets: [COM_WORKER_URL],
   });
 
-  info("Go back to page 1");
+  info("Go back to .com page");
+  const onBrowserLoaded = BrowserTestUtils.browserLoaded(
+    gBrowser.selectedBrowser
+  );
   BrowserTestUtils.loadURI(gBrowser.selectedBrowser, COM_PAGE_URL);
+  await onBrowserLoaded;
   await checkHooks(hooks, {
     available: 2,
     destroyed: 1,
@@ -236,11 +240,17 @@ async function testNavigationToPageWithExistingWorker({
   await waitForRegistrationReady(tab, COM_PAGE_URL);
 
   info("Navigate to another page");
+  let onBrowserLoaded = BrowserTestUtils.browserLoaded(
+    gBrowser.selectedBrowser
+  );
   BrowserTestUtils.loadURI(gBrowser.selectedBrowser, ORG_PAGE_URL);
 
   // Avoid TV failures, where target list still starts thinking that the
   // current domain is .com .
   info("Wait until we have fully navigated to the .org page");
+  // wait for the browser to be loaded otherwise the task spawned in waitForRegistrationReady
+  // might be destroyed (when it still belongs to the previous content process)
+  await onBrowserLoaded;
   await waitForRegistrationReady(tab, ORG_PAGE_URL);
 
   const { hooks, commands, targetCommand } = await watchServiceWorkerTargets({
@@ -261,7 +271,10 @@ async function testNavigationToPageWithExistingWorker({
   await checkHooks(hooks, { available: 1, destroyed: 1, targets: [] });
 
   info("Go back .com page, wait for onAvailable to be called");
+  onBrowserLoaded = BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
   BrowserTestUtils.loadURI(gBrowser.selectedBrowser, COM_PAGE_URL);
+  await onBrowserLoaded;
+
   await checkHooks(hooks, {
     available: 2,
     destroyed: 1,