Bug 754029 - Navigating from a new script tag does not add a session history entry. r=smaug, r=bz

browser/base/content/test/browser_tab_dragdrop.js

@@ -14,7 +14,7 @@ function test()
   ];
 
   function setLocation(i, url) {
-    gBrowser.getBrowserForTab(tabs[i]).contentWindow.location = url;
+    gBrowser.getBrowserForTab(tabs[i]).contentWindow.location.assign(url);
   }
   function moveTabTo(a, b) {
     gBrowser.swapBrowsersAndCloseOther(gBrowser.tabs[b], gBrowser.tabs[a]);

browser/base/content/test/browser_tabfocus.js

@@ -166,13 +166,13 @@ function test() {
     // navigating back maintains the focus in the urlbar.
     browser1.addEventListener("pageshow", _browser_tabfocus_navigation_test_eventOccured, true);
     button1.focus();
-    browser1.contentWindow.location = testPage3;
+    browser1.contentWindow.location.assign(testPage3);
   }
 
   browser1.addEventListener("load", check, true);
   browser2.addEventListener("load", check, true);
-  browser1.contentWindow.location = testPage1;
+  browser1.contentWindow.location.assign(testPage1);
-  browser2.contentWindow.location = testPage2;
+  browser2.contentWindow.location.assign(testPage2);
 }
 
 var _browser_tabfocus_test_lastfocus;

browser/devtools/webconsole/test/head.js

@@ -33,7 +33,7 @@ let tab, browser, hudId, hud, hudBox, filterBox, outputNode, cs;
 function addTab(aURL)
 {
   gBrowser.selectedTab = gBrowser.addTab();
-  content.location = aURL;
+  content.location.assign(aURL);
   tab = gBrowser.selectedTab;
   browser = gBrowser.getBrowserForTab(tab);
 }

docshell/base/nsDocShell.cpp

@@ -1390,16 +1390,6 @@ nsDocShell::LoadURI(nsIURI * aURI,
                 }
             } // parent
         } //parentDS
