Bug 1637641: Make multiple listeners can receive proper cached resources. r=ochameau

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

devtools/shared/resources/resource-watcher.js

@@ -32,12 +32,9 @@ class ResourceWatcher {
     this._availableListeners = new EventEmitter();
     this._destroyedListeners = new EventEmitter();
 
+    // Cache for all resources by the order that the resource was taken.
+    this._cache = [];
     this._listenerCount = new Map();
-
-    // This set is only used to know which resources have been watched and then
-    // unwatched, since the ResourceWatcher doesn't support calling
-    // watch, unwatch and watch again.
-    this._previouslyListenedTypes = new Set();
   }
 
   get contentToolboxFissionPrefValue() {
@@ -69,7 +66,7 @@ class ResourceWatcher {
    *                                  existing resources.
    */
   async watchResources(resources, options) {
-    const { ignoreExistingResources = false } = options;
+    const { onAvailable, ignoreExistingResources = false } = options;
 
     // First ensuring enabling listening to targets.
     // This will call onTargetAvailable for all already existing targets,
@@ -79,15 +76,12 @@ class ResourceWatcher {
     await this._watchAllTargets();
 
     for (const resource of resources) {
-      if (ignoreExistingResources) {
-        // Register listeners after _startListening
-        // so that it avoids the listeners to get cached resources.
-        await this._startListening(resource);
-        this._registerListeners(resource, options);
-      } else {
-        this._registerListeners(resource, options);
-        await this._startListening(resource);
-      }
+      await this._startListening(resource);
+      this._registerListeners(resource, options);
+    }
+
+    if (!ignoreExistingResources) {
+      await this._forwardCachedResources(resources, onAvailable);
     }
   }
 
@@ -168,7 +162,13 @@ class ResourceWatcher {
    *        This Front inherits from TargetMixin and is typically
    *        composed of a BrowsingContextTargetFront or ContentProcessTargetFront.
    */
-  async _onTargetAvailable({ targetFront }) {
+  async _onTargetAvailable({ targetFront, isTargetSwitching }) {
+    if (isTargetSwitching) {
+      this._onWillNavigate(targetFront);
+    }
+
+    targetFront.on("will-navigate", () => this._onWillNavigate(targetFront));
+
     // For each resource type...
     for (const resourceType of Object.values(ResourceWatcher.TYPES)) {
       // ...which has at least one listener...
@@ -213,11 +213,16 @@ class ResourceWatcher {
       resource.targetFront = targetFront;
     }
 
+    // Remove after landing bug 1640641.
+    resource.resourceType = resourceType;
+
     this._availableListeners.emit(resourceType, {
       resourceType,
       targetFront,
       resource,
     });
+
+    this._cache.push(resource);
   }
 
   /**
@@ -227,6 +232,11 @@ class ResourceWatcher {
    * XXX: No usage of this yet. May be useful for the inspector? sources?
    */
   _onResourceDestroyed(targetFront, resourceType, resource) {
+    const index = this._cache.indexOf(resource);
+    if (index >= 0) {
+      this._cache.splice(index, 1);
+    }
+
     this._destroyedListeners.emit(resourceType, {
       resourceType,
       targetFront,
@@ -234,6 +244,17 @@ class ResourceWatcher {
     });
   }
 
+  _onWillNavigate(targetFront) {
+    if (targetFront.isTopLevel) {
+      this._cache = [];
+      return;
+    }
+
+    this._cache = this._cache.filter(
+      cachedResource => cachedResource.targetFront !== targetFront
+    );
+  }
+
   /**
    * Start listening for a given type of resource.
    * For backward compatibility code, we register the legacy listeners on
@@ -244,41 +265,13 @@ class ResourceWatcher {
    *        to be listened.
    */
   async _startListening(resourceType) {
-    const isDocumentEvent =
-      resourceType === ResourceWatcher.TYPES.DOCUMENT_EVENT;
-
     let listeners = this._listenerCount.get(resourceType) || 0;
     listeners++;
-    if (listeners > 1) {
-      // If there are several calls to watch, only the first caller receives
-      // "existing" resources. Throw to avoid inconsistent behaviors
-      if (isDocumentEvent) {
-        // For DOCUMENT_EVENT, return without throwing because this is already
-        // used by several callsites in the netmonitor.
-        // This should be reviewed in Bug 1625909.
-        this._listenerCount.set(resourceType, listeners);
-        return;
-      }
-
-      throw new Error(
-        `The ResourceWatcher is already listening to "${resourceType}", ` +
-          "the client should call `watchResources` only once per resource type."
-      );
-    }
-
-    const wasListening = this._previouslyListenedTypes.has(resourceType);
-    if (wasListening && !isDocumentEvent) {
-      // We already called watch/unwatch for this resource.
-      // This can lead to the onAvailable callback being called twice because we
-      // don't perform any cleanup in _unwatchResourcesForTarget.
-      throw new Error(
-        `The ResourceWatcher previously watched "${resourceType}" ` +
-          "and doesn't support watching again on a previous resource."
-      );
-    }
-
     this._listenerCount.set(resourceType, listeners);
-    this._previouslyListenedTypes.add(resourceType);
+
+    if (listeners > 1) {
+      return;
+    }
 
     // If this is the first listener for this type of resource,
     // we should go through all the existing targets as onTargetAvailable
@@ -291,6 +284,18 @@ class ResourceWatcher {
     await Promise.all(promises);
   }
 
+  async _forwardCachedResources(resourceTypes, onAvailable) {
+    for (const resource of this._cache) {
+      if (resourceTypes.includes(resource.resourceType)) {
+        await onAvailable({
+          resourceType: resource.resourceType,
+          targetFront: resource.targetFront,
+          resource,
+        });
+      }
+    }
+  }
+
   /**
    * Call backward compatibility code from `LegacyListeners` in order to listen for a given
    * type of resource from a given target.
@@ -326,6 +331,11 @@ class ResourceWatcher {
       return;
     }
 
+    // Clear the cached resources of the type.
+    this._cache = this._cache.filter(
+      cachedResource => cachedResource.resourceType !== resourceType
+    );
+
     // If this was the last listener, we should stop watching these events from the actors
     // and the actors should stop watching things from the platform
     const targets = this.targetList.getAllTargets(this.targetList.ALL_TYPES);

devtools/shared/resources/tests/browser.ini

@@ -14,7 +14,6 @@ support-files =
 [browser_resources_console_messages.js]
 [browser_resources_document_events.js]
 [browser_resources_error_messages.js]
-[browser_resources_exceptions.js]
 [browser_resources_platform_messages.js]
 [browser_resources_root_node.js]
 [browser_resources_several_resources.js]

devtools/shared/resources/tests/browser_resources_exceptions.js

@@ -1,64 +0,0 @@
-/* Any copyright is dedicated to the Public Domain.
-   http://creativecommons.org/publicdomain/zero/1.0/ */
-
-"use strict";
-
-// Test the ResourceWatcher API exceptions when using unsupported patterns
-
-const {
-  ResourceWatcher,
-} = require("devtools/shared/resources/resource-watcher");
-
-add_task(async function() {
-  // Open a test tab
-  gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser);
-  const tab = await addTab("data:text/html,ResourceWatcher exception tests");
-
-  const {
-    client,
-    resourceWatcher,
-    targetList,
-  } = await initResourceWatcherAndTarget(tab);
-
-  info("Call watchResources once");
-  const onAvailable = () => {};
-  await resourceWatcher.watchResources([ResourceWatcher.TYPES.ROOT_NODE], {
-    onAvailable,
-  });
-
-  info(
-    "Call watchResources again, should throw because we are already listening"
-  );
-  const expectedMessage1 =
-    `The ResourceWatcher is already listening to "${ResourceWatcher.TYPES.ROOT_NODE}", ` +
-    "the client should call `watchResources` only once per resource type.";
-
-  await Assert.rejects(
-    resourceWatcher.watchResources([ResourceWatcher.TYPES.ROOT_NODE], {
-      onAvailable,
-    }),
-    err => err.message === expectedMessage1
-  );
-
-  info("Call unwatchResources");
-  resourceWatcher.unwatchResources([ResourceWatcher.TYPES.ROOT_NODE], {
-    onAvailable,
-  });
-
-  info(
-    "Call watchResources again, should throw because we already listened previously"
-  );
-  const expectedMessage2 =
-    `The ResourceWatcher previously watched "${ResourceWatcher.TYPES.ROOT_NODE}" ` +
-    "and doesn't support watching again on a previous resource.";
-
-  await Assert.rejects(
-    resourceWatcher.watchResources([ResourceWatcher.TYPES.ROOT_NODE], {
-      onAvailable,
-    }),
-    err => err.message === expectedMessage2
-  );
-
-  targetList.stopListening();
-  await client.close();
-});