Bug 1710647 - [devtools] Remove targetFront event listeners in resourceCommand#_unwatchAllTargets. r=ochameau,bomsy. draft
authorNicolas Chevobbe <nchevobbe@mozilla.com>
Wed, 12 May 2021 06:19:12 +0000
changeset 3755399 b68d2a3332cf248e61d04fd55fd834248405e2d5
parent 3755366 f99981eb9e49a1da41854ce510165cc573ec80f6
child 3755400 dccc325c93093df9d5a2273be4a27974601747a6
push id691531
push userreviewbot
push dateWed, 12 May 2021 06:19:36 +0000
treeherdertry@83f1bf7c47d3 [default view] [failures only]
reviewersochameau, bomsy
bugs1710647, 1710674
milestone90.0a1
Bug 1710647 - [devtools] Remove targetFront event listeners in resourceCommand#_unwatchAllTargets. r=ochameau,bomsy. Summary: Event listeners were set on target available, but they were never removed, which could cause issue if a consumer was watching/unwatching for resources multiple times. A test (that was failing without this patch), is added to ensure we don't regress this. Note that the test is still incorrect and will be properly fixed in Bug 1710674. Test Plan: Reviewers: ochameau, bomsy Subscribers: Bug #: 1710647 Differential Diff: PHID-DIFF-rmkfber6lyafb36tfztg
devtools/client/fronts/targets/target-mixin.js
devtools/shared/commands/resource/resource-command.js
devtools/shared/commands/resource/tests/browser.ini
devtools/shared/commands/resource/tests/browser_resources_watch_unwatch_multiple.js
--- a/devtools/client/fronts/targets/target-mixin.js
+++ b/devtools/client/fronts/targets/target-mixin.js
@@ -74,17 +74,17 @@ function TargetMixin(parentClass) {
       if (cachedEvents.includes(eventName) && this._resourceCache[eventName]) {
         this.off(eventName, this._onResourceEvent.bind(this, eventName));
         for (const cache of this._resourceCache[eventName]) {
           listener(cache);
         }
         delete this._resourceCache[eventName];
       }
 
-      super.on(eventName, listener);
+      return super.on(eventName, listener);
     }
 
     /**
      * Boolean flag to help distinguish Target Fronts from other Fronts.
      * As we are using a Mixin, we can't easily distinguish these fronts via instanceof().
      */
     get isTargetFront() {
       return true;
--- a/devtools/shared/commands/resource/resource-command.js
+++ b/devtools/shared/commands/resource/resource-command.js
@@ -48,16 +48,18 @@ class ResourceCommand {
 
     // WeakMap used to avoid starting a legacy listener twice for the same
     // target + resource-type pair. Legacy listener creation can be subject to
     // race conditions.
     // Maps a target front to an array of resource types.
     this._existingLegacyListeners = new WeakMap();
     this._processingExistingResources = new Set();
 
+    this._offTargetFrontListeners = [];
+
     this._notifyWatchers = this._notifyWatchers.bind(this);
     this._throttledNotifyWatchers = throttle(this._notifyWatchers, 100);
   }
 
   get watcherFront() {
     return this.targetCommand.watcherFront;
   }
 
@@ -264,16 +266,20 @@ class ResourceCommand {
     }
     return this._watchTargetsPromise;
   }
 
   _unwatchAllTargets() {
     if (!this._watchTargetsPromise) {
       return;
     }
+
+    this._offTargetFrontListeners.forEach(off => off());
+    this._offTargetFrontListeners = [];
+
     this._watchTargetsPromise = null;
     this.targetCommand.unwatchTargets(
       this.targetCommand.ALL_TYPES,
       this._onTargetAvailable,
       this._onTargetDestroyed
     );
   }
 
@@ -316,17 +322,19 @@ class ResourceCommand {
         }
       }
     }
 
     if (targetFront.isDestroyed()) {
       return;
     }
 
