Bug 1710674 - [devtools] Fix target-mixin resource cache mechanism. r=ochameau,bomsy. draft
authorNicolas Chevobbe <nchevobbe@mozilla.com>
Wed, 12 May 2021 08:08:25 +0200
changeset 3755403 c1dc830e058bb71affa3c9427a794eb62935de49
parent 3755402 bde8e89918090044a256831668ecd8fc08cd6081
child 3755404 f9124e5708f6ce49e5bc553c7d2bd3d5826d2eff
push id691532
push usernchevobbe@mozilla.com
push dateWed, 12 May 2021 06:21:03 +0000
treeherdertry@f9124e5708f6 [default view] [failures only]
reviewersochameau, bomsy
bugs1710674
milestone90.0a1
Bug 1710674 - [devtools] Fix target-mixin resource cache mechanism. r=ochameau,bomsy. The event listeners that were set in the constructor were not actually removed in the `on` method, as we weren't using the proper function reference. Furthermore, the event listeners were removed only if some resources were put in the cache, which could lead to some buggy behavior when watching/unwatching multiple times. Differential Revision: https://phabricator.services.mozilla.com/D114927
devtools/client/fronts/targets/target-mixin.js
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
@@ -54,34 +54,47 @@ function TargetMixin(parentClass) {
       // `resource-available-form` and `resource-updated-form` events can be emitted
       // by target actors before the ResourceCommand could add event listeners.
       // The target front will cache those events until the ResourceCommand has
       // added the listeners.
       this._resourceCache = {};
 
       // In order to avoid destroying the `_resourceCache[event]`, we need to call `super.on()`
       // instead of `this.on()`.
-      super.on(
+      const offResourceAvailable = super.on(
         "resource-available-form",
         this._onResourceEvent.bind(this, "resource-available-form")
       );
-      super.on(
+      const offResourceUpdated = super.on(
         "resource-updated-form",
         this._onResourceEvent.bind(this, "resource-updated-form")
       );
+
+      this._offResourceEvent = new Map([
+        ["resource-available-form", offResourceAvailable],
+        ["resource-updated-form", offResourceUpdated],
+      ]);
     }
 
     on(eventName, listener) {
-      const cachedEvents = ["resource-available-form", "resource-updated-form"];
-      if (cachedEvents.includes(eventName) && this._resourceCache[eventName]) {
-        this.off(eventName, this._onResourceEvent.bind(this, eventName));
-        for (const cache of this._resourceCache[eventName]) {
-          listener(cache);
+      if (this._offResourceEvent.has(eventName)) {
+        // If a callsite sets an event listener for resource-(available|update)-form:
+
+        // we want to remove the listener we set here in the constructor…
+        const off = this._offResourceEvent.get(eventName);
+        this._offResourceEvent.delete(eventName);
+        off();
+
+        // …and call the new listener with the resources that were put in the cache.
+        if (this._resourceCache[eventName]) {
+          for (const cache of this._resourceCache[eventName]) {
+            listener(cache);
+          }
+          delete this._resourceCache[eventName];
         }
-        delete this._resourceCache[eventName];
       }
 
       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().
@@ -627,16 +640,17 @@ function TargetMixin(parentClass) {
 
       // Remove listeners set in attachConsole
       if (this.removeOnInspectObjectListener) {
         this.removeOnInspectObjectListener();
         this.removeOnInspectObjectListener = null;
       }
 
       this.threadFront = null;
+      this._offResourceEvent = null;
 
       // This event should be emitted before calling super.destroy(), because
       // super.destroy() will remove all event listeners attached to this front.
       this.emit("target-destroyed");
 
       // Not all targets supports attach/detach. For example content process doesn't.
       // Also ensure that the front is still active before trying to do the request.
       if (this.detach && !this.isDestroyed()) {
--- a/devtools/shared/commands/resource/tests/browser_resources_watch_unwatch_multiple.js
+++ b/devtools/shared/commands/resource/tests/browser_resources_watch_unwatch_multiple.js
@@ -47,18 +47,17 @@ add_task(async function() {
 
   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,
+    1,
     "we retrieve the expected number of existing resources"
   );
   is(
     resources[0].message.arguments[0],
     "live message",
     "The resource is the expected one"
   );