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

docshell/base/nsDocShell.cpp

@@ -7964,7 +7964,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 " +

dom/base/nsGlobalWindow.cpp

@@ -6932,20 +6932,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>