Bug 1392067 - Disconnect open extension ports when the message manager goes away r=zombie
☠☠ backed out by 63ecf1e96a20 ☠ ☠
authorRob Wu <rob@robwu.nl>
Fri, 06 Apr 2018 19:52:16 +0100
changeset 471621 9d8b5d05ff0e6c25cd6911b9d44ea0c8499cd4f8
parent 471620 1374936db0183b95619f8e312c377f5a784e59bd
child 471622 07e88c2233970829395cf24bc18854bd268e96c4
push id1728
push userjlund@mozilla.com
push dateMon, 18 Jun 2018 21:12:27 +0000
treeherdermozilla-release@c296fde26f5f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerszombie
bugs1392067
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1392067 - Disconnect open extension ports when the message manager goes away r=zombie - Previously, if a port is disconnected by the other end, then memory would be leaked to `ProxyMessenger.ports` in ExtensionParent.jsm. To fix this, the port descriptor is now saved separately, keyed by port ID instead of message manager. - Previously, when a message manager was disconnected (e.g. window closed/tab crashed), the port is disconnected only if the port was created from that page. This patch adds bookkeeping to keep track of the message managers at both the sender and receiver's side, so that the port is always disconnected when the other side goes away. - The new test browser_ext_port_disconnect_on_crash.js checks whether the ports are disconnected as expected. Previously, the subtest connect_from_tab_to_bg_and_crash_tab failed because of the previous point. - Although not as deterministic as the crash test, the new browser_ext_port_disconnect_on_window_close.js reproduces the original test failure and serves as a regression test for the bug. - Previously, the data structure in ProxyMessenger.ports contained the original `sender` and `recipient`. For the purpose of sending port disconnection messages, these are not necessary and therefore they have been removed. - Fix incorrect JSDoc (type of portId is number, not string) MozReview-Commit-ID: BoaKRVAUKuq
browser/components/extensions/test/browser/browser-common.ini
browser/components/extensions/test/browser/browser_ext_port_disconnect_on_crash.js
browser/components/extensions/test/browser/browser_ext_port_disconnect_on_window_close.js
toolkit/components/extensions/ExtensionChild.jsm
toolkit/components/extensions/ExtensionParent.jsm
toolkit/components/extensions/NativeMessaging.jsm
--- a/browser/components/extensions/test/browser/browser-common.ini
+++ b/browser/components/extensions/test/browser/browser-common.ini
@@ -120,16 +120,19 @@ skip-if = debug && (os == 'linux' || os 
 [browser_ext_pageAction_title.js]
 [browser_ext_popup_api_injection.js]
 [browser_ext_popup_background.js]
 [browser_ext_popup_corners.js]
 [browser_ext_popup_focus.js]
 disabled = bug 1438663
 [browser_ext_popup_sendMessage.js]
 [browser_ext_popup_shutdown.js]
+[browser_ext_port_disconnect_on_crash.js]
+skip-if = !e10s # the tab's process is killed during the test. Without e10s the parent process would die too.
+[browser_ext_port_disconnect_on_window_close.js]
 [browser_ext_runtime_openOptionsPage.js]
 [browser_ext_runtime_openOptionsPage_uninstall.js]
 [browser_ext_runtime_setUninstallURL.js]
 [browser_ext_sessions_forgetClosedTab.js]
 [browser_ext_sessions_forgetClosedWindow.js]
 [browser_ext_sessions_getRecentlyClosed.js]
 [browser_ext_sessions_getRecentlyClosed_private.js]
 [browser_ext_sessions_getRecentlyClosed_tabs.js]
new file mode 100644
--- /dev/null
+++ b/browser/components/extensions/test/browser/browser_ext_port_disconnect_on_crash.js
@@ -0,0 +1,91 @@
+/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
+/* vim: set sts=2 sw=2 et tw=80: */
+"use strict";
+
+add_task(async function connect_from_tab_to_bg_and_crash_tab() {
+  let extension = ExtensionTestUtils.loadExtension({
+    manifest: {
+      "content_scripts": [{
+        "js": ["contentscript.js"],
+        "matches": ["http://example.com/?crashme"],
+      }],
+    },
+
+    background() {
+      browser.runtime.onConnect.addListener((port) => {
+        browser.test.assertEq("tab_to_bg", port.name, "expected port");
+        port.onDisconnect.addListener(() => {
+          browser.test.assertEq(null, port.error, "port should be disconnected without errors");
+          browser.test.sendMessage("port_disconnected");
+        });
+        browser.test.sendMessage("bg_runtime_onConnect");
+      });
+    },
+
+    files: {
+      "contentscript.js": function() {
+        let port = browser.runtime.connect({name: "tab_to_bg"});
+        port.onDisconnect.addListener(() => {
+          browser.test.fail("Unexpected onDisconnect event in content script");
+        });
+      },
+    },
+  });
+
+  await extension.startup();
+
+  let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, "http://example.com/?crashme");
+  await extension.awaitMessage("bg_runtime_onConnect");
+  // Force the message manager to disconnect without giving the content a
+  // chance to send an "Extension:Port:Disconnect" message.
+  await BrowserTestUtils.crashBrowser(tab.linkedBrowser);
+  await extension.awaitMessage("port_disconnected");
+  BrowserTestUtils.removeTab(tab);
+  await extension.unload();
+});
+
+add_task(async function connect_from_bg_to_tab_and_crash_tab() {
+  let extension = ExtensionTestUtils.loadExtension({
+    manifest: {
+      "content_scripts": [{
+        "js": ["contentscript.js"],
+        "matches": ["http://example.com/?crashme"],
+      }],
+    },
+
+    background() {
+      browser.runtime.onMessage.addListener((msg, sender) => {
+        browser.test.assertEq("contentscript_ready", msg, "expected message");
+        let port = browser.tabs.connect(sender.tab.id, {name: "bg_to_tab"});
+        port.onDisconnect.addListener(() => {
+          browser.test.assertEq(null, port.error, "port should be disconnected without errors");
+          browser.test.sendMessage("port_disconnected");
+        });
+      });
+    },
+
+    files: {
+      "contentscript.js": function() {
+        browser.runtime.onConnect.addListener((port) => {
+          browser.test.assertEq("bg_to_tab", port.name, "expected port");
+          port.onDisconnect.addListener(() => {
+            browser.test.fail("Unexpected onDisconnect event in content script");
+          });
+          browser.test.sendMessage("tab_runtime_onConnect");
+        });
+        browser.runtime.sendMessage("contentscript_ready");
+      },
+    },
+  });
+
+  await extension.startup();
+
+  let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, "http://example.com/?crashme");
+  await extension.awaitMessage("tab_runtime_onConnect");
+  // Force the message manager to disconnect without giving the content a
+  // chance to send an "Extension:Port:Disconnect" message.
+  await BrowserTestUtils.crashBrowser(tab.linkedBrowser);
+  await extension.awaitMessage("port_disconnected");
+  BrowserTestUtils.removeTab(tab);
+  await extension.unload();
+});
new file mode 100644
--- /dev/null
+++ b/browser/components/extensions/test/browser/browser_ext_port_disconnect_on_window_close.js
@@ -0,0 +1,36 @@
+/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
+/* vim: set sts=2 sw=2 et tw=80: */
+"use strict";
+
+// Regression test for https://bugzil.la/1392067 .
+add_task(async function connect_from_window_and_close() {
+  let extension = ExtensionTestUtils.loadExtension({
+    background() {
+      browser.runtime.onConnect.addListener((port) => {
+        browser.test.assertEq("page_to_bg", port.name, "expected port");
+        port.onDisconnect.addListener(() => {
+          browser.test.assertEq(null, port.error, "port should be disconnected without errors");
+          browser.test.sendMessage("port_disconnected");
+        });
+        browser.windows.remove(port.sender.tab.windowId);
+      });
+
+      browser.windows.create({url: "page.html"});
+    },
+
+    files: {
+      "page.html":
+        `<!DOCTYPE html><meta charset="utf-8"><script src="page.js"></script>`,
+      "page.js": function() {
+        let port = browser.runtime.connect({name: "page_to_bg"});
+        port.onDisconnect.addListener(() => {
+          browser.test.fail("Unexpected onDisconnect event in page");
+        });
+      },
+    },
+  });
+
+  await extension.startup();
+  await extension.awaitMessage("port_disconnected");
+  await extension.unload();
+});
--- a/toolkit/components/extensions/ExtensionChild.jsm
+++ b/toolkit/components/extensions/ExtensionChild.jsm
@@ -124,17 +124,17 @@ Services.obs.addObserver(StrongPromise, 
 /**
  * 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.
+ * @param {number} id An ID that uniquely identifies this port's channel.
  * @param {object} sender The `port.sender` property.
  * @param {object} recipient The recipient of messages sent from this port.
  */
 class Port {
   constructor(context, senderMM, receiverMMs, name, id, sender, recipient) {
     this.context = context;
     this.senderMM = senderMM;
     this.receiverMMs = receiverMMs;
--- a/toolkit/components/extensions/ExtensionParent.jsm
+++ b/toolkit/components/extensions/ExtensionParent.jsm
@@ -172,16 +172,89 @@ let apiManager = new class extends Schem
           return result;
         }
         target.messageManager.sendAsyncMessage("Extension:SetFrameData", result);
       }
     }
   }
 }();
 
