Ensure that CopyStateFromAndPrune doesn't exceed kMaxSessionHistoryEntries.

content/browser/tab_contents/navigation_controller_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/tab_contents/navigation_controller_impl.h"
 
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/string_number_conversions.h"  // Temporary
 #include "base/string_util.h"
@@ skipping 394 matching lines @@
   NavigationEntry* active_entry = GetActiveEntry();
   return active_entry && !active_entry->IsViewSourceMode() &&
     is_supported_mime_type && !tab_contents_->GetInterstitialPage();
 }
 
 int NavigationControllerImpl::GetLastCommittedEntryIndex() const {
   return last_committed_entry_index_;
 }
 
 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());
 }
 
 NavigationEntry* NavigationControllerImpl::GetEntryAtIndex(
     int index) const {
   return entries_.at(index).get();
 }
 
 NavigationEntry* NavigationControllerImpl::GetEntryAtOffset(
     int offset) const {
@@ skipping 645 matching lines @@
   DCHECK(
       (transient_entry_index_ != -1 &&
        transient_entry_index_ == GetEntryCount() - 1) ||
       (pending_entry_ && (pending_entry_index_ == -1 ||
                           pending_entry_index_ == GetEntryCount() - 1)) ||
       (!pending_entry_ && last_committed_entry_index_ == GetEntryCount() - 1));
 
   // 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 ?
       source->pending_entry_index_ : source->last_committed_entry_index_;
   if (max_source_index == -1)
     max_source_index = source->GetEntryCount();
   else
     max_source_index++;
   InsertEntriesFrom(*source, max_source_index);
 
@@ skipping 140 matching lines @@
     int num_pruned = 0;
     while (last_committed_entry_index_ < (current_size - 1)) {
       num_pruned++;
       entries_.pop_back();
       current_size--;
     }
     if (num_pruned > 0)  // Only notify if we did prune something.
       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;
 
   // This is a new page ID, so we need everybody to know about it.
   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;
 
   // If we were navigating to a slow-to-commit page, and the user performs
   // a session history navigation to the last committed page, RenderViewHost
   // will force the throbber to start, but WebKit will essentially ignore the
   // navigation, and won't send a message to stop the throbber. To prevent this
   // from happening, we drop the navigation here and stop the slow-to-commit
   // page from loading (which would normally happen during the navigation).
   if (pending_entry_index_ != -1 &&
@@ skipping 151 matching lines @@
   for (int i = 0; i < max_index; i++) {
     // When cloning a tab, copy all entries except interstitial pages
     if (source.entries_[i].get()->GetPageType() !=
         content::PAGE_TYPE_INTERSTITIAL) {
       entries_.insert(entries_.begin() + insert_index++,
                       linked_ptr<NavigationEntryImpl>(
                           new NavigationEntryImpl(*source.entries_[i])));
     }
   }
 }