Bug 1655624 - Improve reliability of onMessage's error handling r=zombie

Bug 1655624 happened because the format of an internal error changed,
which caused an internal error to be propagated unexpectedly.
This patch fixes the issue by only propagating errors that are known to
originate from extensions, plus a regression test.

This patch also fixes a few other issues:

- Internal errors are redacted to "An unexpected error occurred",
  which partially fixes bug 1643176.

- Fix minor regression in void rejections: Prior to bug 1583484, an
  onMessage handler that rejected with a void value would cause
  sendMessage to reject. Since bug 1583484 the promise is not rejected,
  as the error is inadvertently ignored due to a runtime error:
  "TypeError: can't access property "result", err is undefined".

- Avoid type confusion of objects with the mozWebExtLocation member.

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

toolkit/components/extensions/ConduitsParent.jsm

@@ -49,7 +49,7 @@ const EXPORTED_SYMBOLS = ["BroadcastConduit", "ConduitsParent"];
  */
 
 const {
-  ExtensionUtils: { DefaultWeakMap },
+  ExtensionUtils: { DefaultWeakMap, ExtensionError },
 } = ChromeUtils.import("resource://gre/modules/ExtensionUtils.jsm");
 
 const { BaseConduit } = ChromeUtils.import(
@@ -288,8 +288,15 @@ class BroadcastConduit extends BaseConduit {
               result = value;
             }
           })
-          // Ignore errors trying to query child Messengers being destroyed.
+          .catch(err => {
-          .catch(err => err.result !== Cr.NS_ERROR_NOT_AVAILABLE && reject(err))
+            // Forward errors that are exposed to extension, but ignore
+            // internal errors such as actor destruction and DataCloneError.
+            if (err instanceof ExtensionError || err?.mozWebExtLocation) {
+              reject(err);
+            } else {
+              Cu.reportError(err);
+            }
+          })
       );
       // Ensure resolving when there are no responses.
       Promise.allSettled(promises).then(() => resolve(result));

toolkit/components/extensions/ExtensionChild.jsm

@@ -53,7 +53,7 @@ const { ExtensionUtils } = ChromeUtils.import(
   "resource://gre/modules/ExtensionUtils.jsm"
 );
 
-const { DefaultMap, LimitedSet, getUniqueId } = ExtensionUtils;
+const { DefaultMap, ExtensionError, LimitedSet, getUniqueId } = ExtensionUtils;
 
 const {
   EventEmitter,
@@ -121,6 +121,15 @@ const ExtensionActivityLogChild = {
   },
 };
 
