Make tab groups not crash if they become discontinuous.

We've had several bugs where tab groups could become discontinuous and
it would be nice if we didn't crash when this happens, so the user can
recover. This patch adds a tab reference count to groups, so they are
guaranteed to not have any tabs in them when they are deleted, even if
the group is not continuous.

Bug: 905491
Change-Id: If585e0c5169830280efa09b37402dbd3367359e1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1709160
Commit-Queue: Charlene Yan <cyan@chromium.org>
Reviewed-by: Charlene Yan <cyan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#679566}

chrome/browser/ui/tabs/tab_group_data.cc

@@ -8,7 +8,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "third_party/skia/include/utils/SkRandom.h"
 
-TabGroupData::TabGroupData() {
+TabGroupData::TabGroupData() : tab_count_(0) {
   static int next_placeholder_title_number = 1;
   title_ = base::ASCIIToUTF16(
       "Group " + base::NumberToString(next_placeholder_title_number));
@@ -17,3 +17,12 @@ TabGroupData::TabGroupData() {
   static SkRandom rand;
   color_ = rand.nextU() | 0xff000000;
 }
+
+void TabGroupData::TabAdded() {
+  ++tab_count_;
+}
+
+void TabGroupData::TabRemoved() {
+  DCHECK_GT(tab_count_, 0);
+  --tab_count_;
+}

chrome/browser/ui/tabs/tab_group_data.h

@@ -19,10 +19,17 @@ class TabGroupData {
 
   base::string16 title() const { return title_; }
   SkColor color() const { return color_; }
+  bool empty() const { return tab_count_ == 0; }
+
+  // Used to reference count the number of tabs in the group, so TabStripModel
+  // knows when this group is safe to delete.
+  void TabAdded();
+  void TabRemoved();
 
  private:
   base::string16 title_;
   SkColor color_;
+  int tab_count_;
 
   DISALLOW_COPY_AND_ASSIGN(TabGroupData);
 };

chrome/browser/ui/tabs/tab_strip_model.cc

@@ -1486,8 +1486,9 @@ int TabStripModel::InsertWebContentsAtImpl(
     }
     data->set_opener(active_contents);
   }
+  data->set_group(group);
   if (group.has_value())
-    data->set_group(group);
+    group_data_[group.value()]->TabAdded();
 
   // TODO(gbillock): Ask the modal dialog manager whether the WebContents should
   // be blocked, or just let the modal dialog manager make the blocking call
@@ -1840,8 +1841,9 @@ void TabStripModel::MoveAndSetGroup(int index,
   // is not possible right now.
   if (index != new_index)
     MoveWebContentsAtImpl(index, new_index, false);
-  WebContentsData* contents_data = contents_data_[new_index].get();
-  contents_data->set_group(new_group);
+  contents_data_[new_index]->set_group(new_group);
+  if (new_group.has_value())
+    group_data_[new_group.value()]->TabAdded();
 
   NotifyGroupChange(new_index, old_group, new_group);
 }
@@ -1868,15 +1870,11 @@ base::Optional<TabGroupId> TabStripModel::UngroupTab(int index) {
     return base::nullopt;
 
   contents_data_[index]->set_group(base::nullopt);
-
+  TabGroupData* group_data = group_data_[group.value()].get();
+  group_data->TabRemoved();
   // Delete the group if we just ungrouped the last tab in that group.
-  if ((!ContainsIndex(index + 1) || GetTabGroupForTab(index + 1) != group) &&
-      (!ContainsIndex(index - 1) || GetTabGroupForTab(index - 1) != group)) {
-    DCHECK(!std::any_of(
-        contents_data_.cbegin(), contents_data_.cend(),
-        [group](const auto& datum) { return datum->group() == group; }));
+  if (group_data->empty())
     group_data_.erase(group.value());
-  }
   return group;
 }
 

chrome/browser/ui/tabs/tab_strip_model_unittest.cc

@@ -3326,6 +3326,22 @@ TEST_F(TabStripModelTest, NewTabWithGroup) {
   strip.CloseAllTabs();
 }
 
+TEST_F(TabStripModelTest, NewTabWithGroupDeletedCorrectly) {
+  TestTabStripModelDelegate delegate;
+  TabStripModel strip(&delegate, profile());
+  strip.AppendWebContents(CreateWebContents(), true);
+  strip.AddToNewGroup({0});
+  strip.InsertWebContentsAt(1, CreateWebContents(), TabStripModel::ADD_NONE,
+                            strip.GetTabGroupForTab(0));
+
+  strip.RemoveFromGroup({1});
+  EXPECT_EQ(strip.ListTabGroups().size(), 1U);
+  strip.RemoveFromGroup({0});
+  EXPECT_EQ(strip.ListTabGroups().size(), 0U);
+
+  strip.CloseAllTabs();
+}
+
 TEST_F(TabStripModelTest, NewTabWithoutIndexInsertsAtEndOfGroup) {
   TestTabStripModelDelegate delegate;
   TabStripModel strip(&delegate, profile());