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 id17252
push userjlebar@mozilla.com
push dateMon, 29 Nov 2010 19:14:27 +0000
treeherdermozilla-central@50eb584944aa [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs615061
milestone2.0b8pre
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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>