Bug 680344: Properly fix up a pending history navigation in nsSHistory::RemoveDuplicate. r=smaug sr=bz a=Asa
authorKyle Huey <khuey@kylehuey.com>
Wed, 24 Aug 2011 08:48:17 -0400
changeset 76434 fc27973f66b18a5cd3c5c6fc44f2c30ea1eb73b4
parent 76433 82e79db5a5fc79e215d2a1964087f0d28c6a9d44
child 76435 f78f9890fbc109f96ac6542f876c4b057ae25134
push id67
push userclegnitto@mozilla.com
push dateFri, 04 Nov 2011 22:39:41 +0000
treeherdermozilla-release@04778346a3b0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug, bz, Asa
bugs680344
milestone8.0a2
Bug 680344: Properly fix up a pending history navigation in nsSHistory::RemoveDuplicate. r=smaug sr=bz a=Asa
docshell/shistory/src/nsSHistory.cpp
--- a/docshell/shistory/src/nsSHistory.cpp
+++ b/docshell/shistory/src/nsSHistory.cpp
@@ -1304,16 +1304,18 @@ PRBool IsSameTree(nsISHEntry* aEntry1, n
   
   return PR_TRUE;
 }
 
 PRBool
 nsSHistory::RemoveDuplicate(PRInt32 aIndex, PRBool aKeepNext)
 {
   NS_ASSERTION(aIndex >= 0, "aIndex must be >= 0!");
+  NS_ASSERTION(aIndex != 0 || aKeepNext,
+               "If we're removing index 0 we must be keeping the next");
   NS_ASSERTION(aIndex != mIndex, "Shouldn't remove mIndex!");
   PRInt32 compareIndex = aKeepNext ? aIndex + 1 : aIndex - 1;
   nsCOMPtr<nsIHistoryEntry> rootHE1, rootHE2;
   GetEntryAtIndex(aIndex, PR_FALSE, getter_AddRefs(rootHE1));
   GetEntryAtIndex(compareIndex, PR_FALSE, getter_AddRefs(rootHE2));
   nsCOMPtr<nsISHEntry> root1 = do_QueryInterface(rootHE1);
   nsCOMPtr<nsISHEntry> root2 = do_QueryInterface(rootHE2);
   if (IsSameTree(root1, root2)) {
@@ -1340,20 +1342,35 @@ nsSHistory::RemoveDuplicate(PRInt32 aInd
       NS_ASSERTION(txToRemove == mListRoot,
                    "Transaction at index 0 should be mListRoot!");
       // We're removing the very first session history transaction!
       mListRoot = txToKeep;
     }
     if (mRootDocShell) {
       static_cast<nsDocShell*>(mRootDocShell)->HistoryTransactionRemoved(aIndex);
     }
+
+    // Adjust our indices to reflect the removed transaction
     if (mIndex > aIndex) {
       mIndex = mIndex - 1;
     }
-    if (mRequestedIndex > aIndex) {
+
+    // NB: If the transaction we are removing is the transaction currently
+    // being navigated to (mRequestedIndex) then we adjust the index
+    // only if we're not keeping the next entry (because if we are keeping
+    // the next entry (because the current is a duplicate of the next), then
+    // that entry slides into the spot that we're currently pointing to.
+    // We don't do this adjustment for mIndex because mIndex cannot equal
+    // aIndex.
+
+    // NB: We don't need to guard on mRequestedIndex being nonzero here,
+    // because either they're strictly greater than aIndex which is at least
+    // zero, or they are equal to aIndex in which case aKeepNext must be true
+    // if aIndex is zero.
+    if (mRequestedIndex > aIndex || (mRequestedIndex == aIndex && !aKeepNext)) {
       mRequestedIndex = mRequestedIndex - 1;
     }
     --mLength;
     return PR_TRUE;
   }
   return PR_FALSE;
 }