Bug 1441333: Part 6 - Use caller location in error reports for StrongPromise errors. r=zombie draft
authorKris Maglione <maglione.k@gmail.com>
Thu, 01 Mar 2018 16:41:21 -0800
changeset 762303 e520381bc2d02d421d4929568f3388f6753f1502
parent 762302 de4d9bfe7332f917734511a4ae9c26bdb1dde0ad
push id101126
push usermaglione.k@gmail.com
push dateFri, 02 Mar 2018 00:45:19 +0000
reviewerszombie
bugs1441333
milestone60.0a1
Bug 1441333: Part 6 - Use caller location in error reports for StrongPromise errors. r=zombie We currently report a useful location in error reports when extensions fail to resolve a promise or call a response callback, but in some slightly less-than-ideal ways. We currently generate a complete stack and parse its string value (which is expensive), and then report the caller location as part of the message, rather than as the error's location and stack. This patch changes that behavior to store a single SavedStack frame, and to properly report that as the location of the error. MozReview-Commit-ID: Jmtf4C1O6pW
toolkit/components/extensions/ExtensionChild.jsm
toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage.js
--- a/toolkit/components/extensions/ExtensionChild.jsm
+++ b/toolkit/components/extensions/ExtensionChild.jsm
@@ -85,35 +85,43 @@ function injectAPI(source, dest) {
  * A finalization witness helper that wraps a sendMessage response and
  * guarantees to either get the promise resolved, or rejected when the
  * wrapped promise goes out of scope.
  *
  * Holding a reference to a returned StrongPromise doesn't prevent the
  * wrapped promise from being garbage collected.
  */
 const StrongPromise = {
+  locations: new Map(),
+
   wrap(promise, channelId, location) {
     return new Promise((resolve, reject) => {
-      const tag = `${channelId}|${location}`;
-      const witness = finalizationService.make("extensions-sendMessage-witness", tag);
+      this.locations.set(channelId, location);
+
+      const witness = finalizationService.make("extensions-sendMessage-witness", channelId);
       promise.then(value => {
+        this.locations.delete(channelId);
         witness.forget();
         resolve(value);
       }, error => {
+        this.locations.delete(channelId);
         witness.forget();
         reject(error);
       });
     });
   },
-  observe(subject, topic, tag) {
-    const pos = tag.indexOf("|");
-    const channel = Number(tag.substr(0, pos));
-    const location = tag.substr(pos + 1);
-    const message = `Promised response from onMessage listener at ${location} went out of scope`;
-    MessageChannel.abortChannel(channel, {message});
+  observe(subject, topic, channelId) {
+    channelId = Number(channelId);
+    let location = this.locations.get(channelId);
+    this.locations.delete(channelId);
+
+    const message = `Promised response from onMessage listener went out of scope`;
+    const error = ChromeUtils.createError(message, location);
+    error.mozWebExtLocation = location;
+    MessageChannel.abortChannel(channelId, error);
   },
 };
 Services.obs.addObserver(StrongPromise, "extensions-sendMessage-witness");
 
 /**
  * Abstraction for a Port object in the extension API.
  *
  * @param {BaseContext} context The context that owns this port.
@@ -391,17 +399,17 @@ class Messenger {
 
   sendNativeMessage(messageManager, msg, recipient, responseCallback) {
     msg = NativeApp.encodeMessage(this.context, msg);
     return this.sendMessage(messageManager, msg, recipient, responseCallback);
   }
 
   _onMessage(name, filter) {
     return new EventManager(this.context, name, fire => {
-      const [location] = new this.context.cloneScope.Error().stack.split("\n", 1);
+      const caller = this.context.getCaller();
 
       let listener = {
         messageFilterPermissive: this.optionalFilter,
         messageFilterStrict: this.filter,
 
         filterMessage: (sender, recipient) => {
           // Exclude messages coming from content scripts for the devtools extension contexts
           // (See Bug 1383310).
@@ -435,19 +443,19 @@ class Messenger {
           sendResponse = Cu.exportFunction(sendResponse, this.context.cloneScope);
 
           // Note: We intentionally do not use runSafe here so that any
           // errors are propagated to the message sender.
           let result = fire.raw(message, sender, sendResponse);
           message = null;
 
           if (result instanceof this.context.cloneScope.Promise) {
-            return StrongPromise.wrap(result, channelId, location);
+            return StrongPromise.wrap(result, channelId, caller);
           } else if (result === true) {
-            return StrongPromise.wrap(promise, channelId, location);
+            return StrongPromise.wrap(promise, channelId, caller);
           }
           return response;
         },
       };
 
       MessageChannel.addListener(this.messageManagers, "Extension:Message", listener);
       return () => {
         MessageChannel.removeListener(this.messageManagers, "Extension:Message", listener);
--- a/toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage.js
@@ -151,20 +151,33 @@ add_task(async function sendMessageRespo
     browser.test.onMessage.addListener(msg => {
       browser.runtime.sendMessage(msg)
         .then(
           response => {
             if (response) {
               browser.test.log(`Got response: ${response}`);
               browser.test.sendMessage(response);
             }
-          }, ({message}) => {
+          }, error => {
+            browser.test.assertEq(
+              "Promised response from onMessage listener went out of scope",
+              error.message,
+              `Promise rejected with the correct error message`);
+
             browser.test.assertTrue(
-              /at background@moz-extension:\/\/[\w-]+\/%7B[\w-]+%7D\.js:4:\d went out/.test(message),
-              `Promise rejected with the correct error message: ${message}`);
+              /^moz-extension:\/\/[\w-]+\/%7B[\w-]+%7D\.js/.test(error.fileName),
+              `Promise rejected with the correct error filename: ${error.fileName}`);
+
+            browser.test.assertEq(
+              4, error.lineNumber,
+              `Promise rejected with the correct error line number`);
+
+            browser.test.assertTrue(
+              /moz-extension:\/\/[\w-]+\/%7B[\w-]+%7D\.js:4/.test(error.stack),
+              `Promise rejected with the correct error stack: ${error.stack}`);
             browser.test.sendMessage("rejected");
           });
     });
     browser.test.sendMessage("ready");
   }
 
   let extension = ExtensionTestUtils.loadExtension({
     background,