From 8c6e83aa8a20f8d1af9aafb86df0d0a77899460c Mon Sep 17 00:00:00 2001
From: Calder Kitagawa <[email protected]>
Date: Fri, 17 Jul 2026 12:10:36 -0700
Subject: [PATCH] [Tab Restore] Fix issue if restore and closure are concurrent

If adding to the TabRestoreService is concurrent with a
restore then the internal state of the service can become
broken (invalid data pointers). Fix this by adding the
`restoring_` check to other restore cases.

To help reduce places where this could occur; commit tab
closures in Java BEFORE starting any restore operation.

Fixed: 501572758
Change-Id: I59ebe30b479c45ed5be3e6deb32f33bd6a6a6964
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8116512
Reviewed-by: Fiaz Muhammad <[email protected]>
Commit-Queue: Calder Kitagawa <[email protected]>
Reviewed-by: Darryl James <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1664102}
---
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentlyClosedBridge.java b/chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentlyClosedBridge.java
index b0080e4..8d6bc9d 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentlyClosedBridge.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/ntp/RecentlyClosedBridge.java
@@ -137,10 +137,24 @@
         return received ? entries : null;
     }
 
+    /**
+     * Commits all pending tab closures in the regular tab model. Restoring a tab can synchronously
+     * commit pending tab group closures, which calls back into TabRestoreService's
+     * CreateHistoricalGroup. To prevent concurrent modification issues in TabRestoreService, we
+     * commit all pending closures before starting restoration.
+     */
+    private void commitTabClosuresInRegularModel() {
+        TabModel regularModel = mTabModelSelector.getModel(/* incognito= */ false);
+        if (regularModel != null) {
+            regularModel.commitAllTabClosures();
+        }
+    }
+
     @Override
     public boolean openRecentlyClosedTab(
             TabModel tabModel, RecentlyClosedTab recentTab, int windowOpenDisposition) {
         assert mTabModelSelector.getModel(tabModel.isIncognito()) == tabModel;
+        commitTabClosuresInRegularModel();
         return RecentlyClosedBridgeJni.get()
                 .openRecentlyClosedTab(
                         mNativeBridge, tabModel, recentTab.getSessionId(), windowOpenDisposition);
@@ -150,6 +164,7 @@
     public boolean openRecentlyClosedEntry(TabModel tabModel, RecentlyClosedEntry recentEntry) {
         assert mTabModelSelector.getModel(tabModel.isIncognitoBranded()) == tabModel
                 && recentEntry instanceof SessionRecentlyClosedEntry;
+        commitTabClosuresInRegularModel();
         SessionRecentlyClosedEntry sessionRecentEntry = (SessionRecentlyClosedEntry) recentEntry;
         return RecentlyClosedBridgeJni.get()
                 .openRecentlyClosedEntry(
@@ -159,6 +174,7 @@
     @Override
     public void openMostRecentlyClosedEntry(TabModel tabModel) {
         assert mTabModelSelector.getModel(tabModel.isIncognito()) == tabModel;
+        commitTabClosuresInRegularModel();
         RecentlyClosedBridgeJni.get().openMostRecentlyClosedEntry(mNativeBridge, tabModel);
     }
 
diff --git a/chrome/android/javatests/src/org/chromium/chrome/browser/ntp/RecentlyClosedBridgeTest.java b/chrome/android/javatests/src/org/chromium/chrome/browser/ntp/RecentlyClosedBridgeTest.java
index 509a9bdc..f65fc05 100644
--- a/chrome/android/javatests/src/org/chromium/chrome/browser/ntp/RecentlyClosedBridgeTest.java
+++ b/chrome/android/javatests/src/org/chromium/chrome/browser/ntp/RecentlyClosedBridgeTest.java
@@ -1670,33 +1670,26 @@
                 });
 
         // 1. Blank tab
-        // 2. Restored tabB
-        // 3. Restored tabC
+        // 2. Restored tabA (Group 2)
         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());
+        Assert.assertEquals(2, tabs.size());
+        Assert.assertEquals(group2Titles[0], ChromeTabUtils.getTitleOnUiThread(tabs.get(1)));
+        Assert.assertEquals(group2Urls[0], ChromeTabUtils.getUrlOnUiThread(tabs.get(1)).getSpec());
         ThreadUtils.runOnUiThreadBlocking(
                 () -> {
                     Assert.assertEquals(
-                            "Group 1", mTabModel.getTabGroupTitle(tabs.get(1).getTabGroupId()));
+                            "Group 2", mTabModel.getTabGroupTitle(tabs.get(1).getTabGroupId()));
                     Assert.assertTrue(mTabModel.isTabInTabGroup(tabs.get(1)));
-                    Assert.assertEquals(
-                            Arrays.asList(new Tab[] {tabs.get(1), tabs.get(2)}),
-                            mTabModel.getRelatedTabList(tabs.get(1).getId()));
                 });
