Backed out 2 changesets (bug 1637641) for failures on browser_resources_client_caching.js. CLOSED TREE
authorCsoregi Natalia <ncsoregi@mozilla.com>
Thu, 28 May 2020 13:24:55 +0300
changeset 532758 272e3c98d0029fe73c0a57c45bc3b11ffa0e2bee
parent 532757 c3f73781a871c11a110eb0abf2b084569a197514
child 532759 5100f853b479e4f21bc1be3663508588f60ba015
push id117351
push userncsoregi@mozilla.com
push dateThu, 28 May 2020 10:28:28 +0000
treeherderautoland@272e3c98d002 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1637641
milestone78.0a1
backs outce69538af12e6d3eadcd61bdb5f92a96749c939b
b6fabe7631779619cfda5f74a03a3eb1d242438e
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
Backed out 2 changesets (bug 1637641) for failures on browser_resources_client_caching.js. CLOSED TREE Backed out changeset ce69538af12e (bug 1637641) Backed out changeset b6fabe763177 (bug 1637641)
devtools/shared/resources/resource-watcher.js
devtools/shared/resources/tests/browser.ini
devtools/shared/resources/tests/browser_resources_client_caching.js
devtools/shared/resources/tests/browser_resources_error_messages.js
devtools/shared/resources/tests/browser_resources_exceptions.js
devtools/shared/resources/tests/head.js
--- a/devtools/shared/resources/resource-watcher.js
+++ b/devtools/shared/resources/resource-watcher.js
@@ -27,19 +27,22 @@ 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
       );
