Fix crash when deleting a tab group and moving a tab at the same time.

If the only tab in a group is added to another group in a way that
causes that tab to move (e.g. there is an intervening ungrouped tab)
then we will crash. This is caused by TabStripModel setting the new
group on WebContentsData and then sending the kMoved notification before
the kGroupChanged notification. TabStripLayoutHelper::MoveTab expects
the group of the tab being moved is not empty according to the model.

This patch fixes the bug by sending kGroupChanged immediately after
setting the group on WebContentsData, so TabStrip will delete the old
group immediately.

Bug: 983961
Change-Id: Ib214a1b7a82f5958e48310477ba9c63d832e3095
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1753059
Commit-Queue: Bret Sepulveda <bsep@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#686670}

chrome/browser/ui/tabs/tab_strip_model.cc

@@ -1889,18 +1889,16 @@ void TabStripModel::MoveTabsIntoGroup(const std::vector<int>& indices,
 void TabStripModel::MoveAndSetGroup(int index,
                                     int new_index,
                                     base::Optional<TabGroupId> new_group) {
+  // Ungroup tab before moving, so that if this is the last tab in the group
+  // observers can delete that group.
   base::Optional<TabGroupId> old_group = UngroupTab(index);
-
-  // TODO(crbug.com/940677): Ideally the delta of type kGroupChanged below would
-  // be batched with the move deltas resulting from MoveWebContentsAt, but that
-  // is not possible right now.
-  if (index != new_index)
-    MoveWebContentsAtImpl(index, new_index, false);
-  contents_data_[new_index]->set_group(new_group);
+  contents_data_[index]->set_group(new_group);
   if (new_group.has_value())
     group_data_.at(new_group.value()).TabAdded();
+  NotifyGroupChange(index, old_group, new_group);
 
-  NotifyGroupChange(new_index, old_group, new_group);
+  if (index != new_index)
+    MoveWebContentsAtImpl(index, new_index, false);
 }
 
 void TabStripModel::NotifyGroupChange(int index,

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

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/views/tabs/tab_strip.h"
+
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_tabstrip.h"
+#include "chrome/browser/ui/tabs/tab_group_id.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/views/frame/browser_view.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "url/gurl.h"
+
+// Integration tests for interactions between TabStripModel and TabStrip.
+class TabStripBrowsertest : public InProcessBrowserTest {
+ public:
+  TabStripModel* tab_strip_model() { return browser()->tab_strip_model(); }
+
+  TabStrip* tab_strip() {
+    return BrowserView::GetBrowserViewForBrowser(browser())->tabstrip();
+  }
+
+  void AppendTab() { chrome::AddTabAt(browser(), GURL(), -1, true); }
+
+  TabGroupId AddTabToNewGroup(int tab_index) {
+    tab_strip_model()->AddToNewGroup({tab_index});
+    return tab_strip_model()->GetTabGroupForTab(tab_index).value();
+  }
+};
+
+// Regression test for crbug.com/983961.
+IN_PROC_BROWSER_TEST_F(TabStripBrowsertest, MoveTabAndDeleteGroup) {
+  AppendTab();
+  AppendTab();
+
+  TabGroupId group = AddTabToNewGroup(0);
+  AddTabToNewGroup(2);
+
+  Tab* tab0 = tab_strip()->tab_at(0);
+  Tab* tab1 = tab_strip()->tab_at(1);
+  Tab* tab2 = tab_strip()->tab_at(2);
+
+  tab_strip_model()->AddToExistingGroup({2}, group);
+
+  EXPECT_EQ(tab0, tab_strip()->tab_at(0));
+  EXPECT_EQ(tab2, tab_strip()->tab_at(1));
+  EXPECT_EQ(tab1, tab_strip()->tab_at(2));
+
+  EXPECT_EQ(group, tab_strip_model()->GetTabGroupForTab(1));
+
+  std::vector<TabGroupId> groups = tab_strip_model()->ListTabGroups();
+  EXPECT_EQ(groups.size(), 1U);
+  EXPECT_EQ(groups[0], group);
+}

chrome/test/BUILD.gn

@@ -1094,6 +1094,7 @@ if (!is_android) {
       "../browser/ui/passwords/google_password_manager_navigation_throttle_browsertest.cc",
       "../browser/ui/tabs/pinned_tab_service_browsertest.cc",
       "../browser/ui/views/tabs/tab_hover_card_bubble_view_browsertest.cc",
+      "../browser/ui/views/tabs/tab_strip_browsertest.cc",
       "../browser/wake_lock/wake_lock_browsertest.cc",
 
       # If this list is used on Android in the future, these browser / speech/*