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}

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

@@ -5,6 +5,7 @@
 #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"
@@ -24,6 +25,7 @@
 #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"
 
@@ -239,6 +241,35 @@ 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));
+  CloseTab(/*index=*/0, /*tab_index=*/1);
+  ASSERT_TRUE(OpenTab(0, GURL(kURL4)));
+
+  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.
+  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,6 +91,9 @@ 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();
 }
@@ -101,6 +104,16 @@ 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_));
@@ -219,11 +232,7 @@ void LocalSessionEventHandlerImpl::AssociateWindows(ReloadTabsOption option,
     }
   }
 
-  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);
-  }
+  CleanupLocalTabs(batch);
 
   // 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,6 +48,7 @@ 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;
@@ -79,6 +80,8 @@ 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,6 +8,7 @@
 #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"
@@ -71,6 +72,7 @@ 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));
@@ -355,6 +357,16 @@ 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);
@@ -578,6 +590,78 @@ 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,6 +313,12 @@ 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,6 +72,7 @@ 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,8 +15,22 @@
 
 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,
@@ -319,7 +333,10 @@ std::vector<const SyncedSession*> SyncedSessionTracker::LookupSessions(
   return sessions;
 }
 
-void SyncedSessionTracker::CleanupSessionImpl(const std::string& session_tag) {
+void SyncedSessionTracker::CleanupSessionImpl(
+    const std::string& session_tag,
+    const base::RepeatingCallback<bool(int /*tab_node_id*/)>&
+        is_tab_node_unsynced_cb) {
   TrackedSession* session = LookupTrackedSession(session_tag);
   if (!session)
     return;
@@ -328,13 +345,36 @@ void SyncedSessionTracker::CleanupSessionImpl(const std::string& session_tag) {
     session->synced_window_map.erase(window_pair.first);
   session->unmapped_windows.clear();
 
-  for (const auto& tab_pair : session->unmapped_tabs) {
-    session->synced_tab_map.erase(tab_pair.first);
+  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);
+    }
 
-    if (session_tag == local_session_tag_)
-      session->tab_node_pool.FreeTab(tab_pair.first);
+    session->synced_tab_map.erase(tab_id);
+    tab_it = session->unmapped_tabs.erase(tab_it);
   }
-  session->unmapped_tabs.clear();
 }
 
 bool SyncedSessionTracker::IsTabUnmappedForTesting(SessionID tab_id) {
@@ -490,14 +530,23 @@ sessions::SessionTab* SyncedSessionTracker::GetTab(
 }
 
 void SyncedSessionTracker::CleanupSession(const std::string& session_tag) {
-  CleanupSessionImpl(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)>());
 }
 
-void SyncedSessionTracker::CleanupLocalTabs(std::set<int>* deleted_node_ids) {
+std::set<int> SyncedSessionTracker::CleanupLocalTabs(
+    const base::RepeatingCallback<bool(int /*tab_node_id*/)>&
+        is_tab_node_unsynced_cb) {
   DCHECK(!local_session_tag_.empty());
   TrackedSession* session = GetTrackedSession(local_session_tag_);
-  CleanupSessionImpl(local_session_tag_);
-  session->tab_node_pool.CleanupTabNodes(deleted_node_ids);
+  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;
 }
 
 int SyncedSessionTracker::LookupTabNodeFromTabId(const std::string& session_tag,
@@ -647,7 +696,10 @@ void UpdateTrackerWithSpecifics(const sync_pb::SessionSpecifics& specifics,
     PopulateSyncedSessionFromSpecifics(session_tag, header, modification_time,
                                        session, tracker);
 
-    // Delete any closed windows and unused tabs as necessary.
+    // 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())
       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,6 +13,7 @@
 #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"
@@ -27,6 +28,10 @@ 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.
@@ -172,10 +177,15 @@ class SyncedSessionTracker {
   // Gets the session tag previously set with InitLocalSession().
   const std::string& GetLocalSessionTag() const;
 
-  // 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);
+  // 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);
 
   // Returns the tab node ID for |tab_id| if an existing tab node was found, or
   // kInvalidTabNodeID otherwise.
@@ -265,7 +275,10 @@ class SyncedSessionTracker {
       bool exclude_local_session) const;
 
   // Implementation of CleanupSession()/CleanupLocalTabs().
-  void CleanupSessionImpl(const std::string& session_tag);
+  void CleanupSessionImpl(
+      const std::string& session_tag,
+      const base::RepeatingCallback<bool(int /*tab_node_id*/)>&
+          is_tab_node_unsynced_cb);
 
   // The client of the sync sessions datatype.
   SyncSessionsClient* const sessions_client_;

components/sync_sessions/tab_node_pool.h

@@ -29,6 +29,8 @@ 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,13 +247,15 @@ 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](const std::pair<int, SyncedTabDelegate*>& entry) {
-                  return entry.second->GetSessionId() == tab_id;
+  base::EraseIf(tab_delegates_, [tab_id](SyncedTabDelegate* tab) {
+    return tab->GetSessionId() == tab_id;
   });
 }
 
@@ -294,10 +296,10 @@ bool TestSyncedWindowDelegate::IsTabPinned(const SyncedTabDelegate* tab) const {
 }
 
 SyncedTabDelegate* TestSyncedWindowDelegate::GetTabAt(int index) const {
-  if (tab_delegates_.find(index) != tab_delegates_.end())
-    return tab_delegates_.find(index)->second;
-
+  if (index >= static_cast<int>(tab_delegates_.size()))
     return nullptr;
+
+  return tab_delegates_[index];
 }
 
 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::map<int, SyncedTabDelegate*> tab_delegates_;
+  std::vector<SyncedTabDelegate*> tab_delegates_;
   bool is_session_restore_in_progress_;
 
   DISALLOW_COPY_AND_ASSIGN(TestSyncedWindowDelegate);