Bug 1389968 - Reject sendMessage() promise when response handle gets GCd draft
authorTomislav Jovanovic <tomica@gmail.com>
Wed, 23 Aug 2017 00:16:48 +0200
changeset 653264 0bcf330f234e9c978e145a681cef6b8a932aef03
parent 649312 4f4487cc2d30d988742109868dcf21c4113f12f5
child 728299 d6f7f5431ff220fd526372bb9ed4626f79d2df33
push id76284
push userbmo:tomica@gmail.com
push dateFri, 25 Aug 2017 19:42:43 +0000
bugs1389968
milestone57.0a1
Bug 1389968 - Reject sendMessage() promise when response handle gets GCd MozReview-Commit-ID: C2g3VSWYKuz
toolkit/components/extensions/ExtensionChild.jsm
toolkit/components/extensions/MessageChannel.jsm
toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage.js
--- a/toolkit/components/extensions/ExtensionChild.jsm
+++ b/toolkit/components/extensions/ExtensionChild.jsm
@@ -19,16 +19,19 @@ this.EXPORTED_SYMBOLS = ["ExtensionChild
 const Ci = Components.interfaces;
 const Cc = Components.classes;
 const Cu = Components.utils;
 const Cr = Components.results;
 
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 
+XPCOMUtils.defineLazyServiceGetter(this, "finalizationService",
+  "@mozilla.org/toolkit/finalizationwitness;1", "nsIFinalizationWitnessService");
+
 XPCOMUtils.defineLazyModuleGetters(this, {
   ExtensionContent: "resource://gre/modules/ExtensionContent.jsm",
   MessageChannel: "resource://gre/modules/MessageChannel.jsm",
   NativeApp: "resource://gre/modules/NativeMessaging.jsm",
   PromiseUtils: "resource://gre/modules/PromiseUtils.jsm",
 });
 
 Cu.import("resource://gre/modules/ExtensionCommon.jsm");
@@ -70,16 +73,43 @@ function injectAPI(source, dest) {
       injectAPI(desc.value, obj);
     } else {
       Object.defineProperty(dest, prop, desc);
     }
   }
 }
 
 /**
+ * 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 = {
+  wrap(promise, id) {
+    return new Promise((resolve, reject) => {
+      const witness = finalizationService.make("extensions-sendMessage-witness", id);
+      promise.then(value => {
+        witness.forget();
+        resolve(value);
+      }, error => {
+        witness.forget();
+        reject(error);
+      });
+    });
+  },
+  observe(subject, topic, id) {
+    MessageChannel.abortChannel(id, {message: "Response handle went out of scope"});
+  },
+};
+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.
  * @param {nsIMessageSender} senderMM The message manager to send messages to.
  * @param {Array<nsIMessageListenerManager>} receiverMMs Message managers to
  *     listen on.
  * @param {string} name Arbitrary port name as defined by the addon.
  * @param {string} id An ID that uniquely identifies this port's channel.
@@ -354,17 +384,17 @@ class Messenger {
         messageFilterStrict: this.filter,
 
         filterMessage: (sender, recipient) => {
           // Ignore the message if it was sent by this Messenger.
           return (sender.contextId !== this.context.contextId &&
                   filter(sender, recipient));
         },
 
-        receiveMessage: ({target, data: holder, sender, recipient}) => {
+        receiveMessage: ({target, data: holder, sender, recipient, channelId}) => {
           if (!this.context.active) {
             return;
           }
 
           let sendResponse;
           let response = undefined;
           let promise = new Promise(resolve => {
             sendResponse = value => {
@@ -376,19 +406,19 @@ class Messenger {
           let message = holder.deserialize(this.context.cloneScope);
           sender = Cu.cloneInto(sender, this.context.cloneScope);
           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);
           if (result instanceof this.context.cloneScope.Promise) {
-            return result;
+            return StrongPromise.wrap(result, channelId);
           } else if (result === true) {
-            return promise;
+            return StrongPromise.wrap(promise, channelId);
           }
           return response;
         },
       };
 
       MessageChannel.addListener(this.messageManagers, "Extension:Message", listener);
       return () => {
         MessageChannel.removeListener(this.messageManagers, "Extension:Message", listener);
--- a/toolkit/components/extensions/MessageChannel.jsm
+++ b/toolkit/components/extensions/MessageChannel.jsm
@@ -639,16 +639,17 @@ this.MessageChannel = {
     }
 
     let target = new MessageManagerProxy(data.target);
 
     let deferred = {
       sender: data.sender,
       messageManager: target,
       channelId: data.channelId,
+      respondingSide: true,
     };
     deferred.promise = new Promise((resolve, reject) => {
       deferred.reject = reject;
 
       this._callHandlers(handlers, data).then(resolve, reject);
     }).then(
       value => {
         let response = {
@@ -763,16 +764,35 @@ this.MessageChannel = {
     let cleanup = () => {
       this.pendingResponses.delete(deferred);
     };
     this.pendingResponses.add(deferred);
     deferred.promise.then(cleanup, cleanup);
   },
 
   /**
+   * Aborts pending message response for the specific channel.
+   *
+   * @param {string} channelId
+   *    A string for channelId of the response.
+   * @param {object} reason
+   *    An object describing the reason the response was aborted.
+   *    Will be passed to the promise rejection handler of the aborted
+   *    response.
+   */
+  abortChannel(channelId, reason) {
+    for (let response of this.pendingResponses) {
+      if (channelId === response.channelId && response.respondingSide) {
+        this.pendingResponses.delete(response);
+        response.reject(reason);
+      }
+    }
+  },
+
+  /**
    * Aborts any pending message responses to senders matching the given
    * filter.
    *
    * @param {object} sender
    *    The object on which to filter senders, as determined by
    *    `matchesFilter`.
    * @param {object} [reason]
    *    An optional object describing the reason the response was aborted.
--- a/toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage.js
@@ -104,8 +104,108 @@ add_task(async function tabsSendMessageB
       "extensionpage.html": `<!DOCTYPE html><meta charset="utf-8"><script src="senderScript.js"></script>`,
     },
   });
 
   await extension.startup();
   await extension.awaitFinish("sendBlob");
   await extension.unload();
 });
+
+add_task(async function sendMessageResponseGC() {
+  function background() {
+    let savedResolve, savedRespond;
+
+    browser.runtime.onMessage.addListener((msg, _, respond) => {
+      browser.test.log(`Got request: ${msg}`);
+      switch (msg) {
+        case "ping":
+          respond("pong");
+          return;
+
+        case "promise-save":
+          return new Promise(resolve => {
+            savedResolve = resolve;
+          });
+        case "promise-resolve":
+          savedResolve("saved-resolve");
+          return;
+        case "promise-never":
+          return new Promise(r => {});
+
+        case "callback-save":
+          savedRespond = respond;
+          return true;
+        case "callback-call":
+          savedRespond("saved-respond");
+          return;
+        case "callback-never":
+          return true;
+      }
+    });
+
+    const frame = document.createElement("iframe");
+    frame.src = "page.html";
+    document.body.appendChild(frame);
+  }
+
+  function page() {
+    browser.test.onMessage.addListener(msg => {
+      browser.runtime.sendMessage(msg)
+        .then(response => {
+          if (response) {
+            browser.test.log(`Got response: ${response}`);
+            browser.test.sendMessage(response);
+          }
+        }, error => {
+          browser.test.assertEq(error.message,
+            "Response handle went out of scope",
+            "The promise rejected with the correct error");
+          browser.test.sendMessage("rejected");
+        }
+      );
+    });
+    browser.test.sendMessage("ready");
+  }
+
+  let extension = ExtensionTestUtils.loadExtension({
+    background,
+    files: {
+      "page.html": "<!DOCTYPE html><meta charset=utf-8><script src=page.js></script>",
+      "page.js": page,
+    },
+  });
+
+  await extension.startup();
+  await extension.awaitMessage("ready");
+
+  // Setup long-running tasks before GC.
+  extension.sendMessage("promise-save");
+  extension.sendMessage("callback-save");
+
+  // Test returning a Promise that can never resolve.
+  extension.sendMessage("promise-never");
+
+  extension.sendMessage("ping");
+  await extension.awaitMessage("pong");
+
+  Services.ppmm.loadProcessScript("data:,Components.utils.forceGC()", false);
+  await extension.awaitMessage("rejected");
+
+  // Test returning `true` without holding the response handle.
+  extension.sendMessage("callback-never");
+
+  extension.sendMessage("ping");
+  await extension.awaitMessage("pong");
+
+  Services.ppmm.loadProcessScript("data:,Components.utils.forceGC()", false);
+  await extension.awaitMessage("rejected");
+
+  // Test that promises from long-running tasks didn't get GCd.
+  extension.sendMessage("promise-resolve");
+  await extension.awaitMessage("saved-resolve");
+
+  extension.sendMessage("callback-call");
+  await extension.awaitMessage("saved-respond");
+
+  ok("Long running tasks responded");
+  await extension.unload();
+});