2 files changed, 74 insertions, 28 deletions

diff --git a/chrome/browser/navigation_controller.cc b/chrome/browser/navigation_controller.cc
index a31db5f..d56d6b1 100644
--- a/chrome/browser/navigation_controller.cc
+++ b/chrome/browser/navigation_controller.cc
@@ -226,24 +226,33 @@ void NavigationController::ReloadDontCheckForRepost() {
 }
 
 void NavigationController::Destroy() {
-  // First, clean out all NULL entries in the map so that we know empty map
-  // means all tabs destroyed.  This is needed since TabContentsWasDestroyed()
-  // won't get called for types that are in our map with a NULL contents.
-  for (int t = 0; t < TAB_CONTENTS_NUM_TYPES; t++) {
-    TabContentsMap::iterator i =
-        tab_contents_map_.find(static_cast<TabContentsType>(t));
-    if (i != tab_contents_map_.end() && !i->second)
-      tab_contents_map_.erase(i);
-  }
-
-  // Now close all tab contents owned by this controller.  We make a list on
-  // the stack because they are removed from the map as they are Destroyed
+  // Close all tab contents owned by this controller.  We make a list on the
+  // stack because they are removed from the map as they are Destroyed
   // (invalidating the iterators), which may or may not occur synchronously.
+  // We also keep track of any NULL entries in the map so that we can clean
+  // them out.
   std::list<TabContents*> tabs_to_destroy;
+  std::list<TabContentsType> tab_types_to_erase;
   for (TabContentsMap::iterator i = tab_contents_map_.begin();
        i != tab_contents_map_.end(); ++i) {
-    DCHECK(i->second);
-    tabs_to_destroy.push_back(i->second);
+    if (i->second)
+      tabs_to_destroy.push_back(i->second);
+    else
+      tab_types_to_erase.push_back(i->first);
+  }
+
+  // Clean out all NULL entries in the map so that we know empty map means all
+  // tabs destroyed.  This is needed since TabContentsWasDestroyed() won't get
+  // called for types that are in our map with a NULL contents.  (We don't do
+  // this by iterating over TAB_CONTENTS_NUM_TYPES because some tests create
+  // additional types.)
+  for (std::list<TabContentsType>::iterator i = tab_types_to_erase.begin();
+       i != tab_types_to_erase.end(); ++i) {
+    TabContentsMap::iterator map_iterator = tab_contents_map_.find(*i);
+    if (map_iterator != tab_contents_map_.end()) {
+      DCHECK(!map_iterator->second);
+      tab_contents_map_.erase(map_iterator);
+    }
   }
 
   // Cancel all the TabContentsCollectors.
@@ -377,23 +386,17 @@ void NavigationController::DidNavigateToEntry(NavigationEntry* entry) {
     alternate_nav_url_fetcher_->OnNavigatedToEntry();
   }
 
-  // If the previous or next entry uses a different tab contents it is now a
-  // safe time to schedule collection because a navigation is neccessary to
-  // get back to them.
+  // It is now a safe time to schedule collection for any tab contents of a
+  // different type, because a navigation is necessary to get back to them.
   int index = GetCurrentEntryIndex();
   if (index < 0 || GetPendingEntryIndex() != -1)
     return;
 
   TabContentsType active_type = GetEntryAtIndex(index)->GetType();
-  if (index > 0) {
-    NavigationEntry* ne = GetEntryAtIndex(index - 1);
-    if (ne->GetType() != active_type)
-      ScheduleTabContentsCollection(ne->GetType());
-  }
-  if (index < (GetEntryCount() - 1)) {
-    NavigationEntry* ne = GetEntryAtIndex(index + 1);
-    if (ne->GetType() != active_type)
-      ScheduleTabContentsCollection(ne->GetType());
+  for (TabContentsMap::iterator i = tab_contents_map_.begin();
+       i != tab_contents_map_.end(); ++i) {
+    if (i->first != active_type)
+      ScheduleTabContentsCollection(i->first);
   }
 }
 
diff --git a/chrome/browser/navigation_controller_unittest.cc b/chrome/browser/navigation_controller_unittest.cc
index 6b8de0d..5986b31 100644
--- a/chrome/browser/navigation_controller_unittest.cc
+++ b/chrome/browser/navigation_controller_unittest.cc
@@ -626,7 +626,7 @@ TEST_F(NavigationControllerTest, LinkClick) {
   EXPECT_FALSE(contents->controller()->CanGoForward());
 }
 
-TEST_F(NavigationControllerTest, DISABLED_SwitchTypes) {
+TEST_F(NavigationControllerTest, SwitchTypes) {
   const GURL url1("test1:foo");
   const GURL url2("test2:foo");
 
@@ -664,11 +664,15 @@ TEST_F(NavigationControllerTest, DISABLED_SwitchTypes) {
   EXPECT_FALSE(contents->controller()->GetPendingEntry());
   EXPECT_FALSE(contents->controller()->CanGoBack());
   EXPECT_TRUE(contents->controller()->CanGoForward());
+
+  // There may be TabContentsCollector tasks pending, so flush them from queue.
+  MessageLoop::current()->Quit();
+  MessageLoop::current()->Run();
 }
 
 // Tests what happens when we begin to navigate to a new contents type, but
 // then that navigation gets discarded instead.
-TEST_F(NavigationControllerTest, DISABLED_SwitchTypes_Discard) {
+TEST_F(NavigationControllerTest, SwitchTypes_Discard) {
   const GURL url1("test1:foo");
   const GURL url2("test2:foo");
 
@@ -694,6 +698,45 @@ TEST_F(NavigationControllerTest, DISABLED_SwitchTypes_Discard) {
   EXPECT_FALSE(contents->controller()->GetPendingEntry());
   EXPECT_FALSE(contents->controller()->CanGoBack());
   EXPECT_FALSE(contents->controller()->CanGoForward());
+
+  // There may be TabContentsCollector tasks pending, so flush them from queue.
+  MessageLoop::current()->Quit();
+  MessageLoop::current()->Run();
+}
+
+// Tests that TabContentsTypes that are not in use are deleted (via a
+// TabContentsCollector task).  Prevents regression of bug 1296773.
+TEST_F(NavigationControllerTest, SwitchTypesCleanup) {
+  const GURL url1("test1:foo");
+  const GURL url2("test2:foo");
+  const GURL url3("test2:bar");
+
+  contents->controller()->LoadURL(url1, PageTransition::TYPED);
+  contents->CompleteNavigation(0);
+
+  contents->controller()->LoadURL(url2, PageTransition::TYPED);
+  contents->CompleteNavigation(0);
+
+  contents->controller()->LoadURL(url3, PageTransition::TYPED);
+  contents->CompleteNavigation(1);
+
+  // Navigate back to the start
+  contents->controller()->GoToIndex(0);
+  contents->CompleteNavigation(0);
+
+  // Now jump to the end
+  contents->controller()->GoToIndex(2);
+  contents->CompleteNavigation(1);
+
+  // There may be TabContentsCollector tasks pending, so flush them from queue.
+  MessageLoop::current()->Quit();
+  MessageLoop::current()->Run();
+
+  // Now that the tasks have been flushed, the first tab type should be gone.
+  ASSERT_TRUE(
+      contents->controller()->GetTabContents(kTestContentsType1) == NULL);
+  ASSERT_EQ(contents, 
+      contents->controller()->GetTabContents(kTestContentsType2));
 }
 
 // Tests that we limit the number of navigation entries created correctly.