Bug 554155 - history.pushState should fire onLocationChange only once. r=smaug

docshell/base/nsDocShell.cpp

@@ -1044,6 +1044,9 @@ ConvertDocShellLoadInfoToLoadType(nsDocShellInfoLoadType aDocShellLoadType)
     case nsIDocShellLoadInfo::loadStopContentAndReplace:
         loadType = LOAD_STOP_CONTENT_AND_REPLACE;
         break;
+    case nsIDocShellLoadInfo::loadPushState:
+        loadType = LOAD_PUSHSTATE;
+        break;
     default:
         NS_NOTREACHED("Unexpected nsDocShellInfoLoadType value");
     }
@@ -1109,6 +1112,9 @@ nsDocShell::ConvertLoadTypeToDocShellLoadInfo(PRUint32 aLoadType)
     case LOAD_STOP_CONTENT_AND_REPLACE:
         docShellLoadType = nsIDocShellLoadInfo::loadStopContentAndReplace;
         break;
+    case LOAD_PUSHSTATE:
+        docShellLoadType = nsIDocShellLoadInfo::loadPushState;
+        break;
     default:
         NS_NOTREACHED("Unexpected load type value");
     }
@@ -9229,11 +9235,12 @@ nsDocShell::AddState(nsIVariant *aData, const nsAString& aTitle,
     // Default max length: 640k chars.
     PRInt32 maxStateObjSize = 0xA0000;
     if (mPrefs) {
-      mPrefs->GetIntPref("browser.history.maxStateObjectSize",
-                         &maxStateObjSize);
+        mPrefs->GetIntPref("browser.history.maxStateObjectSize",
+                           &maxStateObjSize);
+    }
+    if (maxStateObjSize < 0) {
+        maxStateObjSize = 0;
     }
-    if (maxStateObjSize < 0)
-      maxStateObjSize = 0;
     NS_ENSURE_TRUE(dataStr.Length() <= (PRUint32)maxStateObjSize,
                    NS_ERROR_ILLEGAL_VALUE);
 
@@ -9242,7 +9249,7 @@ nsDocShell::AddState(nsIVariant *aData, const nsAString& aTitle,
     nsCOMPtr<nsIURI> oldURI = mCurrentURI;
     nsCOMPtr<nsIURI> newURI;
     if (aURL.Length() == 0) {
-      newURI = mCurrentURI;
+        newURI = mCurrentURI;
     }
     else {
         // 2a: Resolve aURL relative to mURI
@@ -9326,7 +9333,7 @@ nsDocShell::AddState(nsIVariant *aData, const nsAString& aTitle,
     NS_ENSURE_TRUE(sessionHistory, NS_ERROR_FAILURE);
 
     nsCOMPtr<nsISHistoryInternal> shInternal =
-      do_QueryInterface(sessionHistory, &rv);
+        do_QueryInterface(sessionHistory, &rv);
     NS_ENSURE_SUCCESS(rv, rv);
 
     // Step 3: Create a new entry in the session history; this will erase
@@ -9383,21 +9390,25 @@ nsDocShell::AddState(nsIVariant *aData, const nsAString& aTitle,
     }
 
     // Step 6: If the document's URI changed, update document's URI and update
-    // global history
+    // global history.
+    //
+    // We need to call FireOnLocationChange so that the browser's address bar
+    // gets updated and the back button is enabled, but we only need to
+    // explicitly call FireOnLocationChange if we're not calling SetCurrentURI,
+    // since SetCurrentURI will call FireOnLocationChange for us.
     if (!equalURIs) {
         SetCurrentURI(newURI, nsnull, PR_TRUE);
         document->SetDocumentURI(newURI);
 
         AddToGlobalHistory(newURI, PR_FALSE, oldURI);
     }
+    else {
+        FireOnLocationChange(this, nsnull, mCurrentURI);
+    }
 
     // Try to set the title of the current history element
     if (mOSHE)
-      mOSHE->SetTitle(aTitle);
-
-    // We need this to ensure that the back button is enabled after a
-    // pushState, if it wasn't already enabled.
-    FireOnLocationChange(this, nsnull, mCurrentURI);
+        mOSHE->SetTitle(aTitle);
 
     return NS_OK;
 }

docshell/base/nsIDocShellLoadInfo.idl

@@ -93,6 +93,7 @@ interface nsIDocShellLoadInfo : nsISupports
     const long loadNormalBypassCache = 14;
     const long loadNormalBypassProxy = 15;
     const long loadNormalBypassProxyAndCache = 16;
+    const long loadPushState = 17;                 // history.pushState or replaceState
 
     /** Contains a load type as specified by the load* constants */
     attribute nsDocShellInfoLoadType loadType;

docshell/test/browser/Makefile.in

@@ -50,6 +50,7 @@ _BROWSER_TEST_FILES =	\
 		browser_bug441169.js \
 		browser_bug503832.js \
 		file_bug503832.html \
+		browser_bug554155.js \
 		$(NULL)
 
 # the tests below use FUEL, which is a Firefox-specific feature

docshell/test/browser/browser_bug554155.js

@@ -0,0 +1,32 @@
+function test() {
+  waitForExplicitFinish();
+
+  let tab = gBrowser.addTab("http://example.com");
+
+  tab.linkedBrowser.addEventListener('load', function() {
+    let numLocationChanges = 0;
+
+    let listener = {
+      onStateChange:    function() {},
+      onProgressChange: function() {},
+      onStatusChange:   function() {},
+      onSecurityChange: function() {},
+      onLocationChange: function() {
+        numLocationChanges++;
+      }
+    };
+
+    gBrowser.addTabsProgressListener(listener, Components.interfaces.nsIWebProgress.NOTIFY_ALL);
+
+    // pushState to a new URL (http://example.com/foo").  This should trigger
+    // exactly one LocationChange event.
+    tab.linkedBrowser.contentWindow.history.pushState(null, null, "foo");
+
+    executeSoon(function() {
+      is(numLocationChanges, 1,
+         "pushState should cause exactly one LocationChange event.");
+      finish();
+    });
+
+  }, true);
+}