Make some improvements to sync model associator:...

chrome/browser/sync/glue/model_associator.cc

 // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #if defined(BROWSER_SYNC)
 
 #include "chrome/browser/sync/glue/model_associator.h"
 
 #include <stack>
 
+#include "base/hash_tables.h"
 #include "base/message_loop.h"
 #include "base/task.h"
 #include "chrome/browser/bookmarks/bookmark_model.h"
 #include "chrome/browser/sync/engine/syncapi.h"
 #include "chrome/browser/sync/profile_sync_service.h"
 
 namespace browser_sync {
 
 // The sync protocol identifies top-level entities by means of well-known tags,
 // which should not be confused with titles.  Each tag corresponds to a
@@ skipping 84 matching lines @@
   BookmarkNodesSet::iterator iter = child_nodes_.find(&temp_node);
   if (iter != child_nodes_.end()) {
     result = *iter;
     // Remove the matched node so we don't match with it again.
     child_nodes_.erase(iter);
   }
 
   return result;
 }
 
+// Helper class to build an index of bookmark nodes by their IDs.
+class BookmarkNodeIdIndex {
+ public:
+  BookmarkNodeIdIndex() { }
+  ~BookmarkNodeIdIndex() { }
+
+  // Adds the given bookmark node and all its descendants to the ID index.
+  // Does nothing if node is NULL.
+  void AddAll(const BookmarkNode* node);
+
+  // Finds the bookmark node with the given ID.
+  // Returns NULL if none exists with the given id.
+  const BookmarkNode* Find(int64 id) const;
+
+  // Returns the count of nodes in the index.
+  size_t count() const { return node_index_.size(); }
+
+ private:
+  typedef base::hash_map<int64, const BookmarkNode*> BookmarkIdMap;
+  // Map that holds nodes indexed by their ids.
+  BookmarkIdMap node_index_;
+
+  DISALLOW_COPY_AND_ASSIGN(BookmarkNodeIdIndex);
+};
+
+void BookmarkNodeIdIndex::AddAll(const BookmarkNode* node) {
+  if (!node)
+    return;
+
+  node_index_[node->id()] = node;
+
+  if (!node->is_folder())
+    return;
+
+  for (int i = 0; i < node->GetChildCount(); ++i)
+    AddAll(node->GetChild(i));
+}
+
+const BookmarkNode* BookmarkNodeIdIndex::Find(int64 id) const {
+  BookmarkIdMap::const_iterator iter = node_index_.find(id);
+  return iter == node_index_.end() ? NULL : iter->second;
+}
+
 ModelAssociator::ModelAssociator(ProfileSyncService* sync_service)
     : sync_service_(sync_service),
       task_pending_(false) {
   DCHECK(sync_service_);
 }
 
 void ModelAssociator::ClearAll() {
   id_map_.clear();
   id_map_inverse_.clear();
   dirty_associations_sync_ids_.clear();
@@ skipping 24 matching lines @@
     return false;
   DCHECK(sync_node->GetId() == sync_id);
   return true;
 }
 
 const BookmarkNode* ModelAssociator::GetBookmarkNodeFromSyncId(int64 sync_id) {
   int64 node_id;
   if (!GetBookmarkIdFromSyncId(sync_id, &node_id))
     return false;
   BookmarkModel* model = sync_service_->profile()->GetBookmarkModel();
+  // TODO(munjal): Fix issue 26141.
   return model->GetNodeByID(node_id);
 }
 
 void ModelAssociator::AssociateIds(int64 node_id, int64 sync_id) {
   DCHECK_NE(sync_id, sync_api::kInvalidId);
   DCHECK(id_map_.find(node_id) == id_map_.end());
   DCHECK(id_map_inverse_.find(sync_id) == id_map_inverse_.end());
   id_map_[node_id] = sync_id;
   id_map_inverse_[sync_id] = node_id;
   dirty_associations_sync_ids_.insert(sync_id);
@@ skipping 277 matching lines @@
   // create the tagged nodes on demand, and the order in which we probe for
   // them here will impact their positional ordering in that case.
   int64 bookmark_bar_id;
   if (!GetSyncIdForTaggedNode(WideToUTF16(kBookmarkBarTag), &bookmark_bar_id)) {
     // We should always be able to find the permanent nodes.
     sync_service_->OnUnrecoverableError();
     return false;
   }
   int64 other_bookmarks_id;
   if (!GetSyncIdForTaggedNode(WideToUTF16(kOtherBookmarksTag),
-    &other_bookmarks_id)) {
-      // We should always be able to find the permanent nodes.
-      sync_service_->OnUnrecoverableError();
-      return false;
+      &other_bookmarks_id)) {
+    // We should always be able to find the permanent nodes.
+    sync_service_->OnUnrecoverableError();
+    return false;
   }
 
+  // Build a bookmark node ID index since we are going to repeatedly search for
+  // bookmark nodes by their IDs.
+  BookmarkNodeIdIndex id_index;
+  id_index.AddAll(model->GetBookmarkBarNode());
+  id_index.AddAll(model->other_node());
+
   std::stack<int64> dfs_stack;
   dfs_stack.push(other_bookmarks_id);
   dfs_stack.push(bookmark_bar_id);
 
   sync_api::ReadTransaction trans(
       sync_service_->backend()->GetUserShareHandle());
 
+  // Count total number of nodes in sync model so that we can compare that
+  // with the total number of nodes in the bookmark model.
+  int64 sync_node_count = 0;
   while (!dfs_stack.empty()) {
     int64 parent_id = dfs_stack.top();
     dfs_stack.pop();
+    ++sync_node_count;
     sync_api::ReadNode sync_parent(&trans);
     if (!sync_parent.InitByIdLookup(parent_id)) {
       sync_service_->OnUnrecoverableError();
       return false;
     }
 
     int64 external_id = sync_parent.GetExternalId();
     if (external_id == 0)
       return false;
 
-    const BookmarkNode* node = model->GetNodeByID(external_id);
+    const BookmarkNode* node = id_index.Find(external_id);
     if (!node)
       return false;
 
     // Don't try to call NodesMatch on permanent nodes like bookmark bar and
     // other bookmarks. They are not expected to match.
     if (node != model->GetBookmarkBarNode() &&
         node != model->other_node() &&
         !NodesMatch(node, &sync_parent))
       return false;
 
     AssociateIds(external_id, sync_parent.GetId());
 
     // Add all children of the current node to the stack.
     int64 child_id = sync_parent.GetFirstChildId();
     while (child_id != sync_api::kInvalidId) {
       dfs_stack.push(child_id);
       sync_api::ReadNode child_node(&trans);
       if (!child_node.InitByIdLookup(child_id)) {
         sync_service_->OnUnrecoverableError();
         return false;
       }
       child_id = child_node.GetSuccessorId();
     }
   }
   DCHECK(dfs_stack.empty());
-  return true;
+
+  // It's possible that the number of nodes in the bookmark model is not the
+  // same as number of nodes in the sync model. This can happen when the sync
+  // model doesn't get a chance to persist its changes, for example when
+  // Chrome does not shut down gracefully. In such cases we can't trust the
+  // loaded associations.
+  return sync_node_count == id_index.count();
 }
 
 }  // namespace browser_sync
 
 #endif  // defined(BROWSER_SYNC)