@@ -61,32 +64,35 @@ 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 { onAvailable, ignoreExistingResources = false } = options;
+    const { 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) {
-      await this._startListening(resource);
-      this._registerListeners(resource, options);
-    }
-
-    if (!ignoreExistingResources) {
-      await this._forwardCachedResources(resources, onAvailable);
+      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);
+      }
     }
   }
 
   /**
    * Stop watching for given type of resources.
    * See `watchResources` for the arguments as both methods receive the same.
    */
   unwatchResources(resources, options) {
@@ -157,23 +163,17 @@ 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, isTargetSwitching }) {
-    if (isTargetSwitching) {
-      this._onWillNavigate(targetFront);
-    }
-
-    targetFront.on("will-navigate", () => this._onWillNavigate(targetFront));
-
+  async _onTargetAvailable({ 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,99 +208,94 @@ 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++;
-    this._listenerCount.set(resourceType, 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;
+      }
 
-    if (listeners > 1) {
-      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 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,
@@ -326,21 +321,16 @@ 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
@@ -6,20 +6,20 @@ support-files =
   !/devtools/client/shared/test/telemetry-test-helpers.js
   !/devtools/client/shared/test/test-actor.js
   head.js
   fission_document.html
   fission_iframe.html
   test_service_worker.js
   test_worker.js
 
-[browser_resources_client_caching.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_client_caching.js
+++ /dev/null
@@ -1,315 +0,0 @@
-/* Any copyright is dedicated to the Public Domain.
-   http://creativecommons.org/publicdomain/zero/1.0/ */
-
-"use strict";
-
-// Test the cache mechanism of the ResourceWatcher.
-
-const {
-  ResourceWatcher,
-} = require("devtools/shared/resources/resource-watcher");
-
-add_task(async function() {
-  info("Test whether multiple listener can get same cached resources");
-
-  const tab = await addTab("data:text/html,Cache Test");
-
-  const {
-    client,
-    resourceWatcher,
-    targetList,
-  } = await initResourceWatcherAndTarget(tab);
-
-  info("Add messages as existing resources");
-  const messages = ["a", "b", "c"];
-  await logMessages(tab.linkedBrowser, messages);
-
-  info("Register first listener");
-  const cachedResources1 = [];
-  await resourceWatcher.watchResources(
-    [ResourceWatcher.TYPES.CONSOLE_MESSAGE],
-    {
-      onAvailable: ({ resource }) => cachedResources1.push(resource),
-    }
-  );
-
-  info("Register second listener");
-  const cachedResources2 = [];
-  await resourceWatcher.watchResources(
-    [ResourceWatcher.TYPES.CONSOLE_MESSAGE],
-    {
-      onAvailable: ({ resource }) => cachedResources2.push(resource),
-    }
-  );
-
-  assertContents(cachedResources1, messages);
-  assertResources(cachedResources2, cachedResources1);
-
-  await targetList.stopListening();
-  await client.close();
-});
-
-add_task(async function() {
-  info(
-    "Test whether the cache is reflecting existing resources and additional resources"
-  );
-
-  const tab = await addTab("data:text/html,Cache Test");
-
-  const {
-    client,
-    resourceWatcher,
-    targetList,
-  } = await initResourceWatcherAndTarget(tab);
-
-  info("Add messages as existing resources");
-  const existingMessages = ["a", "b", "c"];
-  await logMessages(tab.linkedBrowser, existingMessages);
-
-  info("Register first listener to get all available resources");
-  const availableResources = [];
-  await resourceWatcher.watchResources(
-    [ResourceWatcher.TYPES.CONSOLE_MESSAGE],
-    {
-      onAvailable: ({ resource }) => availableResources.push(resource),
-    }
-  );
-
-  info("Add messages as additional resources");
-  const additionalMessages = ["d", "e"];
-  await logMessages(tab.linkedBrowser, additionalMessages);
-
-  info("Wait until onAvailable is called expected times");
-  const allMessages = [...existingMessages, ...additionalMessages];
-  await waitUntil(() => availableResources.length === allMessages.length);
-
-  info("Register second listener to get the cached resources");
-  const cachedResources = [];
-  await resourceWatcher.watchResources(
-    [ResourceWatcher.TYPES.CONSOLE_MESSAGE],
-    {
-      onAvailable: ({ resource }) => cachedResources.push(resource),
-    }
-  );
-
-  assertContents(availableResources, allMessages);
-  assertResources(cachedResources, availableResources);
-
-  await targetList.stopListening();
-  await client.close();
-});
-
-add_task(async function() {
-  info("Test whether the cache is cleared when navigation");
-
-  const tab = await addTab("data:text/html,Cache Test");
-
-  const {
-    client,
-    resourceWatcher,
-    targetList,
-  } = await initResourceWatcherAndTarget(tab);
-
-  info("Add messages as existing resources");
-  const existingMessages = ["a", "b", "c"];
-  await logMessages(tab.linkedBrowser, existingMessages);
-
-  info("Register first listener");
-  await resourceWatcher.watchResources(
-    [ResourceWatcher.TYPES.CONSOLE_MESSAGE],
-    {
-      onAvailable: () => {},
-    }
-  );
-
-  info("Reload the page");
-  const onReloaded = BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
-  gBrowser.reloadTab(tab);
-  await onReloaded;
-
-  info("Register second listener");
-  const cachedResources = [];
-  await resourceWatcher.watchResources(
-    [ResourceWatcher.TYPES.CONSOLE_MESSAGE],
-    {
-      onAvailable: ({ resource }) => cachedResources.push(resource),
-    }
-  );
-
-  is(cachedResources.length, 0, "The cache in ResourceWatcher is cleared");
-
-  await targetList.stopListening();
-  await client.close();
-});
-
-add_task(async function() {
-  info("Test with multiple resource types");
-
-  const tab = await addTab("data:text/html,Cache Test");
-
-  const {
-    client,
-    resourceWatcher,
-    targetList,
-  } = await initResourceWatcherAndTarget(tab);
-
-  info("Register first listener to get all available resources");
-  const availableResources = [];
-  await resourceWatcher.watchResources(
-    [
-      ResourceWatcher.TYPES.CONSOLE_MESSAGE,
-      ResourceWatcher.TYPES.ERROR_MESSAGE,
-    ],
-    {
-      onAvailable: ({ resource }) => availableResources.push(resource),
-    }
-  );
-
-  info("Add messages as console message");
-  const consoleMessages1 = ["a", "b", "c"];
-  await logMessages(tab.linkedBrowser, consoleMessages1);
-
-  info("Add message as error message");
-  const errorMessages = ["document.doTheImpossible();"];
-  await triggerErrors(tab.linkedBrowser, errorMessages);
-
-  info("Add messages as console message again");
-  const consoleMessages2 = ["d", "e"];
-  await logMessages(tab.linkedBrowser, consoleMessages2);
-
-  info("Register listener to get the cached resources");
-  const cachedResources = [];
-  await resourceWatcher.watchResources(
-    [
-      ResourceWatcher.TYPES.CONSOLE_MESSAGE,
-      ResourceWatcher.TYPES.ERROR_MESSAGE,
-    ],
-    {
-      onAvailable: ({ resource }) => cachedResources.push(resource),
-    }
-  );
-
-  is(
-    availableResources.length,
-    consoleMessages1.length + errorMessages.length + consoleMessages2.length,
-    "Number of the available resources is correct"
-  );
-  assertResources(cachedResources, availableResources);
-
-  await targetList.stopListening();
-  await client.close();
-});
-
-add_task(async function() {
-  info("Test multiple listeners with/without ignoreExistingResources");
-  await testIgnoreExistingResources(true);
-  await testIgnoreExistingResources(false);
-});
-
-async function testIgnoreExistingResources(isFirstListenerIgnoreExisting) {
-  const tab = await addTab("data:text/html,Cache Test");
-
-  const {
-    client,
-    resourceWatcher,
-    targetList,
-  } = await initResourceWatcherAndTarget(tab);
-
-  info("Add messages as existing resources");
-  const existingMessages = ["a", "b", "c"];
-  await logMessages(tab.linkedBrowser, existingMessages);
-
-  info("Register first listener");
-  const cachedResources1 = [];
-  await resourceWatcher.watchResources(
-    [ResourceWatcher.TYPES.CONSOLE_MESSAGE],
-    {
-      onAvailable: ({ resource }) => cachedResources1.push(resource),
-      ignoreExistingResources: isFirstListenerIgnoreExisting,
-    }
-  );
-
-  info("Register second listener");
-  const cachedResources2 = [];
-  await resourceWatcher.watchResources(
-    [ResourceWatcher.TYPES.CONSOLE_MESSAGE],
-    {
-      onAvailable: ({ resource }) => cachedResources2.push(resource),
-      ignoreExistingResources: !isFirstListenerIgnoreExisting,
-    }
-  );
-
-  const cachedResourcesWithFlag = isFirstListenerIgnoreExisting
-    ? cachedResources1
-    : cachedResources2;
-  const cachedResourcesWithoutFlag = isFirstListenerIgnoreExisting
-    ? cachedResources2
-    : cachedResources1;
-
-  info("Check the existing resources both listeners got");
-  assertContents(cachedResourcesWithFlag, []);
-  assertContents(cachedResourcesWithoutFlag, existingMessages);
-
-  info("Add messages as additional resources");
-  const additionalMessages = ["d", "e"];
-  await logMessages(tab.linkedBrowser, additionalMessages);
-
-  info("Wait until onAvailable is called expected times");
-  await waitUntil(
-    () => cachedResourcesWithFlag.length === additionalMessages.length
-  );
-  const allMessages = [...existingMessages, ...additionalMessages];
-  await waitUntil(
-    () => cachedResourcesWithoutFlag.length === allMessages.length
-  );
-
-  info("Check the resources after adding messages");
-  assertContents(cachedResourcesWithFlag, additionalMessages);
-  assertContents(cachedResourcesWithoutFlag, allMessages);
-
-  await targetList.stopListening();
-  await client.close();
-}
-
-function assertContents(resources, expectedMessages) {
-  is(
-    resources.length,
-    expectedMessages.length,
-    "Number of the resources is correct"
-  );
-
-  for (let i = 0; i < expectedMessages.length; i++) {
-    const resource = resources[i];
-    const message = resource.message.arguments[0];
-    const expectedMessage = expectedMessages[i];
-    is(message, expectedMessage, `The ${i}th content is correct`);
-  }
-}
-
-function assertResources(resources, expectedResources) {
-  is(
-    resources.length,
-    expectedResources.length,
-    "Number of the resources is correct"
-  );
-
-  for (let i = 0; i < resources.length; i++) {
-    const resource = resources[i];
-    const expectedResource = expectedResources[i];
-    ok(resource === expectedResource, `The ${i}th resource is correct`);
-  }
-}
-
-function logMessages(browser, messages) {
-  return ContentTask.spawn(browser, { messages }, args => {
-    for (const message of args.messages) {
-      content.console.log(message);
-    }
-  });
-}
-
-async function triggerErrors(browser, errorScripts) {
-  for (const errorScript of errorScripts) {
-    await triggerError(browser, errorScript);
-  }
-}
--- a/devtools/shared/resources/tests/browser_resources_error_messages.js
+++ b/devtools/shared/resources/tests/browser_resources_error_messages.js
@@ -152,17 +152,30 @@ async function triggerErrors(tab) {
   for (const [expression, expected] of expectedPageErrors.entries()) {
     if (
       !expected[noUncaughtException] &&
       !Services.appinfo.browserTabsRemoteAutostart
     ) {
       expectUncaughtException();
     }
 
-    await triggerError(tab.linkedBrowser, expression);
+    await ContentTask.spawn(tab.linkedBrowser, expression, function frameScript(
+      expr
+    ) {
+      const document = content.document;
+      const container = document.createElement("script");
+      document.body.appendChild(container);
+      container.textContent = expr;
+      container.remove();
+    });
+    // Wait a bit between each messages, as uncaught promises errors are not emitted
+    // right away.
+
+    // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
+    await new Promise(res => setTimeout(res, 10));
   }
 }
 
 const noUncaughtException = Symbol();
 
 const expectedPageErrors = new Map([
   [
     "document.doTheImpossible();",
new file mode 100644
--- /dev/null
+++ b/devtools/shared/resources/tests/browser_resources_exceptions.js
@@ -0,0 +1,64 @@
+/* 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();
+});
--- a/devtools/shared/resources/tests/head.js
+++ b/devtools/shared/resources/tests/head.js
@@ -85,26 +85,8 @@ function checkValue(name, value, expecte
   } else if (Array.isArray(expected)) {
     info("checking array for property '" + name + "'");
     checkObject(value, expected);
   } else if (typeof expected == "object") {
     info("checking object for property '" + name + "'");
     checkObject(value, expected);
   }
 }
-
-/**
- * Triggers a script error in the content page.
- */
-async function triggerError(browser, expression) {
-  await ContentTask.spawn(browser, expression, function frameScript(expr) {
-    const document = content.document;
-    const container = document.createElement("script");
-    document.body.appendChild(container);
-    container.textContent = expr;
-    container.remove();
-  });
-  // Wait a bit between each messages, as uncaught promises errors are not emitted
-  // right away.
-
-  // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
-  await new Promise(res => setTimeout(res, 10));
-}