+// A helper to allow us to distinguish trusted errors from unsanitized errors.
+// Extensions can create plain objects with arbitrary properties (such as
+// mozWebExtLocation), but not create instances of ExtensionErrorHolder.
+class ExtensionErrorHolder {
+  constructor(trustedErrorObject) {
+    this.trustedErrorObject = trustedErrorObject;
+  }
+}
+
 /**
  * A finalization witness helper that wraps a sendMessage response and
  * guarantees to either get the promise resolved, or rejected when the
@@ -145,7 +154,7 @@ const StrongPromise = {
   observe(subject, topic, id) {
     let message = "Promised response from onMessage listener went out of scope";
     let { reject, location } = this.stillAlive.get(id);
-    reject({ message, mozWebExtLocation: location });
+    reject(new ExtensionErrorHolder({ message, mozWebExtLocation: location }));
     this.stillAlive.delete(id);
   },
 };
@@ -183,7 +192,19 @@ class MessageEvent extends SimpleEventAPI {
 
     return !responses.length
       ? { received: true, response: false }
-      : Promise.race(responses).then(value => ({ response: true, value }));
+      : Promise.race(responses).then(
+          value => ({ response: true, value }),
+          error => Promise.reject(this.unwrapOrSanitizeError(error))
+        );
+  }
+
+  unwrapOrSanitizeError(error) {
+    if (error instanceof ExtensionErrorHolder) {
+      return error.trustedErrorObject;
+    }
+    // If not a wrapped error, sanitize it and convert to ExtensionError, so
+    // that context.normalizeError will use the error message.
+    return new ExtensionError(error?.message ?? "An unexpected error occurred");
   }
 
   wrapResponse(fire, message, sender) {
@@ -306,15 +327,13 @@ class Messenger {
   }
 
   sendRuntimeMessage({ extensionId, message, callback, ...args }) {
-    let response = this.conduit
+    let response = this.conduit.queryRuntimeMessage({
-      .queryRuntimeMessage({
+      extensionId: extensionId || this.context.extension.id,
-        extensionId: extensionId || this.context.extension.id,
+      holder: holdMessage(message),
-        holder: holdMessage(message),
+      ...args,
-        ...args,
+    });
-      })
+    // If |response| is a rejected promise, the value will be sanitized by
-      .catch(({ message, mozWebExtLocation }) =>
+    // wrapPromise, according to the rules of context.normalizeError.
-        Promise.reject({ message, mozWebExtLocation })
-      );
     return this.context.wrapPromise(response, callback);
   }
 

toolkit/components/extensions/ExtensionParent.jsm

@@ -329,9 +329,13 @@ const ProxyMessenger = {
     arg.firstResponse = true;
     let kind = await this.normalizeArgs(arg, sender);
     let result = await this.conduit.castRuntimeMessage(kind, arg);
-    return result
+    if (!result) {
-      ? result.value
+      // "throw new ExtensionError" cannot be used because then the stack of the
-      : Promise.reject({ message: ERROR_NO_RECEIVERS });
+      // sendMessage call would not be added to the error object generated by
+      // context.normalizeError. Test coverage by test_ext_error_location.js.
+      return Promise.reject({ message: ERROR_NO_RECEIVERS });
+    }
+    return result.value;
   },
 
   async recvPortConnect(arg, { sender }) {

toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage_multiple.js

@@ -0,0 +1,67 @@
+/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
+/* vim: set sts=2 sw=2 et tw=80: */
+"use strict";
+
+// Regression test for bug 1655624: When there are multiple onMessage receivers
+// that both handle the response asynchronously, destroying the context of one
+// recipient should not prevent the other recipient from sending a reply.
+add_task(async function onMessage_ignores_destroyed_contexts() {
+  let extension = ExtensionTestUtils.loadExtension({
+    background() {
+      browser.test.onMessage.addListener(async msg => {
+        if (msg !== "startTest") {
+          return;
+        }
+        try {
+          let res = await browser.runtime.sendMessage("msg_from_bg");
+          browser.test.assertEq(0, res, "Result from onMessage");
+          browser.test.notifyPass("handled_onMessage");
+        } catch (e) {
+          browser.test.fail(`Unexpected error: ${e.message} :: ${e.stack}`);
+          browser.test.notifyFail("handled_onMessage");
+        }
+      });
+    },
+    files: {
+      "tab.html": `
+        <!DOCTYPE html><meta charset="utf-8">
+        <script src="tab.js"></script>
+      `,
+      "tab.js": () => {
+        let where = location.search.slice(1);
+        let resolveOnMessage;
+        browser.runtime.onMessage.addListener(async msg => {
+          browser.test.assertEq("msg_from_bg", msg, `onMessage at ${where}`);
+          browser.test.sendMessage(`received:${where}`);
+          return new Promise(resolve => {
+            resolveOnMessage = resolve;
+          });
+        });
+        browser.test.onMessage.addListener(msg => {
+          if (msg === `resolveOnMessage:${where}`) {
+            resolveOnMessage(0);
+          }
+        });
+      },
+    },
+  });
+  await extension.startup();
+  let tabToCloseEarly = await ExtensionTestUtils.loadContentPage(
+    `moz-extension://${extension.uuid}/tab.html?tabToCloseEarly`,
+    { extension }
+  );
+  let tabToRespond = await ExtensionTestUtils.loadContentPage(
+    `moz-extension://${extension.uuid}/tab.html?tabToRespond`,
+    { extension }
+  );
+  extension.sendMessage("startTest");
+  await Promise.all([
+    extension.awaitMessage("received:tabToCloseEarly"),
+    extension.awaitMessage("received:tabToRespond"),
+  ]);
+  await tabToCloseEarly.close();
+  extension.sendMessage("resolveOnMessage:tabToRespond");
+  await extension.awaitFinish("handled_onMessage");
+  await tabToRespond.close();
+  await extension.unload();
+});

toolkit/components/extensions/test/xpcshell/xpcshell-common.ini

@@ -138,6 +138,7 @@ skip-if = ccov && os == 'linux' # bug 1607581
 [test_ext_runtime_ports.js]
 [test_ext_runtime_sendMessage.js]
 [test_ext_runtime_sendMessage_errors.js]
+[test_ext_runtime_sendMessage_multiple.js]
 [test_ext_runtime_sendMessage_no_receiver.js]
 [test_ext_same_site_cookies.js]
 [test_ext_same_site_redirects.js]