From a61fda7ffd14c12da48490af0d6c2844317e0960 Mon Sep 17 00:00:00 2001
From: Julian Descottes <jdescottes@mozilla.com>
Date: Sun, 10 May 2020 18:42:15 +0000
Subject: [PATCH] Bug 1625961 - Throw when calling ResourceWatcher::watch twice
 for the same type r=nchevobbe,ochameau

Depends on D72668

Differential Revision: https://phabricator.services.mozilla.com/D72854
---
 devtools/shared/resources/resource-watcher.js | 40 +++++++++++++-
 devtools/shared/resources/tests/browser.ini   |  1 +
 .../tests/browser_resources_exceptions.js     | 52 +++++++++++++++++++
 3 files changed, 91 insertions(+), 2 deletions(-)
 create mode 100644 devtools/shared/resources/tests/browser_resources_exceptions.js

diff --git a/devtools/shared/resources/resource-watcher.js b/devtools/shared/resources/resource-watcher.js
index 3b2e9ff12f76..7ac5811759dc 100644
--- a/devtools/shared/resources/resource-watcher.js
+++ b/devtools/shared/resources/resource-watcher.js
@@ -33,6 +33,11 @@ class ResourceWatcher {
     this._destroyedListeners = new EventEmitter();
 
     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() {
@@ -221,12 +226,42 @@ class ResourceWatcher {
    *        to be listened.
    */
   async _startListening(resourceType) {
+    const isDocumentEvent =
+      resourceType === ResourceWatcher.TYPES.DOCUMENT_EVENTS;
+
     let listeners = this._listenerCount.get(resourceType) || 0;
     listeners++;
-    this._listenerCount.set(resourceType, listeners);
     if (listeners > 1) {
-      return;
+      // If there are several calls to watch, only the first caller receives
+      // "existing" resources. Throw to avoid inconsistent behaviors
+      if (isDocumentEvent) {
+        // For DOCUMENT_EVENTS, 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 `watch` 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 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.
@@ -283,6 +318,7 @@ class ResourceWatcher {
     if (listeners > 0) {
       return;
     }
+
     // 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
     for (const targetType of this.targetList.ALL_TYPES) {
diff --git a/devtools/shared/resources/tests/browser.ini b/devtools/shared/resources/tests/browser.ini
index 482e7ab1c9c9..86c7fa8e8c63 100644
--- a/devtools/shared/resources/tests/browser.ini
+++ b/devtools/shared/resources/tests/browser.ini
@@ -14,6 +14,7 @@ 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_target_list_frames.js]
diff --git a/devtools/shared/resources/tests/browser_resources_exceptions.js b/devtools/shared/resources/tests/browser_resources_exceptions.js
new file mode 100644
index 000000000000..11223eba8848
--- /dev/null
+++ b/devtools/shared/resources/tests/browser_resources_exceptions.js
@@ -0,0 +1,52 @@
+/* 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 watch once");
+  const onAvailable = () => {};
+  await resourceWatcher.watch([ResourceWatcher.TYPES.ROOT_NODE], onAvailable);
+
+  info("Call watch again, should throw because we are already listening");
+  const expectedMessage1 =
+    `The ResourceWatcher is already listening to "${ResourceWatcher.TYPES.ROOT_NODE}", ` +
+    "the client should call `watch` only once per resource type.";
+
+  await Assert.rejects(
+    resourceWatcher.watch([ResourceWatcher.TYPES.ROOT_NODE], onAvailable),
+    err => err.message === expectedMessage1
+  );
+
+  info("Call unwatch");
+  resourceWatcher.unwatch([ResourceWatcher.TYPES.ROOT_NODE], onAvailable);
+
+  info("Call watch 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.watch([ResourceWatcher.TYPES.ROOT_NODE], onAvailable),
+    err => err.message === expectedMessage2
+  );
+
+  targetList.stopListening();
+  await client.close();
+});