Fix UAF when closing tabs very quickly.

Bug: 1082755
Change-Id: Id8e174625f16e9a319000eb7427ec8659174c7d4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2204259
Commit-Queue: Taylor Bergquist <tbergquist@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#770758}

chrome/browser/ui/views/tabs/tab_strip.cc

@@ -1613,7 +1613,6 @@ void TabStrip::AddSelectionFromAnchorTo(Tab* tab) {
 }
 
 void TabStrip::CloseTab(Tab* tab, CloseTabSource source) {
-  int model_index = GetModelIndexOf(tab);
   if (tab->closing()) {
     // If the tab is already closing, close the next tab. We do this so that the
     // user can rapidly close tabs by clicking the close button and not have
@@ -1624,37 +1623,12 @@ void TabStrip::CloseTab(Tab* tab, CloseTabSource source) {
       it++;
     }
 
-    model_index =
-        it != all_tabs.end() ? GetModelIndexOf(*it) : TabStripModel::kNoTab;
-  }
-
-  if (!IsValidModelIndex(model_index))
-    return;
-
-  // If we're not allowed to close this tab for whatever reason, we should not
-  // proceed.
-  if (!controller_->BeforeCloseTab(model_index, source))
-    return;
-
-  if (!in_tab_close_ && IsAnimating()) {
-    // Cancel any current animations. We do this as remove uses the current
-    // ideal bounds and we need to know ideal bounds is in a good state.
-    StopAnimating(true);
-  }
-
-  if (GetWidget()) {
-    in_tab_close_ = true;
-    resize_layout_timer_.Stop();
-    if (source == CLOSE_TAB_FROM_TOUCH)
-      StartResizeLayoutTabsFromTouchTimer();
-    else
-      AddMessageLoopObserver();
+    if (it == all_tabs.end())
+      return;
+    tab = *it;
   }
 
-  UpdateHoverCard(nullptr);
-  if (tab->group().has_value())
-    base::RecordAction(base::UserMetricsAction("CloseGroupedTab"));
-  controller_->CloseTab(model_index);
+  CloseTabInternal(GetModelIndexOf(tab), source);
 }
 
 void TabStrip::ShiftTabLeft(Tab* tab) {
@@ -2676,6 +2650,36 @@ int TabStrip::GetViewInsertionIndex(Tab* tab,
   return other_view_index;
 }
 
+void TabStrip::CloseTabInternal(int model_index, CloseTabSource source) {
+  if (!IsValidModelIndex(model_index))
+    return;
+
+  // If we're not allowed to close this tab for whatever reason, we should not
+  // proceed.
+  if (!controller_->BeforeCloseTab(model_index, source))
+    return;
+
+  if (!in_tab_close_ && IsAnimating()) {
+    // Cancel any current animations. We do this as remove uses the current
+    // ideal bounds and we need to know ideal bounds is in a good state.
+    StopAnimating(true);
+  }
+
+  if (GetWidget()) {
+    in_tab_close_ = true;
+    resize_layout_timer_.Stop();
+    if (source == CLOSE_TAB_FROM_TOUCH)
+      StartResizeLayoutTabsFromTouchTimer();
+    else
+      AddMessageLoopObserver();
+  }
+
+  UpdateHoverCard(nullptr);
+  if (tab_at(model_index)->group().has_value())
+    base::RecordAction(base::UserMetricsAction("CloseGroupedTab"));
+  controller_->CloseTab(model_index);
+}
+
 void TabStrip::RemoveTabFromViewModel(int index) {
   Tab* closing_tab = tab_at(index);
   bool closing_tab_was_active = closing_tab->IsActive();

chrome/browser/ui/views/tabs/tab_strip.h

@@ -488,6 +488,9 @@ class TabStrip : public views::AccessiblePaneView,
                             base::Optional<int> from_model_index,
                             int to_model_index) const;
 
+  // Closes the tab at |model_index|.
+  void CloseTabInternal(int model_index, CloseTabSource source);
+
   // Removes the tab at |index| from |tabs_|.
   void RemoveTabFromViewModel(int index);
 

chrome/browser/ui/views/tabs/tab_strip_unittest.cc

@@ -1029,11 +1029,16 @@ TEST_P(TabStripTest, EventsOnClosingTab) {
   controller_->AddTab(1, true);
 
   Tab* first_tab = tab_strip_->tab_at(0);
+  Tab* second_tab = tab_strip_->tab_at(1);
   gfx::Point tab_center = first_tab->bounds().CenterPoint();
 
   EXPECT_EQ(first_tab, tab_strip_->GetEventHandlerForPoint(tab_center));
   tab_strip_->CloseTab(first_tab, CLOSE_TAB_FROM_MOUSE);
   EXPECT_EQ(first_tab, tab_strip_->GetEventHandlerForPoint(tab_center));
+
+  // Closing |first_tab| again should forward to |second_tab| instead.
+  tab_strip_->CloseTab(first_tab, CLOSE_TAB_FROM_MOUSE);
+  EXPECT_TRUE(second_tab->closing());
 }
 
 TEST_P(TabStripTest, GroupHeaderBasics) {