Ensure that CopyStateFromAndPrune doesn't exceed kMaxSessionHistoryEntries.

BUG=93427
TEST=See bug, comment 112


Review URL: http://codereview.chromium.org/9565045

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@124729 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 73 insertions, 6 deletions

diff --git a/content/browser/tab_contents/navigation_controller_impl.cc b/content/browser/tab_contents/navigation_controller_impl.cc
index e8d722d..5cdd8cc 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());
   return static_cast<int>(entries_.size());
 }
 
@@ -1077,6 +1078,12 @@ void NavigationControllerImpl::CopyStateFromAndPrune(
   // Remove all the entries leaving the active entry.
   PruneAllButActive();
 
+  // We now have zero or one entries.  Ensure that adding the entries from
+  // source won't put us over the limit.
+  DCHECK(GetEntryCount() == 0 || GetEntryCount() == 1);
+  if (GetEntryCount() > 0)
+    source->PruneOldestEntryIfFull();
+
   // 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 +1244,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);
-  }
+  PruneOldestEntryIfFull();
 
   entries_.push_back(linked_ptr<NavigationEntryImpl>(entry));
   last_committed_entry_index_ = static_cast<int>(entries_.size()) - 1;
@@ -1250,6 +1253,15 @@ void NavigationControllerImpl::InsertOrReplaceEntry(NavigationEntryImpl* entry,
   tab_contents_->UpdateMaxPageID(entry->GetPageID());
 }
 
+void NavigationControllerImpl::PruneOldestEntryIfFull() {
+  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;
 
diff --git a/content/browser/tab_contents/navigation_controller_impl.h b/content/browser/tab_contents/navigation_controller_impl.h
index a261ecc..e02e8a9 100644
--- a/content/browser/tab_contents/navigation_controller_impl.h
+++ b/content/browser/tab_contents/navigation_controller_impl.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -248,6 +248,10 @@ class CONTENT_EXPORT NavigationControllerImpl
   // Discards the transient entry.
   void DiscardTransientEntry();
 
+  // If we have the maximum number of entries, remove the oldest one in
+  // preparation to add another.
+  void PruneOldestEntryIfFull();
+
   // Returns true if the navigation is redirect.
   bool IsRedirect(const ViewHostMsg_FrameNavigate_Params& params);
 
diff --git a/content/browser/tab_contents/navigation_controller_impl_unittest.cc b/content/browser/tab_contents/navigation_controller_impl_unittest.cc
index 564f47d..71c5de5 100644
--- a/content/browser/tab_contents/navigation_controller_impl_unittest.cc
+++ b/content/browser/tab_contents/navigation_controller_impl_unittest.cc
@@ -2186,6 +2186,57 @@ TEST_F(NavigationControllerTest, CopyStateFromAndPrune3) {
   EXPECT_EQ(0, other_contents->GetMaxPageIDForSiteInstance(instance1));
 }
 
+// Tests CopyStateFromAndPrune with 3 urls in source, 1 in dest,
+// when the max entry count is 3.  We should prune one entry.
+TEST_F(NavigationControllerTest, CopyStateFromAndPruneMaxEntries) {
+  NavigationControllerImpl& controller = controller_impl();
+  size_t original_count = NavigationControllerImpl::max_entry_count();
+  const int kMaxEntryCount = 3;
+
+  NavigationControllerImpl::set_max_entry_count_for_testing(kMaxEntryCount);
+
+  const GURL url1("http://foo/1");
+  const GURL url2("http://foo/2");
+  const GURL url3("http://foo/3");
+  const GURL url4("http://foo/4");
+
+  // Create a PrunedListener to observe prune notifications.
+  PrunedListener listener(&controller);
+
+  NavigateAndCommit(url1);
+  NavigateAndCommit(url2);
+  NavigateAndCommit(url3);
+
+  scoped_ptr<TestTabContents> other_contents(CreateTestTabContents());
+  NavigationControllerImpl& other_controller =
+      other_contents->GetControllerImpl();
+  other_contents->NavigateAndCommit(url4);
+  other_contents->ExpectSetHistoryLengthAndPrune(
+      GetSiteInstanceFromEntry(other_controller.GetEntryAtIndex(0)), 2,
+      other_controller.GetEntryAtIndex(0)->GetPageID());
+  other_controller.CopyStateFromAndPrune(&controller);
+
+  // We should have received a pruned notification.
+  EXPECT_EQ(1, listener.notification_count_);
+  EXPECT_TRUE(listener.details_.from_front);
+  EXPECT_EQ(1, listener.details_.count);
+
+  // other_controller should now contain only 3 urls: url2, url3 and url4.
+
+  ASSERT_EQ(3, other_controller.GetEntryCount());
+
+  ASSERT_EQ(2, other_controller.GetCurrentEntryIndex());
+
+  EXPECT_EQ(url2, other_controller.GetEntryAtIndex(0)->GetURL());
+  EXPECT_EQ(url3, other_controller.GetEntryAtIndex(1)->GetURL());
+  EXPECT_EQ(url4, other_controller.GetEntryAtIndex(2)->GetURL());
+  EXPECT_EQ(1, other_controller.GetEntryAtIndex(0)->GetPageID());
+  EXPECT_EQ(2, other_controller.GetEntryAtIndex(1)->GetPageID());
+  EXPECT_EQ(0, other_controller.GetEntryAtIndex(2)->GetPageID());
+
+  NavigationControllerImpl::set_max_entry_count_for_testing(original_count);
+}
+
 // Tests that navigations initiated from the page (with the history object)
 // work as expected without navigation entries.
 TEST_F(NavigationControllerTest, HistoryNavigate) {