Bug 515190 - Hashchange event should be dispatched synchronously. r=smaug
authorJustin Lebar <justin.lebar+bug@gmail.com>
Thu, 26 Nov 2009 21:01:43 -0800
changeset 37368 c922c6402f7631b27cabb2ba769da5c880c1a43a
parent 37367 2c2c3169ebba3a0e912c4d0bf263498bd91543fb
child 37369 cc65a0921a1b9849cb805f987fa5243005a4e2be
push id11260
push userdgottwald@mozilla.com
push dateThu, 21 Jan 2010 13:30:20 +0000
treeherderautoland@cc65a0921a1b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs515190
milestone1.9.3a1pre
Bug 515190 - Hashchange event should be dispatched synchronously. r=smaug
docshell/base/nsDocShell.cpp
docshell/base/nsDocShell.h
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
@@ -7905,17 +7905,17 @@ nsDocShell::InternalLoad(nsIURI * aURI,
                     shEntry->SetTitle(mTitle);
             }
 
             if (doHashchange) {
                 nsCOMPtr<nsPIDOMWindow> window =
                     do_QueryInterface(mScriptGlobal);
 
                 if (window)
-                    window->DispatchAsyncHashchange();
+                    window->DispatchSyncHashchange();
             }
 
             return NS_OK;
         }
     }
     
     // mContentViewer->PermitUnload can destroy |this| docShell, which
     // causes the next call of CanSavePresentation to crash. 
--- a/docshell/base/nsDocShell.h
+++ b/docshell/base/nsDocShell.h
@@ -359,22 +359,16 @@ protected:
     // exists).  The channel will be suspended until the classification is
     // complete.
     nsresult CheckClassifier(nsIChannel *aChannel);
 
     nsresult ScrollIfAnchor(nsIURI * aURI, PRBool * aWasAnchor,
                             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
     // FireOnLocationChange is called.
     // In all other cases PR_FALSE is returned.
     PRBool OnLoadingSite(nsIChannel * aChannel,
                          PRBool aFireOnLocationChange,
                          PRBool aAddToGlobalHistory = PR_TRUE);
--- a/docshell/test/test_bug385434.html
+++ b/docshell/test/test_bug385434.html
@@ -20,16 +20,17 @@ 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);
 }
 
@@ -37,17 +38,24 @@ 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++;
-  gGen.next();
+  if (gCallbackOnHashchange) {
+    gCallbackOnHashchange = false;
+    gGen.next();
+  }
+}
+
+function enableHashchangeCallback() {
+  gCallbackOnHashchange = true;
 }
 
 function onIframeLoad() {
   if (gCallbackOnIframeLoad) {
     gCallbackOnIframeLoad = false;
     gGen.next();
   }
 }
@@ -108,46 +116,45 @@ 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";
@@ -172,34 +179,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.");
 
-  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,49 @@ 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
-  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();
   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";
 
   // Wait for listeners to be notified of the title change.
   shortWait();
   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;
   is(sheTitle, "Changed", "SHEntry's title should change when we change.");
 
   popup.close();
 
   SimpleTest.finish();
--- a/dom/base/nsGlobalWindow.cpp
+++ b/dom/base/nsGlobalWindow.cpp
@@ -6920,30 +6920,21 @@ nsGlobalWindow::PageHidden()
   nsIFocusManager* fm = nsFocusManager::GetFocusManager();
   if (fm)
     fm->WindowHidden(this);
 
   mNeedsFocus = PR_TRUE;
 }
 
 nsresult
-nsGlobalWindow::DispatchAsyncHashchange()
-{
-  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);
+nsGlobalWindow::DispatchSyncHashchange()
+{
+  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())
       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
@@ -438,17 +438,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 DispatchAsyncHashchange();
+  virtual nsresult DispatchSyncHashchange();
   virtual nsresult SetArguments(nsIArray *aArguments, nsIPrincipal *aOrigin);
 
   static PRBool DOMWindowDumpEnabled();
 
 protected:
   // Object Management
   virtual ~nsGlobalWindow();
   void CleanUp();
@@ -569,18 +569,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
@@ -449,20 +449,19 @@ 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 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;
 
 
   /**
    * Tell this window that there is an observer for orientation changes
    */
   virtual void SetHasOrientationEventListener() = 0;
 
   /**
--- a/dom/tests/mochitest/general/test_bug504220.html
+++ b/dom/tests/mochitest/general/test_bug504220.html
@@ -18,50 +18,42 @@ 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++; 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>
 </pre>
 </body>
 </html>