Bug 515190 - Hashchange event should be dispatched synchronously. r=smaug

docshell/base/nsDocShell.cpp

@@ -7910,7 +7910,7 @@ nsDocShell::InternalLoad(nsIURI * aURI,
                     do_QueryInterface(mScriptGlobal);
 
                 if (window)
-                    window->DispatchAsyncHashchange();
+                    window->DispatchSyncHashchange();
             }
 
             return NS_OK;

docshell/base/nsDocShell.h

@@ -364,12 +364,6 @@ protected:
                             PRUint32 aLoadType, nscoord *cx, nscoord *cy,
                             PRBool * aDoHashchange);
 
-    // Dispatches the hashchange event to the current thread, if the document's
-    // readystate is "complete".
-    nsresult DispatchAsyncHashchange();
-
-    nsresult FireHashchange();
-
     // Returns PR_TRUE if would have called FireOnLocationChange,
     // but did not because aFireOnLocationChange was false on entry.
     // In this case it is the caller's responsibility to ensure

docshell/test/test_bug385434.html

@@ -25,6 +25,7 @@ SimpleTest.waitForExplicitFinish();
 
 var gNumHashchanges = 0;
 var gCallbackOnIframeLoad = false;
+var gCallbackOnHashchange = false;
 
 function statusMsg(msg) {
   var msgElem = document.createElement("p");
@@ -42,7 +43,14 @@ function longWait() {
 // event which was fired.
 function onIframeHashchange() {
   gNumHashchanges++;
-  gGen.next();
+  if (gCallbackOnHashchange) {
+    gCallbackOnHashchange = false;
+    gGen.next();
+  }
+}
+
+function enableHashchangeCallback() {
+  gCallbackOnHashchange = true;
 }
 
 function onIframeLoad() {
@@ -113,36 +121,35 @@ function run_test() {
 
   noEventExpected("No hashchange expected initially.");
 
+  enableHashchangeCallback();
   sendMouseEvent({type: "click"}, "link1", frameCw);
   yield;
   eventExpected("Clicking link1 should trigger a hashchange.");
 
+  enableHashchangeCallback();
   sendMouseEvent({type: "click"}, "link1", frameCw);
   longWait();
   yield;
   // succeed if a hashchange event wasn't triggered while we were waiting
   noEventExpected("Clicking link1 again should not trigger a hashchange.");
 
+  enableHashchangeCallback();
   sendMouseEvent({type: "click"}, "link2", frameCw);
   yield;
   eventExpected("Clicking link2 should trigger a hashchange.");
 
   frameCw.history.go(-1);
-  yield;
   eventExpected("Going back should trigger a hashchange.");
 
   frameCw.history.go(1);
-  yield;
   eventExpected("Going forward should trigger a hashchange.");
 
   frameCw.window.location.hash = "";
-  yield;
   eventExpected("Changing window.location.hash should trigger a hashchange.");
 
   // window.location has a trailing '#' right now, so we append "link1", not
   // "#link1".
   frameCw.window.location = frameCw.window.location + "link1";
-  yield;
   eventExpected("Assigning to window.location should trigger a hashchange.");
 
   // Set up history in the iframe which looks like:
@@ -177,9 +184,8 @@ function run_test() {
   yield;
 
   frameCw.document.location = "file_bug385434_2.html#foo";
-  yield;
-
   eventExpected("frame onhashchange should fire events.");
+
   // iframe should set gSampleEvent
   is(gSampleEvent.target, frameCw,
      "The hashchange event should be targeted to the window.");
@@ -195,6 +201,7 @@ function run_test() {
    *     hashchange is dispatched if the current document readyState is
    *     not "complete" (bug 504837).
    */
+  enableHashchangeCallback();
   frameCw.document.location = "file_bug385434_3.html";
   yield;
   eventExpected("Hashchange should fire even if the document " +

docshell/test/test_bug509055.html

@@ -29,12 +29,6 @@ function shortWait() {
   setTimeout(function() { gGen.next(); }, 0, false);
 }
 
-function onChildHashchange(e) {
-  // gGen might be undefined when we refresh the page, so we have to check here
-  if(gGen)
-    gGen.next();
-}
-
 function onChildLoad(e) {
   if(gGen)
     gGen.next();
@@ -46,7 +40,6 @@ function runTest() {
   var popup = window.open("file_bug509055.html", "popup 0",
                            "height=200,width=200,location=yes," +
                            "menubar=yes,status=yes,toolbar=yes,dependent=yes");
-  popup.hashchangeCallback = onChildHashchange;
   popup.onload = onChildLoad;
   yield; // wait for load
 
@@ -55,11 +48,10 @@ function runTest() {
   shortWait();
   yield;
 
+  // Both setting location.hash and calling history.back() happen
+  // synchronously, so there's no need to yield here.
   popup.location.hash = "#1";
-  yield; // wait for hashchange
-
   popup.history.back();
-  yield; // wait for hashchange
 
   popup.document.title = "Changed";
 
@@ -68,8 +60,8 @@ function runTest() {
   yield;
 
   var sh = popup.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
-                 .getInterface(Components.interfaces.nsIWebNavigation)
-                 .sessionHistory;
+                .getInterface(Components.interfaces.nsIWebNavigation)
+                .sessionHistory;
 
   // Get the title of the inner popup's current SHEntry 
   var sheTitle = sh.getEntryAtIndex(sh.index, false).title;

dom/base/nsGlobalWindow.cpp

@@ -6925,20 +6925,11 @@ nsGlobalWindow::PageHidden()
 }
 
 nsresult
-nsGlobalWindow::DispatchAsyncHashchange()
+nsGlobalWindow::DispatchSyncHashchange()
 {
-  FORWARD_TO_INNER(DispatchAsyncHashchange, (), NS_OK);
-
-  nsCOMPtr<nsIRunnable> event =
-    NS_NEW_RUNNABLE_METHOD(nsGlobalWindow, this, FireHashchange);
-   
-  return NS_DispatchToCurrentThread(event);
-}
-
-nsresult
-nsGlobalWindow::FireHashchange()
-{
-  NS_ENSURE_TRUE(IsInnerWindow(), NS_ERROR_FAILURE);
+  FORWARD_TO_INNER(DispatchSyncHashchange, (), NS_OK);
+  NS_ASSERTION(nsContentUtils::IsSafeToRunScript(),
+               "Must be safe to run script here.");
 
   // Don't do anything if the window is frozen.
   if (IsFrozen())

dom/base/nsGlobalWindow.h

@@ -443,7 +443,7 @@ public:
   virtual PRBool TakeFocus(PRBool aFocus, PRUint32 aFocusMethod);
   virtual void SetReadyForFocus();
   virtual void PageHidden();
-  virtual nsresult DispatchAsyncHashchange();
+  virtual nsresult DispatchSyncHashchange();
   virtual nsresult SetArguments(nsIArray *aArguments, nsIPrincipal *aOrigin);
 
   static PRBool DOMWindowDumpEnabled();
@@ -574,8 +574,7 @@ protected:
                            const nsAString &aPopupWindowName,
                            const nsAString &aPopupWindowFeatures);
   void FireOfflineStatusEvent();
-  nsresult FireHashchange();
-  
+
   void FlushPendingNotifications(mozFlushType aType);
   void EnsureReflowFlushAndPaint();
   nsresult CheckSecurityWidthAndHeight(PRInt32* width, PRInt32* height);

dom/base/nsPIDOMWindow.h

@@ -454,10 +454,9 @@ public:
   virtual void PageHidden() = 0;
 
   /**
-   * Instructs this window to asynchronously dispatch a hashchange event.  This
-   * method must be called on an inner window.
+   * Instructs this window to synchronously dispatch a hashchange event.
    */
-  virtual nsresult DispatchAsyncHashchange() = 0;
+  virtual nsresult DispatchSyncHashchange() = 0;
 
 
   /**

dom/tests/mochitest/general/test_bug504220.html

@@ -23,6 +23,8 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=504220
 /** Test for Bug 504220 **/
 
 function run_test() {
+  SimpleTest.waitForExplicitFinish();
+
   ok("onhashchange" in document.body,
      "document.body should contain 'onhashchange'.");
 
@@ -35,30 +37,20 @@ function run_test() {
 
   // Likewise, document.body.hashchange should mirror window.onhashchange
   numEvents = 0;
-  var func2 = function() { numEvents++; gGen.next() };
+  var func2 = function() { numEvents++; };
   window.onhashchange = func2;
   is(document.body.onhashchange, func2);
 
-  SimpleTest.waitForExplicitFinish();
+  // Change the document's hash.  If we've been running this test manually,
+  // the hash might already be "#foo", so we need to check in order to be
+  // sure we trigger a hashchange.
+  if (location.hash != "#foo")
+    location.hash = "#foo";
+  else
+    location.hash = "#bar";
 
-  function waitForHashchange() {
-    // Change the document's hash.  If we've been running this test manually,
-    // the hash might already be "#foo", so we need to check in order to be
-    // sure we trigger a hashchange.
-    if (location.hash != "#foo")
-      location.hash = "#foo";
-    else
-      location.hash = "#bar";
-
-    yield;
-
-    is(numEvents, 1, "Exactly one hashchange should have been fired.");
-    SimpleTest.finish();
-    yield;
-  }
-
-  var gGen = waitForHashchange();
-  gGen.next();
+  is(numEvents, 1, "Exactly one hashchange should have been fired.");
+  SimpleTest.finish();
 }
 
 </script>