-        else {  
-            // This is the root docshell. If we got here while  
-            // executing an onLoad Handler,this load will not go 
-            // into session history.
-            bool inOnLoadHandler=false;
-            GetIsExecutingOnLoadHandler(&inOnLoadHandler);
-            if (inOnLoadHandler) {
-                loadType = LOAD_NORMAL_REPLACE;
-            }
-        } 
     } // !shEntry
 
     if (shEntry) {
@@ -6487,7 +6477,7 @@ nsDocShell::EndPageLoad(nsIWebProgress * aProgress,
     nsCOMPtr<nsIDocShell> kungFuDeathGrip(this);
 
     // Notify the ContentViewer that the Document has finished loading.  This
-    // will cause any OnLoad(...) and PopState(...) handlers to fire.
+    // will cause any OnLoad(...) handlers to fire.
     if (!mEODForCurrentDocument && mContentViewer) {
         mIsExecutingOnLoadHandler = true;
         mContentViewer->LoadComplete(aStatus);

docshell/test/chrome/Makefile.in

@@ -95,6 +95,8 @@ MOCHITEST_CHROME_FILES =	\
 		test_mozFrameType.xul \
 		mozFrameType_window.xul \
 		test_bug789773.xul \
+		test_bug754029.xul \
+		bug754029_window.xul \
 		$(NULL)
 
 MOCHITEST_CHROME_FILES += \

docshell/test/chrome/bug311007_window.xul

@@ -109,7 +109,7 @@ function step1B(aWebProgress, aRequest, aLocation, aFlags) {
 
 function step2A() {
   gListener.callback = step2B;
-  content.location = kSecureURI;
+  content.location.assign(kSecureURI);
 }
 
 function step2B(aWebProgress, aRequest, aLocation, aFlags) {
@@ -139,7 +139,7 @@ function step2B(aWebProgress, aRequest, aLocation, aFlags) {
 
 function step3A() {
   gListener.callback = step3B;
-  content.location += "#foo";
+  content.location.assign("#foo");
 }
 
 function step3B(aWebProgress, aRequest, aLocation, aFlags) {

docshell/test/chrome/bug754029_window.xul

@@ -0,0 +1,307 @@
+<?xml version="1.0"?>
+<?xml-stylesheet href="chrome://global/skin" type="text/css"?>
+
+<window id="754029Test"
+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
+        width="600"
+        height="600"
+        onload="startup();"
+        title="bug 754029 test">
+
+  <script type="application/javascript" src="docshell_helpers.js"></script>
+  <script type="application/javascript"><![CDATA[
+
+const _kDocshellTestNavURI =
+  "http://test1.example.org:80/tests/docshell/test/navigation/";
+
+const kBlankURI = _kDocshellTestNavURI + "blank.html";
+const kRedirectURI = _kDocshellTestNavURI + "file_bug754029.html";
+
+function startup() {
+  var browser = document.getElementById("content");
+  browser.addEventListener("load", contentOnLoad, true);
+  content.location.href = kBlankURI + "?start";
+}
+
+function contentOnLoad(aEvent) {
+  is(aEvent.originalTarget.nodeName, "#document", "Loading a document.");
+  var browser = document.getElementById("content");
+  var sessionHistory = browser.sessionHistory;
+
+  var _contentLoadURI = function (aHref) {content.location.href = aHref;}
+
+  function contentLoadURI(aHref) {
+    setTimeout(_contentLoadURI, 0, aHref);
+  }
+
+  function indexToSearch(aSessionHistory, aIndex) {
+    return "?" + aSessionHistory.getEntryAtIndex(aIndex, false)
+                                .URI
+                                .QueryInterface(Components.interfaces.nsIURL)
+                                .query;
+  }
+
+  switch(content.location.search) {
+  case "?start":
+    // Expected SH entries are:
+    // 1  * <blank.html          ?start>
+    is(content.history.length, 1, "Initial <about:blank> is replaced.");
+
+    // Requesting <file_bug754029.html?test1>
+    contentLoadURI(kRedirectURI + "?test1");
+    break;
+
+  /*****************************************************************************
+   * Test 1: Load a JS redirecting page; |location.href = ...| is directly in
+   *         <script></script> tag.
+   *
+   * Expected Result: The redirected page should replace the redirecting page's
+   *                  session history.
+   ****************************************************************************/
+  case "?test1":
+    // We can't catch this onload, because redirection is earlier than
+    // firing load event. That is OK.
+
+    // Expected SH entries are:
+    // 0    <?start>
+    // 1  * <?test1>
+    break;
+
+  case "?result1":
+    // Expected SH entries are:
+    // 0    <?start>
+    // x    <?test1>    // replaced.
+    // 1  * <?result1>
+
+    is(sessionHistory.count, 2, "<?result1>: SH's length");
+    is(sessionHistory.index, 1, "<?result1>: Current entry's index");
+
+    is(indexToSearch(sessionHistory, sessionHistory.index),
+       "?result1",
+       "Check if the current SH entry is <?result1>");
+
+    is(indexToSearch(sessionHistory, sessionHistory.index - 1),
+       "?start",
+       "Check if the previous SH entry is not <?test1> but <?start>");
+
+    // Requesting <file_bug754029.html?test2>
+    contentLoadURI(kRedirectURI + "?test2");
+    break;
+
+  /*****************************************************************************
+   * Test 2: Load a JS redirecting page; |location.href = ...| is in
+   *         "load" event handler.
+   *
+   * Expected Result: Replace
+   ****************************************************************************/
+  case "?test2":
+    // Expected SH entries are:
+    // 0    <?start>
+    // x    <?test1>    // replaced.
+    // 1    <?result1>
+    // 2  * <?test2>
+
+    is(sessionHistory.count, 3, "<?test2>: SH's length");
+    is(sessionHistory.index, 2, "<?test2>: Current entry's index");
+
+    is(indexToSearch(sessionHistory, sessionHistory.index),
+       "?test2",
+       "Check if the current SH entry is <?test2>");
+
+    is(indexToSearch(sessionHistory, sessionHistory.index - 1),
+       "?result1",
+       "Check if the previous SH entry is <?result1>");
+    break;
+
+  case "?result2":
+    // Expected SH entries are:
+    // 0    <?start>
+    // x    <?test1>    // replaced.
+    // 1    <?result1>
+    // x    <?test2>    // replaced.
+    // 2  * <?result2>
+
+    is(sessionHistory.count, 3, "<?result2>: SH's length");
+    is(sessionHistory.index, 2, "<?result2>: Current entry's index");
+
+    is(indexToSearch(sessionHistory, sessionHistory.index),
+       "?result2",
+       "Check if the current SH entry is <?result2>");
+
+    is(indexToSearch(sessionHistory, sessionHistory.index - 1),
+       "?result1",
+       "Check if the previous SH entry is not <?test2> but <?resutl1>");
+
+    contentLoadURI(kRedirectURI + "?test3");
+    break;
+
+  /*****************************************************************************
+   * Test 3: Load a JS redirecting page; |location.href = ...| is in
+   *         setTimeout(...)'s call back.
+   *
+   * Expected Result: Not replace
+   ****************************************************************************/
+  case "?test3":
+    // Expected SH entries are:
+    // 0    <?start>
+    // x    <?test1>    // replaced.
+    // 1    <?result1>
+    // x    <?test2>    // replaced.
+    // 2    <?result2>
+    // 3  * <?test3>
+
+    is(sessionHistory.count, 4, "<?test3>: SH's length");
+    is(sessionHistory.index, 3, "<?test3>: Current entry's index");
+
+    is(indexToSearch(sessionHistory, sessionHistory.index),
+       "?test3",
+       "Check if the current SH entry is <?test3>");
+
+    is(indexToSearch(sessionHistory, sessionHistory.index - 1),
+       "?result2",
+       "Check if the previous SH entry is <?result2>");
+    break;
+
+  case "?result3":
+    // Expected SH entries are:
+    // 0    <?start>
+    // x    <?test1>    // replaced.
+    // 1    <?result1>
+    // x    <?test2>    // replaced.
+    // 2    <?result2>
+    // 3    <?test3>    // not replaced.
+    // 4  * <?result3>
+
+    is(sessionHistory.count, 5, "<?result3>: SH's length");
+    is(sessionHistory.index, 4, "<?result3>: Current entry's index");
+
+    is(indexToSearch(sessionHistory, sessionHistory.index),
+       "?result3",
+       "Check if the current SH entry is <?result3>");
+
+    is(indexToSearch(sessionHistory, sessionHistory.index - 1),
+       "?test3",
+       "Check if <?test3> exists.");
+
+    contentLoadURI(kRedirectURI + "?test4");
+    break;
+
+  /*****************************************************************************
+   * Test 4: Load a JS redirecting page; setTimeout(...)'s callback
+   *         is inserting a new script element into the document. And the
+   *         inserted script contains |location.href = ...|.
+   *
+   *         See also:
+   *         https://bugzilla.mozilla.org/attachment.cgi?id=622899
+   *
+   * Expected Result: Not replace
+   ****************************************************************************/
+  case "?test4":
+    // Expected SH entries are:
+    // 0    <?start>
+    // x    <?test1>    // replaced.
+    // 1    <?result1>
+    // x    <?test2>    // replaced.
+    // 2    <?result2>
+    // 3    <?test3>    // not replaced
+    // 4    <?result3>
+    // 5  * <?test4>
+
+    is(sessionHistory.count, 6, "<?test4>: SH's length");
+    is(sessionHistory.index, 5, "<?test4>: Current entry's index");
+
+    is(indexToSearch(sessionHistory, sessionHistory.index),
+       "?test4",
+       "Check if the current SH entry is <?test4>");
+
+    is(indexToSearch(sessionHistory, sessionHistory.index - 1),
+       "?result3",
+       "Check if the previous SH entry is <?result3>");
+
+    break;
+
+  case "?result4":
+    // Expected SH entries are:
+    // 0    <?start>
+    // x    <?test1>    // replaced.
+    // 1    <?result1>
+    // x    <?test2>    // replaced.
+    // 2    <?result2>
+    // 3    <?test3>    // not replaced.
+    // 4    <?result3>
+    // 5    <?test4>    // not replaced.
+    // 6  * <?result4>
+
+    is(sessionHistory.count, 7, "<?test4>: SH's length");
+    is(sessionHistory.index, 6, "<?test4>: Current entry's index");
+
+    is(indexToSearch(sessionHistory, sessionHistory.index),
+       "?result4",
+       "Check if the current SH entry is <?test4>");
+
+    is(indexToSearch(sessionHistory, sessionHistory.index - 1),
+       "?test4",
+       "Check if <?test4> exists.");
+
+    contentLoadURI(kRedirectURI + "?testDOMContentLoaded");
+    break;
+
+  /*****************************************************************************
+   * Test 5: Setting location.href in onDOMContentLoaded() should REPLACE.
+   ****************************************************************************/
+  case "?testDOMContentLoaded":
+    break;
+  case "?resultDOMContentLoaded":
+    is(indexToSearch(sessionHistory, sessionHistory.index),
+       "?resultDOMContentLoaded",
+       "Check if the current SH entry is <?resultDOMContentLoaded>");
+
+    is(indexToSearch(sessionHistory, sessionHistory.index - 1),
+       "?result4",
+       "Check if the perevious entry is not <?testDOMContentLoaded> but " +
+       "<?result4>.");
+
+    contentLoadURI(kRedirectURI + "?testPageshow");
+    break;
+
+  /*****************************************************************************
+   * Test 6: Setting location.href in onpageshow() should REPLACE.
+   ****************************************************************************/
+  case "?testPageshow":
+    break;
+  case "?resultPageshow":
+    is(indexToSearch(sessionHistory, sessionHistory.index),
+       "?resultPageshow",
+       "Check if the current SH entry is <?resultPageshow>");
+
+    is(indexToSearch(sessionHistory, sessionHistory.index - 1),
+       "?resultDOMContentLoaded",
+       "Check if the perevious entry is not <?testPageshow> but " +
+       "<?resultDOMContentLoaded>.");
+
+    contentLoadURI(kRedirectURI + "?testReadystatechange");
+    break;
+
+  /*****************************************************************************
+   * Test 7: Setting location.href in onreadystatechange() should REPLACE.
+   ****************************************************************************/
+  case "?testReadystatechange":
+    break;
+  case "?resultReadystatechange":
+    is(indexToSearch(sessionHistory, sessionHistory.index),
+       "?resultReadystatechange",
+       "Check if the current SH entry is <?resultReadystatechange>");
+
+    is(indexToSearch(sessionHistory, sessionHistory.index - 1),
+       "?resultPageshow",
+       "Check if the perevious entry is not <?testReadystatechange> but " +
+       "<?resultPageshow>.");
+
+    finish();
+    break;
+  }
+}
+  ]]></script>
+
+  <browser type="content-primary" flex="1" id="content" src="about:blank"/>
+</window>

docshell/test/chrome/test_bug754029.xul

@@ -0,0 +1,43 @@
+<?xml version="1.0"?>
+<?xml-stylesheet href="chrome://global/skin" type="text/css"?>
+<?xml-stylesheet
+  href="chrome://mochikit/content/tests/SimpleTest/test.css"
+  type="text/css"?>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=754029.xul
+-->
+<window title="Mozilla Bug 754029"
+  xmlns:html="http://www.w3.org/1999/xhtml"
+  xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
+
+  <title>Test for Bug 754029</title>
+  <script type="application/javascript"
+    src="chrome://mochikit/content/MochiKit/packed.js"></script>
+  <script type="application/javascript"
+    src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
+
+<body  xmlns="http://www.w3.org/1999/xhtml">
+<a target="_blank"
+   href="https://bugzilla.mozilla.org/show_bug.cgi?id=754029">
+   Mozilla Bug 754029</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+
+</div>
+<pre id="test">
+</pre>
+</body>
+
+<script class="testbody" type="application/javascript">
+<![CDATA[
+
+/** Test for Bug 754029 **/
+
+SimpleTest.waitForExplicitFinish();
+window.open("bug754029_window.xul", "bug754029",
+            "chrome,width=600,height=600");
+
+]]>
+</script>
+
+</window>

docshell/test/navigation/Makefile.in

@@ -46,6 +46,7 @@ MOCHITEST_FILES = \
 		frame3.html \
 		goback.html \
 		file_bug534178.html \
+		file_bug754029.html \
 		$(NULL)
 
 ifneq (mobile/android,$(MOZ_BUILD_APP))

docshell/test/navigation/file_bug754029.html

@@ -0,0 +1,84 @@
+<html>
+<head>
+<meta http-equiv="content-type" Content="text/html; charset=utf-8">
+</head>
+<body>
+<script>
+
+// inline <script> tag redirection.
+function test1() {
+  location.href = "blank.html?result1";
+}
+
+// onload() handler redirection.
+function test2() {
+  addEventListener("load", 
+                   function(aEvent) {
+                     location.href = "blank.html?result2";
+                   },
+                   false);
+}
+
+// setTimeout() 100 milisec redirection.
+function test3() {
+  setTimeout(function() {
+               location.href = "blank.html?result3";
+             },
+             100);
+}
+
+// setTimeout() 100 milisec + inline <script> tag redirection.
+function test4() {
+  setTimeout(function() {
+               var ns = document.createElement("script");
+               var nt = document.createTextNode(
+                          "location = 'blank.html?result4'"
+                        );
+               ns.appendChild(nt);
+               document.documentElement.appendChild(ns);
+             },
+             100);
+}
+
+// DOMContentLoaded
+function testDOMContentLoaded() {
+  addEventListener("DOMContentLoaded", 
+                   function(aEvent) {
+                     location.href = "blank.html?resultDOMContentLoaded";
+                   },
+                   false);
+}
+
+// pageshow
+function testPageshow() {
+  addEventListener("pageshow", 
+                   function(aEvent) {
+                     location.href = "blank.html?resultPageshow";
+                   },
+                   false);
+}
+
+// readystatechange for "complete"
+function testReadystatechange() {
+  document.onreadystatechange =
+    function() {
+      if ("complete" == document.readyState) {
+        location.href = "blank.html?resultReadystatechange";
+      }
+    };
+}
+
+switch(location.search) {
+case "?test1": test1(); break;
+case "?test2": test2(); break;
+case "?test3": test3(); break;
+case "?test4": test4(); break;
+case "?testDOMContentLoaded": testDOMContentLoaded(); break;
+case "?testPageshow"        : testPageshow();         break;
+case "?testReadystatechange": testReadystatechange(); break;
+default: throw "Unexpected!";
+}
+
+</script>
+</body>
+</html>

dom/base/nsLocation.cpp

@@ -35,6 +35,7 @@
 #include "nsJSUtils.h"
 #include "jsfriendapi.h"
 #include "nsContentUtils.h"
+#include "nsEventStateManager.h"
 
 static nsresult
 GetContextFromStack(nsIJSContextStack *aStack, JSContext **aContext)
@@ -521,8 +522,71 @@ nsLocation::SetHref(const nsAString& aHref)
   if (NS_FAILED(GetContextFromStack(stack, &cx)))
     return NS_ERROR_FAILURE;
 
+  // According to HTML5 spec, |location.href = ...| must act as if
+  // it were |location.replace(...)| before the page load finishes.
+  //
+  // http://www.w3.org/TR/2011/WD-html5-20110113/history.html#location
+  //
+  // > The href attribute must return the current address of the
+  // > associated Document object, as an absolute URL.
+  // >
+  // > On setting, if the Location object's associated Document
+  // > object has completely loaded, then the user agent must act
+  // > as if the assign() method had been called with the new value
+  // > as its argument. Otherwise, the user agent must act as if
+  // > the replace() method had been called with the new value as its
+  // > argument.
+  //
+  // Note: The spec says the condition is "Document object has completely
+  //       loaded", but that may break some websites. If the user was
+  //       willing to move from one page to another, and was able to do
+  //       so, we should not overwrite the session history entry even
+  //       if the loading has not finished yet.
+  //
+  //       https://www.w3.org/Bugs/Public/show_bug.cgi?id=17041 
+  //
+  // See bug 39938, bug 72197, bug 178729 and bug 754029.
+  // About other browsers:
+  // http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2010-July/027372.html
+
+  bool replace = false;
+  if (!nsEventStateManager::IsHandlingUserInput()) {
+    // "completely loaded" is defined at:
+    //
+    // http://www.w3.org/TR/2012/WD-html5-20120329/the-end.html#completely-loaded
+    //
+    // > 7.  document readiness to "complete", and fire "load".
+    // >
+    // > 8.  "pageshow"
+    // >
+    // > 9.  ApplicationCache
+    // >
+    // > 10. Print in the pending list.
+    // >
+    // > 12. Queue a task to mark the Document as completely loaded.
+    //
+    // Since Gecko doesn't (yet) have a flag corresponding to no. "12.
+    // ... completely loaded", here the logic is a little tricky.
+
+    nsCOMPtr<nsIDocShell> docShell(do_QueryReferent(mDocShell));
+    nsCOMPtr<nsIDocument> document(do_GetInterface(docShell));
+    if (document) {
+      replace =
+        nsIDocument::READYSTATE_COMPLETE != document->GetReadyStateEnum();
+
+      // nsIDocShell::isExecutingOnLoadHandler is true while
+      // the document is handling "load", "pageshow",
+      // "readystatechange" for "complete" and "beforeprint"/"afterprint".
+      //
+      // Maybe this API property needs a better name.
+      if (!replace) {
+        docShell->GetIsExecutingOnLoadHandler(&replace);
+      }
+    }
+  }
+
   if (cx) {
-    rv = SetHrefWithContext(cx, aHref, false);
+    rv = SetHrefWithContext(cx, aHref, replace);
   } else {
     rv = GetHref(oldHref);
 
@@ -532,7 +596,7 @@ nsLocation::SetHref(const nsAString& aHref)
       rv = NS_NewURI(getter_AddRefs(oldUri), oldHref);
 
       if (oldUri) {
-        rv = SetHrefWithBase(aHref, oldUri, false);
+        rv = SetHrefWithBase(aHref, oldUri, replace);
       }
     }
   }
@@ -563,8 +627,6 @@ nsLocation::SetHrefWithBase(const nsAString& aHref, nsIURI* aBase,
   nsresult result;
   nsCOMPtr<nsIURI> newUri;
 
-  nsCOMPtr<nsIDocShell> docShell(do_QueryReferent(mDocShell));
-
   nsAutoCString docCharset;
   if (NS_SUCCEEDED(GetDocumentCharacterSetForURI(aHref, docCharset)))
     result = NS_NewURI(getter_AddRefs(newUri), aHref, docCharset.get(), aBase);
@@ -572,41 +634,7 @@ nsLocation::SetHrefWithBase(const nsAString& aHref, nsIURI* aBase,
     result = NS_NewURI(getter_AddRefs(newUri), aHref, nullptr, aBase);
 
   if (newUri) {
-    /* Check with the scriptContext if it is currently processing a script tag.
+    return SetURI(newUri, aReplace);
-     * If so, this must be a <script> tag with a location.href in it.
-     * we want to do a replace load, in such a situation. 
-     * In other cases, for example if a event handler or a JS timer
-     * had a location.href in it, we want to do a normal load,
-     * so that the new url will be appended to Session History.
-     * This solution is tricky. Hopefully it isn't going to bite
-     * anywhere else. This is part of solution for bug # 39938, 72197
-     * 
-     */
-    bool inScriptTag=false;
-    // Get JSContext from stack.
-    nsCOMPtr<nsIJSContextStack> stack(do_GetService("@mozilla.org/js/xpc/ContextStack;1", &result));
-
-    if (stack) {
-      JSContext *cx;
-
-      result = GetContextFromStack(stack, &cx);
-      if (cx) {
-        nsIScriptContext *scriptContext =
-          nsJSUtils::GetDynamicScriptContext(cx);
-
-        if (scriptContext) {
-          if (scriptContext->GetProcessingScriptTag()) {
-            // Now check to make sure that the script is running in our window,
-            // since we only want to replace if the location is set by a
-            // <script> tag in the same window.  See bug 178729.
-            nsCOMPtr<nsIScriptGlobalObject> ourGlobal(do_GetInterface(docShell));
-            inScriptTag = (ourGlobal == scriptContext->GetGlobalObject());
-          }
-        }  
-      } //cx
-    }  // stack
-
-    return SetURI(newUri, aReplace || inScriptTag);
   }
 
   return result;