Bug 1625905 - Use the ResourceWatcher API to fetch JS Errors. r=ochameau.

In this patch we add a new resource type for page errors.
We don't do anything specific for CSS Warnings yet, as they're going
to be handled as part of Bug 1625910.

A test is added (following devtools/shared/webconsole/test/chrome/test_page_errors.html
as an example).
A couple function that were used for the console-messages test
are moved into head.js as they're also used in the error-message test.

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

devtools/client/webconsole/webconsole-connection-proxy.js

@@ -37,7 +37,6 @@ class WebConsoleConnectionProxy {
 
     this._connecter = null;
 
-    this._onPageError = this._onPageError.bind(this);
     this._onNetworkEvent = this._onNetworkEvent.bind(this);
     this._onNetworkEventUpdate = this._onNetworkEventUpdate.bind(this);
     this._onTabNavigated = this._onTabNavigated.bind(this);
@@ -78,13 +77,8 @@ class WebConsoleConnectionProxy {
         );
       await this.webConsoleUI.setSaveRequestAndResponseBodies(saveBodies);
 
-      // Note that we only fetch PageError from _getCachedMessages.
-      // ConsoleAPI is already fetched via the Resources API.
-      const cachedMessages = await this._getCachedMessages();
       const networkMessages = this._getNetworkMessages();
-      const messages = cachedMessages.concat(networkMessages);
-      messages.sort((a, b) => a.timeStamp - b.timeStamp);
-      this.dispatchMessagesAdd(messages);
+      this.dispatchMessagesAdd(networkMessages);
 
       this._addWebConsoleFrontEventListeners();
 
@@ -120,7 +114,7 @@ class WebConsoleConnectionProxy {
    * @returns Promise
    */
   _attachConsole() {
-    const listeners = ["PageError", "NetworkActivity"];
+    const listeners = ["NetworkActivity"];
     // Enable the forwarding of console messages to the parent process
     // when we open the Browser Console or Toolbox without fission support. If Fission
     // is enabled, we don't use the ContentProcessMessages listener, but attach to the
@@ -139,7 +133,6 @@ class WebConsoleConnectionProxy {
   _addWebConsoleFrontEventListeners() {
     this.webConsoleFront.on("networkEvent", this._onNetworkEvent);
     this.webConsoleFront.on("networkEventUpdate", this._onNetworkEventUpdate);
-    this.webConsoleFront.on("pageError", this._onPageError);
     this.webConsoleFront.on(
       "lastPrivateContextExited",
       this._onLastPrivateContextExited
@@ -158,7 +151,6 @@ class WebConsoleConnectionProxy {
   _removeWebConsoleFrontEventListeners() {
     this.webConsoleFront.off("networkEvent", this._onNetworkEvent);
     this.webConsoleFront.off("networkEventUpdate", this._onNetworkEventUpdate);
-    this.webConsoleFront.off("pageError", this._onPageError);
     this.webConsoleFront.off(
       "lastPrivateContextExited",
       this._onLastPrivateContextExited
@@ -169,32 +161,6 @@ class WebConsoleConnectionProxy {
     );
   }
 
-  /**
-   * Get cached messages from the server.
-   *
-   * @private
-   * @returns A Promise that resolves with the cached messages, or reject if something
-   *          went wrong.
-   */
-  async _getCachedMessages() {
-    const response = await this.webConsoleFront.getCachedMessages([
-      "PageError",
-    ]);
-
-    if (response.error) {
-      throw new Error(
-        `Web Console getCachedMessages error: ${response.error} ${response.message}`
-      );
-    }
-
-    if (this.webConsoleFront.traits.newCacheStructure) {
-      return response.messages;
-    }
-
-    // On older server, we're also getting cached LogMessages, so we need to remove them
-    return response.messages.filter(message => message._type !== "LogMessage");
-  }
-
   /**
    * Get network messages from the server.
    *
@@ -205,22 +171,6 @@ class WebConsoleConnectionProxy {
     return Array.from(this.webConsoleFront.getNetworkEvents());
   }
 
-  /**
-   * The "pageError" message type handler. We redirect any page errors to the UI
-   * for displaying.
-   *
-   * @private
-   * @param object packet
-   *        The message received from the server.
-   */
-  _onPageError(packet) {
-    if (!this.webConsoleUI) {
-      return;
-    }
-    packet.type = "pageError";
-    this.dispatchMessageAdd(packet);
-  }
-
   _clearLogpointMessages(logpointId) {
     if (this.webConsoleUI) {
       this.webConsoleUI.wrapper.dispatchClearLogpointMessages(logpointId);

devtools/client/webconsole/webconsole-ui.js

@@ -335,6 +335,7 @@ class WebConsoleUI {
     await resourceWatcher.watch(
       [
         resourceWatcher.TYPES.CONSOLE_MESSAGES,
+        resourceWatcher.TYPES.ERROR_MESSAGES,
         resourceWatcher.TYPES.PLATFORM_MESSAGES,
       ],
       this._onResourceAvailable
@@ -351,6 +352,14 @@ class WebConsoleUI {
       return;
     }
 
+    if (resourceType == resourceWatcher.TYPES.ERROR_MESSAGES) {
+      // resource is the packet sent from `ConsoleActor.getCachedMessages().messages`
+      // or via ConsoleActor's `pageError` event.
+      resource.type = "pageError";
+      this.wrapper.dispatchMessageAdd(resource);
+      return;
+    }
+
     if (resourceType == resourceWatcher.TYPES.PLATFORM_MESSAGES) {
       // resource is the packet sent from `ConsoleActor.getCachedMessages().messages`
       // or via ConsoleActor's `logMessage` event.

devtools/shared/resources/legacy-listeners/console-messages.js

@@ -4,13 +4,12 @@
 
 "use strict";
 
-const Services = require("Services");
-
 module.exports = async function({
   targetList,
   targetType,
   targetFront,
   isTopLevel,
+  isFissionEnabledOnContentToolbox,
   onAvailable,
 }) {
   // Allow the top level target unconditionnally.
@@ -19,9 +18,7 @@ module.exports = async function({
   // messages via the process targets
   // Also ignore workers as they are not supported yet. (see bug 1592584)
   const isContentToolbox = targetList.targetFront.isLocalTab;
-  const listenForFrames =
-    isContentToolbox &&
-    Services.prefs.getBoolPref("devtools.contenttoolbox.fission");
+  const listenForFrames = isContentToolbox && isFissionEnabledOnContentToolbox;
   const isAllowed =
     isTopLevel ||
     targetType === targetList.TYPES.PROCESS ||

devtools/shared/resources/legacy-listeners/error-messages.js

@@ -0,0 +1,62 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+module.exports = async function({
+  targetList,
+  targetType,
+  targetFront,
+  isTopLevel,
+  isFissionEnabledOnContentToolbox,
+  onAvailable,
+}) {
+  // Allow the top level target unconditionnally.
+  // Also allow frame, but only in content toolbox, when the fission/content toolbox pref is
+  // set. i.e. still ignore them in the content of the browser toolbox as we inspect
+  // messages via the process targets
+  // Also ignore workers as they are not supported yet. (see bug 1592584)
+  const isContentToolbox = targetList.targetFront.isLocalTab;
+  const listenForFrames = isContentToolbox && isFissionEnabledOnContentToolbox;
+  const isAllowed =
+    isTopLevel ||
+    targetType === targetList.TYPES.PROCESS ||
+    (targetType === targetList.TYPES.FRAME && listenForFrames);
+
+  if (!isAllowed) {
+    return;
+  }
+
+  const webConsoleFront = await targetFront.getFront("console");
+
+  // Request notifying about new messages. Here the "PageError" type start listening for
+  // both actual PageErrors (emitted as "pageError" events) as well as LogMessages (
+  // emitted as "logMessage" events). This function only set up the listener on the
+  // webConsoleFront for "pageError".
+  await webConsoleFront.startListeners(["PageError"]);
+
+  // Fetch already existing messages
+  // /!\ The actor implementation requires to call startListeners("PageError") first /!\
+  const { messages } = await webConsoleFront.getCachedMessages(["PageError"]);
+
+  for (let message of messages) {
+    // On older server (< v77), we're also getting LogMessage cached messages, so we need
+    // to ignore those.
+    if (
+      !webConsoleFront.traits.newCacheStructure &&
+      message._type !== "PageError"
+    ) {
+      continue;
+    }
+
+    // Cached messages don't have the same shape as live messages,
+    // so we need to transform them.
+    if (message._type) {
+      message = { pageError: message };
+    }
+    onAvailable(message);
+  }
+
+  webConsoleFront.on("pageError", onAvailable);
+};

devtools/shared/resources/legacy-listeners/moz.build

@@ -4,5 +4,6 @@
 
 DevToolsModules(
     'console-messages.js',
+    'error-messages.js',
     'platform-messages.js',
 )

devtools/shared/resources/resource-watcher.js

@@ -5,6 +5,7 @@
 "use strict";
 
 const EventEmitter = require("devtools/shared/event-emitter");
+const Services = require("Services");
 
 class ResourceWatcher {
   /**
@@ -34,6 +35,16 @@ class ResourceWatcher {
     this._listenerCount = new Map();
   }
 
+  get contentToolboxFissionPrefValue() {
+    if (!this._contentToolboxFissionPrefValue) {
+      this._contentToolboxFissionPrefValue = Services.prefs.getBoolPref(
+        "devtools.contenttoolbox.fission",
+        false
+      );
+    }
+    return this._contentToolboxFissionPrefValue;
+  }
+
   /**
    * Request to start retrieving all already existing instances of given
    * type of resources and also start watching for the one to be created after.
@@ -251,6 +262,7 @@ class ResourceWatcher {
       targetType,
       targetFront,
       isTopLevel,
+      isFissionEnabledOnContentToolbox: this.contentToolboxFissionPrefValue,
       onAvailable,
     });
   }
@@ -300,6 +312,7 @@ class ResourceWatcher {
 
 ResourceWatcher.TYPES = ResourceWatcher.prototype.TYPES = {
   CONSOLE_MESSAGES: "console-messages",
+  ERROR_MESSAGES: "error-messages",
   PLATFORM_MESSAGES: "platform-messages",
 };
 module.exports = { ResourceWatcher };
@@ -310,6 +323,8 @@ module.exports = { ResourceWatcher };
 const LegacyListeners = {
   [ResourceWatcher.TYPES
     .CONSOLE_MESSAGES]: require("devtools/shared/resources/legacy-listeners/console-messages"),
+  [ResourceWatcher.TYPES
+    .ERROR_MESSAGES]: require("devtools/shared/resources/legacy-listeners/error-messages"),
   [ResourceWatcher.TYPES
     .PLATFORM_MESSAGES]: require("devtools/shared/resources/legacy-listeners/platform-messages"),
 };

devtools/shared/resources/tests/browser.ini

@@ -12,6 +12,7 @@ support-files =
   test_worker.js
 
 [browser_resources_console_messages.js]
+[browser_resources_error_messages.js]
 [browser_resources_platform_messages.js]
 [browser_target_list_frames.js]
 [browser_target_list_preffedoff.js]

devtools/shared/resources/tests/browser_resources_console_messages.js

@@ -303,39 +303,3 @@ function checkConsoleAPICall(call, expected) {
 
   checkObject(call, expected);
 }
-
-function checkObject(object, expected) {
-  if (object && object.getGrip) {
-    object = object.getGrip();
-  }
-
-  for (const name of Object.keys(expected)) {
-    const expectedValue = expected[name];
-    const value = object[name];
-    checkValue(name, value, expectedValue);
-  }
-}
-
-function checkValue(name, value, expected) {
-  if (expected === null) {
-    ok(!value, "'" + name + "' is null");
-  } else if (value === undefined) {
-    ok(false, "'" + name + "' is undefined");
-  } else if (value === null) {
-    ok(false, "'" + name + "' is null");
-  } else if (
-    typeof expected == "string" ||
-    typeof expected == "number" ||
-    typeof expected == "boolean"
-  ) {
-    is(value, expected, "property '" + name + "'");
-  } else if (expected instanceof RegExp) {
-    ok(expected.test(value), name + ": " + expected + " matched " + value);
-  } 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);
-  }
-}

devtools/shared/resources/tests/browser_resources_error_messages.js

@@ -0,0 +1,267 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Test the ResourceWatcher API around ERROR_MESSAGES
+// Reproduces assertions from devtools/shared/webconsole/test/chrome/test_page_errors.html
+
+const {
+  ResourceWatcher,
+} = require("devtools/shared/resources/resource-watcher");
+
+// Create a simple server so we have a nice sourceName in the resources packets.
+const httpServer = createTestHTTPServer();
+httpServer.registerPathHandler(`/test_page_errors.html`, (req, res) => {
+  res.setStatusLine(req.httpVersion, 200, "OK");
+  res.write(`<meta charset=utf8>Test Error Messages`);
+});
+
+const TEST_URI = `http://localhost:${httpServer.identity.primaryPort}/test_page_errors.html`;
+
+add_task(async function() {
+  // Disable the preloaded process as it creates processes intermittently
+  // which forces the emission of RDP requests we aren't correctly waiting for.
+  await pushPref("dom.ipc.processPrelaunch.enabled", false);
+
+  // Open a test tab
+  gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser);
+  const tab = await addTab(TEST_URI);
+
+  const {
+    client,
+    resourceWatcher,
+    targetList,
+  } = await initResourceWatcherAndTarget(tab);
+
+  const receivedMessages = [];
+  // The expected messages are the errors, twice (once for cached messages, once for live messages)
+  const expectedMessages = Array.from(expectedPageErrors.values()).concat(
+    Array.from(expectedPageErrors.values())
+  );
+
+  info(
+    "Log some errors *before* calling ResourceWatcher.watch in order to assert the behavior of already existing messages."
+  );
+  await triggerErrors(tab);
+
+  let done;
+  const onAllErrorReceived = new Promise(resolve => (done = resolve));
+
+  await resourceWatcher.watch(
+    [ResourceWatcher.TYPES.ERROR_MESSAGES],
+    ({ resourceType, targetFront, resource }) => {
+      const { pageError } = resource;
+
+      ok(pageError, "The resource has a pageError attribute");
+
+      if (!pageError.sourceName.includes("test_page_errors")) {
+        info(`Ignore error from unknown source: "${pageError.sourceName}"`);
+        return;
+      }
+
+      receivedMessages.push(pageError);
+
+      const index = receivedMessages.length - 1;
+      info("checking received page error #" + index);
+      checkObject(pageError, expectedMessages[index]);
+
+      if (receivedMessages.length == expectedMessages.length) {
+        done();
+      }
+    }
+  );
+
+  info(
+    "Now log errors *after* the call to ResourceWatcher.watch and after having received all existing messages"
+  );
+  await triggerErrors(tab);
+
+  info("Waiting for all expected errors to be received");
+  await onAllErrorReceived;
+  ok(true, "All the expected errors were received");
+
+  Services.console.reset();
+  targetList.stopListening();
+  await client.close();
+});
+
+/**
+ * Triggers all the errors in the content page.
+ */
+function triggerErrors(tab) {
+  return ContentTask.spawn(
+    tab.linkedBrowser,
+    expectedPageErrors,
+    function frameScript(pageErrors) {
+      const document = content.document;
+      for (const expression of pageErrors.keys()) {
+        const container = document.createElement("script");
+        document.body.appendChild(container);
+        container.textContent = expression;
+        container.remove();
+      }
+    }
+  );
+}
+
+const expectedPageErrors = new Map([
+  [
+    "document.doTheImpossible();",
+    {
+      errorMessage: /doTheImpossible/,
+      errorMessageName: undefined,
+      sourceName: /test_page_errors/,
+      category: "content javascript",
+      timeStamp: /^\d+$/,
+      error: true,
+      warning: false,
+    },
+  ],
+  [
+    "(42).toString(0);",
+    {
+      errorMessage: /radix/,
+      errorMessageName: "JSMSG_BAD_RADIX",
+      sourceName: /test_page_errors/,
+      category: "content javascript",
+      timeStamp: /^\d+$/,
+      error: true,
+      warning: false,
+    },
+  ],
+  [
+    "'use strict'; (Object.freeze({name: 'Elsa', score: 157})).score = 0;",
+    {
+      errorMessage: /read.only/,
+      errorMessageName: "JSMSG_READ_ONLY",
+      sourceName: /test_page_errors/,
+      category: "content javascript",
+      timeStamp: /^\d+$/,
+      error: true,
+      warning: false,
+    },
+  ],
+  [
+    "([]).length = -1",
+    {
+      errorMessage: /array length/,
+      errorMessageName: "JSMSG_BAD_ARRAY_LENGTH",
+      sourceName: /test_page_errors/,
+      category: "content javascript",
+      timeStamp: /^\d+$/,
+      error: true,
+      warning: false,
+    },
+  ],
+  [
+    "'abc'.repeat(-1);",
+    {
+      errorMessage: /repeat count.*non-negative/,
+      errorMessageName: "JSMSG_NEGATIVE_REPETITION_COUNT",
+      sourceName: /test_page_errors/,
+      category: "content javascript",
+      timeStamp: /^\d+$/,
+      error: true,
+      warning: false,
+    },
+  ],
+  [
+    "'a'.repeat(2e28);",
+    {
+      errorMessage: /repeat count.*less than infinity/,
+      errorMessageName: "JSMSG_RESULTING_STRING_TOO_LARGE",
+      sourceName: /test_page_errors/,
+      category: "content javascript",
+      timeStamp: /^\d+$/,
+      error: true,
+      warning: false,
+    },
+  ],
+  [
+    "77.1234.toExponential(-1);",
+    {
+      errorMessage: /out of range/,
+      errorMessageName: "JSMSG_PRECISION_RANGE",
+      sourceName: /test_page_errors/,
+      category: "content javascript",
+      timeStamp: /^\d+$/,
+      error: true,
+      warning: false,
+    },
+  ],
+  [
+    "function a() { return; 1 + 1; }",
+    {
+      errorMessage: /unreachable code/,
+      errorMessageName: "JSMSG_STMT_AFTER_RETURN",
+      sourceName: /test_page_errors/,
+      category: "content javascript",
+      timeStamp: /^\d+$/,
+      error: false,
+      warning: true,
+    },
+  ],
+  [
+    "{let a, a;}",
+    {
+      errorMessage: /redeclaration of/,
+      errorMessageName: "JSMSG_REDECLARED_VAR",
+      sourceName: /test_page_errors/,
+      category: "content javascript",
+      timeStamp: /^\d+$/,
+      error: true,
+      warning: false,
+      notes: [
+        {
+          messageBody: /Previously declared at line/,
+          frame: {
+            source: /test_page_errors/,
+          },
+        },
+      ],
+    },
+  ],
+  [
+    `var error = new TypeError("abc");
+      error.name = "MyError";
+      error.message = "here";
+      throw error`,
+    {
+      errorMessage: /MyError: here/,
+      errorMessageName: "",
+      sourceName: /test_page_errors/,
+      category: "content javascript",
+      timeStamp: /^\d+$/,
+      error: true,
+      warning: false,
+    },
+  ],
+  [
+    "DOMTokenList.prototype.contains.call([])",
+    {
+      errorMessage: /does not implement interface/,
+      errorMessageName: "MSG_METHOD_THIS_DOES_NOT_IMPLEMENT_INTERFACE",
+      sourceName: /test_page_errors/,
+      category: "content javascript",
+      timeStamp: /^\d+$/,
+      error: true,
+      warning: false,
+    },
+  ],
+  [
+    `var error2 = new TypeError("abc");
+      error2.name = "MyPromiseError";
+      error2.message = "here2";
+      Promise.reject(error2)`,
+    {
+      errorMessage: /MyPromiseError: here2/,
+      errorMessageName: "",
+      sourceName: /test_page_errors/,
+      category: "content javascript",
+      timeStamp: /^\d+$/,
+      error: true,
+      warning: false,
+    },
+  ],
+]);

devtools/shared/resources/tests/head.js

@@ -53,3 +53,40 @@ async function initResourceWatcherAndTarget(tab) {
 
   return { client, resourceWatcher, targetList };
 }
+
+// Copied from devtools/shared/webconsole/test/chrome/common.js
+function checkObject(object, expected) {
+  if (object && object.getGrip) {
+    object = object.getGrip();
+  }
+
+  for (const name of Object.keys(expected)) {
+    const expectedValue = expected[name];
+    const value = object[name];
+    checkValue(name, value, expectedValue);
+  }
+}
+
+function checkValue(name, value, expected) {
+  if (expected === null) {
+    ok(!value, "'" + name + "' is null");
+  } else if (value === undefined) {
+    ok(false, "'" + name + "' is undefined");
+  } else if (value === null) {
+    ok(false, "'" + name + "' is null");
+  } else if (
+    typeof expected == "string" ||
+    typeof expected == "number" ||
+    typeof expected == "boolean"
+  ) {
+    is(value, expected, "property '" + name + "'");
+  } else if (expected instanceof RegExp) {
+    ok(expected.test(value), name + ": " + expected + " matched " + value);
+  } 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);
+  }
+}