Try to revive the revision 30441 which was reverted in 30443.

BUG=23978
TEST=Added unit test

Review URL: http://codereview.chromium.org/343041

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@30484 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 87 insertions, 6 deletions

diff --git a/chrome/browser/sync/glue/model_associator.cc b/chrome/browser/sync/glue/model_associator.cc
index c481da5..465f58f 100644
--- a/chrome/browser/sync/glue/model_associator.cc
+++ b/chrome/browser/sync/glue/model_associator.cc
@@ -6,6 +6,7 @@
 
 #include <stack>
 
+#include "base/hash_tables.h"
 #include "base/message_loop.h"
 #include "base/task.h"
 #include "chrome/browser/bookmarks/bookmark_model.h"
@@ -110,6 +111,49 @@ const BookmarkNode* BookmarkNodeFinder::FindBookmarkNode(
   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) {
@@ -154,6 +198,7 @@ const BookmarkNode* ModelAssociator::GetBookmarkNodeFromSyncId(int64 sync_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);
 }
 
@@ -451,12 +496,18 @@ bool ModelAssociator::LoadAssociations() {
   }
   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);
@@ -464,9 +515,13 @@ bool ModelAssociator::LoadAssociations() {
   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();
@@ -477,7 +532,7 @@ bool ModelAssociator::LoadAssociations() {
     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;
 
@@ -503,7 +558,13 @@ bool ModelAssociator::LoadAssociations() {
     }
   }
   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
diff --git a/chrome/browser/sync/profile_sync_service_unittest.cc b/chrome/browser/sync/profile_sync_service_unittest.cc
index 760f51f..30fea8b 100644
--- a/chrome/browser/sync/profile_sync_service_unittest.cc
+++ b/chrome/browser/sync/profile_sync_service_unittest.cc
@@ -1235,6 +1235,26 @@ TEST_F(ProfileSyncServiceTestWithData, ModelAssociationPersistence) {
   ExpectModelMatch();
 }
 
+// Tests that when persisted model assocations are used, things work fine.
+TEST_F(ProfileSyncServiceTestWithData, ModelAssociationInvalidPersistence) {
+  LoadBookmarkModel(DELETE_EXISTING_STORAGE, DONT_SAVE_TO_STORAGE);
+  WriteTestDataToBookmarkModel();
+  StartSyncService();
+  ExpectModelMatch();
+  // Force the sync service to shut down and write itself to disk.
+  StopSyncService(SAVE_TO_STORAGE);
+  // Change the bookmark model before restarting sync service to simulate
+  // the situation where bookmark model is different from sync model and
+  // make sure model associator correctly rebuilds associations.
+  const BookmarkNode* bookmark_bar_node = model_->GetBookmarkBarNode();
+  model_->AddURL(bookmark_bar_node, 0, L"xtra", GURL("http://www.xtra.com"));
+  // Now restart the sync service. This time it will try to use the persistent
+  // associations and realize that they are invalid and hence will rebuild
+  // associations.
+  StartSyncService();
+  ExpectModelMatch();
+}
+
 TEST_F(ProfileSyncServiceTestWithData, SortChildren) {
   LoadBookmarkModel(DELETE_EXISTING_STORAGE, DONT_SAVE_TO_STORAGE);
   StartSyncService();