Use more compact tab node IDs for session sync

This changes the behavior of TabNodePool::AssociateWithFreeTabNode() to
always return the smallest available ID, resulting in a more compact
distribution of sync IDs too, hence a lower number of sync entities.

This will allow, in future patches, the deletion of arbitrary tab
entities, without incurring an increase of the number of session sync
entities.

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

components/sync_sessions/synced_session_tracker_unittest.cc

@@ -37,11 +37,11 @@ const char kTag[] = "tag";
 const char kTag2[] = "tag2";
 const char kTag3[] = "tag3";
 const char kTitle[] = "title";
-const int kTabNode1 = 1;
-const int kTabNode2 = 2;
-const int kTabNode3 = 3;
-const int kTabNode4 = 4;
-const int kTabNode5 = 5;
+const int kTabNode1 = 0;
+const int kTabNode2 = 1;
+const int kTabNode3 = 2;
+const int kTabNode4 = 3;
+const int kTabNode5 = 4;
 const SessionID kWindow1 = SessionID::FromSerializedValue(1);
 const SessionID kWindow2 = SessionID::FromSerializedValue(2);
 const SessionID kWindow3 = SessionID::FromSerializedValue(3);
@@ -504,15 +504,13 @@ TEST_F(SyncedSessionTrackerTest, CleanupLocalTabs) {
   // Start with two restored tab nodes.
   GetTracker()->ReassociateLocalTab(kTabNode1, kTab1);
   GetTracker()->ReassociateLocalTab(kTabNode2, kTab2);
-  EXPECT_TRUE(GetLocalTabNodePool()->Empty());
-  EXPECT_FALSE(GetLocalTabNodePool()->Full());
-  EXPECT_EQ(2U, GetLocalTabNodePool()->Capacity());
+  GetTracker()->CleanupLocalTabs(&free_node_ids);
+  EXPECT_TRUE(free_node_ids.empty());
 
   // Associate with no tabs. The tab pool should now be full.
   GetTracker()->ResetSessionTracking(kTag);
   GetTracker()->CleanupLocalTabs(&free_node_ids);
   EXPECT_TRUE(free_node_ids.empty());
-  EXPECT_TRUE(GetLocalTabNodePool()->Full());
 
   // Associate with only 1 tab open. A tab node should be reused.
   GetTracker()->ResetSessionTracking(kTag);
@@ -522,22 +520,14 @@ TEST_F(SyncedSessionTrackerTest, CleanupLocalTabs) {
   GetTracker()->CleanupLocalTabs(&free_node_ids);
   EXPECT_TRUE(free_node_ids.empty());
 
-  // TabNodePool should have one free tab node and one used.
-  EXPECT_EQ(2U, GetLocalTabNodePool()->Capacity());
-  EXPECT_FALSE(GetLocalTabNodePool()->Empty());
-  EXPECT_FALSE(GetLocalTabNodePool()->Full());
-
   // Simulate a tab opening, which should use the last free tab node.
   EXPECT_EQ(kTabNode2, GetTracker()->AssociateLocalTabWithFreeTabNode(kTab2));
   EXPECT_EQ(kTabNode2, GetTracker()->LookupTabNodeFromTabId(kTag, kTab2));
-  EXPECT_TRUE(GetLocalTabNodePool()->Empty());
 
   // Simulate another tab opening, which should create a new associated tab
   // node.
   EXPECT_EQ(kTabNode3, GetTracker()->AssociateLocalTabWithFreeTabNode(kTab3));
   EXPECT_EQ(kTabNode3, GetTracker()->LookupTabNodeFromTabId(kTag, kTab3));
-  EXPECT_EQ(3U, GetLocalTabNodePool()->Capacity());
-  EXPECT_TRUE(GetLocalTabNodePool()->Empty());
 
   // Previous tabs should still be associated.
   EXPECT_EQ(kTabNode1, GetTracker()->LookupTabNodeFromTabId(kTag, kTab1));
@@ -548,8 +538,6 @@ TEST_F(SyncedSessionTrackerTest, CleanupLocalTabs) {
   GetTracker()->ResetSessionTracking(kTag);
   GetTracker()->CleanupLocalTabs(&free_node_ids);
   EXPECT_TRUE(free_node_ids.empty());
-  EXPECT_TRUE(GetLocalTabNodePool()->Full());
-  EXPECT_FALSE(GetLocalTabNodePool()->Empty());
   ASSERT_TRUE(VerifyTabIntegrity(kTag));
 }
 
@@ -600,7 +588,6 @@ TEST_F(SyncedSessionTrackerTest, ReassociateTabMapped) {
             session->windows.at(kWindow1)->wrapped_window.tabs[0].get());
   ASSERT_EQ(GetTracker()->LookupTabNodeIds(kTag).size(),
             GetTracker()->LookupTabNodeIds(kTag).count(kTabNode1));
-  ASSERT_EQ(1U, GetLocalTabNodePool()->Capacity());
   ASSERT_TRUE(VerifyTabIntegrity(kTag));
 }
 
@@ -658,7 +645,6 @@ TEST_F(SyncedSessionTrackerTest, ReassociateTabMappedTwice) {
             session->windows.at(kWindow1)->wrapped_window.tabs[1].get());
   EXPECT_EQ(GetTracker()->LookupTabNodeIds(kTag).size(),
             GetTracker()->LookupTabNodeIds(kTag).count(kTabNode1));
-  EXPECT_EQ(1U, GetLocalTabNodePool()->Capacity());
 
   // Attempting to access the original tab will create a new SessionTab object.
   EXPECT_NE(GetTracker()->GetTab(kTag, kTab1),
@@ -700,7 +686,6 @@ TEST_F(SyncedSessionTrackerTest, ReassociateTabUnmapped) {
             session->windows.at(kWindow1)->wrapped_window.tabs[0].get());
   ASSERT_EQ(GetTracker()->LookupTabNodeIds(kTag).size(),
             GetTracker()->LookupTabNodeIds(kTag).count(kTabNode1));
-  ASSERT_EQ(1U, GetLocalTabNodePool()->Capacity());
   ASSERT_TRUE(VerifyTabIntegrity(kTag));
 }
 
@@ -738,7 +723,6 @@ TEST_F(SyncedSessionTrackerTest, ReassociateTabOldUnmappedNewMapped) {
             session->windows.at(kWindow1)->wrapped_window.tabs[0].get());
   ASSERT_EQ(GetTracker()->LookupTabNodeIds(kTag).size(),
             GetTracker()->LookupTabNodeIds(kTag).count(kTabNode1));
-  ASSERT_EQ(1U, GetLocalTabNodePool()->Capacity());
   ASSERT_TRUE(VerifyTabIntegrity(kTag));
 }
 
