Bug 1403349 - Prevent WebNavigation jsm to leak the sourceTab browser on unpaired CreatedNavigationTarget messages. draft
authorLuca Greco <lgreco@mozilla.com>
Wed, 27 Sep 2017 19:07:41 +0200
changeset 681614 4085e82040b2f64acd8eff681185d5c8eab5804d
parent 681613 5a2d4ceaeffdba44e63d82deace8ce8df89d2527
child 681615 b6a2bf76fc1e878659d938e8121dbe2e94c33667
push id84871
push userluca.greco@alcacoop.it
push dateTue, 17 Oct 2017 13:50:36 +0000
bugs1403349
milestone58.0a1
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
browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget_window_open.js
toolkit/modules/addons/WebNavigation.jsm
--- 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) {