Bug 615061 - Dispatch the hashchange event synchronously. r=smaug, a2.0=blocking
authorJustin Lebar <justin.lebar@gmail.com>
Mon, 29 Nov 2010 11:13:12 -0800
changeset 58340 50eb584944aa3fcf02d17726623f29db67833f26
parent 58339 1f53f85ddfb58ffe51330e52ca122cdcbc3e5222
child 58342 9a0741efda5ce4ce5541f75d8ed330a383f87543
push id1
push usershaver@mozilla.com
push dateTue, 04 Jan 2011 17:58:04 +0000
reviewerssmaug
bugs615061
milestone2.0b8pre
Bug 615061 - Dispatch the hashchange event synchronously. r=smaug, a2.0=blocking
docshell/base/nsDocShell.cpp
docshell/test/file_bug509055.html
docshell/test/test_bug385434.html
docshell/test/test_bug509055.html
dom/base/nsGlobalWindow.cpp
dom/base/nsGlobalWindow.h
dom/base/nsPIDOMWindow.h
dom/tests/mochitest/general/test_bug504220.html
--- a/docshell/base/nsDocShell.cpp
+++ b/docshell/base/nsDocShell.cpp
@@ -8332,17 +8332,17 @@ nsDocShell::InternalLoad(nsIURI * aURI,
             SetDocPendingStateObj(mOSHE);
 
             // Dispatch the popstate and hashchange events, as appropriate
             nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(mScriptGlobal);
             if (window) {
                 window->DispatchSyncPopState();
 
                 if (doHashchange)
-                  window->DispatchSyncHashchange();
+                  window->DispatchAsyncHashchange();
             }
 
             return NS_OK;
         }
     }
     
     // mContentViewer->PermitUnload can destroy |this| docShell, which
     // causes the next call of CanSavePresentation to crash. 
--- a/docshell/test/file_bug509055.html
+++ b/docshell/test/file_bug509055.html
@@ -1,9 +1,9 @@
 <!DOCTYPE HTML>
 <html>
 <head>
   <title>Test inner frame for bug 509055</title>
 </head>
-<body>
+<body onhashchange="hashchangeCallback()">
   file_bug509055.html
 </body>
 </html>
--- a/docshell/test/test_bug385434.html
+++ b/docshell/test/test_bug385434.html
@@ -20,17 +20,16 @@ https://bugzilla.mozilla.org/show_bug.cg
 <pre id="test">
 <script type="application/javascript;version=1.7">
 
 /** Test for Bug 385434 **/
 SimpleTest.waitForExplicitFinish();
 
 var gNumHashchanges = 0;
 var gCallbackOnIframeLoad = false;
-var gCallbackOnHashchange = false;
 
 function statusMsg(msg) {
   var msgElem = document.createElement("p");
   msgElem.appendChild(document.createTextNode(msg));
 
   document.getElementById("status").appendChild(msgElem);
 }
 
@@ -38,24 +37,17 @@ function longWait() {
   setTimeout(function() { gGen.next() }, 1000);
 }
 
 // onIframeHashchange, onIframeLoad, and onIframeScroll are all called by the
 // content we load into our iframe in order to notify the parent frame of an
 // event which was fired.
 function onIframeHashchange() {
   gNumHashchanges++;
-  if (gCallbackOnHashchange) {
-    gCallbackOnHashchange = false;
-    gGen.next();
-  }
-}
-
-function enableHashchangeCallback() {
-  gCallbackOnHashchange = true;
+  gGen.next();
 }
 
 function onIframeLoad() {
   if (gCallbackOnIframeLoad) {
     gCallbackOnIframeLoad = false;
     gGen.next();
   }
 }
