Bug 1352814 - Force session history off for RDM container. r=ochameau draft
authorJ. Ryan Stinnett <jryans@gmail.com>
Tue, 18 Apr 2017 14:07:14 -0500
changeset 564561 b1d2f73cdaa0419f428b3a5496df81c45f5e67ce
parent 563757 2b6a66a98e253ba158f3960f1c68ad49b2ebcdb4
child 624777 72831719bee00a031ba9b39047003c553be9a900
push id54640
push userbmo:jryans@gmail.com
push dateTue, 18 Apr 2017 19:14:52 +0000
reviewersochameau
bugs1352814, 1313933
milestone55.0a1
Bug 1352814 - Force session history off for RDM container. r=ochameau In bug 1313933, we removed the session store black magic that RDM used to do in order to hide the container tab. Unfortunately, that fix appears to have been imperfect. Session store has a fallback path that can still record the current URL, causing the container URL to be recorded anyway, even though we asked nicely to please not do that. In this change, we try a fresh approach of wedging the session history listener for the container tab so it can't record anything. This avoids the racy approach that was used before bug 1313933 while still appearing to block the container URL from being recorded. MozReview-Commit-ID: JZTYzMAvaEM
devtools/client/responsive.html/browser/swap.js
devtools/client/responsive.html/browser/tunnel.js
devtools/client/responsive.html/test/browser/browser.ini
devtools/client/responsive.html/test/browser/browser_hide_container.js
devtools/client/responsive.html/test/browser/browser_navigation.js
devtools/client/responsive.html/test/browser/browser_page_state.js
devtools/client/responsive.html/test/browser/head.js
--- a/devtools/client/responsive.html/browser/swap.js
+++ b/devtools/client/responsive.html/browser/swap.js
@@ -64,16 +64,29 @@ function swapToInnerBrowser({ tab, conta
 
       // 1. Create a temporary, hidden tab to load the tool UI.
       let containerTab = gBrowser.addTab("about:blank", {
         skipAnimation: true,
         forceNotRemote: true,
       });
       gBrowser.hideTab(containerTab);
       let containerBrowser = containerTab.linkedBrowser;
+      // Even though we load the `containerURL` with `LOAD_FLAGS_BYPASS_HISTORY` below,
+      // `SessionHistory.jsm` has a fallback path for tabs with no history which
+      // fabricates a history entry by reading the current URL, and this can cause the
+      // container URL to be recorded in the session store.  To avoid this, we send a
+      // bogus `epoch` value to our container tab, which causes all future history
+      // messages to be ignored.  (Actual navigations are still correctly recorded because
+      // this only affects the container frame, not the content.)  A better fix would be
+      // to just not load the `content-sessionStore.js` frame script at all in the
+      // container tab, but it's loaded for all tab browsers, so this seems a bit harder
+      // to achieve in a nice way.
+      containerBrowser.messageManager.sendAsyncMessage("SessionStore:flush", {
+        epoch: -1,
+      });
       // Prevent the `containerURL` from ending up in the tab's history.
       containerBrowser.loadURIWithFlags(containerURL, {
         flags: Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_HISTORY,
       });
 
       // Copy tab listener state flags to container tab.  Each tab gets its own tab
       // listener and state flags which cache document loading progress.  The state flags
       // are checked when switching tabs to update the browser UI.  The later step of
--- a/devtools/client/responsive.html/browser/tunnel.js
+++ b/devtools/client/responsive.html/browser/tunnel.js
@@ -7,16 +7,18 @@
 const { Ci } = require("chrome");
 const Services = require("Services");
 const { Task } = require("devtools/shared/task");
 const { BrowserElementWebNavigation } = require("./web-navigation");
 const { getStack } = require("devtools/shared/platform/stack");
 
 // A symbol used to hold onto the frame loader from the outer browser while tunneling.
 const FRAME_LOADER = Symbol("devtools/responsive/frame-loader");
+// Export for use in tests.
+exports.OUTER_FRAME_LOADER_SYMBOL = FRAME_LOADER;
 
 function debug(msg) {
   // console.log(msg);
 }
 
 /**
  * Properties swapped between browsers by browser.xml's `swapDocShells`.  See also the
  * list at /devtools/client/responsive.html/docs/browser-swap.md.
--- a/devtools/client/responsive.html/test/browser/browser.ini
+++ b/devtools/client/responsive.html/test/browser/browser.ini
@@ -20,16 +20,17 @@ support-files =
 [browser_device_custom.js]
 [browser_device_modal_error.js]
 [browser_device_modal_exit.js]
 [browser_device_modal_submit.js]
 [browser_device_width.js]
 [browser_dpr_change.js]
 [browser_exit_button.js]
 [browser_frame_script_active.js]
+[browser_hide_container.js]
 [browser_menu_item_01.js]
 [browser_menu_item_02.js]
 [browser_mouse_resize.js]
 [browser_navigation.js]
 [browser_network_throttling.js]
 [browser_page_state.js]
 [browser_permission_doorhanger.js]
 tags = geolocation
new file mode 100644
--- /dev/null
+++ b/devtools/client/responsive.html/test/browser/browser_hide_container.js
@@ -0,0 +1,64 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Ensure that the RDM container tab URL is not recorded in session history.
+
+const TEST_URL = "http://example.com/";
+const CONTAINER_URL = "chrome://devtools/content/responsive.html/index.xhtml";
+
+const { TabStateFlusher } =
+  Cu.import("resource:///modules/sessionstore/TabStateFlusher.jsm", {});
+const SessionStore =
+  Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);
+const { OUTER_FRAME_LOADER_SYMBOL } =
+  require("devtools/client/responsive.html/browser/tunnel");
+
+function flushContainerTabState(tab) {
+  let browser = tab.linkedBrowser;
+  let outerBrowser = {
+    permanentKey: browser.permanentKey,
+    messageManager: browser[OUTER_FRAME_LOADER_SYMBOL].messageManager,
+  };
+  // Try to flush the tab, but if that hangs for a while, resolve anyway.
+  // During this test, we actually expect this to hang because the container tab
+  // doesn't have the right epoch value to reply to the flush correctly.
+  return new Promise(resolve => {
+    TabStateFlusher.flush(outerBrowser).then(resolve);
+    waitForTime(10000).then(resolve);
+  });
+}
+
+add_task(function* () {
+  // Load test URL
+  let tab = yield addTab(TEST_URL);
+  let browser = tab.linkedBrowser;
+
+  // Check session history state
+  let history = yield getSessionHistory(browser);
+  is(history.index - 1, 0, "At page 0 in history");
+  is(history.entries.length, 1, "1 page in history");
+  is(history.entries[0].url, TEST_URL, "Page 0 URL matches");
+
+  // Open RDM
+  yield openRDM(tab);
+
+  // Checking session history directly in content does show the container URL
+  // that we're trying to hide...
+  history = yield getSessionHistory(browser);
+  is(history.index - 1, 0, "At page 0 in history");
+  is(history.entries.length, 1, "1 page in history");
+  is(history.entries[0].url, CONTAINER_URL, "Page 0 URL matches");
+
+  // However, checking the recorded tab state for the outer browser shows the
+  // container URL has been ignored correctly.
+  yield flushContainerTabState(tab);
+  let tabState = JSON.parse(SessionStore.getTabState(tab));
+  is(tabState.index - 1, 0, "At page 0 in history");
+  is(tabState.entries.length, 1, "1 page in history");
+  is(tabState.entries[0].url, TEST_URL, "Page 0 URL matches");
+
+  yield closeRDM(tab);
+  yield removeTab(tab);
+});
--- a/devtools/client/responsive.html/test/browser/browser_navigation.js
+++ b/devtools/client/responsive.html/test/browser/browser_navigation.js
@@ -17,32 +17,32 @@ add_task(function* () {
   // 2. DUMMY_2_URL
   let tab = yield addTab(DUMMY_1_URL);
   let browser = tab.linkedBrowser;
   yield load(browser, TEST_URL);
   yield load(browser, DUMMY_2_URL);
 
   // Check session history state
   let history = yield getSessionHistory(browser);
-  is(history.index, 2, "At page 2 in history");
+  is(history.index - 1, 2, "At page 2 in history");
   is(history.entries.length, 3, "3 pages in history");
-  is(history.entries[0].uri, DUMMY_1_URL, "Page 0 URL matches");
-  is(history.entries[1].uri, TEST_URL, "Page 1 URL matches");
-  is(history.entries[2].uri, DUMMY_2_URL, "Page 2 URL matches");
+  is(history.entries[0].url, DUMMY_1_URL, "Page 0 URL matches");
+  is(history.entries[1].url, TEST_URL, "Page 1 URL matches");
+  is(history.entries[2].url, DUMMY_2_URL, "Page 2 URL matches");
 
   // Go back one so we're at the test page
   yield back(browser);
 
   // Check session history state
   history = yield getSessionHistory(browser);
-  is(history.index, 1, "At page 1 in history");
+  is(history.index - 1, 1, "At page 1 in history");
   is(history.entries.length, 3, "3 pages in history");
-  is(history.entries[0].uri, DUMMY_1_URL, "Page 0 URL matches");
-  is(history.entries[1].uri, TEST_URL, "Page 1 URL matches");
-  is(history.entries[2].uri, DUMMY_2_URL, "Page 2 URL matches");
+  is(history.entries[0].url, DUMMY_1_URL, "Page 0 URL matches");
+  is(history.entries[1].url, TEST_URL, "Page 1 URL matches");
+  is(history.entries[2].url, DUMMY_2_URL, "Page 2 URL matches");
 
   yield openRDM(tab);
 
   ok(browser.webNavigation.canGoBack, "Going back is allowed");
   ok(browser.webNavigation.canGoForward, "Going forward is allowed");
   is(browser.documentURI.spec, TEST_URL, "documentURI matches page 1");
   is(browser.contentTitle, "Page State Test", "contentTitle matches page 1");
 
@@ -84,15 +84,15 @@ add_task(function* () {
   is(browser.documentURI.spec, DUMMY_3_URL, "documentURI matches page 3");
   is(browser.contentTitle, "mochitest index /browser/devtools/",
      "contentTitle matches page 3");
 
   yield closeRDM(tab);
 
   // Check session history state
   history = yield getSessionHistory(browser);
-  is(history.index, 1, "At page 1 in history");
+  is(history.index - 1, 1, "At page 1 in history");
   is(history.entries.length, 2, "2 pages in history");
-  is(history.entries[0].uri, DUMMY_1_URL, "Page 0 URL matches");
-  is(history.entries[1].uri, DUMMY_3_URL, "Page 1 URL matches");
+  is(history.entries[0].url, DUMMY_1_URL, "Page 0 URL matches");
+  is(history.entries[1].url, DUMMY_3_URL, "Page 1 URL matches");
 
   yield removeTab(tab);
 });
--- a/devtools/client/responsive.html/test/browser/browser_page_state.js
+++ b/devtools/client/responsive.html/test/browser/browser_page_state.js
@@ -17,32 +17,32 @@ add_task(function* () {
   // 2. DUMMY_2_URL
   let tab = yield addTab(DUMMY_1_URL);
   let browser = tab.linkedBrowser;
   yield load(browser, TEST_URL);
   yield load(browser, DUMMY_2_URL);
 
   // Check session history state
   let history = yield getSessionHistory(browser);
-  is(history.index, 2, "At page 2 in history");
+  is(history.index - 1, 2, "At page 2 in history");
   is(history.entries.length, 3, "3 pages in history");
-  is(history.entries[0].uri, DUMMY_1_URL, "Page 0 URL matches");
-  is(history.entries[1].uri, TEST_URL, "Page 1 URL matches");
-  is(history.entries[2].uri, DUMMY_2_URL, "Page 2 URL matches");
+  is(history.entries[0].url, DUMMY_1_URL, "Page 0 URL matches");
+  is(history.entries[1].url, TEST_URL, "Page 1 URL matches");
+  is(history.entries[2].url, DUMMY_2_URL, "Page 2 URL matches");
 
   // Go back one so we're at the test page
   yield back(browser);
 
   // Check session history state
   history = yield getSessionHistory(browser);
-  is(history.index, 1, "At page 1 in history");
+  is(history.index - 1, 1, "At page 1 in history");
   is(history.entries.length, 3, "3 pages in history");
-  is(history.entries[0].uri, DUMMY_1_URL, "Page 0 URL matches");
-  is(history.entries[1].uri, TEST_URL, "Page 1 URL matches");
-  is(history.entries[2].uri, DUMMY_2_URL, "Page 2 URL matches");
+  is(history.entries[0].url, DUMMY_1_URL, "Page 0 URL matches");
+  is(history.entries[1].url, TEST_URL, "Page 1 URL matches");
+  is(history.entries[2].url, DUMMY_2_URL, "Page 2 URL matches");
 
   // Click on content to set an altered state that would be lost on reload
   yield BrowserTestUtils.synthesizeMouseAtCenter("body", {}, browser);
 
   let { ui } = yield openRDM(tab);
 
   // Check color inside the viewport
   let color = yield spawnViewportTask(ui, {}, function* () {
@@ -61,16 +61,16 @@ add_task(function* () {
     return content.getComputedStyle(content.document.body)
                   .getPropertyValue("background-color");
   });
   is(color, "rgb(0, 128, 0)",
      "Content is still modified from click in browser tab");
 
   // Check session history state
   history = yield getSessionHistory(browser);
-  is(history.index, 1, "At page 1 in history");
+  is(history.index - 1, 1, "At page 1 in history");
   is(history.entries.length, 3, "3 pages in history");
-  is(history.entries[0].uri, DUMMY_1_URL, "Page 0 URL matches");
-  is(history.entries[1].uri, TEST_URL, "Page 1 URL matches");
-  is(history.entries[2].uri, DUMMY_2_URL, "Page 2 URL matches");
+  is(history.entries[0].url, DUMMY_1_URL, "Page 0 URL matches");
+  is(history.entries[1].url, TEST_URL, "Page 1 URL matches");
+  is(history.entries[2].url, DUMMY_2_URL, "Page 2 URL matches");
 
   yield removeTab(tab);
 });
--- a/devtools/client/responsive.html/test/browser/head.js
+++ b/devtools/client/responsive.html/test/browser/head.js
@@ -269,33 +269,20 @@ const selectDPR = (ui, value) =>
 const selectNetworkThrottling = (ui, value) => Promise.all([
   once(ui, "network-throttling-changed"),
   changeSelectValue(ui, "#global-network-throttling-selector", value)
 ]);
 
 function getSessionHistory(browser) {
   return ContentTask.spawn(browser, {}, function* () {
     /* eslint-disable no-undef */
-    let { interfaces: Ci } = Components;
-    let webNav = docShell.QueryInterface(Ci.nsIWebNavigation);
-    let sessionHistory = webNav.sessionHistory;
-    let result = {
-      index: sessionHistory.index,
-      entries: []
-    };
-
-    for (let i = 0; i < sessionHistory.count; i++) {
-      let entry = sessionHistory.getEntryAtIndex(i, false);
-      result.entries.push({
-        uri: entry.URI.spec,
-        title: entry.title
-      });
-    }
-
-    return result;
+    let { utils: Cu } = Components;
+    const { SessionHistory } =
+      Cu.import("resource://gre/modules/sessionstore/SessionHistory.jsm", {});
+    return SessionHistory.collect(docShell);
     /* eslint-enable no-undef */
   });
 }
 
 function getContentSize(ui) {
   return spawnViewportTask(ui, {}, () => ({
     width: content.screen.width,
     height: content.screen.height