Bug 1370368 - Fire port.onDisconnect after pageshow draft
authorRob Wu <rob@robwu.nl>
Wed, 23 May 2018 15:10:42 +0200
changeset 798805 8fcf0ff2e2220106261c90ee32fa8482542e2ab2
parent 798804 f5825ca851630c3950cc219795e235dc6b75a184
child 798806 f761b8742d18d839773f889526395e790946fada
push id110842
push userbmo:rob@robwu.nl
push dateWed, 23 May 2018 13:45:40 +0000
bugs1370368
milestone62.0a1
Bug 1370368 - Fire port.onDisconnect after pageshow MozReview-Commit-ID: 8i2j38Rr4y5
toolkit/components/extensions/ExtensionChild.jsm
toolkit/components/extensions/test/xpcshell/test_ext_port_disconnect_upon_navigation.js
--- a/toolkit/components/extensions/ExtensionChild.jsm
+++ b/toolkit/components/extensions/ExtensionChild.jsm
@@ -231,17 +231,17 @@ class Port {
    *
    * @param {function} callback Called when the other end disconnects the port.
    *     If the disconnect is caused by an error, the first parameter is an
    *     object with a "message" string property that describes the cause.
    * @returns {function} Function to unregister the listener.
    */
   registerOnDisconnect(callback) {
     let listener = error => {
-      if (this.context.active && !this.disconnected) {
+      if (this.context.active) {
         callback(error);
       }
     };
     this.disconnectListeners.add(listener);
     return () => {
       this.disconnectListeners.delete(listener);
     };
   }
@@ -277,17 +277,30 @@ class Port {
       return;
     }
 
     let onPageHide = (event) => {
       if (event.persisted) {
         // If the page is moved to the bfcache, then scripts in the page will be
         // suspended and the port becomes unreachable from the extension's
         // perspective. Explicitly disconnect the port when that happens.
-        this.disconnect();
+        this.disconnect(null, true);
+
+        let {disconnectListeners} = this;
+        if (disconnectListeners.size) {
+          // This "pageshow" handler is never unregistered because there is
+          // no reliable way to do so if it is in the bfcache.
+          // To avoid memory leaks, the closure only references disconnectListeners.
+          contentWindow.addEventListener("pageshow", function() {
+            for (let listener of disconnectListeners) {
+              listener(null);
+            }
+            disconnectListeners.clear();
+          }, {mozSystemGroup: true, once: true}, false);
+        }
       }
     };
     let unregister = () => {
       this.unregisterMessageFuncs.delete(unregister);
       contentWindow.removeEventListener("pagehide", onPageHide, {mozSystemGroup: true});
     };
     contentWindow.addEventListener("pagehide", onPageHide, {mozSystemGroup: true, once: true}, false);
     this.unregisterMessageFuncs.add(unregister);
@@ -322,27 +335,33 @@ class Port {
   disconnectByOtherEnd(error = null) {
     if (this.disconnected) {
       return;
     }
 
     for (let listener of this.disconnectListeners) {
       listener(error);
     }
+    this.disconnectListeners.clear();
 
     this.handleDisconnection();
   }
 
   /**
    * Disconnect the port from this end.
    *
    * @param {Error|{message: string}} [error] The reason for disconnecting,
    *     if it is an abnormal disconnect.
+   * @param {boolean} [keepDisconnectListener] Whether to keep the onDisconnect
+   *        listeners, so that they can be called later.
    */
-  disconnect(error = null) {
+  disconnect(error = null, keepDisconnectListener = false) {
+    if (!keepDisconnectListener) {
+      this.disconnectListeners.clear();
+    }
     if (this.disconnected) {
       // disconnect() may be called without side effects even after the port is
       // closed - https://developer.chrome.com/extensions/runtime#type-Port
       return;
     }
     this.handleDisconnection();
     if (error) {
       error = {message: this.context.normalizeError(error).message};
--- a/toolkit/components/extensions/test/xpcshell/test_ext_port_disconnect_upon_navigation.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_port_disconnect_upon_navigation.js
@@ -20,32 +20,38 @@ add_task(async function testConnect_and_
         browser.test.assertEq("pageshown,pagehidden", messages.join(","), "expected messages");
         browser.test.sendMessage("port-disconnected");
       });
       browser.test.sendMessage("port-connected");
     });
     browser.test.sendMessage("bg-ready");
   }
   function contentScript() {
+    let expectDisconnect = false;
     let port = browser.runtime.connect();
     port.onDisconnect.addListener(() => {
-      // The background page has an onConnect listener and never disconnects
-      // the port.  The port is disconnected when the page unloads, but that
-      // shouldn't trigger the port.onDisconnect event.
-      browser.test.fail("port.onDisconnect should not be fired");
+      browser.test.assertTrue(expectDisconnect, "onDisconnect should fire after pageshow");
+      browser.test.assertEq(null, port.error, "Disconnect without error");
+      browser.test.assertThrows(() => {
+        port.postMessage("foo");
+      }, "Attempt to postMessage on disconnected port");
+      browser.test.sendMessage("content-script-port-disconnected");
     });
 
     // eslint-disable-next-line mozilla/balanced-listeners
     window.addEventListener("pagehide", () => {
       browser.test.sendMessage("content-script-hide");
       try {
         port.postMessage("pagehidden");
       } catch (e) {
         browser.test.sendMessage(`pagehide error: ${e.message}`);
       }
+      window.addEventListener("pageshow", () => {
+        expectDisconnect = true;
+      }, {once: true});
     });
     // eslint-disable-next-line mozilla/balanced-listeners
     window.addEventListener("pageshow", () => {
       browser.test.sendMessage("content-script-show");
       try {
         port.postMessage("pageshown");
       } catch (e) {
         browser.test.sendMessage(`pageshow error: ${e.message}`);
@@ -88,13 +94,81 @@ add_task(async function testConnect_and_
   await contentPage.spawn(null, () => {
     // Navigate back so the content page is resurrected from the bfcache.
     this.content.history.back();
   });
 
   await extension.awaitMessage("content-script-show");
   await extension.awaitMessage("pageshow error: Attempt to postMessage on disconnected port");
 
+  info("Waiting for port.onDisconnect in content script");
+  await extension.awaitMessage("content-script-port-disconnected");
+
   await contentPage.close();
   await extension.awaitMessage("content-script-hide");
   await extension.awaitMessage("pagehide error: Attempt to postMessage on disconnected port");
   await extension.unload();
 });
+
+add_task(async function testConnect_and_disconnect_on_pagehide() {
+  function background() {
+    browser.runtime.onConnect.addListener(port => {
+      port.onDisconnect.addListener(() => {
+        browser.test.assertEq(null, port.error, "Disconnect without error");
+        browser.test.sendMessage("port-disconnected");
+      });
+      browser.test.sendMessage("port-connected");
+    });
+    browser.test.sendMessage("bg-ready");
+  }
+  function contentScript() {
+    let port = browser.runtime.connect();
+    port.onDisconnect.addListener(() => {
+      browser.test.fail("port.onDisconnect should not be fired because we called port.disconnect()");
+    });
+
+    window.addEventListener("pagehide", () => {
+      port.disconnect();
+      window.addEventListener("pageshow", () => {
+        // Use setTimeout to give other pageshow event listeners a chance
+        // to be handled, if any.
+        setTimeout(() => {
+          browser.test.sendMessage("content-script-show");
+        });
+      }, {once: true});
+      browser.test.sendMessage("content-script-hide");
+    }, {once: true});
+    browser.test.sendMessage("content-script-ready");
+  }
+
+  let extension = ExtensionTestUtils.loadExtension({
+    background,
+    manifest: {
+      content_scripts: [{
+        "matches": ["http://example.com/dummy"],
+        "js": ["content_script.js"],
+        "run_at": "document_start",
+      }],
+    },
+
+    files: {
+      "content_script.js": contentScript,
+    },
+  });
+
+  await extension.startup();
+
+  await extension.awaitMessage("bg-ready");
+
+  let contentPage = await ExtensionTestUtils.loadContentPage(
+    "http://example.com/dummy");
+  await extension.awaitMessage("content-script-ready");
+  await extension.awaitMessage("port-connected");
+  await contentPage.spawn(null, () => {
+    // Navigate so that the content page is moved in and out of the bfcache.
+    this.content.location = "data:text/html;charset=utf-8,<script>history.back()</script>";
+  });
+  await extension.awaitMessage("content-script-hide");
+  await extension.awaitMessage("port-disconnected");
+  await extension.awaitMessage("content-script-show");
+  await contentPage.close();
+  await extension.unload();
+});