[SavedTabGroups] Default save unsaved when updating from V1 to V2

This CL addresses 4 different issues with enabling default save in V2,

1) Enabled default save of groups restored from session
2) Fixed to support asynchronous saving of groups at startup since
the model initialization might have not completed
3) Fixed an issue where session data wasn't restored correctly for the
saved GUID when there were multiple records in the append log
4) Tab Group session metadata was only updated when there was a change
in visual data. This led to cases where the saved GUID wasn't updated.
This was updated to listen for saved tab group model changes when groups
were saved or unsaved

Change-Id: I4d2255c11ad4022be701f0ee32115f5d8977ae6a
Bug: 344016224
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5608434
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: David Pennington <dpenning@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Darryl James <dljames@chromium.org>
Commit-Queue: Eshwar Stalin <estalin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1313032}

chrome/browser/sessions/session_restore.cc

@@ -79,6 +79,7 @@
 #include "chrome/common/url_constants.h"
 #include "components/keep_alive_registry/keep_alive_types.h"
 #include "components/keep_alive_registry/scoped_keep_alive.h"
+#include "components/saved_tab_groups/features.h"
 #include "components/sessions/core/session_types.h"
 #include "components/tab_groups/tab_group_id.h"
 #include "content/public/browser/child_process_security_policy.h"