+// A proxy for extension ports between two DISTINCT message managers.
+// This is used by ProxyMessenger, to ensure that a port always receives a
+// disconnection message when the other side closes, even if that other side
+// fails to send the message before the message manager disconnects.
+class ExtensionPortProxy {
+  /**
+   * @param {number} portId The ID of the port, chosen by the sender.
+   * @param {nsIMessageSender} senderMM
+   * @param {nsIMessageSender} receiverMM Must differ from senderMM.
+   */
+  constructor(portId, senderMM, receiverMM) {
+    this.portId = portId;
+    this.senderMM = senderMM;
+    this.receiverMM = receiverMM;
+  }
+
+  register() {
+    if (ProxyMessenger.portsById.has(this.portId)) {
+      throw new Error(`Extension port IDs may not be re-used: ${this.portId}`);
+    }
+    ProxyMessenger.portsById.set(this.portId, this);
+    ProxyMessenger.ports.get(this.senderMM).add(this);
+    ProxyMessenger.ports.get(this.receiverMM).add(this);
+  }
+
+  unregister() {
+    ProxyMessenger.portsById.delete(this.portId);
+    this._unregisterFromMessageManager(this.senderMM);
+    this._unregisterFromMessageManager(this.receiverMM);
+  }
+
+  _unregisterFromMessageManager(messageManager) {
+    let ports = ProxyMessenger.ports.get(messageManager);
+    ports.delete(this);
+    if (ports.size === 0) {
+      ProxyMessenger.ports.delete(messageManager);
+    }
+  }
+
+  /**
+   * Associate the port with `newMessageManager` instead of `messageManager`.
+   *
+   * @param {nsIMessageSender} messageManager The message manager to replace.
+   * @param {nsIMessageSender} newMessageManager
+   */
+  replaceMessageManager(messageManager, newMessageManager) {
+    if (this.senderMM === messageManager) {
+      this.senderMM = newMessageManager;
+    } else if (this.receiverMM === messageManager) {
+      this.receiverMM = newMessageManager;
+    } else {
+      throw new Error("This ExtensionPortProxy is not associated with the given message manager");
+    }
+
+    this._unregisterFromMessageManager(messageManager);
+
+    if (this.senderMM === this.receiverMM) {
+      this.unregister();
+    } else {
+      ProxyMessenger.ports.get(newMessageManager).add(this);
+    }
+  }
+
+  getOtherMessageManager(messageManager) {
+    if (this.senderMM === messageManager) {
+      return this.receiverMM;
+    } else if (this.receiverMM === messageManager) {
+      return this.senderMM;
+    }
+    throw new Error("This ExtensionPortProxy is not associated with the given message manager");
+  }
+}
+
 // Subscribes to messages related to the extension messaging API and forwards it
 // to the relevant message manager. The "sender" field for the `onMessage` and
 // `onConnect` events are updated if needed.
 ProxyMessenger = {
   _initialized: false,
 
   init() {
     if (this._initialized) {
@@ -200,42 +273,54 @@ ProxyMessenger = {
 
     MessageChannel.addListener(messageManagers, "Extension:Connect", this);
     MessageChannel.addListener(messageManagers, "Extension:Message", this);
     MessageChannel.addListener(messageManagers, "Extension:Port:Disconnect", this);
     MessageChannel.addListener(messageManagers, "Extension:Port:PostMessage", this);
 
     Services.obs.addObserver(this, "message-manager-disconnect");
 
-    this.ports = new DefaultMap(() => new Map());
+    // Data structures to look up proxied extension ports by message manager,
+    // and by (numeric) portId. These are maintained by ExtensionPortProxy.
+    // Map[nsIMessageSender -> Set(ExtensionPortProxy)]
+    this.ports = new DefaultMap(() => new Set());
+    // Map[portId -> ExtensionPortProxy]
+    this.portsById = new Map();
   },
 
   observe(subject, topic, data) {
     if (topic === "message-manager-disconnect") {
       if (this.ports.has(subject)) {
         let ports = this.ports.get(subject);
         this.ports.delete(subject);
-
-        for (let [portId, {sender, recipient, receiverMM}] of ports.entries()) {
-          recipient.portId = portId;
-          MessageChannel.sendMessage(receiverMM, "Extension:Port:Disconnect", null, {
-            sender,
-            recipient,
+        for (let port of ports) {
+          MessageChannel.sendMessage(port.getOtherMessageManager(subject), "Extension:Port:Disconnect", null, {
+            // Usually sender.contextId must be set to the sender's context ID
+            // to avoid dispatching the port.onDisconnect event at the sender.
+            // The sender is certainly unreachable because its message manager
+            // was disconnected, so the sender can be left empty.
+            sender: {},
+            recipient: {portId: port.portId},
             responseType: MessageChannel.RESPONSE_TYPE_NONE,
           }).catch(() => {});
+          port.unregister();
         }
       }
     }
   },
 
   handleEvent(event) {
     if (event.type === "SwapDocShells") {
       let {messageManager} = event.originalTarget;
       if (this.ports.has(messageManager)) {
-        this.ports.set(event.detail.messageManager, this.ports.get(messageManager));
+        let ports = this.ports.get(messageManager);
+        let newMessageManager = event.detail.messageManager;
+        for (let port of ports) {
+          port.replaceMessageManager(messageManager, newMessageManager);
+        }
         this.ports.delete(messageManager);
         event.detail.addEventListener("SwapDocShells", this, {once: true});
       }
     }
   },
 
   async receiveMessage({target, messageName, channelId, sender, recipient, data, responseType}) {
     if (recipient.toNativeApp) {
@@ -255,17 +340,20 @@ ProxyMessenger = {
     }
 
     const noHandlerError = {
       result: MessageChannel.RESULT_NO_HANDLER,
       message: "No matching message handler for the given recipient.",
     };
 
     let extension = GlobalManager.extensionMap.get(sender.extensionId);
-    let receiverMM = this.getMessageManagerForRecipient(recipient);
+    let {
+      messageManager: receiverMM,
+      xulBrowser: receiverBrowser,
+    } = this.getMessageManagerForRecipient(recipient);
     if (!extension || !receiverMM) {
       return Promise.reject(noHandlerError);
     }
 
     if ((messageName == "Extension:Message" ||
          messageName == "Extension:Connect") &&
         apiManager.global.tabGetSender) {
       // From ext-tabs.js, undefined on Android.
@@ -274,25 +362,36 @@ ProxyMessenger = {
 
     let promise1 = MessageChannel.sendMessage(receiverMM, messageName, data, {
       sender,
       recipient,
       responseType,
     });
 
     if (messageName === "Extension:Connect") {
-      target.addEventListener("SwapDocShells", this, {once: true});
-
-      this.ports.get(target.messageManager).set(data.portId, {receiverMM, sender, recipient});
-      promise1.catch(() => {
-        this.ports.get(target.messageManager).delete(data.portId);
-      });
+      // Register a proxy for the extension port if the message managers differ,
+      // so that a disconnect message can be sent to the other end when either
+      // message manager disconnects.
+      if (target.messageManager !== receiverMM) {
+        // The source of Extension:Connect is always inside a <browser>, whereas
+        // the recipient can be a process (and not be associated with a <browser>).
+        target.addEventListener("SwapDocShells", this, {once: true});
+        if (receiverBrowser) {
+          receiverBrowser.addEventListener("SwapDocShells", this, {once: true});
+        }
+        let port = new ExtensionPortProxy(data.portId, target.messageManager, receiverMM);
+        port.register();
+        promise1.catch(() => {
+          port.unregister();
+        });
+      }
     } else if (messageName === "Extension:Port:Disconnect") {
-      if (target.messageManager) {
-        this.ports.get(target.messageManager).delete(data.portId);
+      let port = this.portsById.get(data.portId);
+      if (port) {
+        port.unregister();
       }
     }
 
 
     if (!(extension.isEmbedded || recipient.toProxyScript) || !extension.remote) {
       return promise1;
     }
 
@@ -331,59 +430,62 @@ ProxyMessenger = {
     }
     return result;
   },
 
   /**
    * @param {object} recipient An object that was passed to
    *     `MessageChannel.sendMessage`.
    * @param {Extension} extension
-   * @returns {object|null} The message manager matching the recipient if found.
+   * @returns {{messageManager: nsIMessageSender, xulBrowser: XULElement}}
+   *          The message manager matching the recipient, if found.
+   *          And the <browser> owning the message manager, if any.
    */
   getMessageManagerForRecipient(recipient) {
     // tabs.sendMessage / tabs.connect
     if ("tabId" in recipient) {
       // `tabId` being set implies that the tabs API is supported, so we don't
       // need to check whether `tabTracker` exists.
       let tab = apiManager.global.tabTracker.getTab(recipient.tabId, null);
       if (!tab) {
-        return null;
+        return {messageManager: null, xulBrowser: null};
       }
 
       // There can be no recipients in a tab pending restore,
       // So we bail early to avoid instantiating the lazy browser.
       let node = tab.browser || tab;
       if (node.getAttribute("pending") === "true") {
-        return null;
+        return {messageManager: null, xulBrowser: null};
       }
 
       let browser = tab.linkedBrowser || tab.browser;
 
       // Options panels in the add-on manager currently require
       // special-casing, since their message managers aren't currently
       // connected to the tab's top-level message manager. To deal with
       // this, we find the options <browser> for the tab, and use that
       // directly, insteead.
       if (browser.currentURI.cloneIgnoringRef().spec === "about:addons") {
         let optionsBrowser = browser.contentDocument.querySelector(".inline-options-browser");
         if (optionsBrowser) {
           browser = optionsBrowser;
         }
       }
 
-      return browser.messageManager;
+      return {messageManager: browser.messageManager, xulBrowser: browser};
     }
 
     // runtime.sendMessage / runtime.connect
     let extension = GlobalManager.extensionMap.get(recipient.extensionId);
     if (extension) {
-      return extension.parentMessageManager;
+      // A process message manager
+      return {messageManager: extension.parentMessageManager, xulBrowser: null};
     }
 
-    return null;
+    return {messageManager: null, xulBrowser: null};
   },
 };
 
 // Responsible for loading extension APIs into the right globals.
 GlobalManager = {
   // Map[extension ID -> Extension]. Determines which extension is
   // responsible for content under a particular extension ID.
   extensionMap: new Map(),
--- a/toolkit/components/extensions/NativeMessaging.jsm
+++ b/toolkit/components/extensions/NativeMessaging.jsm
@@ -102,17 +102,17 @@ var NativeApp = class extends EventEmitt
   }
 
   /**
    * Open a connection to a native messaging host.
    *
    * @param {BaseContext} context The context associated with the port.
    * @param {nsIMessageSender} messageManager The message manager used to send
    *     and receive messages from the port's creator.
-   * @param {string} portId A unique internal ID that identifies the port.
+   * @param {number} portId A unique internal ID that identifies the port.
    * @param {object} sender The object describing the creator of the connection
    *     request.
    * @param {string} application The name of the native messaging host.
    */
   static onConnectNative(context, messageManager, portId, sender, application) {
     let app = new NativeApp(context, application);
     let port = new ExtensionChild.Port(context, messageManager, [Services.mm], "", portId, sender, sender);
     app.once("disconnect", (what, err) => port.disconnect(err));