Bug 1352814 - Force session history off for RDM container. r=ochameau, a=gchang
☠☠ backed out by 953ebf1c3215 ☠ ☠
authorJ. Ryan Stinnett <jryans@gmail.com>
Tue, 18 Apr 2017 14:07:14 -0500
changeset 396013 a9bdcbca4218cac28516e3a0e5385ff5202143a3
parent 396012 8dbf9446a94471fc1245bb4d625de7d751d13bd7
child 396014 382a2d282d384db4581c979e603276f88559319b
push id1468
push userasasaki@mozilla.com
push dateMon, 05 Jun 2017 19:31:07 +0000
treeherdermozilla-release@0641fc6ee9d1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersochameau, gchang
bugs1352814, 1313933
milestone54.0
Bug 1352814 - Force session history off for RDM container. r=ochameau, a=gchang 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]
 [browser_resize_cmd.js]
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
@@ -270,33 +270,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