Bug 1403349 - Prevent WebNavigation jsm to leak the sourceTab browser on unpaired CreatedNavigationTarget messages.
This applies the following changes:
- store a weak reference to the browser element in the WebNavigation.jsm Manager's map
of pending CreatedNavigationTarget messages
- when a CreatedNavigationTarget message is received from a sourceTab
for a created window that is not currently tracked in the map
(e.g. it has been immediately closed), the message received from the sourceTab
is not saved in the map of the pending CreatedNavigationTarget (and a message
is logged in the console to make easier to investigate issues related to discarded
CreatedNavigationTarget events).
- adds an additional assertion to the related test case to ensure that no CreatedNavigationTarget
message is still pending in the WebNavigation/jsm's Manager.
MozReview-Commit-ID: FijQ8IqiY8L
--- a/browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget_window_open.js
+++ b/browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget_window_open.js
@@ -53,16 +53,26 @@ async function runTestCase({extension, o
is(webNavMsg.sourceTabId, sourceTabId, "Got the expected sourceTabId property");
is(webNavMsg.sourceFrameId, sourceFrameId, "Got the expected sourceFrameId property");
is(webNavMsg.url, url, "Got the expected url property");
is(completedNavMsg.tabId, createdTabId, "Got the expected webNavigation.onCompleted tabId property");
is(completedNavMsg.url, url, "Got the expected webNavigation.onCompleted url property");
}
+// Test that there are no pending createdNavigationTarget messages still tracked
+// in WebNavigation.jsm (to be called before the extension is unloaded, because
+// once the last extension which have subscribed a webNavigation event is unloaded
+// all the pending created navigation target data is completely cleared).
+function assertNoPendingCreatedNavigationTargetData() {
+ const {Manager} = Cu.import("resource://gre/modules/WebNavigation.jsm", {});
+ Assert.equal(Manager.createdNavigationTargetByOuterWindowId.size, 0,
+ "There should be no pending createdNavigationTarget messages in WebNavigation");
+}
+
add_task(async function test_window_open() {
const tab1 = await BrowserTestUtils.openNewForegroundTab(gBrowser, SOURCE_PAGE);
gBrowser.selectedTab = tab1;
const extension = ExtensionTestUtils.loadExtension({
background,
manifest: {
@@ -103,16 +113,18 @@ add_task(async function test_window_open
},
expectedWebNavProps: {
sourceTabId: expectedSourceTab.sourceTabId,
sourceFrameId: 0,
url: `${OPENED_PAGE}#new-win-from-window-open`,
},
});
+ assertNoPendingCreatedNavigationTargetData();
+
await BrowserTestUtils.removeTab(tab1);
await extension.unload();
});
add_task(async function test_window_open_from_subframe() {
const tab1 = await BrowserTestUtils.openNewForegroundTab(gBrowser, SOURCE_PAGE);
@@ -158,16 +170,18 @@ add_task(async function test_window_open
},
expectedWebNavProps: {
sourceTabId: expectedSourceTab.sourceTabId,
sourceFrameId: expectedSourceTab.sourceTabFrames[1].frameId,
url: `${OPENED_PAGE}#new-win-from-window-open-subframe`,
},
});
+ assertNoPendingCreatedNavigationTargetData();
+
await BrowserTestUtils.removeTab(tab1);
await extension.unload();
});
add_task(async function test_window_open_close_from_browserAction_popup() {
const tab1 = await BrowserTestUtils.openNewForegroundTab(gBrowser, SOURCE_PAGE);
@@ -222,12 +236,14 @@ add_task(async function test_window_open
},
expectedWebNavProps: {
sourceTabId: expectedSourceTab.sourceTabId,
sourceFrameId: 0,
url: `${OPENED_PAGE}#new-tab-from-window-open`,
},
});
+ assertNoPendingCreatedNavigationTargetData();
+
await BrowserTestUtils.removeTab(tab1);
await extension.unload();
});
--- a/toolkit/modules/addons/WebNavigation.jsm
+++ b/toolkit/modules/addons/WebNavigation.jsm
@@ -315,32 +315,64 @@ var Manager = {
// We are going to receive two message manager messages for a single
// onCreatedNavigationTarget event related to a window.open that is happening
// in the child process (one from the source tab and one from the created tab),
// the unique createdWindowId (the outerWindowID of the created docShell)
// to pair them together.
const pairedMessage = this.createdNavigationTargetByOuterWindowId.get(createdOuterWindowId);
+ if (!isSourceTab) {
+ if (pairedMessage) {
+ // This should not happen, print a warning before overwriting the unexpected pending data.
+ Services.console.logStringMessage(
+ `Discarding onCreatedNavigationTarget for ${createdOuterWindowId}: ` +
+ "unexpected pending data while receiving the created tab data"
+ );
+ }
+
+ // Store a weak reference to the browser XUL element, so that we don't prevent
+ // it to be garbage collected if it has been destroyed.
+ const browserWeakRef = Cu.getWeakReference(browser);
+
+ this.createdNavigationTargetByOuterWindowId.set(createdOuterWindowId, {
+ browserWeakRef,
+ data,
+ });
+
+ return;
+ }
+
if (!pairedMessage) {
- this.createdNavigationTargetByOuterWindowId.set(createdOuterWindowId, {browser, data});
+ // The sourceTab should always be received after the message coming from the created
+ // top level frame because the "webNavigation-createdNavigationTarget-from-js" observers
+ // subscribed by WebNavigationContent.js are going to be executed in reverse order
+ // (See http://searchfox.org/mozilla-central/rev/f54c1723be/xpcom/ds/nsObserverList.cpp#76)
+ // and the observer subscribed to the created target will be the last one subscribed
+ // to the ObserverService (and the first one to be triggered).
+ Services.console.logStringMessage(
+ `Discarding onCreatedNavigationTarget for ${createdOuterWindowId}: ` +
+ "received source tab data without any created tab data available"
+ );
+
return;
}
this.createdNavigationTargetByOuterWindowId.delete(createdOuterWindowId);
- let sourceTabBrowser;
- let createdTabBrowser;
+ let sourceTabBrowser = browser;
+ let createdTabBrowser = pairedMessage.browserWeakRef.get();
- if (isSourceTab) {
- sourceTabBrowser = browser;
- createdTabBrowser = pairedMessage.browser;
- } else {
- sourceTabBrowser = pairedMessage.browser;
- createdTabBrowser = browser;
+ if (!createdTabBrowser) {
+ Services.console.logStringMessage(
+ `Discarding onCreatedNavigationTarget for ${createdOuterWindowId}: ` +
+ "the created tab has been already destroyed"
+ );
+
+ return;
}
this.fire("onCreatedNavigationTarget", createdTabBrowser, {}, {
sourceTabBrowser, sourceFrameId, url,
});
},
onStateChange(browser, data) {