Split slot index calculation into separate functions

The original GetSlotIndexForTabModelIndex served two purposes: getting
the slot for an existing tab, and getting the insertion index for a
new slot. This splits it into separate methods with common logic
remaining in a shared method.

The logic had a bug when requesting the first tab of a group while
tabs just before it are animating closed. This bug is fixed.

To help debug the associated crash, CHECKs are added.

Bug: 1138748
Change-Id: I6e0d797bb70e9aa1da813dbf1fecf228290e55b9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2515363
Commit-Queue: Collin Baker <collinbaker@chromium.org>
Reviewed-by: Taylor Bergquist <tbergquist@chromium.org>
Cr-Commit-Position: refs/heads/master@{#824672}

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

@@ -949,7 +949,7 @@ class TabStrip::TabDragContextImpl : public TabDragContext {
     if (dragging_group == other_group) {
       // |dragging_tabs| can only be empty if dragging in from another window,
       // in which case |dragging_group| can't be the same as |other_group|.
-      DCHECK(dragging_tabs.size() > 0);
+      DCHECK_GT(dragging_tabs.size(), 0u);
       if (candidate_index <= dragging_tabs.front() ||
           dragging_tabs.back() >= GetTabCount() - 1)
         return dragging_tabs.front();
@@ -1010,8 +1010,6 @@ TabStrip::TabStrip(std::unique_ptr<TabStripController> controller)
       layout_helper_(std::make_unique<TabStripLayoutHelper>(
           controller_.get(),
           base::BindRepeating(&TabStrip::tabs_view_model,
-                              base::Unretained(this)),
-          base::BindRepeating(&TabStrip::GetGroupHeaders,
                               base::Unretained(this)))),
       drag_context_(std::make_unique<TabDragContextImpl>(this)) {
   Init();

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

@@ -68,13 +68,10 @@ struct TabStripLayoutHelper::TabSlot {
   std::unique_ptr<TabAnimation> animation;
 };
 
-TabStripLayoutHelper::TabStripLayoutHelper(
-    const TabStripController* controller,
-    GetTabsCallback get_tabs_callback,
-    GetGroupHeadersCallback get_group_headers_callback)
+TabStripLayoutHelper::TabStripLayoutHelper(const TabStripController* controller,
+                                           GetTabsCallback get_tabs_callback)
     : controller_(controller),
       get_tabs_callback_(get_tabs_callback),
-      get_group_headers_callback_(get_group_headers_callback),
       active_tab_width_(TabStyle::GetStandardWidth()),
       inactive_tab_width_(TabStyle::GetStandardWidth()),
       first_non_pinned_tab_index_(0),
@@ -106,15 +103,14 @@ void TabStripLayoutHelper::InsertTabAt(int model_index,
                                        Tab* tab,
                                        TabPinned pinned) {
   const int slot_index =
-      GetSlotIndexForTabModelIndex(model_index, tab->group());
+      GetSlotInsertionIndexForNewTab(model_index, tab->group());
   slots_.insert(slots_.begin() + slot_index,
                 TabSlot::CreateForTab(tab, TabOpen::kOpen, pinned));
 }
 
 void TabStripLayoutHelper::RemoveTabAt(int model_index, Tab* tab) {
   TabAnimation* animation =
-      slots_[GetSlotIndexForTabModelIndex(model_index, tab->group())]
-          .animation.get();
+      slots_[GetSlotIndexForExistingTab(model_index)].animation.get();
   animation->AnimateTo(animation->target_state().WithOpen(TabOpen::kClosed));
   animation->CompleteAnimation();
 }
@@ -152,13 +148,12 @@ void TabStripLayoutHelper::MoveTab(
     base::Optional<tab_groups::TabGroupId> moving_tab_group,
     int prev_index,
     int new_index) {
-  const int prev_slot_index =
-      GetSlotIndexForTabModelIndex(prev_index, moving_tab_group);
+  const int prev_slot_index = GetSlotIndexForExistingTab(prev_index);
   TabSlot moving_tab = std::move(slots_[prev_slot_index]);
   slots_.erase(slots_.begin() + prev_slot_index);
 
   const int new_slot_index =
-      GetSlotIndexForTabModelIndex(new_index, moving_tab_group);
+      GetSlotInsertionIndexForNewTab(new_index, moving_tab_group);
   slots_.insert(slots_.begin() + new_slot_index, std::move(moving_tab));
 
   if (moving_tab_group.has_value())
@@ -166,11 +161,8 @@ void TabStripLayoutHelper::MoveTab(
 }
 
 void TabStripLayoutHelper::SetTabPinned(int model_index, TabPinned pinned) {
-  views::ViewModelT<Tab>* tabs = get_tabs_callback_.Run();
   TabAnimation* animation =
-      slots_[GetSlotIndexForTabModelIndex(model_index,
-                                          tabs->view_at(model_index)->group())]
-          .animation.get();
+      slots_[GetSlotIndexForExistingTab(model_index)].animation.get();
   animation->AnimateTo(animation->target_state().WithPinned(pinned));
   animation->CompleteAnimation();
 }
@@ -179,7 +171,7 @@ void TabStripLayoutHelper::InsertGroupHeader(tab_groups::TabGroupId group,
                                              TabGroupHeader* header) {
   std::vector<int> tabs_in_group = controller_->ListTabsInGroup(group);
   const int header_slot_index =
-      GetSlotIndexForTabModelIndex(tabs_in_group[0], group);
+      GetSlotInsertionIndexForNewTab(tabs_in_group[0], group);
   slots_.insert(
       slots_.begin() + header_slot_index,
       TabSlot::CreateForGroupHeader(group, header, TabPinned::kUnpinned));
@@ -203,24 +195,21 @@ void TabStripLayoutHelper::UpdateGroupHeaderIndex(
   slots_.erase(slots_.begin() + slot_index);
   std::vector<int> tabs_in_group = controller_->ListTabsInGroup(group);
   const int first_tab_slot_index =
-      GetSlotIndexForTabModelIndex(tabs_in_group[0], group);
+      GetSlotInsertionIndexForNewTab(tabs_in_group[0], group);
   slots_.insert(slots_.begin() + first_tab_slot_index, std::move(header_slot));
 }
 
 void TabStripLayoutHelper::SetActiveTab(int prev_active_index,
                                         int new_active_index) {
-  views::ViewModelT<Tab>* tabs = get_tabs_callback_.Run();
   // Set active state without animating by retargeting the existing animation.
   if (prev_active_index >= 0) {
-    const int prev_slot_index = GetSlotIndexForTabModelIndex(
-        prev_active_index, tabs->view_at(prev_active_index)->group());
+    const int prev_slot_index = GetSlotIndexForExistingTab(prev_active_index);
     TabAnimation* animation = slots_[prev_slot_index].animation.get();
     animation->RetargetTo(
         animation->target_state().WithActive(TabActive::kInactive));
   }
   if (new_active_index >= 0) {
-    const int new_slot_index = GetSlotIndexForTabModelIndex(
-        new_active_index, tabs->view_at(new_active_index)->group());
+    const int new_slot_index = GetSlotIndexForExistingTab(new_active_index);
     TabAnimation* animation = slots_[new_slot_index].animation.get();
     animation->RetargetTo(
         animation->target_state().WithActive(TabActive::kActive));
@@ -247,9 +236,7 @@ int TabStripLayoutHelper::UpdateIdealBounds(int available_width) {
   const int active_tab_model_index = controller_->GetActiveIndex();
   const int active_tab_slot_index =
       controller_->IsValidIndex(active_tab_model_index)
-          ? GetSlotIndexForTabModelIndex(
-                active_tab_model_index,
-                tabs->view_at(active_tab_model_index)->group())
+          ? GetSlotIndexForExistingTab(active_tab_model_index)
           : TabStripModel::kNoTab;
 
   int current_tab_model_index = 0;
@@ -307,23 +294,15 @@ std::vector<gfx::Rect> TabStripLayoutHelper::CalculateIdealBounds(
                                            ? tabstrip_width_override_
                                            : available_width;
 
-  views::ViewModelT<Tab>* tabs = get_tabs_callback_.Run();
-  std::map<tab_groups::TabGroupId, TabGroupHeader*> group_headers =
-      get_group_headers_callback_.Run();
-
   const int active_tab_model_index = controller_->GetActiveIndex();
   const int active_tab_slot_index =
       controller_->IsValidIndex(active_tab_model_index)
-          ? GetSlotIndexForTabModelIndex(
-                active_tab_model_index,
-                tabs->view_at(active_tab_model_index)->group())
+          ? GetSlotIndexForExistingTab(active_tab_model_index)
           : TabStripModel::kNoTab;
   const int pinned_tab_count = GetPinnedTabCount();
   const int last_pinned_tab_index = pinned_tab_count - 1;
   const int last_pinned_tab_slot_index =
-      pinned_tab_count > 0 ? GetSlotIndexForTabModelIndex(
-                                 last_pinned_tab_index,
-                                 tabs->view_at(last_pinned_tab_index)->group())
+      pinned_tab_count > 0 ? GetSlotIndexForExistingTab(last_pinned_tab_index)
                            : TabStripModel::kNoTab;
 
   TabLayoutConstants layout_constants = GetTabLayoutConstants();
@@ -359,28 +338,87 @@ std::vector<gfx::Rect> TabStripLayoutHelper::CalculateIdealBounds(
                             tab_width_override_);
 }
 
-int TabStripLayoutHelper::GetSlotIndexForTabModelIndex(
-    int model_index,
+int TabStripLayoutHelper::GetSlotIndexForExistingTab(int model_index) const {
+  const int original_slot_index =
+      GetFirstSlotIndexForTabModelIndex(model_index);
+  CHECK_LT(original_slot_index, static_cast<int>(slots_.size()))
+      << "model_index = " << model_index
+      << " does not represent an existing tab";
+
+  int slot_index = original_slot_index;
+
+  if (slots_[slot_index].type == ViewType::kTab) {
+    CHECK(!slots_[slot_index].animation->IsClosing());
+    return slot_index;
+  }
+
+  // If |slot_index| is a group header we must return the next slot that
+  // is not animating closed.
+  if (slots_[slot_index].type == ViewType::kGroupHeader) {
+    // Skip all slots animating closed.
+    do {
+      slot_index += 1;
+    } while (slot_index < static_cast<int>(slots_.size()) &&
+             slots_[slot_index].animation->IsClosing());
+
+    // Double check we arrived at a tab.
+    CHECK_LT(slot_index, static_cast<int>(slots_.size()))
+        << "group header at " << original_slot_index
+        << " not followed by an open tab";
+    CHECK_EQ(slots_[slot_index].type, ViewType::kTab);
+  }
+
+  return slot_index;
+}
+
+int TabStripLayoutHelper::GetSlotInsertionIndexForNewTab(
+    int new_model_index,
     base::Optional<tab_groups::TabGroupId> group) const {
+  int slot_index = GetFirstSlotIndexForTabModelIndex(new_model_index);
+
+  if (slot_index == static_cast<int>(slots_.size()))
+    return slot_index;
+
+  // If |slot_index| points to a group header and the new tab's |group|
+  // matches, the tab goes to the right of the header to keep it
+  // contiguous.
+  if (slots_[slot_index].type == ViewType::kGroupHeader &&
+      static_cast<const TabGroupHeader*>(slots_[slot_index].view)->group() ==
+          group) {
+    return slot_index + 1;
+  }
+
+  return slot_index;
+}
+
+int TabStripLayoutHelper::GetFirstSlotIndexForTabModelIndex(
+    int model_index) const {
   int current_model_index = 0;
-  for (size_t i = 0; i < slots_.size(); i++) {
-    const bool model_space_index =
-        slots_[i].type == ViewType::kTab && !slots_[i].animation->IsClosing();
-    if (current_model_index == model_index) {
-      if (model_space_index) {
-        return i;
-      } else if (slots_[i].type == ViewType::kGroupHeader) {
-        // If the tab is in the header's group, then it should be to its right,
-        // so as to be contiguous with the group. If not, it goes to its left.
-        return static_cast<TabGroupHeader*>(slots_[i].view)->group() == group
-                   ? i + 1
-                   : i;
-      }
-    }
-    if (model_space_index)
-      ++current_model_index;
+
+  // Conceptually we assign a model index to each slot equal to the
+  // number of open tabs preceeding it. Group headers will have the same
+  // index as the tab before it, and each open tab will have the index
+  // of the previous slot plus 1. Closing tabs are not counted, and are
+  // skipped altogether.
+  //
+  // We simply return the first slot that has a matching model index.
+  for (int slot_index = 0; slot_index < static_cast<int>(slots_.size());
+       ++slot_index) {
+    if (slots_[slot_index].animation->IsClosing())
+      continue;
+
+    if (model_index == current_model_index)
+      return slot_index;
+
+    if (slots_[slot_index].type == ViewType::kTab)
+      current_model_index += 1;
   }
-  DCHECK_EQ(model_index, current_model_index);
+
+  // If there's no slot in |slots_| corresponding to |model_index|, then
+  // |model_index| may represent the first tab past the end of the
+  // tabstrip. In this case we should return the first-past-the-end
+  // index in |slots_|.
+  CHECK_EQ(current_model_index, model_index) << "model_index is too large";
   return slots_.size();
 }
 

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

@@ -5,7 +5,6 @@
 #ifndef CHROME_BROWSER_UI_VIEWS_TABS_TAB_STRIP_LAYOUT_HELPER_H_
 #define CHROME_BROWSER_UI_VIEWS_TABS_TAB_STRIP_LAYOUT_HELPER_H_
 
-#include <map>
 #include <vector>
 
 #include "base/callback.h"
@@ -33,12 +32,9 @@ class TabGroupId;
 class TabStripLayoutHelper {
  public:
   using GetTabsCallback = base::RepeatingCallback<views::ViewModelT<Tab>*()>;
-  using GetGroupHeadersCallback = base::RepeatingCallback<
-      std::map<tab_groups::TabGroupId, TabGroupHeader*>()>;
 
   TabStripLayoutHelper(const TabStripController* controller,
-                       GetTabsCallback get_tabs_callback,
-                       GetGroupHeadersCallback get_group_headers_callback);
+                       GetTabsCallback get_tabs_callback);
   TabStripLayoutHelper(const TabStripLayoutHelper&) = delete;
   TabStripLayoutHelper& operator=(const TabStripLayoutHelper&) = delete;
   ~TabStripLayoutHelper();
@@ -132,12 +128,26 @@ class TabStripLayoutHelper {
   std::vector<gfx::Rect> CalculateIdealBounds(
       base::Optional<int> available_width);
 
-  // Given a tab's |model_index| and |group|, returns the index of its
-  // corresponding TabSlot in |slots_|.
-  int GetSlotIndexForTabModelIndex(
-      int model_index,
+  // Given |model_index| for a tab already present in |slots_|, return
+  // the corresponding index in |slots_|.
+  int GetSlotIndexForExistingTab(int model_index) const;
+
+  // For a new tab at |new_model_index|, get the insertion index in
+  // |slots_|. |group| is the new tab's group.
+  int GetSlotInsertionIndexForNewTab(
+      int new_model_index,
       base::Optional<tab_groups::TabGroupId> group) const;
 
+  // Used internally in the above two functions. For a tabstrip with N
+  // tabs, this takes 0 <= |model_index| <= N and returns the first
+  // possible slot corresponding to this model index.
+  //
+  // This means that if |model_index| is the first tab in a group, the
+  // returned slot index will point to the group header. For other tabs,
+  // the slot index corresponding to that tab will be returned. Finally,
+  // if |model_index| = N, slots_.size() will be returned.
+  int GetFirstSlotIndexForTabModelIndex(int model_index) const;
+
   // Given a group ID, returns the index of its header's corresponding TabSlot
   // in |slots_|.
   int GetSlotIndexForGroupHeader(tab_groups::TabGroupId group) const;
@@ -164,9 +174,8 @@ class TabStripLayoutHelper {
   // The owning tabstrip's controller.
   const TabStripController* const controller_;
 
-  // Callbacks to get the necessary View objects from the owning tabstrip.
+  // Callback to get the necessary View objects from the owning tabstrip.
   GetTabsCallback get_tabs_callback_;
-  GetGroupHeadersCallback get_group_headers_callback_;
 
   // Current collation of tabs and group headers, along with necessary data to
   // run layout and animations for those Views.

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

@@ -1348,4 +1348,55 @@ TEST_P(TabStripTest, DISABLED_NewTabButtonFlushWithTopOfTabStrip) {
   // EXPECT_EQ(0, tab_strip_->new_tab_button()->bounds().y());
 }
 
+// Regression test for a crash when closing a tab under certain
+// conditions. If the first tab in a group was animating closed,
+// attempting to close the next tab could result in a crash. This was
+// due to TabStripLayoutHelper mistakenly mapping the next tab's model
+// index to the closing tab's slot. See https://crbug.com/1138748 for a
+// related crash.
+TEST_P(TabStripTest, CloseTabInGroupWhilePreviousTabAnimatingClosed) {
+  controller_->AddTab(0, true);
+  controller_->AddTab(1, false);
+  controller_->AddTab(2, false);
+
+  auto group_id = tab_groups::TabGroupId::GenerateNew();
+  controller_->MoveTabIntoGroup(1, group_id);
+  controller_->MoveTabIntoGroup(2, group_id);
+
+  CompleteAnimationAndLayout();
+  ASSERT_EQ(3, tab_strip_->tab_count());
+  ASSERT_EQ(3, tab_strip_->GetModelCount());
+  EXPECT_EQ(base::nullopt, tab_strip_->tab_at(0)->group());
+  EXPECT_EQ(group_id, tab_strip_->tab_at(1)->group());
+  EXPECT_EQ(group_id, tab_strip_->tab_at(2)->group());
+
+  // We have the following tabs:
+  // 1. An ungrouped tab with model index 0
+  // 2. A tab in |group_id| with model index 1
+  // 3. A tab in |group_id| with model index 2
+  controller_->RemoveTab(1);
+
+  // After closing the first tab, we now have:
+  // 1. An ungrouped tab with model index 0
+  // 2. A closing tab in |group_id| with no model index
+  // 3. A tab in |group_id| with model index 1.
+  //
+  // Closing the tab at model index 1 should result in (3) above being
+  // closed.
+  controller_->RemoveTab(1);
+
+  // We should now have:
+  // 1. An ungrouped tab with model index 0
+  // 2. A closing tab in |group_id| with no model index
+  // 3. A closing tab in |group_id| with no model index.
+
+  CompleteAnimationAndLayout();
+
+  // After finishing animations, there should be exactly 1 tab in no
+  // group.
+  EXPECT_EQ(1, tab_strip_->tab_count());
+  EXPECT_EQ(base::nullopt, tab_strip_->tab_at(0)->group());
+  EXPECT_EQ(1, tab_strip_->GetModelCount());
+}
+
 INSTANTIATE_TEST_SUITE_P(All, TabStripTest, ::testing::Values(false, true));