Fixes possible crash in TabStripModel. If during close all a tab was

removed out from under the TabStripModel it would still attempt to
remove the tab. At least this is my best guess as to what is causing
the crash.

BUG=34135
TEST=none

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

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

2 files changed, 66 insertions, 4 deletions

diff --git a/chrome/browser/tabs/tab_strip_model.cc b/chrome/browser/tabs/tab_strip_model.cc
index 5862727..711fe49 100644
--- a/chrome/browser/tabs/tab_strip_model.cc
+++ b/chrome/browser/tabs/tab_strip_model.cc
@@ -789,8 +789,18 @@ bool TabStripModel::IsNewTabAtEndOfTabStrip(TabContents* contents) const {
 
 bool TabStripModel::InternalCloseTabs(const std::vector<int>& indices,
                                       uint32 close_types) {
+  if (indices.empty())
+    return true;
+
   bool retval = true;
 
+  // Map the indices to TabContents, that way if deleting a tab deletes other
+  // tabs we're ok. Crashes seem to indicate during tab deletion other tabs are
+  // getting removed.
+  std::vector<TabContents*> tabs;
+  for (size_t i = 0; i < indices.size(); ++i)
+    tabs.push_back(GetContentsAt(indices[i]));
+
   // We only try the fast shutdown path if the whole browser process is *not*
   // shutting down. Fast shutdown during browser termination is handled in
   // BrowserShutdown.
@@ -824,11 +834,16 @@ bool TabStripModel::InternalCloseTabs(const std::vector<int>& indices,
   }
 
   // We now return to our regularly scheduled shutdown procedure.
-  for (size_t i = 0; i < indices.size(); ++i) {
-    TabContents* detached_contents = GetContentsAt(indices[i]);
+  for (size_t i = 0; i < tabs.size(); ++i) {
+    TabContents* detached_contents = tabs[i];
+    int index = GetIndexOfTabContents(detached_contents);
+    // Make sure we still contain the tab.
+    if (index == kNoTab)
+      continue;
+
     detached_contents->OnCloseStarted();
 
-    if (!delegate_->CanCloseContentsAt(indices[i])) {
+    if (!delegate_->CanCloseContentsAt(index)) {
       retval = false;
       continue;
     }
@@ -847,7 +862,7 @@ bool TabStripModel::InternalCloseTabs(const std::vector<int>& indices,
       continue;
     }
 
-    InternalCloseTab(detached_contents, indices[i],
+    InternalCloseTab(detached_contents, index,
                      (close_types & CLOSE_CREATE_HISTORICAL_TAB) != 0);
   }
 
diff --git a/chrome/browser/tabs/tab_strip_model_unittest.cc b/chrome/browser/tabs/tab_strip_model_unittest.cc
index 81847e9..26ac70d 100644
--- a/chrome/browser/tabs/tab_strip_model_unittest.cc
+++ b/chrome/browser/tabs/tab_strip_model_unittest.cc
@@ -36,6 +36,38 @@
 
 using testing::_;
 
+namespace {
+
+// Class used to delete a TabContents when another TabContents is destroyed.
+class DeleteTabContentsOnDestroyedObserver : public NotificationObserver {
+ public:
+  DeleteTabContentsOnDestroyedObserver(TabContents* source,
+                                       TabContents* tab_to_delete)
+      : source_(source),
+        tab_to_delete_(tab_to_delete) {
+    registrar_.Add(this,
+                   NotificationType::TAB_CONTENTS_DESTROYED,
+                   Source<TabContents>(source));
+  }
+
+  virtual void Observe(NotificationType type,
+                       const NotificationSource& source,
+                       const NotificationDetails& details) {
+    TabContents* tab_to_delete = tab_to_delete_;
+    tab_to_delete_ = NULL;
+    delete tab_to_delete;
+  }
+
+ private:
+  TabContents* source_;
+  TabContents* tab_to_delete_;
+  NotificationRegistrar registrar_;
+
+  DISALLOW_COPY_AND_ASSIGN(DeleteTabContentsOnDestroyedObserver);
+};
+
+}  // namespace
+
 class TabStripDummyDelegate : public TabStripModelDelegate {
  public:
   explicit TabStripDummyDelegate(TabContents* dummy)
@@ -1835,3 +1867,18 @@ TEST_F(TabStripModelTest, ReplaceSendsSelected) {
 
   strip.CloseAllTabs();
 }
+
+// Makes sure TabStripModel handles the case of deleting a tab while removing
+// another tab.
+TEST_F(TabStripModelTest, DeleteFromDestroy) {
+  TabStripDummyDelegate delegate(NULL);
+  TabStripModel strip(&delegate, profile());
+  TabContents* contents1 = CreateTabContents();
+  TabContents* contents2 = CreateTabContents();
+  strip.AppendTabContents(contents1, true);
+  strip.AppendTabContents(contents2, true);
+  // DeleteTabContentsOnDestroyedObserver deletes contents1 when contents2 sends
+  // out notification that it is being destroyed.
+  DeleteTabContentsOnDestroyedObserver observer(contents2, contents1);
+  strip.CloseAllTabs();
+}