Revert "Reland "Avoid recycling sync tabs if commit pending""

This reverts commit a61d2bb4.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 612980 as the
culprit for flakes in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/flake/flake-culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyQwsSDEZsYWtlQ3VscHJpdCIxY2hyb21pdW0vYTYxZDJiYjQzMDRkZjgyOGRhNjBhZjA3ZjBiMDVmMzNlNDlmZDE4Zgw

Sample Failed Build: https://ci.chromium.org/buildbot/chromium.chromiumos/linux-chromeos-dbg/9304

Sample Failed Step: sync_integration_tests

Sample Flaky Test: SingleClientSessionsSyncTest.NavigateThenCloseTabThenOpenTab

Original change's description:
> Reland "Avoid recycling sync tabs if commit pending"
> 
> This is a reland of 62414cfc
> 
> Test improved to avoid flakes.
> 
> Original change's description:
> > Avoid recycling sync tabs if commit pending
> >
> > When a tab is closed, it's possible that the corresponding history
> > hasn't been committed yet, and hence there is a risk that synced history
> > is lost if the entity is recycled (for another tab that is opened).
> >
> > In this patch, and behind a feature toggle, this issue is prevented by
> > *not* freeing tab nodes while the sync entity is unsynced. Old tabs are
> > excluded from this (to avoid problems with expired history) and a max
> > cap is also introduced to the number of tabs in this state, in order to
> > avoid memory regressions.
> >
> > Bug: 882489
> > Change-Id: I6dd796642f9553f2713a0814731897a4ffb13f0b
> > Reviewed-on: https://chromium-review.googlesource.com/c/1356541
> > Commit-Queue: Mikel Astiz <mastiz@chromium.org>
> > Reviewed-by: Marc Treib <treib@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#612971}
> 
> TBR=treib@chromium.org
> 
> Bug: 882489
> Change-Id: Ib3f3ff9e4d620512435d9b22ba7b2c338f92203f
> Reviewed-on: https://chromium-review.googlesource.com/c/1357086
> Reviewed-by: Mikel Astiz <mastiz@chromium.org>
> Commit-Queue: Mikel Astiz <mastiz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#612980}

Change-Id: I69bf304dc196b29f2b6ccf5f15ff8dd77ca7d610
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 882489, 910947
Reviewed-on: https://chromium-review.googlesource.com/c/1357826
Cr-Commit-Position: refs/heads/master@{#612982}

chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc

@@ -5,7 +5,6 @@
 #include "base/macros.h"
 #include "base/run_loop.h"
 #include "base/test/metrics/histogram_tester.h"
-#include "base/test/scoped_feature_list.h"
 #include "chrome/browser/sessions/session_service.h"
 #include "chrome/browser/sessions/session_tab_helper.h"
 #include "chrome/browser/sync/session_sync_service_factory.h"
@@ -25,7 +24,6 @@
 #include "components/sync/protocol/proto_value_conversions.h"
 #include "components/sync/test/fake_server/sessions_hierarchy.h"
 #include "components/sync_sessions/session_sync_service.h"
-#include "components/sync_sessions/synced_session_tracker.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/mojo/window_open_disposition.mojom.h"
 
@@ -49,7 +47,6 @@ using sessions_helper::OpenTabFromSourceIndex;
 using sessions_helper::ScopedWindowMap;
 using sessions_helper::SessionWindowMap;
 using sessions_helper::SyncedSessionVector;
-using sessions_helper::WaitForTabsToLoad;
 using sessions_helper::WindowsMatch;
 using typed_urls_helper::GetUrlFromClient;
 
@@ -230,7 +227,6 @@ IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, NavigateThenCloseTab) {
   // Close one of the two tabs immediately after issuing an navigation. We also
   // issue another navigation to make sure association logic kicks in.
   NavigateTab(0, GURL(kURL3));
-  ASSERT_TRUE(WaitForTabsToLoad(0, {GURL(kURL1), GURL(kURL3)}));
   CloseTab(/*index=*/0, /*tab_index=*/1);
   NavigateTab(0, GURL(kURL4));
 
@@ -243,38 +239,6 @@ IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, NavigateThenCloseTab) {
       IsUrlSyncedChecker(kURL3, GetFakeServer(), GetSyncService(0)).Wait());
 }
 
-IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest,
-                       NavigateThenCloseTabThenOpenTab) {
-  base::test::ScopedFeatureList override_features;
-  override_features.InitAndEnableFeature(
-      sync_sessions::kDeferRecyclingOfSyncTabNodesIfUnsynced);
-
-  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
-  ASSERT_TRUE(CheckInitialState(0));
-
-  // Two tabs are opened initially.
-  ASSERT_TRUE(OpenTab(0, GURL(kURL1)));
-  ASSERT_TRUE(OpenTab(0, GURL(kURL2)));
-  WaitForHierarchyOnServer(SessionsHierarchy({{kURL1, kURL2}}));
-
-  // Close one of the two tabs immediately after issuing an navigation. In
-  // addition, a new tab is opened.
-  NavigateTab(0, GURL(kURL3));
-  ASSERT_TRUE(WaitForTabsToLoad(0, {GURL(kURL1), GURL(kURL3)}));
-  CloseTab(/*index=*/0, /*tab_index=*/1);
-  ASSERT_TRUE(OpenTab(0, GURL(kURL4)));
-
-  DLOG(INFO) << "Waiting for kURL4 to be synced";
-  ASSERT_TRUE(
-      IsUrlSyncedChecker(kURL4, GetFakeServer(), GetSyncService(0)).Wait());
-
-  // All URLs should be synced, for synced history to be complete. In
-  // particular, |kURL3| should be synced despite the tab being closed.
-  DLOG(INFO) << "Waiting for kURL3 to be synced";
-  EXPECT_TRUE(
-      IsUrlSyncedChecker(kURL3, GetFakeServer(), GetSyncService(0)).Wait());
-}
-
 IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, TimestampMatchesHistory) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
 

components/sync_sessions/local_session_event_handler_impl.cc

@@ -91,9 +91,6 @@ LocalSessionEventHandlerImpl::~LocalSessionEventHandlerImpl() {}
 
 void LocalSessionEventHandlerImpl::OnSessionRestoreComplete() {
   std::unique_ptr<WriteBatch> batch = delegate_->CreateLocalSessionWriteBatch();
-  // The initial state of the tracker may contain tabs that are unmmapped but
-  // haven't been marked as free yet.
-  CleanupLocalTabs(batch.get());
   AssociateWindows(RELOAD_TABS, batch.get());
   batch->Commit();
 }
@@ -104,16 +101,6 @@ LocalSessionEventHandlerImpl::GetTabSpecificsFromDelegateForTest(
   return GetTabSpecificsFromDelegate(tab_delegate);
 }
 
-void LocalSessionEventHandlerImpl::CleanupLocalTabs(WriteBatch* batch) {
-  std::set<int> deleted_tab_node_ids =
-      session_tracker_->CleanupLocalTabs(base::BindRepeating(
-          &Delegate::IsTabNodeUnsynced, base::Unretained(delegate_)));
-
-  for (int tab_node_id : deleted_tab_node_ids) {
-    batch->Delete(tab_node_id);
-  }
-}
-
 void LocalSessionEventHandlerImpl::AssociateWindows(ReloadTabsOption option,
                                                     WriteBatch* batch) {
   DCHECK(!IsSessionRestoreInProgress(sessions_client_));
@@ -232,7 +219,11 @@ void LocalSessionEventHandlerImpl::AssociateWindows(ReloadTabsOption option,
     }
   }
 
-  CleanupLocalTabs(batch);
+  std::set<int> deleted_tab_node_ids;
+  session_tracker_->CleanupLocalTabs(&deleted_tab_node_ids);
+  for (int tab_node_id : deleted_tab_node_ids) {
+    batch->Delete(tab_node_id);
+  }
 
   // Always update the header.  Sync takes care of dropping this update
   // if the entity specifics are identical (i.e windows, client name did

