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

UltraBlame original commit: 46abf66e3b2090e7ebcde77aefd81982689cc148

Author: marco-c

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;
             }
           })
-          
-          .catch(err => err.result !== Cr.NS_ERROR_NOT_AVAILABLE && reject(err))
+          .catch(err => {
+            
+            
+            if (err instanceof ExtensionError || err?.mozWebExtLocation) {
+              reject(err);
+            } else {
+              Cu.reportError(err);
+            }
+          })
       );
 
       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,
@@ -124,6 +124,15 @@ const ExtensionActivityLogChild = {
 
 
 
+class ExtensionErrorHolder {
+  constructor(trustedErrorObject) {
+    this.trustedErrorObject = trustedErrorObject;
+  }
+}
+
+
+
+
 
 
 const StrongPromise = {
@@ -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;
+    }
+    
+    
+    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
-      .queryRuntimeMessage({
-        extensionId: extensionId || this.context.extension.id,
-        holder: holdMessage(message),
-        ...args,
-      })
-      .catch(({ message, mozWebExtLocation }) =>
-        Promise.reject({ message, mozWebExtLocation })
-      );
+    let response = this.conduit.queryRuntimeMessage({
+      extensionId: extensionId || this.context.extension.id,
+      holder: holdMessage(message),
+      ...args,
+    });
+    
+    
     return this.context.wrapPromise(response, callback);
   }
 

toolkit/components/extensions/ExtensionParent.jsm

@@ -329,9 +329,10 @@ const ProxyMessenger = {
     arg.firstResponse = true;
     let kind = await this.normalizeArgs(arg, sender);
     let result = await this.conduit.castRuntimeMessage(kind, arg);
-    return result
-      ? result.value
-      : Promise.reject({ message: ERROR_NO_RECEIVERS });
+    if (!result) {
+      throw new ExtensionError(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 @@
+
+
+"use strict";
+
+
+
+
+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]