Bug 640387 - onhashchange should be fired when navigating between history entries created via history.pushState if the entries' URIs differ only in their hashes. r=smaug
authorJustin Lebar <justin.lebar@gmail.com>
Thu, 31 Mar 2011 12:35:04 -0400
changeset 68613 228d41c976007bd1da5b46b5356050b4eb744f68
parent 68612 a2e48e0c44bbc8aebdfe2d9a52c9757c3101e453
child 68614 84bf34708885882cf4db1d6826fcb6d60dd810fa
push idunknown
push userunknown
push dateunknown
reviewerssmaug
bugs640387
milestone6.0a1
Bug 640387 - onhashchange should be fired when navigating between history entries created via history.pushState if the entries' URIs differ only in their hashes. r=smaug
content/base/public/nsContentUtils.h
content/base/src/nsContentUtils.cpp
docshell/base/nsDocShell.cpp
docshell/base/nsDocShell.h
docshell/shistory/public/nsISHEntry.idl
docshell/shistory/src/nsSHEntry.cpp
docshell/shistory/src/nsSHEntry.h
docshell/test/Makefile.in
docshell/test/file_bug640387.html
docshell/test/test_bug385434.html
docshell/test/test_bug640387_1.html
docshell/test/test_bug640387_2.html
--- a/content/base/public/nsContentUtils.h
+++ b/content/base/public/nsContentUtils.h
@@ -1410,16 +1410,24 @@ public:
   static PRUint32 FilterDropEffect(PRUint32 aAction, PRUint32 aEffectAllowed);
 
   /**
    * Return true if aURI is a local file URI (i.e. file://).
    */
   static PRBool URIIsLocalFile(nsIURI *aURI);
 
   /**
+   * Given a URI, return set beforeHash to the part before the '#', and
+   * afterHash to the remainder of the URI, including the '#'.
+   */
+  static nsresult SplitURIAtHash(nsIURI *aURI,
+                                 nsACString &aBeforeHash,
+                                 nsACString &aAfterHash);
+
+  /**
    * Get the application manifest URI for this document.  The manifest URI
    * is specified in the manifest= attribute of the root element of the
    * document.
    *
    * @param aDocument The document that lists the manifest.
    * @param aURI The manifest URI.
    */
   static void GetOfflineAppManifest(nsIDocument *aDocument, nsIURI **aURI);
--- a/content/base/src/nsContentUtils.cpp
+++ b/content/base/src/nsContentUtils.cpp
@@ -5007,16 +5007,42 @@ nsContentUtils::URIIsLocalFile(nsIURI *a
   nsCOMPtr<nsINetUtil> util = do_QueryInterface(sIOService);
 
   return util && NS_SUCCEEDED(util->ProtocolHasFlags(aURI,
                                 nsIProtocolHandler::URI_IS_LOCAL_FILE,
                                 &isFile)) &&
          isFile;
 }
 
