Bug 1637641: Make multiple listeners can receive proper cached resources. r=ochameau
☠☠ backed out by 272e3c98d002 ☠ ☠
authorDaisuke Akatsuka <daisuke@birchill.co.jp>
Thu, 28 May 2020 09:07:04 +0000
changeset 532730 b6fabe7631779619cfda5f74a03a3eb1d242438e
parent 532729 cb80856ac40b28cf28f6299d4c3b14df4974157b
child 532731 ce69538af12e6d3eadcd61bdb5f92a96749c939b
push id117344
push userdakatsuka.birchill@mozilla.com
push dateThu, 28 May 2020 09:09:15 +0000
treeherderautoland@ce69538af12e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersochameau
bugs1637641
milestone78.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
devtools/shared/resources/tests/browser.ini
devtools/shared/resources/tests/browser_resources_exceptions.js
--- a/devtools/shared/resources/resource-watcher.js
+++ b/devtools/shared/resources/resource-watcher.js
@@ -27,22 +27,19 @@ class ResourceWatcher {
     this._onTargetAvailable = this._onTargetAvailable.bind(this);
     this._onTargetDestroyed = this._onTargetDestroyed.bind(this);
 
     this._onResourceAvailable = this._onResourceAvailable.bind(this);
 
     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() {
     if (!this._contentToolboxFissionPrefValue) {
       this._contentToolboxFissionPrefValue = Services.prefs.getBoolPref(
         "devtools.contenttoolbox.fission",
         false
       );
@@ -64,35 +61,32 @@ class ResourceWatcher {
    *                                  Function which will be called each time a resource in
    *                                  the remote target is destroyed.
    *        - {boolean} ignoreExistingResources:
    *                                  This attribute is optional. Default value is false.
    *                                  If set to true, onAvailable won't be called with
    *                                  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,
     // as well as for the one created later.
     // Do this *before* calling _startListening in order to register
     // "resource-available" listener before requesting for the resources in _startListening.
     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);
     }
   }
 
   /**
    * Stop watching for given type of resources.
    * See `watchResources` for the arguments as both methods receive the same.
    */
   unwatchResources(resources, options) {
@@ -163,17 +157,23 @@ class ResourceWatcher {
   /**
    * Method called by the TargetList for each already existing or target which has just been created.
    *
    * @param {Front} targetFront
    *        The Front of the target that is available.
    *        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...
       if (!this._listenerCount.get(resourceType)) {
         continue;
       }
       // ...request existing resource and new one to come from this one target
       await this._watchResourcesForTarget(targetFront, resourceType);
@@ -208,94 +208,99 @@ class ResourceWatcher {
    *        which describes the resource.
    */
   _onResourceAvailable(targetFront, resourceType, resource) {
     // Put the targetFront on the resource for easy retrieval.
     if (!resource.targetFront) {
       resource.targetFront = targetFront;
     }
 
+    // Remove after landing bug 1640641.
+    resource.resourceType = resourceType;
+
     this._availableListeners.emit(resourceType, {
       resourceType,
       targetFront,
       resource,
     });
+
+    this._cache.push(resource);
   }
 
   /**
    * Called everytime a resource is destroyed in the remote target.
    * See _onResourceAvailable for the argument description.
    *
    * 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,
       resource,
     });
   }
 
+  _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
    * each individual target
    *
    * @param {String} resourceType
    *        One string of ResourceWatcher.TYPES, which designates the types of resources
    *        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;
-      }
+    this._listenerCount.set(resourceType, listeners);
 
-      throw new Error(
-        `The ResourceWatcher is already listening to "${resourceType}", ` +
-          "the client should call `watchResources` only once per resource type."
-      );
+    if (listeners > 1) {
+      return;
     }
 
-    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 this is the first listener for this type of resource,
     // we should go through all the existing targets as onTargetAvailable
     // has already been called for these existing targets.
     const promises = [];
     const targets = this.targetList.getAllTargets(this.targetList.ALL_TYPES);
     for (const target of targets) {
       promises.push(this._watchResourcesForTarget(target, resourceType));
     }
     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.
    */
   _watchResourcesForTarget(targetFront, resourceType) {
     const onAvailable = this._onResourceAvailable.bind(
       this,
       targetFront,
@@ -321,16 +326,21 @@ class ResourceWatcher {
       );
     }
     listeners--;
     this._listenerCount.set(resourceType, listeners);
     if (listeners > 0) {
       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);
     for (const target of targets) {
       this._unwatchResourcesForTarget(target, resourceType);
     }
   }
 
--- a/devtools/shared/resources/tests/browser.ini
+++ b/devtools/shared/resources/tests/browser.ini
@@ -9,17 +9,16 @@ support-files =
   fission_document.html
   fission_iframe.html
   test_service_worker.js
   test_worker.js
 
 [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]
 [browser_target_list_frames.js]
 [browser_target_list_getAllTargets.js]
 [browser_target_list_preffedoff.js]
 [browser_target_list_processes.js]
 [browser_target_list_switchToTarget.js]
deleted file mode 100644
--- a/devtools/shared/resources/tests/browser_resources_exceptions.js
+++ /dev/null
@@ -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();
-});