@@ -959,13 +960,19 @@ class SessionRestoreImpl : public BrowserListObserver {
 
     for (const std::unique_ptr<sessions::SessionTabGroup>& session_tab_group :
          tab_groups) {
-      if (session_tab_group->saved_guid.has_value() &&
-          saved_tab_group_keyed_service) {
-        const base::Uuid& saved_guid =
-            base::Uuid::ParseLowercase(session_tab_group->saved_guid.value());
+      if (saved_tab_group_keyed_service) {
+        if (session_tab_group->saved_guid.has_value()) {
+          const base::Uuid& saved_guid =
+              base::Uuid::ParseLowercase(session_tab_group->saved_guid.value());
 
-        saved_tab_group_keyed_service->StoreLocalToSavedId(
-            saved_guid, new_group_ids.at(session_tab_group->id));
+          saved_tab_group_keyed_service->ConnectRestoredGroupToSaveId(
+              saved_guid, new_group_ids.at(session_tab_group->id));
+        } else if (tab_groups::IsTabGroupsSaveV2Enabled()) {
+          // Default save any groups that are not save yet. This happens when
+          // a user goes from V1 of SavedTabGroups to V2 through an update.
+          saved_tab_group_keyed_service->SaveRestoredGroup(
+              new_group_ids.at(session_tab_group->id));
+        }
       }
 
       TabGroup* model_tab_group =

chrome/browser/sessions/session_restore_browsertest.cc

@@ -83,12 +83,14 @@
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
+#include "chrome/common/webui_url_constants.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/testing_profile.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/keep_alive_registry/keep_alive_types.h"
 #include "components/keep_alive_registry/scoped_keep_alive.h"
 #include "components/memory_pressure/fake_memory_pressure_monitor.h"
+#include "components/saved_tab_groups/features.h"
 #include "components/sessions/content/content_live_tab.h"
 #include "components/sessions/content/content_test_helper.h"
 #include "components/sessions/core/serialized_navigation_entry_test_helper.h"
@@ -4515,3 +4517,159 @@ IN_PROC_BROWSER_TEST_P(SessionRestoreStaleSessionCookieDeletionTest,
   EXPECT_EQ(HasCookie(new_browser, "other_page_session_stale_cookie"),
             !ShouldDeleteStaleSessionCookiesOnStartup());
 }
+
+class SavedTabGroupSessionRestoreTest : public SessionRestoreTest {
+ public:
+  SavedTabGroupSessionRestoreTest() {
+    feature_list_.InitAndEnableFeature(tab_groups::kTabGroupsSaveV2);
+  }
+  SavedTabGroupSessionRestoreTest(const SavedTabGroupSessionRestoreTest&) =
+      delete;
+  SavedTabGroupSessionRestoreTest& operator=(
+      const SavedTabGroupSessionRestoreTest&) = delete;
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
+// This test simulates migrating from V1 of SavedTabGroups to V2. A user may
+// have unsaved groups at the time they update the browser. We must ensure all
+// groups are saved by default correctly. See crbug.com/344016224.
+IN_PROC_BROWSER_TEST_F(SavedTabGroupSessionRestoreTest,
+                       UnsavedGroupDefaultSavedAfterBrowserRestart) {
+  // Add a second tab.
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), GURL(chrome::kChromeUINewTabPageURL),
+      WindowOpenDisposition::NEW_FOREGROUND_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP);
+
+  // Add the tab to a group. We use the restore version of the AddToGroup
+  // function to prevent the group from being saved by default. The group id sis
+  // respun when restoring to prevent collisions. Just create an arbitrary one
+  // for now.
+  browser()->tab_strip_model()->AddToGroupForRestore(
+      {0}, tab_groups::TabGroupId::GenerateNew());
+
+  // Expect no groups have been saved at this point.
+  tab_groups::SavedTabGroupKeyedService* service =
+      tab_groups::SavedTabGroupServiceFactory::GetForProfile(
+          browser()->profile());
+  ASSERT_NE(service, nullptr);
+  EXPECT_TRUE(service->model()->IsEmpty());
+
+  // Close the browser and restore the last session
+  Browser* restored = QuitBrowserAndRestore(browser());
+  TabStripModel* tab_strip_model = restored->tab_strip_model();
+  const int tabs = tab_strip_model->count();
+  ASSERT_EQ(2, tabs);
+
+  // Expect the unsaved group has been saved at this point.
+  EXPECT_EQ(1, service->model()->Count());
+}
+
+// This test simulates creating a default group (using the default group color
+// and no title). Ensure on restart there was no duplicated groups and that the
+// restored group is connected to the saved group.
+IN_PROC_BROWSER_TEST_F(SavedTabGroupSessionRestoreTest,
+                       NoDuplicatesOfDefaultSavedGroupAfterBrowserRestart) {
+  // Add a second tab.
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), GURL(chrome::kChromeUINewTabPageURL),
+      WindowOpenDisposition::NEW_FOREGROUND_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP);
+
+  // Add the tab to a new group.
+  browser()->tab_strip_model()->AddToNewGroup({0});
+
+  // Expect the newly created to be saved at this point.
+  tab_groups::SavedTabGroupKeyedService* service =
+      tab_groups::SavedTabGroupServiceFactory::GetForProfile(
+          browser()->profile());
+  ASSERT_NE(service, nullptr);
+  EXPECT_EQ(1, service->model()->Count());
+
+  // Close the browser and restore the last session
+  Browser* restored = QuitBrowserAndRestore(browser());
+  TabStripModel* tab_strip_model = restored->tab_strip_model();
+  const int tabs = tab_strip_model->count();
+  ASSERT_EQ(2, tabs);
+
+  // Expect the restored browser to still has 1 saved group i.e. no duplicates.
+  EXPECT_EQ(1, service->model()->Count());
+}
+
+// This test simulates creating multiple groups with different visual data
+// and ensuring on restart they are restored with the same information.
+IN_PROC_BROWSER_TEST_F(SavedTabGroupSessionRestoreTest,
+                       MultipleSavedGroupsAfterBrowserRestart) {
+  // Add a second tab.
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), GURL(chrome::kChromeUINewTabPageURL),
+      WindowOpenDisposition::NEW_FOREGROUND_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP);
+
+  // Add a third tab.
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), GURL(chrome::kChromeUINewTabPageURL),
+      WindowOpenDisposition::NEW_FOREGROUND_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP);
+
+  tab_groups::SavedTabGroupKeyedService* service =
+      tab_groups::SavedTabGroupServiceFactory::GetForProfile(
+          browser()->profile());
+  ASSERT_NE(service, nullptr);
+
+  // Add the tab to a new groups.
+  auto group1 = browser()->tab_strip_model()->AddToNewGroup({0});
+  base::Uuid group1_saved_guid = service->model()->Get(group1)->saved_guid();
+
+  auto group2 = browser()->tab_strip_model()->AddToNewGroup({1});
+  base::Uuid group2_saved_guid = service->model()->Get(group2)->saved_guid();
+
+  // Expect the newly created to be saved at this point.
+  EXPECT_EQ(2, service->model()->Count());
+
+  // Update the visual data of the new groups.
+  browser()
+      ->tab_strip_model()
+      ->group_model()
+      ->GetTabGroup(group1)
+      ->SetVisualData(tab_groups::TabGroupVisualData(
+                          u"Group1", tab_groups::TabGroupColorId::kGrey),
+                      true);
+
+  browser()
+      ->tab_strip_model()
+      ->group_model()
+      ->GetTabGroup(group2)
+      ->SetVisualData(tab_groups::TabGroupVisualData(
+                          u"Group2", tab_groups::TabGroupColorId::kBlue),
+                      true);
+
+  // Close the browser and restore the last session
+  Browser* restored = QuitBrowserAndRestore(browser());
+  TabStripModel* tab_strip_model = restored->tab_strip_model();
+  const int tabs = tab_strip_model->count();
+  ASSERT_EQ(3, tabs);
+
+  // Expect the restored browser to still has 2 saved group.
+  EXPECT_EQ(2, service->model()->Count());
+
+  auto* saved_group1 = service->model()->Get(group1_saved_guid);
+  ASSERT_NE(saved_group1, nullptr);
+  ASSERT_TRUE(saved_group1->local_group_id().has_value());
+  auto* local_group1 = tab_strip_model->group_model()->GetTabGroup(
+      saved_group1->local_group_id().value());
+  EXPECT_EQ(u"Group1", local_group1->visual_data()->title());
+  EXPECT_EQ(tab_groups::TabGroupColorId::kGrey,
+            local_group1->visual_data()->color());
+
+  auto* saved_group2 = service->model()->Get(group2_saved_guid);
+  ASSERT_NE(saved_group2, nullptr);
+  ASSERT_TRUE(saved_group2->local_group_id().has_value());
+  auto* local_group2 = tab_strip_model->group_model()->GetTabGroup(
+      saved_group2->local_group_id().value());
+  EXPECT_EQ(u"Group2", local_group2->visual_data()->title());
+  EXPECT_EQ(tab_groups::TabGroupColorId::kBlue,
+            local_group2->visual_data()->color());
+}

