[Sync] Add logic to reassociate tab nodes after restart.

chrome/browser/sync/glue/tab_node_pool.cc

Index: chrome/browser/sync/glue/tab_node_pool.cc
diff --git a/chrome/browser/sync/glue/tab_node_pool.cc b/chrome/browser/sync/glue/tab_node_pool.cc
index 69ed09f83d70f7746a380af96b6c0beb72d27733..e3ce0a70dc4b5100a03944b05bfd93408847d360 100644
--- a/chrome/browser/sync/glue/tab_node_pool.cc
+++ b/chrome/browser/sync/glue/tab_node_pool.cc
@@ -20,11 +20,10 @@ static const char kNoSessionsFolderError[] =
     "Server did not create the top-level sessions node. We "
     "might be running against an out-of-date server.";
 
-TabNodePool::TabNodePool(
-    ProfileSyncService* sync_service)
-    : tab_pool_fp_(-1),
-      sync_service_(sync_service) {
-}
+static const size_t kMaxNumberOfFreeNodes = 25;
+
+TabNodePool::TabNodePool(ProfileSyncService* sync_service)
+    : max_used_tab_node_id_(0), sync_service_(sync_service) {}
 
 TabNodePool::~TabNodePool() {}
 
@@ -35,14 +34,31 @@ std::string TabNodePool::TabIdToTag(
   return base::StringPrintf("%s %" PRIuS "", machine_tag.c_str(), tab_node_id);
 }
 