-
         tabCount = getRecentEntriesAndReturnActiveTabCount(recentEntries);
-        Assert.assertEquals(3, tabCount);
+        Assert.assertEquals(2, tabCount);
         Assert.assertEquals(1, recentEntries.size());
         assertEntryIs(
                 recentEntries.get(0),
                 RecentlyClosedGroup.class,
-                new String[] {"Group 2"},
-                group2Titles,
-                group2Urls);
+                new String[] {"Group 1"},
+                group1Titles,
+                group1Urls);
     }
 
     // TODO(crbug.com/40218713): Add a test a case where bulk closures remain in the native service,
@@ -1763,6 +1756,62 @@
         assertTabsAre(event.getTabs(), titles, urls);
     }
 
+    @Test
+    @MediumTest
+    public void testOpenRecentlyClosedTab_CommitsPendingClosures() {
+        final String urlA = getUrl(TEST_PAGE_A);
+        final String urlB = getUrl(TEST_PAGE_B);
+        final Tab tabA = mActivityTestRule.loadUrlInNewTab(urlA, /* incognito= */ false);
+        final Tab tabB = mActivityTestRule.loadUrlInNewTab(urlB, /* incognito= */ false);
+
+        ThreadUtils.runOnUiThreadBlocking(
+                () -> {
+                    // Close tabB without undo so it goes to recently closed.
+                    closeTabs(TabClosureParams.closeTab(tabB).allowUndo(false).build());
+                    // Close tabA with undo so it is pending.
+                    closeTabs(TabClosureParams.closeTab(tabA).allowUndo(true).build());
+                });
+
+        // Verify tabA closure is pending.
+        ThreadUtils.runOnUiThreadBlocking(
+                () -> {
+                    Assert.assertTrue(mTabModel.isClosurePending(tabA.getId()));
+                });
+
+        final List<RecentlyClosedEntry> recentEntries = new ArrayList<>();
+        ThreadUtils.runOnUiThreadBlocking(
+                () -> {
+                    recentEntries.addAll(
+                            mRecentlyClosedBridge.getRecentlyClosedEntries(MAX_ENTRY_COUNT));
+                });
+
+        RecentlyClosedTab recentTabB = null;
+        for (RecentlyClosedEntry entry : recentEntries) {
+            if (entry instanceof RecentlyClosedTab) {
+                RecentlyClosedTab rt = (RecentlyClosedTab) entry;
+                if (rt.getUrl().getSpec().equals(urlB)) {
+                    recentTabB = rt;
+                    break;
+                }
+            }
+        }
+        Assert.assertNotNull(recentTabB);
+        final RecentlyClosedTab finalRecentTabB = recentTabB;
+
+        ThreadUtils.runOnUiThreadBlocking(
+                () -> {
+                    // Restore tabB. This should commit tabA's closure.
+                    mRecentlyClosedBridge.openRecentlyClosedTab(
+                            mTabModel, finalRecentTabB, WindowOpenDisposition.NEW_FOREGROUND_TAB);
+                });
+
+        // Verify tabA closure is no longer pending (it was committed).
+        ThreadUtils.runOnUiThreadBlocking(
+                () -> {
+                    Assert.assertFalse(mTabModel.isClosurePending(tabA.getId()));
+                });
+    }
+
     private List<Tab> getAllTabs() {
         final List<Tab> list = new ArrayList<>();
         ThreadUtils.runOnUiThreadBlocking(
diff --git a/components/sessions/core/tab_restore_service_helper.cc b/components/sessions/core/tab_restore_service_helper.cc
index 14a14d2..8ee937a 100644
--- a/components/sessions/core/tab_restore_service_helper.cc
+++ b/components/sessions/core/tab_restore_service_helper.cc
@@ -304,6 +304,10 @@
 }
 
 void TabRestoreServiceHelper::BrowserClosing(LiveTabContext* context) {
+  if (restoring_) {
+    return;
+  }
+
   closing_contexts_.insert(context);
 
   auto window = std::make_unique<Window>();
@@ -386,6 +390,10 @@
 void TabRestoreServiceHelper::CreateHistoricalGroup(
     LiveTabContext* context,
     const tab_groups::TabGroupId& id) {
+  if (restoring_) {
+    return;
+  }
+
   closing_groups_.insert(id);
 
   auto group = CreateHistoricalGroupImpl(context, id);
@@ -397,6 +405,10 @@
 void TabRestoreServiceHelper::CreateHistoricalSplit(
     LiveTabContext* context,
     const split_tabs::SplitTabId& id) {
+  if (restoring_) {
+    return;
+  }
+
   std::vector<std::pair<int, LiveTab*>> split_tabs;
   for (int tab_index = 0; tab_index < context->GetTabCount(); ++tab_index) {
     if (context->GetSplitForTab(tab_index) == id) {
diff --git a/chrome/browser/sessions/tab_restore_service_unittest.cc b/chrome/browser/sessions/tab_restore_service_unittest.cc
index a495e72..e5f6eb37 100644
--- a/chrome/browser/sessions/tab_restore_service_unittest.cc
+++ b/chrome/browser/sessions/tab_restore_service_unittest.cc
@@ -1645,3 +1645,76 @@
 
   EXPECT_TRUE(service_->entries().empty());
 }
+
+TEST_F(TabRestoreServiceImplWithMockClientTest,
+       DontCreateGroupEntryWhileRestoring) {
+  ON_CALL(*mock_tab_restore_service_client_, ShouldTrackURLForRestore(_))
+      .WillByDefault(Return(true));
+
+  // Create a group entry with two tabs and place it at the back of the list so
+  // that it would be the first entry dropped if the list were ever pruned.
+  auto group = std::make_unique<sessions::tab_restore::Group>();
+  group->group_id = tab_groups::TabGroupId::GenerateNew();
+  for (int i = 0; i < 2; ++i) {
+    auto tab = std::make_unique<Tab>();
+    tab->navigations.push_back(ContentTestHelper::CreateNavigation(
+        base::StringPrintf("http://group/%d", i), "title"));
+    tab->current_navigation_index = 0;
+    group->tabs.push_back(std::move(tab));
+  }
+  const SessionID group_entry_id = group->id;
+  mutable_entries()->push_back(std::move(group));
+
+  // Fill the rest of the list up to kMaxEntries with tab entries that are
+  // newer than the group entry.
+  const size_t max_entries = kMaxEntries;
+  for (size_t i = 0; i < max_entries - 1; ++i) {
+    auto tab = std::make_unique<Tab>();
+    tab->navigations.push_back(ContentTestHelper::CreateNavigation(
+        base::StringPrintf("http://%d", static_cast<int>(i)), "title"));
+    tab->current_navigation_index = 0;
+    mutable_entries()->push_front(std::move(tab));
+  }
+  ASSERT_EQ(max_entries, service_->entries().size());
+
+  // A separate context that simulates a tab group closure being committed
+  // while the restore below is in progress.
+  testing::NiceMock<MockLiveTabContext> closing_context;
+  testing::NiceMock<MockLiveTab> closing_tab;
+  tab_groups::TabGroupId closing_group_id =
+      tab_groups::TabGroupId::GenerateNew();
+  tab_groups::TabGroupVisualData closing_group_visual_data;
+  ON_CALL(closing_context, GetSessionID())
+      .WillByDefault(Return(SessionID::NewUnique()));
+  ON_CALL(closing_context, GetTabCount()).WillByDefault(Return(1));
+  ON_CALL(closing_context, GetTabGroupForTab(0))
+      .WillByDefault(Return(closing_group_id));
+  ON_CALL(closing_context, GetVisualDataForGroup(_))
+      .WillByDefault(Return(&closing_group_visual_data));
+  ON_CALL(closing_context, GetLiveTabAt(0)).WillByDefault(Return(&closing_tab));
+  ON_CALL(closing_tab, GetEntryCount()).WillByDefault(Return(1));
+  ON_CALL(closing_tab, GetEntryAtIndex(_))
+      .WillByDefault(
+          Return(ContentTestHelper::CreateNavigation("http://closing", "T")));
+
+  // Restoring a tab can synchronously commit pending tab group closures, which
+  // calls back into CreateHistoricalGroup. The service must not add a new
+  // entry while a restore is already in progress.
+  testing::NiceMock<MockLiveTabContext> restore_context;
+  int restored_tab_count = 0;
+  ON_CALL(restore_context, AddRestoredTab(_, _, _, _, _))
+      .WillByDefault([&](const sessions::tab_restore::Tab&, int, bool, bool,
+                         sessions::tab_restore::Type) -> sessions::LiveTab* {
+        ++restored_tab_count;
+        EXPECT_TRUE(service_->IsRestoring());
+        service_->CreateHistoricalGroup(&closing_context, closing_group_id);
+        EXPECT_EQ(group_entry_id, service_->entries().back()->id);
+        return nullptr;
+      });
+
+  service_->RestoreEntryById(&restore_context, group_entry_id,
+                             WindowOpenDisposition::NEW_FOREGROUND_TAB);
+
+  EXPECT_EQ(2, restored_tab_count);
+  EXPECT_EQ(max_entries - 1, service_->entries().size());
+}

diff --git a/chrome/browser/sessions/tab_restore_service_unittest.cc b/chrome/browser/sessions/tab_restore_service_unittest.cc
index a495e72..e5f6eb37 100644
--- a/chrome/browser/sessions/tab_restore_service_unittest.cc
+++ b/chrome/browser/sessions/tab_restore_service_unittest.cc
@@ -1645,3 +1645,76 @@
 
   EXPECT_TRUE(service_->entries().empty());
 }
+
+TEST_F(TabRestoreServiceImplWithMockClientTest,
+       DontCreateGroupEntryWhileRestoring) {
+  ON_CALL(*mock_tab_restore_service_client_, ShouldTrackURLForRestore(_))
+      .WillByDefault(Return(true));
+
+  // Create a group entry with two tabs and place it at the back of the list so
+  // that it would be the first entry dropped if the list were ever pruned.
+  auto group = std::make_unique<sessions::tab_restore::Group>();
+  group->group_id = tab_groups::TabGroupId::GenerateNew();
+  for (int i = 0; i < 2; ++i) {
+    auto tab = std::make_unique<Tab>();
+    tab->navigations.push_back(ContentTestHelper::CreateNavigation(
+        base::StringPrintf("http://group/%d", i), "title"));
+    tab->current_navigation_index = 0;
+    group->tabs.push_back(std::move(tab));
+  }
+  const SessionID group_entry_id = group->id;
+  mutable_entries()->push_back(std::move(group));
+
+  // Fill the rest of the list up to kMaxEntries with tab entries that are
+  // newer than the group entry.
+  const size_t max_entries = kMaxEntries;
+  for (size_t i = 0; i < max_entries - 1; ++i) {
+    auto tab = std::make_unique<Tab>();
+    tab->navigations.push_back(ContentTestHelper::CreateNavigation(
+        base::StringPrintf("http://%d", static_cast<int>(i)), "title"));
+    tab->current_navigation_index = 0;
+    mutable_entries()->push_front(std::move(tab));
+  }
+  ASSERT_EQ(max_entries, service_->entries().size());
+
+  // A separate context that simulates a tab group closure being committed
+  // while the restore below is in progress.
+  testing::NiceMock<MockLiveTabContext> closing_context;
+  testing::NiceMock<MockLiveTab> closing_tab;
+  tab_groups::TabGroupId closing_group_id =
+      tab_groups::TabGroupId::GenerateNew();
+  tab_groups::TabGroupVisualData closing_group_visual_data;
+  ON_CALL(closing_context, GetSessionID())
+      .WillByDefault(Return(SessionID::NewUnique()));
+  ON_CALL(closing_context, GetTabCount()).WillByDefault(Return(1));
+  ON_CALL(closing_context, GetTabGroupForTab(0))
+      .WillByDefault(Return(closing_group_id));
+  ON_CALL(closing_context, GetVisualDataForGroup(_))
+      .WillByDefault(Return(&closing_group_visual_data));
+  ON_CALL(closing_context, GetLiveTabAt(0)).WillByDefault(Return(&closing_tab));
+  ON_CALL(closing_tab, GetEntryCount()).WillByDefault(Return(1));
+  ON_CALL(closing_tab, GetEntryAtIndex(_))
+      .WillByDefault(
+          Return(ContentTestHelper::CreateNavigation("http://closing", "T")));
+
+  // Restoring a tab can synchronously commit pending tab group closures, which
+  // calls back into CreateHistoricalGroup. The service must not add a new
+  // entry while a restore is already in progress.
+  testing::NiceMock<MockLiveTabContext> restore_context;
+  int restored_tab_count = 0;
+  ON_CALL(restore_context, AddRestoredTab(_, _, _, _, _))
+      .WillByDefault([&](const sessions::tab_restore::Tab&, int, bool, bool,
+                         sessions::tab_restore::Type) -> sessions::LiveTab* {
+        ++restored_tab_count;
+        EXPECT_TRUE(service_->IsRestoring());
+        service_->CreateHistoricalGroup(&closing_context, closing_group_id);
+        EXPECT_EQ(group_entry_id, service_->entries().back()->id);
+        return nullptr;
+      });
+
+  service_->RestoreEntryById(&restore_context, group_entry_id,
+                             WindowOpenDisposition::NEW_FOREGROUND_TAB);
+
+  EXPECT_EQ(2, restored_tab_count);
+  EXPECT_EQ(max_entries - 1, service_->entries().size());
+}