Ensure that CopyStateFromAndPrune doesn't exceed kMaxSessionHistoryEntries.

content/browser/tab_contents/navigation_controller_impl.cc

Index: content/browser/tab_contents/navigation_controller_impl.cc
diff --git a/content/browser/tab_contents/navigation_controller_impl.cc b/content/browser/tab_contents/navigation_controller_impl.cc
index e8d722d1a1843bd974b0ffc670af912c2836c958..817e94b574fe45a7872e29d312e508648e466bbc 100644
--- a/content/browser/tab_contents/navigation_controller_impl.cc
+++ b/content/browser/tab_contents/navigation_controller_impl.cc
@@ -412,6 +412,7 @@ int NavigationControllerImpl::GetLastCommittedEntryIndex() const {
 }
 
 int NavigationControllerImpl::GetEntryCount() const {
+  DCHECK(entries_.size() <= max_entry_count());

[Charlie Reis, 2012/03/02 02:37:12]

This would have caught the bug in the first place, and it should catch any other
cases that we end up exceeding the max entry count.

   return static_cast<int>(entries_.size());
 }
 
@@ -1077,6 +1078,11 @@ void NavigationControllerImpl::CopyStateFromAndPrune(
   // Remove all the entries leaving the active entry.
   PruneAllButActive();
 
+  // Ensure that adding the entries from source won't put us over the limit,
+  // if we have an entry to keep.
+  if (GetEntryCount() > 0)

[cbentzel, 2012/03/02 16:47:36]

You should DCHECK that GetEntryCount is only 0 or 1 here. I am pretty confident
that PruneAllButActive guarantees this, and you rely on it here.

Another option to be less fragile is to do the insertion of entries followed by
a prune loop which cuts out the first K entries when there are max + K entries
in the history.

[Charlie Reis, 2012/03/02 17:48:24]

Done.
I thought about that, but it makes it much harder to reuse code--
InsertOrReplace deletes in advance, and RemoveEntryAtIndex calls GetEntryCount
(which now has an arguably useful DCHECK to ensure we never go over the limit).

I'd prefer to stick with the existing approach that prunes in advance and never
exceeds the limit.

+    source->PruneFirstEntryIfFull();
+
   // Insert the entries from source. Don't use source->GetCurrentEntryIndex as
   // we don't want to copy over the transient entry.
   int max_source_index = source->pending_entry_index_ != -1 ?
@@ -1237,11 +1243,7 @@ void NavigationControllerImpl::InsertOrReplaceEntry(NavigationEntryImpl* entry,
       NotifyPrunedEntries(this, false, num_pruned);
   }
 
-  if (entries_.size() >= max_entry_count()) {
-    DCHECK(last_committed_entry_index_ > 0);
-    RemoveEntryAtIndex(0);
-    NotifyPrunedEntries(this, true, 1);
-  }
+  PruneFirstEntryIfFull();
 
   entries_.push_back(linked_ptr<NavigationEntryImpl>(entry));
   last_committed_entry_index_ = static_cast<int>(entries_.size()) - 1;
@@ -1250,6 +1252,15 @@ void NavigationControllerImpl::InsertOrReplaceEntry(NavigationEntryImpl* entry,
   tab_contents_->UpdateMaxPageID(entry->GetPageID());
 }
 
+void NavigationControllerImpl::PruneFirstEntryIfFull() {
+  if (entries_.size() >= max_entry_count()) {
+    DCHECK_EQ(max_entry_count(), entries_.size());
+    DCHECK(last_committed_entry_index_ > 0);
+    RemoveEntryAtIndex(0);
+    NotifyPrunedEntries(this, true, 1);
+  }
+}
+
 void NavigationControllerImpl::NavigateToPendingEntry(ReloadType reload_type) {
   needs_reload_ = false;