Makes the TabStripModel delete any phantom tabcontents from its

destructor. This isn't really needed now that we've disabled phantom
tabs, but I'm doing in case we end up reenabling phantom tabs.

BUG=45541
TEST=covered by unit test.

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

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

2 files changed, 51 insertions, 0 deletions

diff --git a/chrome/browser/tabs/tab_strip_model.cc b/chrome/browser/tabs/tab_strip_model.cc
index 32c55fd..5447576 100644
--- a/chrome/browser/tabs/tab_strip_model.cc
+++ b/chrome/browser/tabs/tab_strip_model.cc
@@ -71,6 +71,16 @@ TabStripModel::~TabStripModel() {
   FOR_EACH_OBSERVER(TabStripModelObserver, observers_,
                     TabStripModelDeleted());
 
+  // Before deleting any phantom tabs remove our notification observers so that
+  // we don't attempt to notify our delegate or do any processing.
+  registrar_.RemoveAll();
+
+  // Phantom tabs still have valid TabConents that we own and need to delete.
+  for (int i = count() - 1; i >= 0; --i) {
+    if (IsPhantomTab(i))
+      delete contents_data_[i]->contents;
+  }
+
   STLDeleteContainerPointers(contents_data_.begin(), contents_data_.end());
   delete order_controller_;
 }
diff --git a/chrome/browser/tabs/tab_strip_model_unittest.cc b/chrome/browser/tabs/tab_strip_model_unittest.cc
index 7089226..d21d7b5 100644
--- a/chrome/browser/tabs/tab_strip_model_unittest.cc
+++ b/chrome/browser/tabs/tab_strip_model_unittest.cc
@@ -6,6 +6,7 @@
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/path_service.h"
+#include "base/scoped_ptr.h"
 #include "base/string_util.h"
 #include "base/stl_util-inl.h"
 #include "base/string_util.h"
@@ -20,11 +21,16 @@
 #include "chrome/browser/tabs/tab_strip_model.h"
 #include "chrome/browser/tabs/tab_strip_model_order_controller.h"
 #include "chrome/common/extensions/extension.h"
+#include "chrome/common/notification_observer_mock.h"
+#include "chrome/common/notification_registrar.h"
+#include "chrome/common/notification_service.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/property_bag.h"
 #include "chrome/common/url_constants.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using testing::_;
+
 class TabStripDummyDelegate : public TabStripModelDelegate {
  public:
   explicit TabStripDummyDelegate(TabContents* dummy)
@@ -1933,3 +1939,38 @@ TEST_F(TabStripModelTest, Phantom) {
   // Clean up the phantom tabs.
   tabstrip.CloseAllTabs();
 }
+
+// Makes sure the TabStripModel deletes phantom TabContents from its destructor.
+TEST_F(TabStripModelTest, DeletePhantomTabContents) {
+  if (!browser_defaults::kPhantomTabsEnabled)
+    return;
+
+  TabStripDummyDelegate delegate(NULL);
+  scoped_ptr<TabStripModel> strip(new TabStripModel(&delegate, profile()));
+
+  strip->AddTabContents(CreateTabContents(), -1, PageTransition::TYPED,
+                        TabStripModel::ADD_PINNED);
+
+  // This will make the tab go phantom.
+  delete strip->GetTabContentsAt(0);
+
+  ASSERT_TRUE(strip->IsPhantomTab(0));
+
+  NotificationRegistrar registrar;
+  NotificationObserverMock notification_observer;
+  MockTabStripModelObserver tabstrip_observer;
+  registrar.Add(&notification_observer,
+                NotificationType::TAB_CONTENTS_DESTROYED,
+                NotificationService::AllSources());
+  strip->AddObserver(&tabstrip_observer);
+
+  // Deleting the strip should delete the phanton tabcontents and result in one
+  // TAB_CONTENTS_DESTROYED.
+  EXPECT_CALL(notification_observer, Observe(_, _, _)).Times(1);
+
+  // Delete the tabstrip.
+  strip.reset();
+
+  // And there should have been no methods sent to the TabStripModelObserver.
+  EXPECT_EQ(0, tabstrip_observer.GetStateCount());
+}