chrome/browser/ui/browser.cc

@@ -187,6 +187,7 @@
 #include "components/paint_preview/buildflags/buildflags.h"
 #include "components/permissions/permission_request_manager.h"
 #include "components/prefs/pref_service.h"
+#include "components/saved_tab_groups/features.h"
 #include "components/search/search.h"
 #include "components/sessions/content/session_tab_helper.h"
 #include "components/sessions/core/session_types.h"
@@ -397,6 +398,26 @@ bool ShouldShowCookieMigrationNoticeForBrowser(const Browser& browser) {
   return last_window_for_profile;
 }
 
+void UpdateTabGroupSessionMetadata(
+    Browser* browser,
+    const tab_groups::TabGroupId& group_id,
+    const std::optional<std::string> saved_group_id) {
+  SessionService* const session_service =
+      SessionServiceFactory::GetForProfile(browser->profile());
+  if (!session_service) {
+    return;
+  }
+
+  const tab_groups::TabGroupVisualData* visual_data =
+      browser->tab_strip_model()
+          ->group_model()
+          ->GetTabGroup(group_id)
+          ->visual_data();
+
+  session_service->SetTabGroupMetadata(browser->session_id(), group_id,
+                                       visual_data, saved_group_id);
+}
+
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -577,6 +598,12 @@ Browser::Browser(const CreateParams& params)
 
   tab_strip_model_->AddObserver(this);
 
+  auto* saved_tab_group_keyed_service =
+      tab_groups::SavedTabGroupServiceFactory::GetForProfile(profile_);
+  if (saved_tab_group_keyed_service && tab_groups::IsTabGroupsSaveV2Enabled()) {
+    saved_tab_group_keyed_service->model()->AddObserver(this);
+  }
+
   ThemeServiceFactory::GetForProfile(profile_)->AddObserver(this);
 
   profile_pref_registrar_.Init(profile_->GetPrefs());