-void TabNodePool::AddTabNode(int64 sync_id) {
-  tab_syncid_pool_.resize(tab_syncid_pool_.size() + 1);
-  tab_syncid_pool_[static_cast<size_t>(++tab_pool_fp_)] = sync_id;
+void TabNodePool::AddTabNode(int64 sync_id,
+                             const SessionID& tab_id,
+                             size_t tab_node_id) {
+  DCHECK_GT(sync_id, 0);

[Nicolas Zea, 2013/06/19 21:35:33]

DCHECK_GT(sync_id, syncer::kInvalidId);

also DCHECK_GT(tab_node_id, kInvalidTabId)

[shashi, 2013/06/20 00:49:32]

Done.

+  DCHECK(tab_syncid_tab_id_map_.find(sync_id) == tab_syncid_tab_id_map_.end());
+  tab_syncid_tab_id_map_[sync_id] = tab_id.id();
+  if (max_used_tab_node_id_ < tab_node_id)
+    max_used_tab_node_id_ = tab_node_id;
+}
+
+void TabNodePool::AssociateTabNode(int64 sync_id, SessionID::id_type tab_id) {
+  DCHECK_GT(sync_id, 0);
+  // Remove node from free node pool and associate it with tab.
+  std::set<int64>::iterator it = free_nodes_pool_.find(sync_id);
+  DCHECK(it != free_nodes_pool_.end());
+  if (it != free_nodes_pool_.end()) {

[Nicolas Zea, 2013/06/19 21:35:33]

nit: don't handle DCHECK's. Remove the if statement and let this crash in the
case of a bug

[shashi, 2013/06/20 00:49:32]

Done.

+    free_nodes_pool_.erase(it);
+  }
+  DCHECK(tab_syncid_tab_id_map_.find(sync_id) == tab_syncid_tab_id_map_.end());
+  tab_syncid_tab_id_map_[sync_id] = tab_id;
 }
 
 int64 TabNodePool::GetFreeTabNode() {
   DCHECK_GT(machine_tag_.length(), 0U);
-  if (tab_pool_fp_ == -1) {
+  if (free_nodes_pool_.empty()) {
     // Tab pool has no free nodes, allocate new one.
     syncer::WriteTransaction trans(FROM_HERE, sync_service_->GetUserShare());
     syncer::ReadNode root(&trans);
@@ -51,7 +67,7 @@ int64 TabNodePool::GetFreeTabNode() {
       LOG(ERROR) << kNoSessionsFolderError;
       return syncer::kInvalidId;
     }
-    size_t tab_node_id = tab_syncid_pool_.size();
+    size_t tab_node_id = ++max_used_tab_node_id_;
     std::string tab_node_tag = TabIdToTag(machine_tag_, tab_node_id);
     syncer::WriteNode tab_node(&trans);
     syncer::WriteNode::InitUniqueByCreationResult result =
@@ -69,23 +85,69 @@ int64 TabNodePool::GetFreeTabNode() {
     specifics.set_tab_node_id(tab_node_id);
     tab_node.SetSessionSpecifics(specifics);
 
-    // Grow the pool by 1 since we created a new node. We don't actually need
-    // to put the node's id in the pool now, since the pool is still empty.
-    // The id will be added when that tab is closed and the node is freed.
-    tab_syncid_pool_.resize(tab_node_id + 1);
-    DVLOG(1) << "Adding sync node "
-             << tab_node.GetId() << " to tab syncid pool";
+    // Grow the pool by 1 since we created a new node.
+    DVLOG(1) << "Adding sync node " << tab_node.GetId()
+             << " to tab syncid pool";
+    free_nodes_pool_.insert(tab_node.GetId());
     return tab_node.GetId();
   } else {
-    // There are nodes available, grab next free and decrement free pointer.
-    return tab_syncid_pool_[static_cast<size_t>(tab_pool_fp_--)];
+    // Return the next free node.
+    return *free_nodes_pool_.begin();
   }
 }
 
 void TabNodePool::FreeTabNode(int64 sync_id) {
-  // Pool size should always match # of free tab nodes.
-  DCHECK_LT(tab_pool_fp_, static_cast<int64>(tab_syncid_pool_.size()));
-  tab_syncid_pool_[static_cast<size_t>(++tab_pool_fp_)] = sync_id;
+  SyncIDToTabIDMap::iterator it = tab_syncid_tab_id_map_.find(sync_id);
+  DCHECK(it != tab_syncid_tab_id_map_.end());
+  if (it != tab_syncid_tab_id_map_.end()) {

[Nicolas Zea, 2013/06/19 21:35:33]

here too

[shashi, 2013/06/20 00:49:32]

Done.

+    tab_syncid_tab_id_map_.erase(it);
+  }
+  // If number of free nodes exceed number of maximum allowed free nodes,
+  // delete the sync node.
+  if (free_nodes_pool_.size() < kMaxNumberOfFreeNodes) {
+    free_nodes_pool_.insert(sync_id);
+  } else {
+    if (free_nodes_pool_.find(sync_id) == free_nodes_pool_.end()) {

[Nicolas Zea, 2013/06/19 21:35:33]

This also seems like it should just be DCHECK'd. Double free's are not permitted

[shashi, 2013/06/20 00:49:32]

Done, moved the DCHECK about since this constraint should always be true.
On 2013/06/19 21:35:33, Nicolas Zea wrote:

+      syncer::WriteTransaction trans(FROM_HERE, sync_service_->GetUserShare());
+      syncer::WriteNode tab_node(&trans);
+      if (tab_node.InitByIdLookup(sync_id) != syncer::BaseNode::INIT_OK) {
+        LOG(ERROR) << "Could not find sync node with id: " << sync_id;
+        return;
+      }
+      tab_node.Tombstone();
+    }
+  }
+}
+
+bool TabNodePool::ReassociateTabNode(int64 sync_id, SessionID::id_type tab_id) {
+  SyncIDToTabIDMap::iterator it = tab_syncid_tab_id_map_.find(sync_id);
+  if (it != tab_syncid_tab_id_map_.end()) {
+    tab_syncid_tab_id_map_[sync_id] = tab_id;
+    return true;
+  }
+  return false;
+}
+
+SessionID::id_type TabNodePool::TabIDForSyncID(int64 sync_id) const {
+  SyncIDToTabIDMap::const_iterator it = tab_syncid_tab_id_map_.find(sync_id);
+  if (it != tab_syncid_tab_id_map_.end()) {
+    return it->second;
+  }
+  return kInvalidTabID;
+}
+
+void TabNodePool::FreeUnusedTabNodes(const std::set<int64>& used_sync_ids) {

[Nicolas Zea, 2013/06/19 21:35:33]

Is this method still necessary? Can't the caller just directly call FreeTabNode
directly for all the sync ids?

[shashi, 2013/06/20 00:49:32]

This method is needed so that we delete nodes for which there are no tabs. E.g.
On a session restore n out of n+1 tabs are restored, 1 tab is lost (due to a
bug/ or the restore limit of 25 restored tabs) but all n+1 sync nodes of old
tabs exist. In this case we can no longer free the sync node for lost tab if we
do not check it in syncid_tabid_map_.

+  for (SyncIDToTabIDMap::iterator it = tab_syncid_tab_id_map_.begin();
+       it != tab_syncid_tab_id_map_.end();) {
+    if (used_sync_ids.find(it->first) == used_sync_ids.end()) {
+      // This sync node is not used, return it to free node pool.
+      int64 sync_id = it->first;
+      ++it;
+      FreeTabNode(sync_id);
+    } else {
+      ++it;
+    }
+  }
 }
 
 }  // namespace browser_sync