Use RemoveEntryById to erase the entry when restore recent closed tabs

This CL will use the method RemoveEntryById to remove the entry from
entries list of tab restore. Because the iterator may be invaild after
the time-consuming operation.

Change-Id: I70fc581f41fea1a9f88d496b92d367458b77f865
Bug: 390075226
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6176858
Reviewed-by: Calder Kitagawa <ckitagawa@chromium.org>
Commit-Queue: Calder Kitagawa <ckitagawa@chromium.org>
Reviewed-by: Stefan Kuhne <skuhne@chromium.org>
Reviewed-by: Sky Malice <skym@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1408035}

chrome/android/javatests/src/org/chromium/chrome/browser/ntp/RecentlyClosedBridgeTest.java

@@ -1605,6 +1605,86 @@ public class RecentlyClosedBridgeTest {
         assertTabsAre(recentTabs, titles, urls);
     }
 
+    /** Tests opening the closed tab group when another group is pending closure. */
+    @Test
+    @MediumTest
+    public void testOpenRecentlyClosedTabGroupEntryWhenHasPendingTabGroupClosure() {
+        if (mTabGroupModelFilter == null) return;
+
+        // Tab order is inverted in RecentlyClosedEntry as most recent comes first so log data in
+        // reverse.
+        final String[] group1Urls = new String[] {getUrl(TEST_PAGE_C), getUrl(TEST_PAGE_B)};
+        final String[] group2Urls = new String[] {getUrl(TEST_PAGE_A)};
+        final Tab tabA = sActivityTestRule.loadUrlInNewTab(group2Urls[0], /* incognito= */ false);
+        final Tab tabB = sActivityTestRule.loadUrlInNewTab(group1Urls[1], /* incognito= */ false);
+        final Tab tabC = sActivityTestRule.loadUrlInNewTab(group1Urls[0], /* incognito= */ false);
+
+        final String[] group1Titles = new String[2];
+        final String[] group2Titles = new String[1];
+        ThreadUtils.runOnUiThreadBlocking(
+                () -> {
+                    mTabGroupModelFilter.mergeTabsToGroup(tabC.getId(), tabB.getId());
+                    mTabGroupModelFilter.setTabGroupTitle(tabB.getId(), "Group 1");
+                    mTabGroupModelFilter.createSingleTabGroup(tabA, true);
+                    mTabGroupModelFilter.setTabGroupTitle(tabA.getId(), "Group 2");
+                    group2Titles[0] = tabA.getTitle();
+                    group1Titles[1] = tabB.getTitle();
+                    group1Titles[0] = tabC.getTitle();
+                    closeTabs(
+                            TabClosureParams.closeTabs(List.of(tabB, tabC))
+                                    .hideTabGroups(false)
+                                    .build());
+                    mTabModel.commitAllTabClosures();
+                });
+        final List<RecentlyClosedEntry> recentEntries = new ArrayList<>();
+        int tabCount = getRecentEntriesAndReturnActiveTabCount(recentEntries);
+        Assert.assertEquals(2, tabCount);
+        Assert.assertEquals(1, recentEntries.size());
+        assertEntryIs(
+                recentEntries.get(0),
+                RecentlyClosedGroup.class,
+                new String[] {"Group 1"},
+                group1Titles,
+                group1Urls);
+
+        // Close Group 2 and restore Group 1.
+        ThreadUtils.runOnUiThreadBlocking(
+                () -> {
+                    closeTabs(
+                            TabClosureParams.closeTabs(List.of(tabA)).hideTabGroups(false).build());
+                    mRecentlyClosedBridge.openMostRecentlyClosedEntry(mTabModel);
+                });
+
+        // 1. Blank tab
+        // 2. Restored tabB
+        // 3. Restored tabC
+        final List<Tab> tabs = getAllTabs();
+        Assert.assertEquals(3, tabs.size());
+        Assert.assertEquals(group1Titles[1], ChromeTabUtils.getTitleOnUiThread(tabs.get(1)));
+        Assert.assertEquals(group1Urls[1], ChromeTabUtils.getUrlOnUiThread(tabs.get(1)).getSpec());
+        Assert.assertEquals(group1Titles[0], ChromeTabUtils.getTitleOnUiThread(tabs.get(2)));
+        Assert.assertEquals(group1Urls[0], ChromeTabUtils.getUrlOnUiThread(tabs.get(2)).getSpec());
+        ThreadUtils.runOnUiThreadBlocking(
+                () -> {
+                    Assert.assertEquals(
+                            "Group 1", mTabGroupModelFilter.getTabGroupTitle(tabs.get(1).getId()));
+                    Assert.assertTrue(mTabGroupModelFilter.isTabInTabGroup(tabs.get(1)));
+                    Assert.assertEquals(
+                            Arrays.asList(new Tab[] {tabs.get(1), tabs.get(2)}),
+                            mTabGroupModelFilter.getRelatedTabList(tabs.get(1).getId()));
+                });
+
+        tabCount = getRecentEntriesAndReturnActiveTabCount(recentEntries);
+        Assert.assertEquals(3, tabCount);
+        Assert.assertEquals(1, recentEntries.size());
+        assertEntryIs(
+                recentEntries.get(0),
+                RecentlyClosedGroup.class,
+                new String[] {"Group 2"},
+                group2Titles,
+                group2Urls);
+    }
+
     // TODO(crbug.com/40218713): Add a test a case where bulk closures remain in the native service,
     // but the flag state is flipped.
 

components/sessions/core/tab_restore_service_helper.cc

@@ -692,7 +692,10 @@ std::vector<LiveTab*> TabRestoreServiceHelper::RestoreEntryById(
 
         if (window.tabs.empty()) {
           // Remove the entry if there is nothing left to restore.
-          entries_.erase(entry_iterator);
+          // The entries_ may by changed after the tabs restored and the
+          // entry_iterator may be no longer valid. So call RemoveEntryById here
+          // instead of entries_.erase(entry_iterator).
+          RemoveEntryById(id);
         }
       }
 
@@ -745,7 +748,10 @@ std::vector<LiveTab*> TabRestoreServiceHelper::RestoreEntryById(
             CHECK(ValidateGroup(group));
             group.tabs.erase(group.tabs.begin() + i);
             if (group.tabs.empty()) {
-              entries_.erase(entry_iterator);
+              // The entries_ may by changed after the tabs restored and the
+              // entry_iterator may be no longer valid. So call RemoveEntryById
+              // here instead of entries_.erase(entry_iterator).
+              RemoveEntryById(id);
             }
 
             break;
@@ -759,7 +765,10 @@ std::vector<LiveTab*> TabRestoreServiceHelper::RestoreEntryById(
   }
 
   if (entry_id_matches_restore_id) {
-    entries_.erase(entry_iterator);
+    // The entries_ may by changed after the tabs restored and the
+    // entry_iterator may be no longer valid. So call RemoveEntryById here
+    // instead of entries_.erase(entry_iterator).
+    RemoveEntryById(id);
   }
 
   restoring_ = false;