@@ -637,6 +664,12 @@ Browser::Browser(const CreateParams& params)
 }
 
 Browser::~Browser() {
+  auto* saved_tab_group_keyed_service =
+      tab_groups::SavedTabGroupServiceFactory::GetForProfile(profile_);
+  if (saved_tab_group_keyed_service && tab_groups::IsTabGroupsSaveV2Enabled()) {
+    saved_tab_group_keyed_service->model()->RemoveObserver(this);
+  }
+
   // Stop observing notifications and destroy the tab monitor before continuing
   // with destruction. Profile destruction will unload extensions and reentrant
   // calls to Browser:: should be avoided while it is being torn down.
@@ -1385,29 +1418,19 @@ void Browser::OnTabGroupChanged(const TabGroupChange& change) {
   DCHECK(!IsRelevantToAppSessionService(type_));
   DCHECK(tab_strip_model_->group_model());
   if (change.type == TabGroupChange::kVisualsChanged) {
-    SessionService* const session_service =
-        SessionServiceFactory::GetForProfile(profile_);
-    if (session_service) {
-      const tab_groups::TabGroupVisualData* visual_data =
-          tab_strip_model_->group_model()
-              ->GetTabGroup(change.group)
-              ->visual_data();
-      const tab_groups::SavedTabGroupKeyedService* const
-          saved_tab_group_keyed_service =
-              tab_groups::SavedTabGroupServiceFactory::GetForProfile(profile_);
-      std::optional<std::string> saved_guid;
+    std::optional<std::string> saved_guid;
 
-      if (saved_tab_group_keyed_service) {
-        const tab_groups::SavedTabGroup* const saved_group =
-            saved_tab_group_keyed_service->model()->Get(change.group);
-        if (saved_group) {
-          saved_guid = saved_group->saved_guid().AsLowercaseString();
-        }
+    const tab_groups::SavedTabGroupKeyedService* const
+        saved_tab_group_keyed_service =
+            tab_groups::SavedTabGroupServiceFactory::GetForProfile(profile_);
+    if (saved_tab_group_keyed_service) {
+      const tab_groups::SavedTabGroup* const saved_group =
+          saved_tab_group_keyed_service->model()->Get(change.group);
+      if (saved_group) {
+        saved_guid = saved_group->saved_guid().AsLowercaseString();
       }
-
-      session_service->SetTabGroupMetadata(session_id(), change.group,
-                                           visual_data, std::move(saved_guid));
     }
+    UpdateTabGroupSessionMetadata(this, change.group, std::move(saved_guid));
   } else if (change.type == TabGroupChange::kClosed) {
     sessions::TabRestoreService* tab_restore_service =
         TabRestoreServiceFactory::GetForProfile(profile());
@@ -1460,6 +1483,42 @@ void Browser::TabStripEmpty() {
   instant_controller_.reset();
 }
 
+void Browser::SavedTabGroupAddedLocally(const base::Uuid& guid) {
+  // See comment in Browser::OnTabGroupChanged
+  DCHECK(!IsRelevantToAppSessionService(type_));
+  DCHECK(tab_strip_model_->group_model());
+
+  const tab_groups::SavedTabGroupKeyedService* const
+      saved_tab_group_keyed_service =
+          tab_groups::SavedTabGroupServiceFactory::GetForProfile(profile_);
+  CHECK(saved_tab_group_keyed_service);
+
+  const tab_groups::SavedTabGroup* const added_group =
+      saved_tab_group_keyed_service->model()->Get(guid);
+  CHECK(added_group);
+
+  if (!added_group->local_group_id().has_value()) {
+    return;
+  }
+
+  UpdateTabGroupSessionMetadata(this, added_group->local_group_id().value(),
+                                guid.AsLowercaseString());
+}
+
+void Browser::SavedTabGroupRemovedLocally(
+    const tab_groups::SavedTabGroup* removed_group) {
+  // See comment in Browser::OnTabGroupChanged
+  DCHECK(!IsRelevantToAppSessionService(type_));
+  DCHECK(tab_strip_model_->group_model());
+
+  if (!removed_group->local_group_id().has_value()) {
+    return;
+  }
+
+  UpdateTabGroupSessionMetadata(this, removed_group->local_group_id().value(),
+                                std::nullopt);
+}
+
 void Browser::SetTopControlsShownRatio(content::WebContents* web_contents,
                                        float ratio) {
   window_->SetTopControlsShownRatio(web_contents, ratio);

chrome/browser/ui/browser.h

@@ -34,6 +34,7 @@
 #include "chrome/browser/ui/unload_controller.h"
 #include "components/paint_preview/buildflags/buildflags.h"
 #include "components/prefs/pref_change_registrar.h"
+#include "components/saved_tab_groups/saved_tab_group_model_observer.h"
 #include "components/sessions/core/session_id.h"
 #include "components/translate/content/browser/content_translate_driver.h"
 #include "components/zoom/zoom_observer.h"
@@ -128,6 +129,7 @@ enum class BrowserClosingStatus {
 // scoped to an instance of this class, usually via direct or indirect ownership
 // of a std::unique_ptr. See BrowserWindowFeatures and TabFeatures.
 class Browser : public TabStripModelObserver,
+                public tab_groups::SavedTabGroupModelObserver,
                 public WebContentsCollection::Observer,
                 public content::WebContentsDelegate,
                 public ChromeWebModalDialogManagerDelegate,
@@ -762,6 +764,11 @@ class Browser : public TabStripModelObserver,
                               int index) override;
   void TabStripEmpty() override;
 
+  // Overridden from tab_groups::SavedTabGroupModelObserver:
+  void SavedTabGroupAddedLocally(const base::Uuid& guid) override;
+  void SavedTabGroupRemovedLocally(
+      const tab_groups::SavedTabGroup* removed_group) override;
+
   // Overridden from content::WebContentsDelegate:
   void ActivateContents(content::WebContents* contents) override;
   void SetTopControlsShownRatio(content::WebContents* web_contents,

chrome/browser/ui/tabs/saved_tab_groups/saved_tab_group_keyed_service.cc

@@ -106,29 +106,37 @@ SavedTabGroupKeyedService::GetSavedTabGroupControllerDelegate() {
   return bridge_.change_processor()->GetControllerDelegate();
 }
 
-void SavedTabGroupKeyedService::StoreLocalToSavedId(
+void SavedTabGroupKeyedService::ConnectRestoredGroupToSaveId(
     const base::Uuid& saved_guid,
     const tab_groups::TabGroupId local_group_id) {
-  // Avoid linking SavedTabGroups that are already open.
-  const SavedTabGroup* const group = model()->Get(saved_guid);
-  if (group && group->local_group_id().has_value()) {
-    return;
-  }
-
-  // If there is no saved group with guid `saved_guid`, the group must
-  // have been unsaved since this session closed.
-  if (model()->is_loaded() && !group) {
-    return;
-  }
-
-  // The model could already be loaded when restoring groups from a previously
-  // crashed session / window. This means we will have to manually trigger the
-  // local to saved group linking.
   if (model()->is_loaded()) {
+    const SavedTabGroup* const group = model()->Get(saved_guid);
+    // If there is no saved group with guid `saved_guid`, the group must
+    // have been unsaved since this session closed.
+    if (!group) {
+      return;
+    }
+
+    // Avoid linking SavedTabGroups that are already open.
+    if (group->local_group_id().has_value()) {
+      return;
+    }
+
     ConnectLocalTabGroup(local_group_id, saved_guid);
   } else {
-    saved_guid_to_local_group_id_mapping_.emplace_back(saved_guid,
-                                                       local_group_id);
+    restored_groups_to_connect_on_load_.emplace_back(saved_guid,
+                                                     local_group_id);
+  }
+}
+
+void SavedTabGroupKeyedService::SaveRestoredGroup(
+    const tab_groups::TabGroupId& local_group_id) {
+  if (model()->is_loaded()) {
+    CHECK(!model()->Contains(local_group_id))
+        << "This group is somehow saved already when it shouldn't be.";
+    SaveGroup(local_group_id);
+  } else {
+    restored_groups_to_save_on_load_.emplace_back(local_group_id);
   }
 }
 
@@ -351,7 +359,7 @@ void SavedTabGroupKeyedService::SavedTabGroupModelLoaded() {
   }
 
   for (const auto& [saved_guid, local_group_id] :
-       saved_guid_to_local_group_id_mapping_) {
+       restored_groups_to_connect_on_load_) {
     if (model()->is_loaded() && !model()->Contains(saved_guid)) {
       continue;
     }
@@ -359,14 +367,13 @@ void SavedTabGroupKeyedService::SavedTabGroupModelLoaded() {
     ConnectLocalTabGroup(local_group_id, saved_guid);
   }
 
-  // Clear `saved_guid_to_local_group_id_mapping_` to save space when finished.
-  //
-  // TODO(dljames): Investigate using a single use callback to connect local and
-  // saved groups together. There are crashes that occur when restarting the
-  // browser before the browser process completely shuts down. The callback will
-  // also remove the need of `saved_guid_to_local_group_id_mapping_`.
-  saved_guid_to_local_group_id_mapping_.clear();
-  CHECK(saved_guid_to_local_group_id_mapping_.empty());
+  for (const auto& local_group_id : restored_groups_to_save_on_load_) {
+    SaveGroup(local_group_id);
+  }
+
+  // Clear restored groups to connect and save now that we have processed them.
+  restored_groups_to_connect_on_load_.clear();
+  restored_groups_to_save_on_load_.clear();
 }
 
 void SavedTabGroupKeyedService::SavedTabGroupRemovedFromSync(

chrome/browser/ui/tabs/saved_tab_groups/saved_tab_group_keyed_service.h

@@ -42,11 +42,6 @@ class SavedTabGroupKeyedService : public KeyedService,
   GetSavedTabGroupControllerDelegate();
   Profile* profile() { return profile_; }
 
-  // Populates `saved_guid_to_local_group_id_mapping_` with a pair to link once
-  // SavedTabGroupModelLoaded is called.
-  void StoreLocalToSavedId(const base::Uuid& saved_guid,
-                           const tab_groups::TabGroupId local_group_id);
-
   // SavedTabGroupController
   std::optional<tab_groups::TabGroupId> OpenSavedTabGroupInBrowser(
       Browser* browser,
@@ -70,6 +65,17 @@ class SavedTabGroupKeyedService : public KeyedService,
       const base::Uuid& group_guid,
       const std::optional<base::Uuid>& tab_guid) override;
 
+  // Connects local tab group to the saved guid from session restore.
+  // This can be called prior to the saved tab group model is loaded or
+  // that the `saved_guid` could no longer be present in the model.
+  void ConnectRestoredGroupToSaveId(
+      const base::Uuid& saved_guid,
+      const tab_groups::TabGroupId local_group_id);
+
+  // Saves a restored group. This can be called prior to the saved tab
+  // group model is loaded. These groups are saved when the model is loaded.
+  void SaveRestoredGroup(const tab_groups::TabGroupId& group_id);
+
  private:
   // Adds tabs to `tab_group` if `saved_group` was modified and has more tabs
   // than `tab_group` when a restore happens.
@@ -152,12 +158,12 @@ class SavedTabGroupKeyedService : public KeyedService,
   // (saved and unsaved).
   base::RepeatingTimer metrics_timer_;
 
-  // Keeps track of the ids of session restored tab groups that were once saved
-  // in order to link them together again once the SavedTabGroupModelLoaded is
-  // called. After the model is loaded, this variable is emptied to conserve
-  // memory.
+  // Keeps track of restored group to connect to model load.
   std::vector<std::pair<base::Uuid, tab_groups::TabGroupId>>
-      saved_guid_to_local_group_id_mapping_;
+      restored_groups_to_connect_on_load_;
+
+  // Keeps track of the groups to save on model load.
+  std::vector<tab_groups::TabGroupId> restored_groups_to_save_on_load_;
 };
 
 }  // namespace tab_groups

chrome/browser/ui/tabs/saved_tab_groups/saved_tab_group_keyed_service_unittest.cc

@@ -265,7 +265,7 @@ TEST_F(SavedTabGroupKeyedServiceUnitTest, AlreadyOpenedGroupIsFocused) {
   // Store the guid to tab_group_id association in the keyed service. We should
   // expect at the end of the test, `tab_group_id_3` has no association with the
   // SavedTabGroupModel at all.
-  service()->StoreLocalToSavedId(guid_1, tab_group_id_1);
+  service()->ConnectRestoredGroupToSaveId(guid_1, tab_group_id_1);
 
   // Populate the SavedTabGroupModel with some test data to simulate the browser
   // loading in persisted data on startup.
@@ -312,7 +312,7 @@ TEST_F(SavedTabGroupKeyedServiceUnitTest,
   // Store the guid to tab_group_id association in the keyed service. We should
   // expect at the end of the test, `tab_group_id_3` has no association with the
   // SavedTabGroupModel at all.
-  service()->StoreLocalToSavedId(guid_1, tab_group_id_1);
+  service()->ConnectRestoredGroupToSaveId(guid_1, tab_group_id_1);
 
   // Populate the SavedTabGroupModel with some test data to simulate the browser
   // loading in persisted data on startup.
@@ -369,20 +369,20 @@ TEST_F(SavedTabGroupKeyedServiceUnitTest,
       browser_1->tab_strip_model()->AddToNewGroup({0});
   const base::Uuid guid_1 = base::Uuid::GenerateRandomV4();
 
-  service()->StoreLocalToSavedId(guid_1, tab_group_id_1);
+  service()->ConnectRestoredGroupToSaveId(guid_1, tab_group_id_1);
 
   // Notify the KeyedService that the SavedTabGroupModel has loaded all local
   // data triggered by the completion of SavedTabGroupModel::LoadStoredEntries.
   service()->model()->LoadStoredEntries(/*groups=*/{}, /*tabs=*/{});
 
-  // Expect calling StoreLocalToSavedId before the model is loaded does not link
-  // non-existent saved groups.
+  // Expect calling ConnectRestoredGroupToSaveId before the model is loaded does
+  // not link non-existent saved groups.
   EXPECT_FALSE(service()->model()->Contains(tab_group_id_1));
   EXPECT_FALSE(service()->model()->Contains(guid_1));
 
   // Expect calling StoreLocalSavedId after the model is loaded does not link
   // non-existent saved groups.
-  service()->StoreLocalToSavedId(guid_1, tab_group_id_1);
+  service()->ConnectRestoredGroupToSaveId(guid_1, tab_group_id_1);
   EXPECT_FALSE(service()->model()->Contains(tab_group_id_1));
   EXPECT_FALSE(service()->model()->Contains(guid_1));
 }
@@ -412,8 +412,8 @@ TEST_F(SavedTabGroupKeyedServiceUnitTest,
   // Store the guid to tab_group_id association in the keyed service. We should
   // expect at the end of the test, `tab_group_id_3` has no association with the
   // SavedTabGroupModel at all.
-  service()->StoreLocalToSavedId(guid_1, tab_group_id_1);
-  service()->StoreLocalToSavedId(guid_2, tab_group_id_2);
+  service()->ConnectRestoredGroupToSaveId(guid_1, tab_group_id_1);
+  service()->ConnectRestoredGroupToSaveId(guid_2, tab_group_id_2);
 
   // Populate the SavedTabGroupModel with some test data to simulate the browser
   // loading in persisted data on startup.
@@ -471,7 +471,7 @@ TEST_F(SavedTabGroupKeyedServiceUnitTest,
   const base::Uuid guid = base::Uuid::GenerateRandomV4();
 
   // Store the guid to tab_group_id association in the keyed service.
-  service()->StoreLocalToSavedId(guid, tab_group_id);
+  service()->ConnectRestoredGroupToSaveId(guid, tab_group_id);
 
   // Populate the SavedTabGroupModel with some test data to simulate the browser
   // loading persisted data on startup.
@@ -534,7 +534,7 @@ TEST_F(SavedTabGroupKeyedServiceUnitTest,
   const base::Uuid guid = base::Uuid::GenerateRandomV4();
 
   // Store the guid to tab_group_id association in the keyed service.
-  service()->StoreLocalToSavedId(guid, tab_group_id);
+  service()->ConnectRestoredGroupToSaveId(guid, tab_group_id);
 
   // Populate the SavedTabGroupModel with some test data to simulate the browser
   // loading persisted data on startup.
@@ -596,7 +596,7 @@ TEST_F(SavedTabGroupKeyedServiceUnitTest,
   const base::Uuid guid = base::Uuid::GenerateRandomV4();
 
   // Store the guid to tab_group_id association in the keyed service.
-  service()->StoreLocalToSavedId(guid, tab_group_id);
+  service()->ConnectRestoredGroupToSaveId(guid, tab_group_id);
 
   // Populate the SavedTabGroupModel with some test data to simulate the browser
   // loading persisted data on startup.

components/saved_tab_groups/saved_tab_group_model.cc

@@ -537,8 +537,10 @@ std::set<base::Uuid> SavedTabGroupModel::UpdateLocalCacheGuid(
   return updated_group_ids;
 }
 
-void SavedTabGroupModel::LoadStoredEntries(std::vector<SavedTabGroup> groups,
-                                           std::vector<SavedTabGroupTab> tabs) {
+void SavedTabGroupModel::LoadStoredEntries(
+    std::vector<SavedTabGroup> groups,
+    std::vector<SavedTabGroupTab> tabs,
+    base::OnceCallback<void()> on_loaded_callback) {
   // `entries` is not ordered such that groups are guaranteed to be
   // at the front of the vector. As such, we can run into the case where we
   // try to add a tab to a group that does not exist for us yet.
@@ -555,6 +557,9 @@ void SavedTabGroupModel::LoadStoredEntries(std::vector<SavedTabGroup> groups,
   }
 
   is_loaded_ = true;
+
+  std::move(on_loaded_callback).Run();
+
   for (auto& observer : observers_) {
     observer.SavedTabGroupModelLoaded();
   }

components/saved_tab_groups/saved_tab_group_model.h

@@ -9,6 +9,7 @@
 #include <optional>
 #include <vector>
 
+#include "base/functional/callback_helpers.h"
 #include "base/memory/raw_ptr.h"
 #include "base/observer_list.h"
 #include "components/saved_tab_groups/saved_tab_group.h"
@@ -153,8 +154,10 @@ class SavedTabGroupModel {
 
   // Loads the model from the storage. `tabs` must have a corresponding group in
   // `groups`.
-  void LoadStoredEntries(std::vector<SavedTabGroup> groups,
-                         std::vector<SavedTabGroupTab> tabs);
+  void LoadStoredEntries(
+      std::vector<SavedTabGroup> groups,
+      std::vector<SavedTabGroupTab> tabs,
+      base::OnceCallback<void()> on_loaded_callback = base::DoNothing());
 
   // Functions that should be called when a SavedTabGroup's corresponding
   // TabGroup is closed or opened.

components/saved_tab_groups/saved_tab_group_sync_bridge.cc

@@ -70,7 +70,8 @@ base::Time TimeFromWindowsEpochMicros(int64_t time_windows_epoch_micros) {
 
 std::vector<proto::SavedTabGroupData> LoadStoredEntries(
     std::vector<proto::SavedTabGroupData> stored_entries,
-    SavedTabGroupModel* model) {
+    SavedTabGroupModel* model,
+    base::OnceCallback<void()> on_loaded_callback) {
   std::vector<SavedTabGroup> groups;
   std::unordered_set<std::string> group_guids;
 
@@ -98,7 +99,8 @@ std::vector<proto::SavedTabGroupData> LoadStoredEntries(
     tabs_missing_groups.push_back(std::move(proto));
   }
 
-  model->LoadStoredEntries(std::move(groups), std::move(tabs));
+  model->LoadStoredEntries(std::move(groups), std::move(tabs),
+                           std::move(on_loaded_callback));
   return tabs_missing_groups;
 }
 
@@ -943,9 +945,16 @@ void SavedTabGroupSyncBridge::OnReadAllMetadata(
 
   // Update `model_` with any data stored in local storage except for orphaned
   // tabs. Orphaned tabs will be returned and added to `tabs_missing_groups_` in
-  // case their missing group ever arrives.
-  tabs_missing_groups_ =
-      LoadStoredEntries(std::move(stored_entries), model_.get());
+  // case their missing group ever arrives. Use `on_loaded_callback` to start
+  // observing the model immediately after loading the stored entries. This
+  // ensures the sync bridge is able to observe restored groups on startup.
+  tabs_missing_groups_ = LoadStoredEntries(
+      std::move(stored_entries), model_.get(),
+      base::BindOnce(&SavedTabGroupSyncBridge::StartObservingModel,
+                     base::Unretained(this)));
+}
+
+void SavedTabGroupSyncBridge::StartObservingModel() {
   observation_.Observe(model_);
 }
 

components/saved_tab_groups/saved_tab_group_sync_bridge.h

@@ -179,6 +179,10 @@ class SavedTabGroupSyncBridge : public syncer::ModelTypeSyncBridge,
       const std::optional<syncer::ModelError>& error,
       std::unique_ptr<syncer::MetadataBatch> metadata_batch);
 
+  // Callback passed to `LoadStoredEntries` to start observing model after
+  // loading the stored entries.
+  void StartObservingModel();
+
   // Called to migrate the SavedTabGroupSpecfics to SavedTabGroupData.
   void MigrateSpecificsToSavedTabGroupData(
       std::unique_ptr<syncer::ModelTypeStore::RecordList> entries);

components/sessions/core/session_service_commands.cc

@@ -690,8 +690,12 @@ void CreateTabsAndWindows(
           if (!iter.ReadString(&saved_guid)) {
             return;
           }
-
           group->saved_guid = saved_guid;
+        } else {
+          // Explicitly update the |saved_guid| to nullopt if the group
+          // isn't saved. This is to ensure the right value is set when there
+          // are multiple entries in the append log file.
+          group->saved_guid = std::nullopt;
         }
 
         break;

components/sessions/core/tab_restore_service_impl.cc

@@ -1265,6 +1265,11 @@ void TabRestoreServiceImpl::PersistenceDelegate::CreateEntriesFromCommands(
             break;
           }
           current_tab->saved_group_id = base::Uuid::ParseLowercase(saved_id);
+        } else {
+          // Explicitly update the nullopt if the group isn't saved. This is to
+          // ensure the right value is set when there are multiple entries in
+          // the append log file.
+          current_tab->saved_group_id = std::nullopt;
         }
 
         current_tab->group =