@@ -116,45 +108,46 @@ function run_test() {
 
   enableIframeLoadCallback();
   frameCw.document.location = "file_bug385434_1.html";
   // Wait for the iframe to load and for our callback to fire
   yield;
 
   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:
   //   file_bug385434_1.html#link1
   //   file_bug385434_2.html
   //   file_bug385434_1.html#foo      <-- current page
   enableIframeLoadCallback();
   frameCw.window.location = "file_bug385434_2.html";
@@ -179,34 +172,34 @@ function run_test() {
    *     the event is targeted at the window object
    *     the event's cancelable, bubbles settings are correct
    */
   enableIframeLoadCallback();
   frameCw.document.location = "file_bug385434_2.html";
   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.");
   is(gSampleEvent.type, "hashchange",
      "Event type should be 'hashchange'.");
   is(gSampleEvent.cancelable, false,
      "The hashchange event shouldn't be cancelable.");
   is(gSampleEvent.bubbles, false,
      "The hashchange event shouldn't bubble.");
 
   /*
    * TEST 3 tests that:
    *     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 " +
                 "hasn't finished loading.");
 
   SimpleTest.finish();
   yield;
 }
--- a/docshell/test/test_bug509055.html
+++ b/docshell/test/test_bug509055.html
@@ -24,57 +24,72 @@ https://bugzilla.mozilla.org/show_bug.cg
 SimpleTest.waitForExplicitFinish();
 
 var gGen;
 
 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
+  dump("onChildHashchange() called.\n");
+  if(gGen)
+    gGen.next();
+}
+
 function onChildLoad(e) {
   if(gGen)
     gGen.next();
 }
 
 function runTest() {
   netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
 
   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
-
-  // Not sure why this wait is necessary, but without it, the change to
-  // location.hash below doesn't create a SHEntry or enable the back button.
-  shortWait();
+  dump('Waiting for initial load.\n');
   yield;
 
-  // Both setting location.hash and calling history.back() happen
-  // synchronously, so there's no need to yield here.
+  // Without this wait, the change to location.hash below doesn't create a
+  // SHEntry or enable the back button.
+  shortWait();
+  dump('Got initial load.  Spinning event loop.\n');
+  yield;
+
   popup.location.hash = "#1";
+  dump('Waiting for hashchange.\n');
+  yield;
+
   popup.history.back();
+  dump('Waiting for second hashchange.\n');
+  yield; // wait for hashchange
 
   popup.document.title = "Changed";
 
   // Wait for listeners to be notified of the title change.
   shortWait();
+  dump('Got second hashchange.  Spinning event loop.\n');
   yield;
 
   var sh = popup.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
                 .getInterface(Components.interfaces.nsIWebNavigation)
                 .sessionHistory;
 
   // Get the title of the inner popup's current SHEntry 
   var sheTitle = sh.getEntryAtIndex(sh.index, false).title;
   is(sheTitle, "Changed", "SHEntry's title should change when we change.");
 
   popup.close();
 
   SimpleTest.finish();
+  dump('Final yield.\n');
   yield;
 }
 
 window.addEventListener('load', function() {
   gGen = runTest();
   gGen.next();
 }, false);
 
--- a/dom/base/nsGlobalWindow.cpp
+++ b/dom/base/nsGlobalWindow.cpp
@@ -7501,21 +7501,30 @@ nsGlobalWindow::PageHidden()
   nsIFocusManager* fm = nsFocusManager::GetFocusManager();
   if (fm)
     fm->WindowHidden(this);
 
   mNeedsFocus = PR_TRUE;
 }
 
 nsresult
-nsGlobalWindow::DispatchSyncHashchange()
-{
-  FORWARD_TO_INNER(DispatchSyncHashchange, (), NS_OK);
-  NS_ASSERTION(nsContentUtils::IsSafeToRunScript(),
-               "Must be safe to run script here.");
+nsGlobalWindow::DispatchAsyncHashchange()
+{
+  FORWARD_TO_INNER(DispatchAsyncHashchange, (), NS_OK);
+
+  nsCOMPtr<nsIRunnable> event =
+    NS_NewRunnableMethod(this, &nsGlobalWindow::FireHashchange);
+
+  return NS_DispatchToCurrentThread(event);
+}
+
+nsresult
+nsGlobalWindow::FireHashchange()
+{
+  NS_ENSURE_TRUE(IsInnerWindow(), NS_ERROR_FAILURE);
 
   // Don't do anything if the window is frozen.
   if (IsFrozen())
     return NS_OK;
 
   // Dispatch the hashchange event, which doesn't bubble and isn't cancelable,
   // to the outer window.
   return nsContentUtils::DispatchTrustedEvent(mDoc, GetOuterWindow(),
--- a/dom/base/nsGlobalWindow.h
+++ b/dom/base/nsGlobalWindow.h
@@ -541,17 +541,17 @@ public:
 
   virtual NS_HIDDEN_(void)
     CacheXBLPrototypeHandler(nsXBLPrototypeHandler* aKey,
                              nsScriptObjectHolder& aHandler);
 
   virtual PRBool TakeFocus(PRBool aFocus, PRUint32 aFocusMethod);
   virtual void SetReadyForFocus();
   virtual void PageHidden();
-  virtual nsresult DispatchSyncHashchange();
+  virtual nsresult DispatchAsyncHashchange();
   virtual nsresult DispatchSyncPopState();
 
   virtual nsresult SetArguments(nsIArray *aArguments, nsIPrincipal *aOrigin);
 
   static PRBool DOMWindowDumpEnabled();
 
   void MaybeForgiveSpamCount();
   PRBool IsClosedOrClosing() {
@@ -696,16 +696,17 @@ protected:
                             PRBool *aFreeSecurityPass, JSContext **aCXused);
   PRBool PopupWhitelisted();
   PopupControlState RevisePopupAbuseLevel(PopupControlState);
   void     FireAbuseEvents(PRBool aBlocked, PRBool aWindow,
                            const nsAString &aPopupURL,
                            const nsAString &aPopupWindowName,
                            const nsAString &aPopupWindowFeatures);
   void FireOfflineStatusEvent();
+  nsresult FireHashchange();
 
   void FlushPendingNotifications(mozFlushType aType);
   void EnsureReflowFlushAndPaint();
   nsresult CheckSecurityWidthAndHeight(PRInt32* width, PRInt32* height);
   nsresult CheckSecurityLeftAndTop(PRInt32* left, PRInt32* top);
   static PRBool CanSetProperty(const char *aPrefName);
 
   static void MakeScriptDialogTitle(nsAString &aOutTitle);
--- a/dom/base/nsPIDOMWindow.h
+++ b/dom/base/nsPIDOMWindow.h
@@ -519,19 +519,20 @@ public:
 
   /**
    * Indicates that the page in the window has been hidden. This is used to
    * reset the focus state.
    */
   virtual void PageHidden() = 0;
 
   /**
-   * Instructs this window to synchronously dispatch a hashchange event.
+   * Instructs this window to asynchronously dispatch a hashchange event.  This
+   * method must be called on an inner window.
    */
-  virtual nsresult DispatchSyncHashchange() = 0;
+  virtual nsresult DispatchAsyncHashchange() = 0;
 
   /**
    * Instructs this window to synchronously dispatch a popState event.
    */
   virtual nsresult DispatchSyncPopState() = 0;
 
   /**
    * Tell this window that there is an observer for orientation changes
--- a/dom/tests/mochitest/general/test_bug504220.html
+++ b/dom/tests/mochitest/general/test_bug504220.html
@@ -18,42 +18,50 @@ https://bugzilla.mozilla.org/show_bug.cg
   <div id="status" style="display: none"></div>
 </div>
 <pre id="test">
 <script type="application/javascript;version=1.7">
 
 /** Test for Bug 504220 **/
 
 function run_test() {
-  SimpleTest.waitForExplicitFinish();
-
   ok("onhashchange" in document.body,
      "document.body should contain 'onhashchange'.");
 
   ok("onhashchange" in window, "window should contain 'onhashchange'.");
 
   // window.onhashchange should mirror document.body.onhashchange.
   var func = function() { return 42; };
   document.body.onhashchange = func;
   is(window.onhashchange, func);
 
   // Likewise, document.body.hashchange should mirror window.onhashchange
   numEvents = 0;
-  var func2 = function() { numEvents++; };
+  var func2 = function() { numEvents++; gGen.next() };
   window.onhashchange = func2;
   is(document.body.onhashchange, func2);
 
-  // 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";
+  SimpleTest.waitForExplicitFinish();
 
-  is(numEvents, 1, "Exactly one hashchange should have been fired.");
-  SimpleTest.finish();
+  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();
 }
 
 </script>
 </pre>
 </body>
 </html>