Bug 1631451 - [devtools] Destroy target front via Watcher's target-destroyed-form. r=nchevobbe,jdescottes draft
authorAlexandre Poirot <poirot.alex@gmail.com>
Thu, 01 Apr 2021 15:16:08 +0000
changeset 3637645 720055f165acd54505d7eede27ea70a817de5e16
parent 3637644 fa65c9d3e4c50574dfaa6197c146e74d36e9a74d
child 3637646 bdec7185a2b5dd3505ef4ce3ea347a7d10589dac
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] Destroy target front via Watcher's target-destroyed-form. r=nchevobbe,jdescottes That, instead of tabDetached event, fired on the target actor themself. Differential Revision: https://phabricator.services.mozilla.com/D108578 Differential Diff: PHID-DIFF-xw2djryfyjhi3l35dje4
devtools/client/fronts/targets/target-mixin.js
devtools/shared/commands/target/target-command.js
devtools/shared/resources/legacy-target-watchers/legacy-serviceworkers-watcher.js
--- a/devtools/client/fronts/targets/target-mixin.js
+++ b/devtools/client/fronts/targets/target-mixin.js
@@ -560,35 +560,26 @@ function TargetMixin(parentClass) {
       return this.threadFront;
     }
 
     /**
      * Setup listeners.
      */
     _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);
     }
 
     /**
      * Teardown listeners.
      */
     _removeListeners() {
       // Remove listeners set in _addListeners
       if (this.client) {
         this.client.off("closed", this.destroy);
       }
-      this.off("tabDetached", this.destroy);
 
       // Remove listeners set in attachConsole
       if (this.removeOnInspectObjectListener) {
         this.removeOnInspectObjectListener();
         this.removeOnInspectObjectListener = null;
       }
     }
 
--- a/devtools/shared/commands/target/target-command.js
+++ b/devtools/shared/commands/target/target-command.js
@@ -139,17 +139,19 @@ class TargetCommand extends EventEmitter
     // Handle top level target switching
     // Note that, for now, `_onTargetAvailable` isn't called for the *initial* top level target.
     // i.e. the one that is passed to TargetCommand constructor.
     if (targetFront.isTopLevel) {
       // First report that all existing targets are destroyed
       for (const target of this._targets) {
         // We only consider the top level target to be switched
         const isDestroyedTargetSwitching = target == this.targetFront;
-        this._onTargetDestroyed(target, isDestroyedTargetSwitching);
+        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();
 
@@ -189,29 +191,65 @@ class TargetCommand extends EventEmitter
     if (targetFront.isTopLevel) {
       await this.startListening({ onlyLegacy: true });
     }
 
     // To be consumed by tests triggering frame navigations, spawning workers...
     this.emitForTests("processed-available-target", targetFront);
   }
 
-  _onTargetDestroyed(targetFront, isTargetSwitching = false) {
+  /**
+   * 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 } = {}
+  ) {
     // 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
     // only if that's about the existing top level target.
     if (targetFront == this.targetFront) {
       isTargetSwitching = true;
     }
     this._destroyListeners.emit(targetFront.targetType, {
       targetFront,
       isTargetSwitching,
     });
     this._targets.delete(targetFront);
+
+    if (shouldDestroyTargetFront) {
+      targetFront.destroy();
+    }
   }
 
   _setListening(type, value) {
     if (value) {
       this._listenersStarted.add(type);
     } else {
       this._listenersStarted.delete(type);
     }
--- a/devtools/shared/resources/legacy-target-watchers/legacy-serviceworkers-watcher.js
+++ b/devtools/shared/resources/legacy-target-watchers/legacy-serviceworkers-watcher.js
@@ -142,17 +142,20 @@ class LegacyServiceWorkersWatcher extend
 
   _onNavigate() {
     const allServiceWorkerTargets = this._getAllServiceWorkerTargets();
     const shouldDestroy = this._shouldDestroyTargetsOnNavigation();
 
     for (const target of allServiceWorkerTargets) {
       const isRegisteredBefore = this.targetList.isTargetRegistered(target);
       if (shouldDestroy && isRegisteredBefore) {
-        this.onTargetDestroyed(target);
+        // 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.targetList.isTargetRegistered(target);
       const isValidTarget = this._supportWorkerTarget(target);
       if (isValidTarget && !isRegisteredAfter) {
         // If the target is still valid for the current top target, call