From d9f3e3b3f887e1811b981ac8a0622ee6720e6173 Mon Sep 17 00:00:00 2001 From: Rob Wu Date: Wed, 12 Aug 2020 14:28:04 +0000 Subject: [PATCH] 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 --- .../components/extensions/ConduitsParent.jsm | 13 +++- .../components/extensions/ExtensionChild.jsm | 43 ++++++++---- .../components/extensions/ExtensionParent.jsm | 10 ++- .../test_ext_runtime_sendMessage_multiple.js | 67 +++++++++++++++++++ .../test/xpcshell/xpcshell-common.ini | 1 + 5 files changed, 116 insertions(+), 18 deletions(-) create mode 100644 toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage_multiple.js diff --git a/toolkit/components/extensions/ConduitsParent.jsm b/toolkit/components/extensions/ConduitsParent.jsm index 2e4a9b5f5345..ef55cd8d6262 100644 --- a/toolkit/components/extensions/ConduitsParent.jsm +++ b/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 => err.result !== Cr.NS_ERROR_NOT_AVAILABLE && reject(err)) + .catch(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)); diff --git a/toolkit/components/extensions/ExtensionChild.jsm b/toolkit/components/extensions/ExtensionChild.jsm index 41d0b344fecc..5da616d6518e 100644 --- a/toolkit/components/extensions/ExtensionChild.jsm +++ b/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 - .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, + }); + // If |response| is a rejected promise, the value will be sanitized by + // wrapPromise, according to the rules of context.normalizeError. return this.context.wrapPromise(response, callback); } diff --git a/toolkit/components/extensions/ExtensionParent.jsm b/toolkit/components/extensions/ExtensionParent.jsm index fc21d41aa259..b7bf99accb73 100644 --- a/toolkit/components/extensions/ExtensionParent.jsm +++ b/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 - ? result.value - : Promise.reject({ message: ERROR_NO_RECEIVERS }); + if (!result) { + // "throw new ExtensionError" cannot be used because then the stack of the + // 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 }) { diff --git a/toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage_multiple.js b/toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage_multiple.js new file mode 100644 index 000000000000..9827a329e3db --- /dev/null +++ b/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": ` + + + `, + "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(); +}); diff --git a/toolkit/components/extensions/test/xpcshell/xpcshell-common.ini b/toolkit/components/extensions/test/xpcshell/xpcshell-common.ini index 8a0e63eaf4ea..7ee793cd8b4d 100644 --- a/toolkit/components/extensions/test/xpcshell/xpcshell-common.ini +++ b/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]