+nsresult
+nsContentUtils::SplitURIAtHash(nsIURI *aURI,
+                               nsACString &aBeforeHash,
+                               nsACString &aAfterHash)
+{
+  // See bug 225910 for why we can't do this using nsIURL.
+
+  aBeforeHash.Truncate();
+  aAfterHash.Truncate();
+
+  NS_ENSURE_ARG_POINTER(aURI);
+
+  nsCAutoString spec;
+  nsresult rv = aURI->GetSpec(spec);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  PRInt32 index = spec.FindChar('#');
+  if (index == -1) {
+    index = spec.Length();
+  }
+
+  aBeforeHash.Assign(Substring(spec, 0, index));
+  aAfterHash.Assign(Substring(spec, index));
+  return NS_OK;
+}
+
 /* static */
 nsIScriptContext*
 nsContentUtils::GetContextForEventHandlers(nsINode* aNode,
                                            nsresult* aRv)
 {
   *aRv = NS_OK;
   nsIDocument* ownerDoc = aNode->GetOwnerDoc();
   if (!ownerDoc) {
--- a/docshell/base/nsDocShell.cpp
+++ b/docshell/base/nsDocShell.cpp
@@ -210,16 +210,17 @@
 #include "nsIWebBrowserChromeFocus.h"
 
 #if NS_PRINT_PREVIEW
 #include "nsIDocumentViewerPrint.h"
 #include "nsIWebBrowserPrint.h"
 #endif
 
 #include "nsPluginError.h"
+#include "nsContentUtils.h"
 
 static NS_DEFINE_CID(kDOMScriptObjectFactoryCID,
                      NS_DOM_SCRIPT_OBJECT_FACTORY_CID);
 static NS_DEFINE_CID(kAppShellCID, NS_APPSHELL_CID);
 
 #if defined(DEBUG_bryner) || defined(DEBUG_chb)
 //#define DEBUG_DOCSHELL_FOCUS
 #define DEBUG_PAGE_CACHE
@@ -8210,84 +8211,95 @@ nsDocShell::InternalLoad(nsIURI * aURI,
         // reset loadType so we don't have to add lots of tests for
         // LOAD_NORMAL_EXTERNAL after this point
         aLoadType = LOAD_NORMAL;
     }
 
     mAllowKeywordFixup =
       (aFlags & INTERNAL_LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP) != 0;
     mURIResultedInDocument = PR_FALSE;  // reset the clock...
-   
-    //
-    // First:
-    // Check to see if the new URI is an anchor in the existing document.
-    // Skip this check if we're doing some sort of abnormal load, if the
-    // new load is a non-history load and has postdata, or if we're doing
-    // a history load and the page identifiers of mOSHE and aSHEntry
-    // don't match.
-    //
-    PRBool allowScroll = PR_TRUE;
-    if (!aSHEntry) {
-        allowScroll = (aPostData == nsnull);
-    } else if (mOSHE) {
-        PRUint32 ourPageIdent;
-        mOSHE->GetPageIdentifier(&ourPageIdent);
-        PRUint32 otherPageIdent;
-        aSHEntry->GetPageIdentifier(&otherPageIdent);
-        allowScroll = (ourPageIdent == otherPageIdent);
-#ifdef DEBUG
-        if (allowScroll) {
-            nsCOMPtr<nsIInputStream> currentPostData;
-            mOSHE->GetPostData(getter_AddRefs(currentPostData));
-            NS_ASSERTION(currentPostData == aPostData,
-                         "Different POST data for entries for the same page?");
-        }
-#endif
-    }
 
     if (aLoadType == LOAD_NORMAL ||
         aLoadType == LOAD_STOP_CONTENT ||
         LOAD_TYPE_HAS_FLAGS(aLoadType, LOAD_FLAGS_REPLACE_HISTORY) ||
         aLoadType == LOAD_HISTORY ||
         aLoadType == LOAD_LINK) {
 
-        PRBool wasAnchor = PR_FALSE;
-        PRBool doHashchange = PR_FALSE;
-
-        // Get the position of the scrollers.
-        nscoord cx = 0, cy = 0;
-        GetCurScrollPos(ScrollOrientation_X, &cx);
-        GetCurScrollPos(ScrollOrientation_Y, &cy);
-
-        if (allowScroll) {
-            NS_ENSURE_SUCCESS(ScrollIfAnchor(aURI, &wasAnchor, aLoadType,
-                                             &doHashchange),
-                              NS_ERROR_FAILURE);
-        }
-
-        // If this is a history load, aSHEntry will have document identifier X
-        // if it was created as a result of a History.pushState() from a
-        // SHEntry with doc ident X, or if it was created by changing the hash
-        // of the URI corresponding to a SHEntry with doc ident X.
+        // Split mCurrentURI and aURI on the '#' character.  Make sure we read
+        // the return values of SplitURIAtHash; it might fail because
+        // mCurrentURI is null, for instance, and we don't want to allow a
+        // short-circuited navigation in that case.
+        nsCAutoString curBeforeHash, curHash, newBeforeHash, newHash;
+        nsresult splitRv1, splitRv2;
+        splitRv1 = nsContentUtils::SplitURIAtHash(mCurrentURI, curBeforeHash, curHash);
+        splitRv2 = nsContentUtils::SplitURIAtHash(aURI, newBeforeHash, newHash);
+
+        PRBool sameExceptHashes = NS_SUCCEEDED(splitRv1) &&
+                                  NS_SUCCEEDED(splitRv2) &&
+                                  curBeforeHash.Equals(newBeforeHash);
+
         PRBool sameDocIdent = PR_FALSE;
         if (mOSHE && aSHEntry) {
-          PRUint64 ourDocIdent, otherDocIdent;
-          mOSHE->GetDocIdentifier(&ourDocIdent);
-          aSHEntry->GetDocIdentifier(&otherDocIdent);
-          sameDocIdent = (ourDocIdent == otherDocIdent);
-        }
-
-        // Do a short-circuited load if the new URI differs from the current
-        // URI only in the hash, or if the two entries belong to the same
-        // document and don't point to the same object.
+            // We're doing a history load.
+
+            PRUint64 ourDocIdent, otherDocIdent;
+            mOSHE->GetDocIdentifier(&ourDocIdent);
+            aSHEntry->GetDocIdentifier(&otherDocIdent);
+            sameDocIdent = (ourDocIdent == otherDocIdent);
+
+#ifdef DEBUG
+            if (sameDocIdent) {
+                nsCOMPtr<nsIInputStream> currentPostData;
+                mOSHE->GetPostData(getter_AddRefs(currentPostData));
+                NS_ASSERTION(currentPostData == aPostData,
+                             "Different POST data for entries for the same page?");
+            }
+#endif
+        }
+
+        // A short-circuited load happens when we navigate between two SHEntries
+        // for the same document.  We do a short-circuited load under two
+        // circumstances.  Either
+        //
+        //  a) we're navigating between two different SHEntries which have the
+        //     same document identifiers, or
+        //
+        //  b) we're navigating to a new shentry whose URI differs from the
+        //     current URI only in its hash, the new hash is non-empty, and
+        //     we're not doing a POST.
         //
-        // (If we didn't check that the SHEntries are different objects,
-        // history.go(0) would short-circuit instead of triggering a true
-        // load, and we wouldn't dispatch an onload event to the page.)
-        if (wasAnchor || (sameDocIdent && (mOSHE != aSHEntry))) {
+        // The restriction tha the SHEntries in (a) must be different ensures
+        // that history.go(0) and the like trigger full refreshes, rather than
+        // short-circuited loads.
+        PRBool doShortCircuitedLoad = (sameDocIdent && mOSHE != aSHEntry) ||
+                                      (!aSHEntry && aPostData == nsnull &&
+                                       sameExceptHashes && !newHash.IsEmpty());
+
+        // Fire a hashchange event if we're doing a short-circuited load and the
+        // URIs differ only in their hashes.
+        PRBool doHashchange = doShortCircuitedLoad &&
+                              sameExceptHashes &&
+                              !curHash.Equals(newHash);
+
+        if (doShortCircuitedLoad) {
+            // Save the position of the scrollers.
+            nscoord cx = 0, cy = 0;
+            GetCurScrollPos(ScrollOrientation_X, &cx);
+            GetCurScrollPos(ScrollOrientation_Y, &cy);
+
+            // We scroll the window precisely when we fire a hashchange event.
+            if (doHashchange) {
+                // Take the '#' off the hashes before passing them to
+                // ScrollToAnchor.
+                nsDependentCSubstring curHashName(curHash, 1);
+                nsDependentCSubstring newHashName(newHash, 1);
+                rv = ScrollToAnchor(curHashName, newHashName, aLoadType);
+                NS_ENSURE_SUCCESS(rv, rv);
+            }
+
             mLoadType = aLoadType;
             mURIResultedInDocument = PR_TRUE;
 
             /* we need to assign mLSHE to aSHEntry right here, so that on History loads,
              * SetCurrentURI() called from OnNewURI() will send proper
              * onLocationChange() notifications to the browser to update
              * back/forward buttons.
              */
@@ -8302,44 +8314,33 @@ nsDocShell::InternalLoad(nsIURI * aURI,
                 mOSHE->GetOwner(getter_AddRefs(owner));
             }
             // Pass true for aCloneSHChildren, since we're not
             // changing documents here, so all of our subframes are
             // still relevant to the new session history entry.
             OnNewURI(aURI, nsnull, owner, mLoadType, PR_TRUE, PR_TRUE, PR_TRUE);
 
             nsCOMPtr<nsIInputStream> postData;
-            PRUint32 pageIdent = PR_UINT32_MAX;
+            PRUint64 docIdent = PRUint64(-1);
             nsCOMPtr<nsISupports> cacheKey;
 
             if (mOSHE) {
                 /* save current position of scroller(s) (bug 59774) */
                 mOSHE->SetScrollPosition(cx, cy);
                 // Get the postdata and page ident from the current page, if
                 // the new load is being done via normal means.  Note that
                 // "normal means" can be checked for just by checking for
                 // LOAD_CMD_NORMAL, given the loadType and allowScroll check
                 // above -- it filters out some LOAD_CMD_NORMAL cases that we
                 // wouldn't want here.
                 if (aLoadType & LOAD_CMD_NORMAL) {
                     mOSHE->GetPostData(getter_AddRefs(postData));
-                    mOSHE->GetPageIdentifier(&pageIdent);
+                    mOSHE->GetDocIdentifier(&docIdent);
                     mOSHE->GetCacheKey(getter_AddRefs(cacheKey));
                 }
-
-                if (mLSHE && wasAnchor) {
-                    // If it's an anchor load, set mLSHE's doc identifier to
-                    // mOSHE's doc identifier -- These are the same documents,
-                    // as far as HTML5 is concerned.
-                    PRUint64 docIdent;
-                    rv = mOSHE->GetDocIdentifier(&docIdent);
-                    if (NS_SUCCEEDED(rv)) {
-                        mLSHE->SetDocIdentifier(docIdent);
-                    }
-                }
             }
 
             /* Assign mOSHE to mLSHE. This will either be a new entry created
              * by OnNewURI() for normal loads or aSHEntry for history loads.
              */
             if (mLSHE) {
                 SetHistoryEntry(&mOSHE, mLSHE);
                 // Save the postData obtained from the previous page
@@ -8349,20 +8350,20 @@ nsDocShell::InternalLoad(nsIURI * aURI,
                 if (postData)
                     mOSHE->SetPostData(postData);
 
                 // Make sure we won't just repost without hitting the
                 // cache first
                 if (cacheKey)
                     mOSHE->SetCacheKey(cacheKey);
 
-                // Propagate our page ident to the new mOSHE so that
-                // we'll know it just differed by a scroll on the page.
-                if (pageIdent != PR_UINT32_MAX)
-                    mOSHE->SetPageIdentifier(pageIdent);
+                // Propagate our document identifier to the new mOSHE so that
+                // we'll know it's related by an anchor navigation or pushState.
+                if (docIdent != PRUint64(-1))
+                    mOSHE->SetDocIdentifier(docIdent);
             }
 
             /* restore previous position of scroller(s), if we're moving
              * back in history (bug 59774)
              */
             if (mOSHE && (aLoadType == LOAD_HISTORY || aLoadType == LOAD_RELOAD_NORMAL))
             {
                 nscoord bx, by;
@@ -9047,149 +9048,50 @@ nsresult nsDocShell::DoChannelLoad(nsICh
                              (mLoadType == LOAD_LINK),
                              this);
     NS_ENSURE_SUCCESS(rv, rv);
 
     return NS_OK;
 }
 
 nsresult
-nsDocShell::ScrollIfAnchor(nsIURI * aURI, PRBool * aWasAnchor,
-                           PRUint32 aLoadType, PRBool * aDoHashchange)
-{
-    NS_ASSERTION(aURI, "null uri arg");
-    NS_ASSERTION(aWasAnchor, "null anchor arg");
-    NS_PRECONDITION(aDoHashchange, "null hashchange arg");
-
-    if (!aURI || !aWasAnchor) {
-        return NS_ERROR_FAILURE;
-    }
-
-    *aWasAnchor = PR_FALSE;
-    *aDoHashchange = PR_FALSE;
-
+nsDocShell::ScrollToAnchor(nsACString & aCurHash, nsACString & aNewHash,
+                           PRUint32 aLoadType)
+{
     if (!mCurrentURI) {
         return NS_OK;
     }
 
     nsCOMPtr<nsIPresShell> shell;
     nsresult rv = GetPresShell(getter_AddRefs(shell));
     if (NS_FAILED(rv) || !shell) {
         // If we failed to get the shell, or if there is no shell,
         // nothing left to do here.
-        
         return rv;
     }
 
-    // NOTE: we assume URIs are absolute for comparison purposes
-
-    nsCAutoString currentSpec;
-    NS_ENSURE_SUCCESS(mCurrentURI->GetSpec(currentSpec),
-                      NS_ERROR_FAILURE);
-
-    nsCAutoString newSpec;
-    NS_ENSURE_SUCCESS(aURI->GetSpec(newSpec), NS_ERROR_FAILURE);
-
-    // Search for hash marks in the current URI and the new URI and
-    // take a copy of everything to the left of the hash for
-    // comparison.
-
-    const char kHash = '#';
-
-    // Split the new URI into a left and right part
-    // (assume we're parsing it out right)
-    nsACString::const_iterator urlStart, urlEnd, refStart, refEnd;
-    newSpec.BeginReading(urlStart);
-    newSpec.EndReading(refEnd);
-    
-    PRInt32 hashNew = newSpec.FindChar(kHash);
-    if (hashNew == 0) {
-        return NS_OK;           // Strange URI
-    }
-
-    if (hashNew > 0) {
-        // found it
-        urlEnd = urlStart;
-        urlEnd.advance(hashNew);
-        
-        refStart = urlEnd;
-        ++refStart;             // advanced past '#'
-        
-    }
-    else {
-        // no hash at all
-        urlEnd = refStart = refEnd;
-    }
-    const nsACString& sNewLeft = Substring(urlStart, urlEnd);
-    const nsACString& sNewRef =  Substring(refStart, refEnd);
-                                          
-    // Split the current URI in a left and right part
-    nsACString::const_iterator currentLeftStart, currentLeftEnd,
-                               currentRefStart, currentRefEnd;
-    currentSpec.BeginReading(currentLeftStart);
-    currentSpec.EndReading(currentRefEnd);
-
-    PRInt32 hashCurrent = currentSpec.FindChar(kHash);
-    if (hashCurrent == 0) {
-        return NS_OK;           // Strange URI 
-    }
-
-    if (hashCurrent > 0) {
-        currentLeftEnd = currentLeftStart;
-        currentLeftEnd.advance(hashCurrent);
-
-        currentRefStart = currentLeftEnd;
-        ++currentRefStart; // advance past '#'
-    }
-    else {
-        // no hash at all in currentSpec
-        currentLeftEnd = currentRefStart = currentRefEnd;
-    }
-
     // If we have no new anchor, we do not want to scroll, unless there is a
     // current anchor and we are doing a history load.  So return if we have no
     // new anchor, and there is no current anchor or the load is not a history
     // load.
-    NS_ASSERTION(hashNew != 0 && hashCurrent != 0,
-                 "What happened to the early returns above?");
-    if (hashNew == kNotFound &&
-        (hashCurrent == kNotFound || aLoadType != LOAD_HISTORY)) {
+    if ((aCurHash.IsEmpty() || aLoadType != LOAD_HISTORY) &&
+        aNewHash.IsEmpty()) {
         return NS_OK;
     }
 
-    // Compare the URIs.
-    //
-    // NOTE: this is a case sensitive comparison because some parts of the
-    // URI are case sensitive, and some are not. i.e. the domain name
-    // is case insensitive but the the paths are not.
-    //
-    // This means that comparing "http://www.ABC.com/" to "http://www.abc.com/"
-    // will fail this test.
-
-    if (!Substring(currentLeftStart, currentLeftEnd).Equals(sNewLeft)) {
-        return NS_OK;           // URIs not the same
-    }
-
-    // Now we know we are dealing with an anchor
-    *aWasAnchor = PR_TRUE;
-
-    // We should fire a hashchange event once we're done here if the two hashes
-    // are different.
-    *aDoHashchange = !Substring(currentRefStart, currentRefEnd).Equals(sNewRef);
-
     // Both the new and current URIs refer to the same page. We can now
     // browse to the hash stored in the new URI.
 
-    if (!sNewRef.IsEmpty()) {
+    if (!aNewHash.IsEmpty()) {
         // anchor is there, but if it's a load from history,
         // we don't have any anchor jumping to do
         PRBool scroll = aLoadType != LOAD_HISTORY &&
                         aLoadType != LOAD_RELOAD_NORMAL;
 
-        char *str = ToNewCString(sNewRef);
+        char *str = ToNewCString(aNewHash);
         if (!str) {
             return NS_ERROR_OUT_OF_MEMORY;
         }
 
         // nsUnescape modifies the string that is passed into it.
         nsUnescape(str);
 
         // We assume that the bytes are in UTF-8, as it says in the
@@ -9221,17 +9123,17 @@ nsDocShell::ScrollIfAnchor(nsIURI * aURI
             nsCOMPtr<nsITextToSubURI> textToSubURI =
                 do_GetService(NS_ITEXTTOSUBURI_CONTRACTID, &rv);
             NS_ENSURE_SUCCESS(rv, rv);
 
             // Unescape and convert to unicode
             nsXPIDLString uStr;
 
             rv = textToSubURI->UnEscapeAndConvert(PromiseFlatCString(aCharset).get(),
-                                                  PromiseFlatCString(sNewRef).get(),
+                                                  PromiseFlatCString(aNewHash).get(),
                                                   getter_Copies(uStr));
             NS_ENSURE_SUCCESS(rv, rv);
 
             // Ignore return value of GoToAnchor, since it will return an error
             // if there is no such anchor in the document, which is actually a
             // success condition for us (we want to update the session history
             // with the new URI no matter whether we actually scrolled
             // somewhere).
--- a/docshell/base/nsDocShell.h
+++ b/docshell/base/nsDocShell.h
@@ -327,18 +327,18 @@ protected:
                                PRBool aBypassClassifier,
                                PRBool aForceAllowCookies);
     NS_IMETHOD AddHeadersToChannel(nsIInputStream * aHeadersData, 
                                   nsIChannel * aChannel);
     virtual nsresult DoChannelLoad(nsIChannel * aChannel,
                                    nsIURILoader * aURILoader,
                                    PRBool aBypassClassifier);
 
-    nsresult ScrollIfAnchor(nsIURI * aURI, PRBool * aWasAnchor,
-                            PRUint32 aLoadType, PRBool * aDoHashchange);
+    nsresult ScrollToAnchor(nsACString & curHash, nsACString & newHash,
+                            PRUint32 aLoadType);
 
     // Tries to stringify a given variant by converting it to JSON.  This only
     // works if the variant is backed by a JSVal.
     nsresult StringifyJSValVariant(JSContext *aCx, nsIVariant *aData,
                                    nsAString &aResult);
 
     // Returns PR_TRUE if would have called FireOnLocationChange,
     // but did not because aFireOnLocationChange was false on entry.
--- a/docshell/shistory/public/nsISHEntry.idl
+++ b/docshell/shistory/public/nsISHEntry.idl
@@ -53,17 +53,17 @@ interface nsISupportsArray;
 %{C++
 struct nsIntRect;
 class nsDocShellEditorData;
 %}
 [ref] native nsIntRect(nsIntRect);
 [ptr] native nsDocShellEditorDataPtr(nsDocShellEditorData);
 
 
-[scriptable, uuid(39b73c3a-48eb-4189-8069-247279c3c42d)]
+[scriptable, uuid(5f3ebf43-6944-45fb-b1b1-78a05bf9370b)]
 interface nsISHEntry : nsIHistoryEntry
 {
     /** URI for the document */
     void setURI(in nsIURI aURI);
 
     /** Referrer URI */
     attribute nsIURI referrerURI;
 
@@ -135,31 +135,21 @@ interface nsISHEntry : nsIHistoryEntry
 
     /**
      * An ID to help identify this entry from others during
      * subframe navigation
      */
     attribute unsigned long ID;
 
     /**
-     * pageIdentifier is an integer that should be the same for two entries
-     * attached to the same docshell only if the two entries are entries for
-     * the same page in the sense that one could go from the state represented
-     * by one to the state represented by the other simply by scrolling (so the
-     * entries are separated by an anchor traversal or a subframe navigation in
-     * some other frame).
-     */
-    attribute unsigned long pageIdentifier;
-
-    /**
      * docIdentifier is an integer that should be the same for two entries
      * attached to the same docshell if and only if the two entries are entries
      * for the same document.  In practice, two entries A and B will have the
-     * same docIdentifier if they have the same pageIdentifier or if B was
-     * created by A calling history.pushState().
+     * same docIdentifier if we arrived at B by clicking an anchor link in A or
+     * if B was created by A's calling history.pushState().
      */
     attribute unsigned long long docIdentifier;
 
     /**
      * Changes this entry's doc identifier to a new value which is unique
      * among those of all other entries.
      */
     void setUniqueDocIdentifier();
--- a/docshell/shistory/src/nsSHEntry.cpp
+++ b/docshell/shistory/src/nsSHEntry.cpp
@@ -100,17 +100,16 @@ static void StopTrackingEntry(nsSHEntry 
 //*****************************************************************************
 //***    nsSHEntry: Object Management
 //*****************************************************************************
 
 
 nsSHEntry::nsSHEntry() 
   : mLoadType(0)
   , mID(gEntryID++)
-  , mPageIdentifier(mID)
   , mDocIdentifier(gEntryDocIdentifier++)
   , mScrollPositionX(0)
   , mScrollPositionY(0)
   , mIsFrameNavigation(PR_FALSE)
   , mSaveLayoutState(PR_TRUE)
   , mExpired(PR_FALSE)
   , mSticky(PR_TRUE)
   , mDynamicallyCreated(PR_FALSE)
@@ -125,17 +124,16 @@ nsSHEntry::nsSHEntry(const nsSHEntry &ot
   : mURI(other.mURI)
   , mReferrerURI(other.mReferrerURI)
   // XXX why not copy mDocument?
   , mTitle(other.mTitle)
   , mPostData(other.mPostData)
   , mLayoutHistoryState(other.mLayoutHistoryState)
   , mLoadType(0)         // XXX why not copy?
   , mID(other.mID)
-  , mPageIdentifier(other.mPageIdentifier)
   , mDocIdentifier(other.mDocIdentifier)
   , mScrollPositionX(0)  // XXX why not copy?
   , mScrollPositionY(0)  // XXX why not copy?
   , mIsFrameNavigation(other.mIsFrameNavigation)
   , mSaveLayoutState(other.mSaveLayoutState)
   , mExpired(other.mExpired)
   , mSticky(PR_TRUE)
   , mDynamicallyCreated(other.mDynamicallyCreated)
@@ -386,28 +384,16 @@ NS_IMETHODIMP nsSHEntry::GetID(PRUint32 
 }
 
 NS_IMETHODIMP nsSHEntry::SetID(PRUint32  aID)
 {
   mID = aID;
   return NS_OK;
 }
 
-NS_IMETHODIMP nsSHEntry::GetPageIdentifier(PRUint32 * aResult)
-{
-  *aResult = mPageIdentifier;
-  return NS_OK;
-}
-
-NS_IMETHODIMP nsSHEntry::SetPageIdentifier(PRUint32 aPageIdentifier)
-{
-  mPageIdentifier = aPageIdentifier;
-  return NS_OK;
-}
-
 NS_IMETHODIMP nsSHEntry::GetDocIdentifier(PRUint64 * aResult)
 {
   *aResult = mDocIdentifier;
   return NS_OK;
 }
 
 NS_IMETHODIMP nsSHEntry::SetDocIdentifier(PRUint64 aDocIdentifier)
 {
--- a/docshell/shistory/src/nsSHEntry.h
+++ b/docshell/shistory/src/nsSHEntry.h
@@ -94,17 +94,16 @@ private:
   nsCOMPtr<nsIContentViewer>      mContentViewer;
   nsCOMPtr<nsIDocument>           mDocument; // document currently being observed
   nsString                        mTitle;
   nsCOMPtr<nsIInputStream>        mPostData;
   nsCOMPtr<nsILayoutHistoryState> mLayoutHistoryState;
   nsCOMArray<nsISHEntry>          mChildren;
   PRUint32                        mLoadType;
   PRUint32                        mID;
-  PRUint32                        mPageIdentifier;
   PRInt64                         mDocIdentifier;
   PRInt32                         mScrollPositionX;
   PRInt32                         mScrollPositionY;
   PRPackedBool                    mIsFrameNavigation;
   PRPackedBool                    mSaveLayoutState;
   PRPackedBool                    mExpired;
   PRPackedBool                    mSticky;
   PRPackedBool                    mDynamicallyCreated;
--- a/docshell/test/Makefile.in
+++ b/docshell/test/Makefile.in
@@ -92,16 +92,19 @@ include $(topsrcdir)/config/rules.mk
 		file_bug580069_2.sjs \
 		test_bug590573.html \
 		file_bug590573_1.html \
 		file_bug590573_2.html \
 		test_bug598895.html \
 		test_bug634834.html \
 		file_bug634834.html \
 		test_bug637644.html \
+		test_bug640387_1.html \
+		test_bug640387_2.html \
+		file_bug640387.html \
 		test_framedhistoryframes.html \
 		test_windowedhistoryframes.html \
 		historyframes.html \
 		$(NULL)
 
 ifeq ($(MOZ_WIDGET_TOOLKIT),cocoa)
 _TEST_FILES += \
 		test_bug511449.html \
new file mode 100644
--- /dev/null
+++ b/docshell/test/file_bug640387.html
@@ -0,0 +1,26 @@
+<html>
+<body onhashchange='hashchange()' onload='load()' onpopstate='popstate()'>
+
+<script>
+function hashchange() {
+  var f = (opener || parent).childHashchange;
+  if (f)
+    f();
+}
+
+function load() {
+  var f = (opener || parent).childLoad;
+  if (f)
+    f();
+}
+
+function popstate() {
+  var f = (opener || parent).childPopstate;
+  if (f)
+    f();
+}
+</script>
+
+Not much to see here...
+</body>
+</html>
--- a/docshell/test/test_bug385434.html
+++ b/docshell/test/test_bug385434.html
@@ -130,20 +130,16 @@ function run_test() {
   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
new file mode 100644
--- /dev/null
+++ b/docshell/test/test_bug640387_1.html
@@ -0,0 +1,110 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=640387
+-->
+<head>
+  <title>Test for Bug 640387</title>
+  <script type="application/javascript" src="/MochiKit/packed.js"></script>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=640387">Mozilla Bug 640387</a>
+
+<script type='application/javascript;version=1.7'>
+SimpleTest.waitForExplicitFinish();
+
+function test() {
+  /* Spin the event loop so we get out of the onload handler. */
+  SimpleTest.executeSoon(function() { gGen.next() });
+  yield;
+
+  popup.history.pushState('', '', '#hash1');
+  popup.history.pushState('', '', '#hash2');
+
+  // Now the history looks like:
+  //   file_bug640387.html
+  //   file_bug640387.html#hash1
+  //   file_bug640387.html#hash2  <-- current
+
+  // Going back should trigger a hashchange, which will wake us up from the
+  // yield.
+  popup.history.back();
+  yield;
+  ok(true, 'Got first hashchange.');
+
+  // Going back should wake us up again.
+  popup.history.back();
+  yield;
+  ok(true, 'Got second hashchange.');
+
+  // Now the history looks like:
+  //   file_bug640387.html        <-- current
+  //   file_bug640387.html#hash1
+  //   file_bug640387.html#hash2
+
+  // Going forward should trigger a hashchange.
+  popup.history.forward();
+  yield;
+  ok(true, 'Got third hashchange.');
+
+  // Now modify the history so it looks like:
+  //   file_bug640387.html
+  //   file_bug640387.html#hash1
+  //   file_bug640387.html#hash1  <-- current
+  popup.history.pushState('', '', '#hash1');
+
+  // Now when we go back, we should not get a hashchange.  Instead, wait for a
+  // popstate.  We need to asynchronously go back because popstate is fired
+  // sync.
+  gHashchangeExpected = false;
+  gCallbackOnPopstate = true;
+  SimpleTest.executeSoon(function() { popup.history.back() });
+  yield;
+  ok(true, 'Got popstate.');
+  gCallbackOnPopstate = false;
+
+  // Spin the event loop so hashchange has a chance to fire, if it's going to.
+  SimpleTest.executeSoon(function() { gGen.next() });
+  yield;
+
+  popup.close();
+  SimpleTest.finish();
+  yield;
+}
+
+gGen = null;
+function childLoad() {
+  gGen = test();
+  gGen.next();
+}
+
+gHashchangeExpected = true;
+function childHashchange() {
+  if (gHashchangeExpected) {
+    gGen.next();
+  }
+  else {
+    ok(false, "Got hashchange when we weren't expecting one.");
+  }
+}
+
+gCallbackOnPopstate = false;
+function childPopstate() {
+  if (gCallbackOnPopstate) {
+    gGen.next();
+  }
+}
+
+/* We need to run this test in a popup, because navigating an iframe
+ * back/forwards tends to cause intermittent orange. */
+popup = window.open('file_bug640387.html');
+
+/* Control now flows up to childLoad(), called once the popup loads. */
+
+</script>
+
+</body>
+</html>
new file mode 100644
--- /dev/null
+++ b/docshell/test/test_bug640387_2.html
@@ -0,0 +1,91 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=640387
+-->
+<head>
+  <title>Test for Bug 640387</title>
+  <script type="application/javascript" src="/MochiKit/packed.js"></script>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=640387">Mozilla Bug 640387</a>
+
+<!-- Test that, when going from
+
+  http://example.com/#foo
+
+to
+
+  http://example.com/
+
+via a non-history load, we do a true load, rather than a scroll. -->
+
+<script type='application/javascript;version=1.7'>
+SimpleTest.waitForExplicitFinish();
+
+callbackOnLoad = false;
+function childLoad() {
+  if (callbackOnLoad) {
+    callbackOnLoad = false;
+    gGen.next();
+  }
+}
+
+errorOnHashchange = false;
+callbackOnHashchange = false;
+function childHashchange() {
+  if (errorOnHashchange) {
+    ok(false, 'Got unexpected hashchange.');
+  }
+  if (callbackOnHashchange) {
+    callbackOnHashchange = false;
+    gGen.next();
+  }
+}
+
+function run_test() {
+  var iframe = $('iframe').contentWindow;
+
+  ok(true, 'Got first load');
+
+  // Spin the event loop so we exit the onload handler.
+  SimpleTest.executeSoon(function() { gGen.next() });
+  yield;
+
+  let origLocation = iframe.location + '';
+  callbackOnHashchange = true;
+  iframe.location.hash = '#1';
+  // Wait for a hashchange event.
+  yield;
+
+  ok(true, 'Got hashchange.');
+
+  iframe.location = origLocation;
+  // This should produce a load event and *not* a hashchange, because the
+  // result of the load is a different document than we had previously.
+  callbackOnLoad = true;
+  errorOnHashchange = true;
+  yield;
+
+  ok(true, 'Got final load.');
+
+  // Spin the event loop to give hashchange a chance to fire, if it's going to.
+  SimpleTest.executeSoon(function() { gGen.next() });
+  yield;
+
+  SimpleTest.finish();
+  yield;
+}
+
+callbackOnLoad = true;
+gGen = run_test();
+
+</script>
+
+<iframe id='iframe' src='file_bug640387.html'></iframe>
+
+</body>
+</html>