@@ -788,7 +772,6 @@ TEST_F(SyncedSessionTrackerTest, ReassociateTabSameTabId) {
             session->windows.at(kWindow1)->wrapped_window.tabs[0].get());
   ASSERT_EQ(GetTracker()->LookupTabNodeIds(kTag).size(),
             GetTracker()->LookupTabNodeIds(kTag).count(kTabNode1));
-  ASSERT_EQ(1U, GetLocalTabNodePool()->Capacity());
   ASSERT_TRUE(VerifyTabIntegrity(kTag));
 }
 
@@ -843,7 +826,6 @@ TEST_F(SyncedSessionTrackerTest, ReassociateTabOldMappedNewUnmapped) {
   // GetSession as well as the GetTab.
   ASSERT_EQ(GetTracker()->GetTab(kTag, kTab2),
             session->windows.at(kWindow1)->wrapped_window.tabs[0].get());
-  ASSERT_EQ(2U, GetLocalTabNodePool()->Capacity());
   ASSERT_TRUE(VerifyTabIntegrity(kTag));
 }
 

components/sync_sessions/tab_node_pool.cc

@@ -5,6 +5,7 @@
 #include "components/sync_sessions/tab_node_pool.h"
 
 #include <algorithm>
+#include <limits>
 
 #include "base/logging.h"
 #include "components/sync/base/model_type.h"
