Bug 543435 - Only reuse inner window for uncommitted initial documents. r=smaug,asuth,credential-management-reviewers,webrtc-reviewers,pehrsons,hsivonen,dimi

The conceptual change is small but it requires several test changes.

In principle, tests cannot open a new empty window, modify the global, then
load their target document and expect their modifications to stick. Only if the
target URI is opened directly, the synchronously available window is reused. So
either tests need to do that, or rely on messages.

Differential Revision: https://phabricator.services.mozilla.com/D265268

Author: vinhill

docshell/base/nsDocShell.cpp

@@ -2527,7 +2527,8 @@ Maybe<ClientInfo> nsDocShell::GetInitialClientInfo() const {
       mScriptGlobal ? mScriptGlobal->GetCurrentInnerWindow() : nullptr;
   Document* doc = innerWindow ? innerWindow->GetExtantDoc() : nullptr;
 
-  if (!doc || !doc->IsInitialDocument()) {
+  if (!doc || !doc->IsUncommittedInitialDocument()) {
+    // We won't reuse the inner window so let the channel determine one
     return Maybe<ClientInfo>();
   }
 

docshell/test/chrome/test_bug608669.xhtml

@@ -49,12 +49,13 @@ function* doTest() {
   is(notificationCount, 0, "initial count");
 
   // create a new window
-  var testWin = chromeWindow.open("", "bug 608669", "chrome,width=600,height=600");
+  var testWin = chromeWindow.open("bug608669.xhtml", "bug 608669", "chrome,width=600,height=600");
+  // Synchronously modify the transient about:blank window
   testWin.x = "y";
+  is(testWin.location.href, 'about:blank');
   is(notificationCount, 0, "after created window");
 
-  // Try loading in the window
-  testWin.location = "bug608669.xhtml";
+  // Wait for the window to load
   chromeWindow.onmessage = nextTest;
   yield undefined;
   is(notificationCount, 1, "after first load");

docshell/test/mochitest/test_bug1729662.html

@@ -9,59 +9,46 @@
     SimpleTest.waitForExplicitFinish();
     SimpleTest.requestFlakyTimeout("Need to wait to make sure an event does not fire");
 
-    async function runTest() {
-      let win = window.open();
-      let goneBackAndForwardOnce = new Promise((resolve) => {
-        let timeoutID;
-
-        // We should only get one load event in win.
-        let bc = new BroadcastChannel("bug1729662");
-        bc.addEventListener("message", () => {
-          bc.addEventListener("message", () => {
-            clearTimeout(timeoutID);
-            resolve(false);
-          });
-        }, { once: true });
-
-        let goneBack = false, goneForward = false;
-        win.addEventListener("popstate", ({ state }) => {
-          // We should only go back and forward once, if we get another
-          // popstate after that then we should fall through to the
-          // failure case below.
-          if (!(goneBack && goneForward)) {
-            // Check if this is the popstate for the forward (the one for
-            // back will have state == undefined).
-            if (state == 1) {
-              ok(goneBack, "We should have gone back before going forward");
-
-              goneForward = true;
-
-              // Wait a bit to make sure there are no more popstate events.
-              // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
-              timeoutID = setTimeout(resolve, 1000, true);
-
-              return;
-            }
+    function waitForEvent(target, event) {
+      return new Promise(resolve =>
+        target.addEventListener(event, resolve, { once: true }));
+    }
 
-            // Check if we've gone back once before, if we get another
-            // popstate after that then we should fall through to the
-            // failure case below.
-            if (!goneBack) {
-              goneBack = true;
+    async function runTest() {
+      // Target page needs to be the initial load so that the synchronously
+      // available window is reused and we can attach popstate listeners.
+      const win = window.open('file_bug1729662.html');
+
+      let timeoutID;
+      let loadCount = 0;
+      let bc = new BroadcastChannel("bug1729662");
+      bc.onmessage = ({ data }) => {
+        if (data != 'load') return;
+        if (++loadCount > 1) {
+          // We should only get one load event in win
+          clearTimeout(timeoutID);
+        }
+      };
 
-              return;
-            }
-          }
+      // We should only go back and forward once
+      // The popstate for back will have state == null, see file_bug1729662.html
+      const state1 = await waitForEvent(win, 'popstate');
+      is(state1.state, null, 'got expected state for history.back');
 
-          clearTimeout(timeoutID);
-          resolve(false);
-        });
-      });
+      const state2 = await waitForEvent(win, 'popstate');
+      is(state2.state, 1, 'got expected state for history.forward');
 
-      win.location = "file_bug1729662.html";
+      // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
+      const timeout = new Promise(resolve => timeoutID = setTimeout(resolve, 1000, 'timeout'));
+      const result = await Promise.race([
+        waitForEvent(win, 'popstate'),
+        timeout
+      ]);
+      is(result, 'timeout', 'Got no more popstate');
 
-      ok(await goneBackAndForwardOnce, "Stopped navigating history");
+      is(loadCount, isXOrigin ? 0 : 1, 'Got exactly one (zero for xorigin) load events in win');
 
+      clearTimeout(timeoutID);
       win.close();
 
       SimpleTest.finish();

dom/base/nsGlobalWindowInner.cpp

@@ -1916,7 +1916,7 @@ nsresult nsGlobalWindowInner::EnsureClientSource() {
 
   // Try to get the reserved client from the LoadInfo.  A Client is
   // reserved at the start of the channel load if there is not an
-  // initial about:blank document that will be reused.  It is also
+  // initial uncommitted about:blank whose window will be reused. It is also
   // created if the channel load encounters a cross-origin redirect.
   if (loadInfo) {
     UniquePtr<ClientSource> reservedClient =

dom/base/nsGlobalWindowOuter.cpp

@@ -1716,7 +1716,9 @@ nsIScriptContext* nsGlobalWindowOuter::GetScriptContext() { return mContext; }
 
 bool nsGlobalWindowOuter::WouldReuseInnerWindow(Document* aNewDocument) {
   // We reuse the inner window when:
-  // a. We are currently at our original document.
+  // a. The current document is transient, i.e. a temporary placeholder while
+  //    an async load is ongoing. This is equivalent to the uncommitted initial
+  //    document.
   // b. At least one of the following conditions are true:
   // -- The new document is the same as the old document. This means that we're
   //    getting called from document.open().
@@ -1726,7 +1728,7 @@ bool nsGlobalWindowOuter::WouldReuseInnerWindow(Document* aNewDocument) {
     return false;
   }
 
-  if (!mDoc->IsInitialDocument()) {
+  if (!mDoc->IsUncommittedInitialDocument()) {
     return false;
   }
 

dom/base/test/file_bug1126851_post_unload.html

@@ -0,0 +1,5 @@
+<script>
+addEventListener('unload', () => {
+  window.opener.postMessage('unload', '*');
+});
+</script>

dom/base/test/mochitest.toml

@@ -175,6 +175,7 @@ support-files = [
   "referrer_change_server.sjs",
   "file_change_policy_redirect.html",
   "file_bug1198095.js",
+  "file_bug1126851_post_unload.html",
   "file_bug1250148.sjs",
   "file_bug1268962.sjs",
   "iframe_meta_refresh.sjs",

dom/base/test/test_bug1126851.html

@@ -17,8 +17,10 @@
   let gotUnload = false;
 
   function runTest() {
-    win = window.open("about:blank", "");
-    win.onunload = function() {
+    win = window.open("file_bug1126851_post_unload.html", "");
+    // Reload replaces the inner window so we cannot wait for win.onunload
+    // and instead must rely on messages from the popup.
+    window.onmessage = function() {
       if (gotUnload) {
         SimpleTest.finish();
       } else {
@@ -36,8 +38,10 @@
         win.location.reload();
       }
     }
-    evt = new win.Event("resize", { bubbles: true });
-    win.document.dispatchEvent(evt);
+    win.onload = function() {
+      evt = new win.Event("resize", { bubbles: true });
+      win.document.dispatchEvent(evt);
+    }
   }
 
 

dom/base/test/test_window_focus_by_close_and_open.html

@@ -9,19 +9,17 @@
 SimpleTest.waitForExplicitFinish();
 
 function start() {
-  var w = window.open("", "_blank");
+  var w = window.open("file_window_focus_by_close_and_open.html", "_blank");
   w.finished = function() {
     ok(true, "1st new window had focus");
     w.close();
-    w = window.open("", "_blank");
+    w = window.open("file_window_focus_by_close_and_open.html", "_blank");
     w.finished = function() {
       ok(true, "2nd new window had focus");
       w.close();
       SimpleTest.finish();
     };
-    w.location = "file_window_focus_by_close_and_open.html";
   };
-  w.location = "file_window_focus_by_close_and_open.html";
 }
 </script>
 </head>

dom/encoding/test/test_submit_euckr.html

@@ -14,20 +14,17 @@
 </form>
 <script>
 
-runTest();
+async_test((t) => {
+  const form = document.forms[0];
+  const iframe = document.querySelector('iframe');
 
-function runTest() {
-  var t = async_test("Test for euc-kr encoded form submission");
-  var f = document.forms[0];
-  var ifr = frames.ifr;
-  ifr.onload = function() {
-    t.step(function() {
-      assert_equals("".split.call(ifr.location, "?")[1], "a=%81A");
-    });
-    t.done();
-  };
-  f.submit();
-}
+  // note: listener must be on the iframe, not the window that will be replaced
+  iframe.onload = t.step_func_done(() =>
+    assert_equals("".split.call(iframe.contentWindow.location, "?")[1], "a=%81A")
+  );
+
+  form.submit();
+}, "Test for euc-kr encoded form submission");
 
 </script>
 </body>