Clean up tab nodes aggressively behind feature toggle

A new feature toggle is introduced to influence how
TabNodePool::CleanupTabNodes() issues deletions of tab nodes (i.e. sync
entities for SESSIONS).

The new mode is very aggressive and is conceptually similar to removing
the notion of 'free' nodes, although this is not obvious in the code
because it depends on how TabNodePool is used by upper layers (i.e.
FreeTab() is followed closely by CleanupTabNodes() for the local
session, in SyncedSessionTracker::CleanupLocalTabs()).

The risk that this mode (disabled by default) entails is that closed
tabs may never have the chance to sync, so if the user closed the tab
accidentally, or expected to see the tab in other devices, it would be
a regression. This doesn't seem very realistic, so let's experiment via
Finch and assess the impact of the change.

Bug: 882489
Change-Id: I76609434e5a733b8b5e4c94820ef2f29dd862ce5
Reviewed-on: https://chromium-review.googlesource.com/c/1264597
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597085}

components/sync_sessions/tab_node_pool.cc

@@ -18,6 +18,9 @@ namespace sync_sessions {
 const size_t TabNodePool::kFreeNodesLowWatermark = 25;
 const size_t TabNodePool::kFreeNodesHighWatermark = 100;
 
+const base::Feature kTabNodePoolImmediateDeletion{
+    "TabNodePoolImmediateDeletion", base::FEATURE_DISABLED_BY_DEFAULT};
+
 TabNodePool::TabNodePool() : max_used_tab_node_id_(kInvalidTabNodeID) {}
 
 // static
@@ -89,10 +92,12 @@ int TabNodePool::AssociateWithFreeTabNode(SessionID tab_id) {
     // were never associated before (but are within 0..max_used_tab_node_id_).
     if (!free_nodes_pool_.empty()) {
       tab_node_id = *free_nodes_pool_.begin();
+      DCHECK_LE(tab_node_id, max_used_tab_node_id_);
     }
     if (!missing_nodes_pool_.empty() &&
         *missing_nodes_pool_.begin() < tab_node_id) {
       tab_node_id = *missing_nodes_pool_.begin();
+      DCHECK_LE(tab_node_id, max_used_tab_node_id_);
       AddTabNode(tab_node_id);
     }
   }
@@ -142,6 +147,27 @@ SessionID TabNodePool::GetTabIdFromTabNodeId(int tab_node_id) const {
 }
 
 void TabNodePool::CleanupTabNodes(std::set<int>* deleted_node_ids) {
+  if (base::FeatureList::IsEnabled(kTabNodePoolImmediateDeletion)) {
+    // Convert all free nodes into missing nodes, each representing a deletion.
+    deleted_node_ids->insert(free_nodes_pool_.begin(), free_nodes_pool_.end());
+    missing_nodes_pool_.insert(free_nodes_pool_.begin(),
+                               free_nodes_pool_.end());
+    free_nodes_pool_.clear();
+
+    // As an optimization to save memory, update |max_used_tab_node_id_| and
+    // shrink |missing_nodes_pool_| appropriately.
+    if (nodeid_tabid_map_.empty()) {
+      max_used_tab_node_id_ = kInvalidTabNodeID;
+    } else {
+      max_used_tab_node_id_ = nodeid_tabid_map_.rbegin()->first;
+    }
+
+    missing_nodes_pool_.erase(
+        missing_nodes_pool_.upper_bound(max_used_tab_node_id_),
+        missing_nodes_pool_.end());
+    return;
+  }
+
   // If number of free nodes exceed kFreeNodesHighWatermark,
   // delete sync nodes till number reaches kFreeNodesLowWatermark.
   // Note: This logic is to mitigate temporary disassociation issues with old
@@ -182,4 +208,8 @@ std::set<int> TabNodePool::GetAllTabNodeIds() const {
   return tab_node_ids;
 }
 
+int TabNodePool::GetMaxUsedTabNodeIdForTest() const {
+  return max_used_tab_node_id_;
+}
+
 }  // namespace sync_sessions

components/sync_sessions/tab_node_pool.h

@@ -11,6 +11,7 @@
 #include <set>
 #include <string>
 
+#include "base/feature_list.h"
 #include "base/macros.h"
 #include "components/sessions/core/session_id.h"
 
@@ -28,6 +29,8 @@ namespace sync_sessions {
 // 1. Associated   : Sync node is used and associated with a tab.
 // 2. Free         : Sync node is unused.
 
+extern const base::Feature kTabNodePoolImmediateDeletion;
+
 class TabNodePool {
  public:
   TabNodePool();
@@ -76,8 +79,9 @@ class TabNodePool {
   // Returns tab node IDs for all known (used or free) tab nodes.
   std::set<int> GetAllTabNodeIds() const;
 
+  int GetMaxUsedTabNodeIdForTest() const;
+
  private:
-  friend class SyncTabNodePoolTest;
   using TabNodeIDToTabIDMap = std::map<int, SessionID>;
   using TabIDToTabNodeIDMap = std::map<SessionID, int>;
 

components/sync_sessions/tab_node_pool_unittest.cc

@@ -6,39 +6,42 @@
 
 #include <vector>
 
+#include "base/test/scoped_feature_list.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace sync_sessions {
+namespace {
+
+using testing::UnorderedElementsAre;
+
+const int kTabNodeId1 = 10;
+const int kTabNodeId2 = 5;
+const int kTabNodeId3 = 30;
+const SessionID kTabId1 = SessionID::FromSerializedValue(1010);
+const SessionID kTabId2 = SessionID::FromSerializedValue(1020);
+const SessionID kTabId3 = SessionID::FromSerializedValue(1030);
+const SessionID kTabId4 = SessionID::FromSerializedValue(1040);
+const SessionID kTabId5 = SessionID::FromSerializedValue(1050);
+const SessionID kTabId6 = SessionID::FromSerializedValue(1060);
 
 class SyncTabNodePoolTest : public testing::Test {
  protected:
   SyncTabNodePoolTest() {}
 
-  int GetMaxUsedTabNodeId() const { return pool_.max_used_tab_node_id_; }
+  int GetMaxUsedTabNodeId() const { return pool_.GetMaxUsedTabNodeIdForTest(); }
 
   void AddFreeTabNodes(const std::vector<int>& node_ids) {
+    const SessionID kTmpTabId = SessionID::FromSerializedValue(123);
     for (int node_id : node_ids) {
-      pool_.free_nodes_pool_.insert(node_id);
+      pool_.ReassociateTabNode(node_id, kTmpTabId);
+      pool_.FreeTab(kTmpTabId);
     }
   }
 
   TabNodePool pool_;
 };
 
-namespace {
-
-using testing::UnorderedElementsAre;
-
-const int kTabNodeId1 = 10;
-const int kTabNodeId2 = 5;
-const int kTabNodeId3 = 30;
-const SessionID kTabId1 = SessionID::FromSerializedValue(1010);
-const SessionID kTabId2 = SessionID::FromSerializedValue(1020);
-const SessionID kTabId3 = SessionID::FromSerializedValue(1030);
-const SessionID kTabId4 = SessionID::FromSerializedValue(1040);
-const SessionID kTabId5 = SessionID::FromSerializedValue(1050);
-
 TEST_F(SyncTabNodePoolTest, TabNodeIdIncreases) {
   std::set<int> deleted_node_ids;
 
@@ -123,18 +126,6 @@ TEST_F(SyncTabNodePoolTest, ReassociateThenFree) {
   EXPECT_EQ(2, pool_.AssociateWithFreeTabNode(kTabId5));
 }
 
-TEST_F(SyncTabNodePoolTest, AddGet) {
-  AddFreeTabNodes({5, 10});
-
-  ASSERT_EQ(TabNodePool::kInvalidTabNodeID,
-            pool_.GetTabNodeIdFromTabId(kTabId1));
-  EXPECT_EQ(5, pool_.AssociateWithFreeTabNode(kTabId1));
-  ASSERT_EQ(TabNodePool::kInvalidTabNodeID,
-            pool_.GetTabNodeIdFromTabId(kTabId2));
-  // 5 is now used, should return 10.
-  EXPECT_EQ(10, pool_.AssociateWithFreeTabNode(kTabId2));
-}
-
 TEST_F(SyncTabNodePoolTest, AssociateWithFreeTabNode) {
   ASSERT_EQ(TabNodePool::kInvalidTabNodeID,
             pool_.GetTabNodeIdFromTabId(kTabId1));
@@ -208,6 +199,80 @@ TEST_F(SyncTabNodePoolTest, AssociateWithFreeTabNodeReturnsMinimum) {
   EXPECT_EQ(2, pool_.AssociateWithFreeTabNode(kTabId5));
 }
 
+TEST_F(SyncTabNodePoolTest, AggressiveCleanupTabNodesMiddle) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(kTabNodePoolImmediateDeletion);
+
+  pool_.ReassociateTabNode(/*tab_node_id=*/0, kTabId1);
+  pool_.ReassociateTabNode(/*tab_node_id=*/1, kTabId2);
+  pool_.ReassociateTabNode(/*tab_node_id=*/2, kTabId3);
+
+  pool_.FreeTab(kTabId2);
+
+  std::set<int> deleted_node_ids;
+  pool_.CleanupTabNodes(&deleted_node_ids);
+
+  EXPECT_THAT(deleted_node_ids, UnorderedElementsAre(1));
+  EXPECT_EQ(2, GetMaxUsedTabNodeId());
+  EXPECT_EQ(1, pool_.AssociateWithFreeTabNode(kTabId4));
+  EXPECT_EQ(3, pool_.AssociateWithFreeTabNode(kTabId5));
+}
+
+TEST_F(SyncTabNodePoolTest, AggressiveCleanupTabNodesMax) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(kTabNodePoolImmediateDeletion);
+
+  pool_.ReassociateTabNode(/*tab_node_id=*/0, kTabId1);
+  pool_.ReassociateTabNode(/*tab_node_id=*/1, kTabId2);
+  pool_.ReassociateTabNode(/*tab_node_id=*/2, kTabId3);
+
+  pool_.FreeTab(kTabId3);
+
+  std::set<int> deleted_node_ids;
+  pool_.CleanupTabNodes(&deleted_node_ids);
+
+  EXPECT_THAT(deleted_node_ids, UnorderedElementsAre(2));
+  EXPECT_EQ(1, GetMaxUsedTabNodeId());
+  EXPECT_EQ(2, pool_.AssociateWithFreeTabNode(kTabId4));
+  EXPECT_EQ(3, pool_.AssociateWithFreeTabNode(kTabId5));
+}
+
+TEST_F(SyncTabNodePoolTest, AggressiveCleanupTabNodesMultiple) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(kTabNodePoolImmediateDeletion);
+
+  pool_.ReassociateTabNode(/*tab_node_id=*/0, kTabId1);
+  pool_.ReassociateTabNode(/*tab_node_id=*/1, kTabId2);
+  pool_.ReassociateTabNode(/*tab_node_id=*/2, kTabId3);
+
+  pool_.FreeTab(kTabId1);
+  pool_.FreeTab(kTabId2);
+
+  std::set<int> deleted_node_ids;
+  pool_.CleanupTabNodes(&deleted_node_ids);
+
+  EXPECT_THAT(deleted_node_ids, UnorderedElementsAre(0, 1));
+  EXPECT_EQ(2, GetMaxUsedTabNodeId());
+  EXPECT_EQ(0, pool_.AssociateWithFreeTabNode(kTabId4));
+  EXPECT_EQ(1, pool_.AssociateWithFreeTabNode(kTabId5));
+  EXPECT_EQ(3, pool_.AssociateWithFreeTabNode(kTabId6));
+}
+
+TEST_F(SyncTabNodePoolTest, AggressiveCleanupTabNodesAll) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(kTabNodePoolImmediateDeletion);
+
+  pool_.ReassociateTabNode(/*tab_node_id=*/0, kTabId1);
+
+  pool_.FreeTab(kTabId1);
+
+  std::set<int> deleted_node_ids;
+  pool_.CleanupTabNodes(&deleted_node_ids);
+  EXPECT_THAT(deleted_node_ids, UnorderedElementsAre(0));
+  EXPECT_EQ(-1, GetMaxUsedTabNodeId());
+  EXPECT_EQ(0, pool_.AssociateWithFreeTabNode(kTabId4));
+}
+
 }  // namespace
 
 }  // namespace sync_sessions