components/sync_sessions/local_session_event_handler_impl.h

@@ -48,7 +48,6 @@ class LocalSessionEventHandlerImpl : public LocalSessionEventHandler {
    public:
     virtual ~Delegate();
     virtual std::unique_ptr<WriteBatch> CreateLocalSessionWriteBatch() = 0;
-    virtual bool IsTabNodeUnsynced(int tab_node_id) = 0;
     // Analogous to SessionsGlobalIdMapper.
     virtual void TrackLocalNavigationId(base::Time timestamp,
                                         int unique_id) = 0;
@@ -80,8 +79,6 @@ class LocalSessionEventHandlerImpl : public LocalSessionEventHandler {
  private:
   enum ReloadTabsOption { RELOAD_TABS, DONT_RELOAD_TABS };
 
-  void CleanupLocalTabs(WriteBatch* batch);
-
   void AssociateWindows(ReloadTabsOption option,
                         WriteBatch* batch);
 

components/sync_sessions/local_session_event_handler_impl_unittest.cc

@@ -8,7 +8,6 @@
 #include <vector>
 
 #include "base/strings/stringprintf.h"
-#include "base/test/scoped_feature_list.h"
 #include "components/sessions/core/serialized_navigation_entry.h"
 #include "components/sessions/core/serialized_navigation_entry_test_helper.h"
 #include "components/sync/base/time.h"
@@ -72,7 +71,6 @@ class MockDelegate : public LocalSessionEventHandlerImpl::Delegate {
 
   MOCK_METHOD0(CreateLocalSessionWriteBatch,
                std::unique_ptr<LocalSessionEventHandlerImpl::WriteBatch>());
-  MOCK_METHOD1(IsTabNodeUnsynced, bool(int tab_node_id));
   MOCK_METHOD2(TrackLocalNavigationId,
                void(base::Time timestamp, int unique_id));
   MOCK_METHOD1(OnPageFaviconUpdated, void(const GURL& page_url));
@@ -357,16 +355,6 @@ TEST_F(LocalSessionEventHandlerImplTest, DontUpdateWindowIdForPlaceholderTab) {
   UpdateTrackerWithSpecifics(placeholder_tab, base::Time::Now(),
                              &session_tracker_);
 
-  // Mimic the header being restored from peristence too.
-  session_tracker_.PutWindowInSession(
-      kSessionTag, SessionID::FromSerializedValue(kWindowId1));
-  session_tracker_.PutTabInWindow(kSessionTag,
-                                  SessionID::FromSerializedValue(kWindowId1),
-                                  SessionID::FromSerializedValue(kTabId1));
-  session_tracker_.PutTabInWindow(kSessionTag,
-                                  SessionID::FromSerializedValue(kWindowId1),
-                                  SessionID::FromSerializedValue(kTabId2));
-
   // Window ID has changed when the browser is started.
   TestSyncedWindowDelegate* window = AddWindow(kWindowId2);
   AddTab(kWindowId2, kFoo1, kTabId1);
@@ -590,78 +578,6 @@ TEST_F(LocalSessionEventHandlerImplTest, PropagateNewTab) {
   AddTab(kWindowId1, kBar1, kTabId2);
 }
 
-TEST_F(LocalSessionEventHandlerImplTest, PropagateClosedTab) {
-  AddWindow(kWindowId1);
-  AddTab(kWindowId1, kFoo1, kTabId1);
-  TestSyncedTabDelegate* tab2 = AddTab(kWindowId1, kBar1, kTabId2);
-
-  InitHandler();
-
-  // Closing a tab (later below) is expected to verify if the sync entity is
-  // unsynced.
-  EXPECT_CALL(mock_delegate_, IsTabNodeUnsynced(/*tab_node_id=*/0));
-
-  // Closing a tab is expected to update the header and the remaining tab (this
-  // test issues a navigation for it, but it would have been updated anyway).
-  auto mock_batch = std::make_unique<StrictMock<MockWriteBatch>>();
-  EXPECT_CALL(
-      *mock_batch,
-      Put(Pointee(MatchesHeader(kSessionTag, {kWindowId1}, {kTabId2}))));
-  EXPECT_CALL(*mock_batch,
-              Put(Pointee(MatchesTab(kSessionTag, kWindowId1, kTabId2,
-                                     /*tab_node_id=*/1, /*urls=*/{kBar1}))));
-  EXPECT_CALL(*mock_batch, Commit());
-  EXPECT_CALL(mock_delegate_, CreateLocalSessionWriteBatch())
-      .WillOnce(Return(ByMove(std::move(mock_batch))));
-
-  // Close tab and force reassociation.
-  window_getter_.CloseTab(SessionID::FromSerializedValue(kTabId1));
-  handler_->OnLocalTabModified(tab2);
-}
-
-TEST_F(LocalSessionEventHandlerImplTest,
-       PropagateClosedTabWithDeferredRecyclingAndImmediateDeletion) {
-  base::test::ScopedFeatureList feature_list;
-  feature_list.InitWithFeatures(
-      /*enabled_features=*/{kDeferRecyclingOfSyncTabNodesIfUnsynced,
-                            kTabNodePoolImmediateDeletion},
-      /*disabled_features=*/{});
-
-  // We start with three tabs.
-  AddWindow(kWindowId1);
-  AddTab(kWindowId1, kFoo1, kTabId1);
-  AddTab(kWindowId1, kBar1, kTabId2);
-  TestSyncedTabDelegate* tab3 = AddTab(kWindowId1, kBaz1, kTabId3);
-
-  InitHandler();
-
-  // |kTabId2| is unsynced, so it shouldn't be deleted even if it's closed.
-  EXPECT_CALL(mock_delegate_, IsTabNodeUnsynced(/*tab_node_id=*/0))
-      .WillOnce(Return(false));
-  EXPECT_CALL(mock_delegate_, IsTabNodeUnsynced(/*tab_node_id=*/1))
-      .WillOnce(Return(true));
-
-  // Closing two tabs (later below) is expected to update the header and the
-  // remaining tab. In addition, one of the two closed tabs (the one that is
-  // synced) should be deleted.
-  auto mock_batch = std::make_unique<StrictMock<MockWriteBatch>>();
-  EXPECT_CALL(
-      *mock_batch,
-      Put(Pointee(MatchesHeader(kSessionTag, {kWindowId1}, {kTabId3}))));
-  EXPECT_CALL(*mock_batch,
-              Put(Pointee(MatchesTab(kSessionTag, kWindowId1, kTabId3,
-                                     /*tab_node_id=*/2, /*urls=*/{kBaz1}))));
-  EXPECT_CALL(*mock_batch, Delete(/*tab_node_id=*/0));
-  EXPECT_CALL(*mock_batch, Commit());
-  EXPECT_CALL(mock_delegate_, CreateLocalSessionWriteBatch())
-      .WillOnce(Return(ByMove(std::move(mock_batch))));
-
-  // Close two tabs and force reassociation.
-  window_getter_.CloseTab(SessionID::FromSerializedValue(kTabId1));
-  window_getter_.CloseTab(SessionID::FromSerializedValue(kTabId2));
-  handler_->OnLocalTabModified(tab3);
-}
-
 TEST_F(LocalSessionEventHandlerImplTest, PropagateNewCustomTab) {
   InitHandler();
 

components/sync_sessions/session_sync_bridge.cc

@@ -313,12 +313,6 @@ SessionSyncBridge::CreateLocalSessionWriteBatch() {
       change_processor());
 }
 
-bool SessionSyncBridge::IsTabNodeUnsynced(int tab_node_id) {
-  const std::string storage_key = SessionStore::GetTabStorageKey(
-      syncing_->store->local_session_info().session_tag, tab_node_id);
-  return change_processor()->IsEntityUnsynced(storage_key);
-}
-
 void SessionSyncBridge::TrackLocalNavigationId(base::Time timestamp,
                                                int unique_id) {
   global_id_mapper_.TrackNavigationId(timestamp, unique_id);

components/sync_sessions/session_sync_bridge.h

@@ -72,7 +72,6 @@ class SessionSyncBridge : public syncer::ModelTypeSyncBridge,
   // LocalSessionEventHandlerImpl::Delegate implementation.
   std::unique_ptr<LocalSessionEventHandlerImpl::WriteBatch>
   CreateLocalSessionWriteBatch() override;
-  bool IsTabNodeUnsynced(int tab_node_id) override;
   void TrackLocalNavigationId(base::Time timestamp, int unique_id) override;
   void OnPageFaviconUpdated(const GURL& page_url) override;
   void OnFaviconVisited(const GURL& page_url, const GURL& favicon_url) override;

components/sync_sessions/synced_session_tracker.cc

@@ -15,22 +15,8 @@
 
 namespace sync_sessions {
 
-const base::Feature kDeferRecyclingOfSyncTabNodesIfUnsynced{
-    "DeferRecyclingOfSyncTabNodesIfUnsynced",
-    base::FEATURE_DISABLED_BY_DEFAULT};
-
 namespace {
 
-// Maximum time we allow a local tab stay unmapped (i.e. closed) but not freed
-// due to data not having been committed yet. After that time, the data will
-// be dropped.
-constexpr base::TimeDelta kMaxUnmappedButUnsyncedLocalTabAge =
-    base::TimeDelta::FromDays(1);
-// This is a generous cap to avoid issues with situations like sync being in
-// error state (e.g. auth error) during which many tabs could be opened and
-// closed, and still the information would not be committed.
-constexpr int kMaxUnmappedButUnsyncedLocalTabCount = 20;
-
 // Helper for iterating through all tabs within a window, and all navigations
 // within a tab, to find if there's a valid syncable url.
 bool ShouldSyncSessionWindow(SyncSessionsClient* sessions_client,
@@ -333,10 +319,7 @@ std::vector<const SyncedSession*> SyncedSessionTracker::LookupSessions(
   return sessions;
 }
 
-void SyncedSessionTracker::CleanupSessionImpl(
-    const std::string& session_tag,
-    const base::RepeatingCallback<bool(int /*tab_node_id*/)>&
-        is_tab_node_unsynced_cb) {
+void SyncedSessionTracker::CleanupSessionImpl(const std::string& session_tag) {
   TrackedSession* session = LookupTrackedSession(session_tag);
   if (!session)
     return;
@@ -345,36 +328,13 @@ void SyncedSessionTracker::CleanupSessionImpl(
     session->synced_window_map.erase(window_pair.first);
   session->unmapped_windows.clear();
 
-  int num_unmapped_and_unsynced = 0;
-  auto tab_it = session->unmapped_tabs.begin();
-  while (tab_it != session->unmapped_tabs.end()) {
-    SessionID tab_id = tab_it->first;
-
-    if (session_tag == local_session_tag_) {
-      int tab_node_id = session->tab_node_pool.GetTabNodeIdFromTabId(tab_id);
-      const base::TimeDelta time_since_last_modified =
-          base::Time::Now() - tab_it->second->timestamp;
-
-      if ((time_since_last_modified < kMaxUnmappedButUnsyncedLocalTabAge) &&
-          num_unmapped_and_unsynced < kMaxUnmappedButUnsyncedLocalTabCount &&
-          is_tab_node_unsynced_cb.Run(tab_node_id) &&
-          base::FeatureList::IsEnabled(
-              kDeferRecyclingOfSyncTabNodesIfUnsynced)) {
-        // Our caller has decided that this tab node cannot be reused at this
-        // point because there are pending changes to be committed that would
-        // otherwise be lost). Hence, it stays unmapped but we do not free the
-        // tab node for now (until future retries).
-        ++tab_it;
-        ++num_unmapped_and_unsynced;
-        continue;
-      }
-
-      session->tab_node_pool.FreeTab(tab_id);
-    }
+  for (const auto& tab_pair : session->unmapped_tabs) {
+    session->synced_tab_map.erase(tab_pair.first);
 
-    session->synced_tab_map.erase(tab_id);
-    tab_it = session->unmapped_tabs.erase(tab_it);
+    if (session_tag == local_session_tag_)
+      session->tab_node_pool.FreeTab(tab_pair.first);
   }
+  session->unmapped_tabs.clear();
 }
 
 bool SyncedSessionTracker::IsTabUnmappedForTesting(SessionID tab_id) {
@@ -530,23 +490,14 @@ sessions::SessionTab* SyncedSessionTracker::GetTab(
 }
 
 void SyncedSessionTracker::CleanupSession(const std::string& session_tag) {
-  DCHECK_NE(session_tag, local_session_tag_);
-  // |is_tab_node_unsynced_cb| is only used for the local session, not needed
-  // here.
-  CleanupSessionImpl(
-      session_tag,
-      /*=is_tab_node_unsynced_cb=*/base::RepeatingCallback<bool(int)>());
+  CleanupSessionImpl(session_tag);
 }
 
-std::set<int> SyncedSessionTracker::CleanupLocalTabs(
-    const base::RepeatingCallback<bool(int /*tab_node_id*/)>&
-        is_tab_node_unsynced_cb) {
+void SyncedSessionTracker::CleanupLocalTabs(std::set<int>* deleted_node_ids) {
   DCHECK(!local_session_tag_.empty());
   TrackedSession* session = GetTrackedSession(local_session_tag_);
-  CleanupSessionImpl(local_session_tag_, is_tab_node_unsynced_cb);
-  std::set<int> deleted_node_ids;
-  session->tab_node_pool.CleanupTabNodes(&deleted_node_ids);
-  return deleted_node_ids;
+  CleanupSessionImpl(local_session_tag_);
+  session->tab_node_pool.CleanupTabNodes(deleted_node_ids);
 }
 
 int SyncedSessionTracker::LookupTabNodeFromTabId(const std::string& session_tag,
@@ -696,10 +647,7 @@ void UpdateTrackerWithSpecifics(const sync_pb::SessionSpecifics& specifics,
     PopulateSyncedSessionFromSpecifics(session_tag, header, modification_time,
                                        session, tracker);
 
-    // Delete any closed windows and unused tabs as necessary. We exclude the
-    // local session here because it should be cleaned up explicitly with
-    // CleanupLocalTabs().
-    if (session_tag != tracker->GetLocalSessionTag())
+    // Delete any closed windows and unused tabs as necessary.
     tracker->CleanupSession(session_tag);
   } else if (specifics.has_tab()) {
     const sync_pb::SessionTab& tab_s = specifics.tab();

components/sync_sessions/synced_session_tracker.h

@@ -13,7 +13,6 @@
 #include <string>
 #include <vector>
 
-#include "base/feature_list.h"
 #include "base/macros.h"
 #include "components/sessions/core/session_id.h"
 #include "components/sessions/core/session_types.h"
@@ -28,10 +27,6 @@ namespace sync_sessions {
 
 class SyncSessionsClient;
 
-// TODO(crbug.com/882489): Remove feature toggle during code cleanup when a
-// satisfying solution is found for closed tabs.
-extern const base::Feature kDeferRecyclingOfSyncTabNodesIfUnsynced;
-
 // Class to manage synced sessions. The tracker will own all SyncedSession
 // and SyncedSessionTab objects it creates, and deletes them appropriately on
 // destruction.
@@ -177,15 +172,10 @@ class SyncedSessionTracker {
   // Gets the session tag previously set with InitLocalSession().
   const std::string& GetLocalSessionTag() const;
 
-  // Similar to CleanupSession() but also marks unmapped tabs (i.e. closed ones)
-  // as free tab nodes (which can be reused by future tabs) and triggers garbage
-  // collection (i.e. deletion) of free tab nodes. It returns the set of locally
-  // free tab nodes to be deleted. |is_tab_node_unsynced_cb| allows callers to
-  // prevent tab nodes from being "free" (and hence reusable), which in practice
-  // is useful to avoid overriding data that hasn't been synced yet.
-  std::set<int> CleanupLocalTabs(
-      const base::RepeatingCallback<bool(int /*tab_node_id*/)>&
-          is_tab_node_unsynced_cb);
+  // Similar to CleanupSession() but also triggers garbage collection of free
+  // tab nodes and consequently fills |deleted_node_ids| with the set of
+  // locally free tab nodes to be deleted.
+  void CleanupLocalTabs(std::set<int>* deleted_node_ids);
 
   // Returns the tab node ID for |tab_id| if an existing tab node was found, or
   // kInvalidTabNodeID otherwise.
@@ -275,10 +265,7 @@ class SyncedSessionTracker {
       bool exclude_local_session) const;
 
   // Implementation of CleanupSession()/CleanupLocalTabs().
-  void CleanupSessionImpl(
-      const std::string& session_tag,
-      const base::RepeatingCallback<bool(int /*tab_node_id*/)>&
-          is_tab_node_unsynced_cb);
+  void CleanupSessionImpl(const std::string& session_tag);
 
   // The client of the sync sessions datatype.
   SyncSessionsClient* const sessions_client_;

components/sync_sessions/tab_node_pool.h

@@ -29,8 +29,6 @@ namespace sync_sessions {
 // 1. Associated   : Sync node is used and associated with a tab.
 // 2. Free         : Sync node is unused.
 
-// TODO(crbug.com/882489): Remove feature toggle during code cleanup when a
-// satisfying solution is found for closed tabs.
 extern const base::Feature kTabNodePoolImmediateDeletion;
 
 class TabNodePool {

components/sync_sessions/test_synced_window_delegates_getter.cc

@@ -247,15 +247,13 @@ TestSyncedWindowDelegate::~TestSyncedWindowDelegate() = default;
 
 void TestSyncedWindowDelegate::OverrideTabAt(int index,
                                              SyncedTabDelegate* delegate) {
-  if (index >= static_cast<int>(tab_delegates_.size()))
-    tab_delegates_.resize(index + 1, nullptr);
-
   tab_delegates_[index] = delegate;
 }
 
 void TestSyncedWindowDelegate::CloseTab(SessionID tab_id) {
-  base::EraseIf(tab_delegates_, [tab_id](SyncedTabDelegate* tab) {
-    return tab->GetSessionId() == tab_id;
+  base::EraseIf(tab_delegates_,
+                [tab_id](const std::pair<int, SyncedTabDelegate*>& entry) {
+                  return entry.second->GetSessionId() == tab_id;
                 });
 }
 
@@ -296,10 +294,10 @@ bool TestSyncedWindowDelegate::IsTabPinned(const SyncedTabDelegate* tab) const {
 }
 
 SyncedTabDelegate* TestSyncedWindowDelegate::GetTabAt(int index) const {
-  if (index >= static_cast<int>(tab_delegates_.size()))
-    return nullptr;
+  if (tab_delegates_.find(index) != tab_delegates_.end())
+    return tab_delegates_.find(index)->second;
 
-  return tab_delegates_[index];
+  return nullptr;
 }
 
 SessionID TestSyncedWindowDelegate::GetTabIdAt(int index) const {

components/sync_sessions/test_synced_window_delegates_getter.h

@@ -144,7 +144,7 @@ class TestSyncedWindowDelegate : public SyncedWindowDelegate {
   const SessionID window_id_;
   const sync_pb::SessionWindow_BrowserType window_type_;
 
-  std::vector<SyncedTabDelegate*> tab_delegates_;
+  std::map<int, SyncedTabDelegate*> tab_delegates_;
   bool is_session_restore_in_progress_;
 
   DISALLOW_COPY_AND_ASSIGN(TestSyncedWindowDelegate);