@@ -27,10 +28,11 @@ TabNodePool::~TabNodePool() {}
 
 void TabNodePool::AddTabNode(int tab_node_id) {
   DCHECK_GT(tab_node_id, kInvalidTabNodeID);
+  DCHECK_LE(tab_node_id, max_used_tab_node_id_);
   DCHECK(nodeid_tabid_map_.find(tab_node_id) == nodeid_tabid_map_.end());
   DVLOG(1) << "Adding tab node " << tab_node_id << " to pool.";
-  max_used_tab_node_id_ = std::max(max_used_tab_node_id_, tab_node_id);
   free_nodes_pool_.insert(tab_node_id);
+  missing_nodes_pool_.erase(tab_node_id);
 }
 
 void TabNodePool::AssociateTabNode(int tab_node_id, SessionID tab_id) {
@@ -76,14 +78,24 @@ int TabNodePool::AssociateWithFreeTabNode(SessionID tab_id) {
   DCHECK_EQ(0U, tabid_nodeid_map_.count(tab_id));
 
   int tab_node_id;
-  if (free_nodes_pool_.empty()) {
-    // Tab pool has no free nodes, allocate new one.
+  if (free_nodes_pool_.empty() && missing_nodes_pool_.empty()) {
+    // Tab pool has neither free nodes nor "holes" within the ID range, so
+    // allocate a new one by extending the range.
     tab_node_id = ++max_used_tab_node_id_;
     AddTabNode(tab_node_id);
   } else {
-    // Return the next free node.
+    tab_node_id = std::numeric_limits<int>::max();
+    // Take the smallest available, either from the freed pool or from IDs that
+    // 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();
     }
+    if (!missing_nodes_pool_.empty() &&
+        *missing_nodes_pool_.begin() < tab_node_id) {
+      tab_node_id = *missing_nodes_pool_.begin();
+      AddTabNode(tab_node_id);
+    }
+  }
 
   AssociateTabNode(tab_node_id, tab_id);
   return tab_node_id;
@@ -108,7 +120,13 @@ void TabNodePool::ReassociateTabNode(int tab_node_id, SessionID tab_id) {
     // This node was already associated with another tab. Free it.
     FreeTab(nodeid_it->second);
   } else {
-    // This is a new tab node. Add it before association.
+    // This is a new tab node. Add it before association. We also need to
+    // remember the "holes".
+    for (int missing_tab_node_id = max_used_tab_node_id_ + 1;
+         missing_tab_node_id < tab_node_id; ++missing_tab_node_id) {
+      missing_nodes_pool_.insert(missing_tab_node_id);
+    }
+    max_used_tab_node_id_ = std::max(max_used_tab_node_id_, tab_node_id);
     AddTabNode(tab_node_id);
   }
 
@@ -128,15 +146,16 @@ void TabNodePool::CleanupTabNodes(std::set<int>* deleted_node_ids) {
   // delete sync nodes till number reaches kFreeNodesLowWatermark.
   // Note: This logic is to mitigate temporary disassociation issues with old
   // clients: https://crbug.com/259918. Newer versions do not need this.
-  if (free_nodes_pool_.size() > kFreeNodesHighWatermark) {
-    for (std::set<int>::iterator free_it = free_nodes_pool_.begin();
-         free_it != free_nodes_pool_.end();) {
-      deleted_node_ids->insert(*free_it);
-      free_nodes_pool_.erase(free_it++);
-      if (free_nodes_pool_.size() <= kFreeNodesLowWatermark) {
+  if (free_nodes_pool_.size() <= kFreeNodesHighWatermark) {
     return;
   }
-    }
+
+  while (free_nodes_pool_.size() > kFreeNodesLowWatermark) {
+    // We delete the largest IDs first, to achieve more compaction.
+    const int tab_node_id = *free_nodes_pool_.rbegin();
+    deleted_node_ids->insert(tab_node_id);
+    missing_nodes_pool_.insert(tab_node_id);
+    free_nodes_pool_.erase(tab_node_id);
   }
 }
 
@@ -155,26 +174,6 @@ void TabNodePool::DeleteTabNode(int tab_node_id) {
   nodeid_tabid_map_.erase(it);
 }
 
-// Clear tab pool.
-void TabNodePool::Clear() {
-  free_nodes_pool_.clear();
-  nodeid_tabid_map_.clear();
-  tabid_nodeid_map_.clear();
-  max_used_tab_node_id_ = kInvalidTabNodeID;
-}
-
-size_t TabNodePool::Capacity() const {
-  return nodeid_tabid_map_.size() + free_nodes_pool_.size();
-}
-
-bool TabNodePool::Empty() const {
-  return free_nodes_pool_.empty();
-}
-
-bool TabNodePool::Full() {
-  return nodeid_tabid_map_.empty();
-}
-
 std::set<int> TabNodePool::GetAllTabNodeIds() const {
   std::set<int> tab_node_ids = free_nodes_pool_;
   for (const auto& entry : nodeid_tabid_map_) {

components/sync_sessions/tab_node_pool.h

@@ -73,19 +73,6 @@ class TabNodePool {
   // when remote deletions are received.
   void DeleteTabNode(int tab_node_id);
 
-  // Clear tab pool.
-  void Clear();
-
-  // Return the number of tab nodes this client currently has allocated
-  // (including both free and associated nodes).
-  size_t Capacity() const;
-
-  // Return empty status (all tab nodes are in use).
-  bool Empty() const;
-
-  // Return full status (no tab nodes are in use).
-  bool Full();
-
   // Returns tab node IDs for all known (used or free) tab nodes.
   std::set<int> GetAllTabNodeIds() const;
 
@@ -115,10 +102,14 @@ class TabNodePool {
   // The node ids for the set of free sync nodes.
   std::set<int> free_nodes_pool_;
 
-  // The maximum used tab_node id for a sync node. A new sync node will always
-  // be created with max_used_tab_node_id_ + 1.
+  // The maximum used tab_node id for a sync node.
   int max_used_tab_node_id_;
 
+  // Not actual tab nodes, but instead represent "holes", i.e. tab node IDs
+  // that are not used within the range [0..max_used_tab_node_id_). This
+  // allows AssociateWithFreeTabNode() to return a compact distribution of IDs.
+  std::set<int> missing_nodes_pool_;
+
   DISALLOW_COPY_AND_ASSIGN(TabNodePool);
 };
 

components/sync_sessions/tab_node_pool_unittest.cc

@@ -32,10 +32,12 @@ using testing::UnorderedElementsAre;
 
 const int kTabNodeId1 = 10;
 const int kTabNodeId2 = 5;
-const int kTabNodeId3 = 1000;
-const SessionID kTabId1 = SessionID::FromSerializedValue(1);
-const SessionID kTabId2 = SessionID::FromSerializedValue(2);
-const SessionID kTabId3 = SessionID::FromSerializedValue(3);
+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;
@@ -50,7 +52,6 @@ TEST_F(SyncTabNodePoolTest, TabNodeIdIncreases) {
   // Freeing a tab node does not change max_used_tab_node_id_.
   pool_.FreeTab(kTabId3);
   pool_.CleanupTabNodes(&deleted_node_ids);
-  EXPECT_TRUE(deleted_node_ids.empty());
   pool_.FreeTab(kTabId2);
   pool_.CleanupTabNodes(&deleted_node_ids);
   EXPECT_TRUE(deleted_node_ids.empty());
@@ -68,49 +69,33 @@ TEST_F(SyncTabNodePoolTest, TabNodeIdIncreases) {
   pool_.CleanupTabNodes(&deleted_node_ids);
   EXPECT_TRUE(deleted_node_ids.empty());
   EXPECT_EQ(kTabNodeId3, GetMaxUsedTabNodeId());
-  EXPECT_TRUE(pool_.Empty());
 }
 
 TEST_F(SyncTabNodePoolTest, Reassociation) {
   // Reassociate tab node 1 with tab id 1.
   pool_.ReassociateTabNode(kTabNodeId1, kTabId1);
-  EXPECT_EQ(1U, pool_.Capacity());
-  EXPECT_TRUE(pool_.Empty());
-  EXPECT_FALSE(pool_.Full());
   EXPECT_EQ(kTabId1, pool_.GetTabIdFromTabNodeId(kTabNodeId1));
   EXPECT_FALSE(pool_.GetTabIdFromTabNodeId(kTabNodeId2).is_valid());
 
   // Introduce a new tab node associated with the same tab. The old tab node
   // should get added to the free pool
   pool_.ReassociateTabNode(kTabNodeId2, kTabId1);
-  EXPECT_EQ(2U, pool_.Capacity());
-  EXPECT_FALSE(pool_.Empty());
-  EXPECT_FALSE(pool_.Full());
   EXPECT_FALSE(pool_.GetTabIdFromTabNodeId(kTabNodeId1).is_valid());
   EXPECT_EQ(kTabId1, pool_.GetTabIdFromTabNodeId(kTabNodeId2));
 
   // Reassociating the same tab node/tab should have no effect.
   pool_.ReassociateTabNode(kTabNodeId2, kTabId1);
-  EXPECT_EQ(2U, pool_.Capacity());
-  EXPECT_FALSE(pool_.Empty());
-  EXPECT_FALSE(pool_.Full());
   EXPECT_FALSE(pool_.GetTabIdFromTabNodeId(kTabNodeId1).is_valid());
   EXPECT_EQ(kTabId1, pool_.GetTabIdFromTabNodeId(kTabNodeId2));
 
   // Reassociating the new tab node with a new tab should just update the
   // association tables.
   pool_.ReassociateTabNode(kTabNodeId2, kTabId2);
-  EXPECT_EQ(2U, pool_.Capacity());
-  EXPECT_FALSE(pool_.Empty());
-  EXPECT_FALSE(pool_.Full());
   EXPECT_FALSE(pool_.GetTabIdFromTabNodeId(kTabNodeId1).is_valid());
   EXPECT_EQ(kTabId2, pool_.GetTabIdFromTabNodeId(kTabNodeId2));
 
   // Reassociating the first tab node should make the pool empty.
   pool_.ReassociateTabNode(kTabNodeId1, kTabId1);
-  EXPECT_EQ(2U, pool_.Capacity());
-  EXPECT_TRUE(pool_.Empty());
-  EXPECT_FALSE(pool_.Full());
   EXPECT_EQ(kTabId1, pool_.GetTabIdFromTabNodeId(kTabNodeId1));
   EXPECT_EQ(kTabId2, pool_.GetTabIdFromTabNodeId(kTabNodeId2));
 }
@@ -119,55 +104,31 @@ TEST_F(SyncTabNodePoolTest, ReassociateThenFree) {
   std::set<int> deleted_node_ids;
 
   // Verify old tab nodes are reassociated correctly.
-  pool_.ReassociateTabNode(kTabNodeId1, kTabId1);
-  pool_.ReassociateTabNode(kTabNodeId2, kTabId2);
-  pool_.ReassociateTabNode(kTabNodeId3, kTabId3);
-  EXPECT_EQ(3u, pool_.Capacity());
-  EXPECT_TRUE(pool_.Empty());
+  pool_.ReassociateTabNode(/*tab_node_id=*/0, kTabId1);
+  pool_.ReassociateTabNode(/*tab_node_id=*/1, kTabId2);
+  pool_.ReassociateTabNode(/*tab_node_id=*/2, kTabId3);
   // Free tabs 2 and 3.
   pool_.FreeTab(kTabId2);
   pool_.FreeTab(kTabId3);
-  pool_.CleanupTabNodes(&deleted_node_ids);
-  EXPECT_TRUE(deleted_node_ids.empty());
-  // Free node pool should have 2 and 3.
-  EXPECT_FALSE(pool_.Empty());
-  EXPECT_EQ(3u, pool_.Capacity());
 
-  // Free all nodes
-  pool_.FreeTab(kTabId1);
-  pool_.CleanupTabNodes(&deleted_node_ids);
-  EXPECT_TRUE(deleted_node_ids.empty());
-  EXPECT_TRUE(pool_.Full());
-
-  // Reassociate tab nodes.
-  std::vector<int> sync_ids;
-  for (int i = 1; i <= 3; ++i) {
-    const SessionID tab_id = SessionID::FromSerializedValue(i);
   EXPECT_EQ(TabNodePool::kInvalidTabNodeID,
-              pool_.GetTabNodeIdFromTabId(tab_id));
-    sync_ids.push_back(pool_.AssociateWithFreeTabNode(tab_id));
-  }
-
-  EXPECT_TRUE(pool_.Empty());
-  EXPECT_THAT(sync_ids,
-              UnorderedElementsAre(kTabNodeId1, kTabNodeId2, kTabNodeId3));
-}
+            pool_.GetTabNodeIdFromTabId(kTabId2));
+  EXPECT_EQ(TabNodePool::kInvalidTabNodeID,
+            pool_.GetTabNodeIdFromTabId(kTabId3));
+  EXPECT_NE(TabNodePool::kInvalidTabNodeID,
+            pool_.GetTabNodeIdFromTabId(kTabId1));
 
-TEST_F(SyncTabNodePoolTest, Init) {
-  EXPECT_TRUE(pool_.Empty());
-  EXPECT_TRUE(pool_.Full());
+  // Free node pool should have 1 (for kTabId2) and 2 (for kTabId3).
+  EXPECT_EQ(1, pool_.AssociateWithFreeTabNode(kTabId4));
+  EXPECT_EQ(2, pool_.AssociateWithFreeTabNode(kTabId5));
 }
 
 TEST_F(SyncTabNodePoolTest, AddGet) {
   AddFreeTabNodes({5, 10});
 
-  EXPECT_EQ(2U, pool_.Capacity());
   ASSERT_EQ(TabNodePool::kInvalidTabNodeID,
             pool_.GetTabNodeIdFromTabId(kTabId1));
   EXPECT_EQ(5, pool_.AssociateWithFreeTabNode(kTabId1));
-  EXPECT_FALSE(pool_.Empty());
-  EXPECT_FALSE(pool_.Full());
-  EXPECT_EQ(2U, pool_.Capacity());
   ASSERT_EQ(TabNodePool::kInvalidTabNodeID,
             pool_.GetTabNodeIdFromTabId(kTabId2));
   // 5 is now used, should return 10.
@@ -210,12 +171,6 @@ TEST_F(SyncTabNodePoolTest, TabPoolFreeNodeLimits) {
     EXPECT_TRUE(deleted_node_ids.empty());
   }
 
-  // Except one node all nodes should be in FreeNode pool.
-  EXPECT_FALSE(pool_.Full());
-  EXPECT_FALSE(pool_.Empty());
-  // Total capacity = 1 Associated Node + kFreeNodesHighWatermark free node.
-  EXPECT_EQ(TabNodePool::kFreeNodesHighWatermark + 1, pool_.Capacity());
-
   // Freeing the last sync node should drop the free nodes to
   // kFreeNodesLowWatermark.
   pool_.FreeTab(
@@ -224,9 +179,33 @@ TEST_F(SyncTabNodePoolTest, TabPoolFreeNodeLimits) {
   EXPECT_EQ(TabNodePool::kFreeNodesHighWatermark + 1 -
                 TabNodePool::kFreeNodesLowWatermark,
             deleted_node_ids.size());
-  EXPECT_FALSE(pool_.Empty());
-  EXPECT_TRUE(pool_.Full());
-  EXPECT_EQ(TabNodePool::kFreeNodesLowWatermark, pool_.Capacity());
+  // Make sure the highest ones are deleted.
+  EXPECT_EQ(0U,
+            deleted_node_ids.count(TabNodePool::kFreeNodesLowWatermark - 1));
+  EXPECT_NE(0U, deleted_node_ids.count(TabNodePool::kFreeNodesLowWatermark));
+  EXPECT_NE(0U, deleted_node_ids.count(TabNodePool::kFreeNodesHighWatermark));
+}
+
+TEST_F(SyncTabNodePoolTest, AssociateWithFreeTabNodesContiguous) {
+  pool_.ReassociateTabNode(/*tab_node_id=*/2, kTabId1);
+  EXPECT_EQ(0, pool_.AssociateWithFreeTabNode(kTabId2));
+  EXPECT_EQ(1, pool_.AssociateWithFreeTabNode(kTabId3));
+  // Tab node 2 is already used, so it should be skipped.
+  EXPECT_EQ(3, pool_.AssociateWithFreeTabNode(kTabId4));
+}
+
+// Tests that, when *both* a free tab node and a "hole" exists,
+// AssociateWithFreeTabNode() returns the smallest of them.
+TEST_F(SyncTabNodePoolTest, AssociateWithFreeTabNodeReturnsMinimum) {
+  // Set up the pool such that tab node 1 is freed, and nodes 0 and 2 are holes
+  // (missing).
+  pool_.ReassociateTabNode(/*tab_node_id=*/1, kTabId1);
+  pool_.ReassociateTabNode(/*tab_node_id=*/3, kTabId2);
+  pool_.FreeTab(kTabId1);
+
+  EXPECT_EQ(0, pool_.AssociateWithFreeTabNode(kTabId3));
+  EXPECT_EQ(1, pool_.AssociateWithFreeTabNode(kTabId4));
+  EXPECT_EQ(2, pool_.AssociateWithFreeTabNode(kTabId5));
 }
 
 }  // namespace