-    targetFront.on("will-navigate", () => this._onWillNavigate(targetFront));
+    const offWillNavigate = targetFront.on("will-navigate", () =>
+      this._onWillNavigate(targetFront)
+    );
 
     // If we are target switching, we already stop & start listening to all the
     // currently monitored resources.
     if (!isTargetSwitching) {
       // For each resource type...
       for (const resourceType of Object.values(ResourceCommand.TYPES)) {
         // ...which has at least one listener...
         if (!this._listenedResources.has(resourceType)) {
@@ -338,29 +346,36 @@ class ResourceCommand {
         await this._watchResourcesForTarget(targetFront, resourceType);
       }
     }
 
     // Compared to the TargetCommand and Watcher.watchTargets,
     // We do call Watcher.watchResources, but the events are fired on the target.
     // That's because the Watcher runs in the parent process/main thread, while resources
     // are available from the target's process/thread.
-    targetFront.on(
+    const offResourceAvailable = targetFront.on(
       "resource-available-form",
       this._onResourceAvailable.bind(this, { targetFront })
     );
-    targetFront.on(
+    const offResourceUpdated = targetFront.on(
       "resource-updated-form",
       this._onResourceUpdated.bind(this, { targetFront })
     );
-    targetFront.on(
+    const offResourceDestroyed = targetFront.on(
       "resource-destroyed-form",
       this._onResourceDestroyed.bind(this, { targetFront })
     );
 
+    this._offTargetFrontListeners.push(
+      offWillNavigate,
+      offResourceAvailable,
+      offResourceUpdated,
+      offResourceDestroyed
+    );
+
     if (isTargetSwitching) {
       await Promise.all(
         resources.map(resourceType =>
           this._startListening(resourceType, {
             bypassListenerCount: true,
           })
         )
       );
--- a/devtools/shared/commands/resource/tests/browser.ini
+++ b/devtools/shared/commands/resource/tests/browser.ini
@@ -45,9 +45,10 @@ support-files =
 [browser_resources_several_resources.js]
 [browser_resources_sources.js]
 [browser_resources_stylesheets.js]
 [browser_resources_target_destroy.js]
 [browser_resources_target_resources_race.js]
 [browser_resources_target_switching.js]
 [browser_resources_thread_states.js]
 [browser_resources_unwatch_early.js]
+[browser_resources_watch_unwatch_multiple.js]
 [browser_resources_websocket.js]
new file mode 100644
--- /dev/null
+++ b/devtools/shared/commands/resource/tests/browser_resources_watch_unwatch_multiple.js
@@ -0,0 +1,67 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Check that watching/unwatching multiple times works as expected
+
+add_task(async function() {
+  const START_URL = "data:text/html;charset=utf-8,foo";
+  const tab = await addTab(START_URL);
+
+  const { client, resourceCommand, targetCommand } = await initResourceCommand(
+    tab
+  );
+
+  let resources = [];
+  const onAvailable = _resources => {
+    resources.push(..._resources);
+  };
+
+  info("Watch for console messages resources");
+  await resourceCommand.watchResources(
+    [resourceCommand.TYPES.CONSOLE_MESSAGE],
+    { onAvailable }
+  );
+
+  is(resources.length, 0, "no console message resources was received");
+
+  info("Log a message in the page");
+  await SpecialPowers.spawn(gBrowser.selectedBrowser, [], function() {
+    content.console.log("live message");
+  });
+
+  await waitFor(() => resources.length === 1);
+  is(
+    resources[0].message.arguments[0],
+    "live message",
+    "The resource was received"
+  );
+
+  info("Unwatching resources…");
+  resourceCommand.unwatchResources([resourceCommand.TYPES.CONSOLE_MESSAGE], {
+    onAvailable,
+  });
+  // clearing resources
+  resources = [];
+
+  info("…and watching again");
+  await resourceCommand.watchResources(
+    [resourceCommand.TYPES.CONSOLE_MESSAGE],
+    { onAvailable }
+  );
+  is(
+    resources.length,
+    // We should get only 1 resource. This will be fixed in Bug 1710674
+    2,
+    "we retrieve the expected number of existing resources"
+  );
+  is(
+    resources[0].message.arguments[0],
+    "live message",
+    "The resource is the expected one"
+  );
+
+  targetCommand.destroy();
+  await client.close();
+});