Bug 1370368 - Fire port.onDisconnect after pageshow
MozReview-Commit-ID: 8i2j38Rr4y5
--- 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();
+});