Makes sure bookmark ids are unique on reading from file. If not unique

we reassign them. The particular scenario that can lead to this is
when migrating bookmarks out of the db we never reset the ids of the
newly created nodes, so that we wrote all to the file with ids of
0. I'm not patching that code so that I have coverage of the unit
test, and we'll handle fix up during reading anyway.

BUG=24060
TEST=covered by unit tests

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

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

3 files changed, 61 insertions, 8 deletions

diff --git a/chrome/browser/bookmarks/bookmark_codec.cc b/chrome/browser/bookmarks/bookmark_codec.cc
index ebf48db..401bbd2 100644
--- a/chrome/browser/bookmarks/bookmark_codec.cc
+++ b/chrome/browser/bookmarks/bookmark_codec.cc
@@ -35,7 +35,7 @@ static const int kCurrentVersion = 1;
 
 BookmarkCodec::BookmarkCodec()
     : ids_reassigned_(false),
-      ids_missing_(false),
+      ids_valid_(true),
       maximum_id_(0) {
 }
 
@@ -66,15 +66,17 @@ bool BookmarkCodec::Decode(BookmarkNode* bb_node,
                            BookmarkNode* other_folder_node,
                            int64* max_id,
                            const Value& value) {
+  ids_.clear();
   ids_reassigned_ = false;
-  ids_missing_ = false;
+  ids_valid_ = true;
   maximum_id_ = 0;
   stored_checksum_.clear();
   InitializeChecksum();
   bool success = DecodeHelper(bb_node, other_folder_node, value);
   FinalizeChecksum();
-  // If either the checksums differ or some IDs were missing, reassign IDs.
-  if (ids_missing_ || computed_checksum() != stored_checksum())
+  // If either the checksums differ or some IDs were missing/not unique,
+  // reassign IDs.
+  if (!ids_valid_ || computed_checksum() != stored_checksum())
     ReassignIDs(bb_node, other_folder_node);
   *max_id = maximum_id_ + 1;
   return success;
@@ -189,8 +191,15 @@ bool BookmarkCodec::DecodeNode(const DictionaryValue& value,
 
   std::string id_string;
   int64 id = 0;
-  if (!value.GetString(kIdKey, &id_string) || !StringToInt64(id_string, &id))
-    ids_missing_ = true;
+  if (ids_valid_) {
+    if (!value.GetString(kIdKey, &id_string) ||
+        !StringToInt64(id_string, &id) ||
+        ids_.count(id) != 0) {
+      ids_valid_ = false;
+    } else {
+      ids_.insert(id);
+    }
+  }
 
   maximum_id_ = std::max(maximum_id_, id);
 
diff --git a/chrome/browser/bookmarks/bookmark_codec.h b/chrome/browser/bookmarks/bookmark_codec.h
index 25e9cb0..257a80f 100644
--- a/chrome/browser/bookmarks/bookmark_codec.h
+++ b/chrome/browser/bookmarks/bookmark_codec.h
@@ -9,6 +9,7 @@
 #ifndef CHROME_BROWSER_BOOKMARKS_BOOKMARK_CODEC_H_
 #define CHROME_BROWSER_BOOKMARKS_BOOKMARK_CODEC_H_
 
+#include <set>
 #include <string>
 
 #include "base/basictypes.h"
@@ -137,8 +138,13 @@ class BookmarkCodec {
   // Whether or not IDs were reassigned by the codec.
   bool ids_reassigned_;
 
-  // Whether or not IDs were missing for some bookmark nodes during decoding.
-  bool ids_missing_;
+  // Whether or not IDs are valid. This is initially true, but set to false
+  // if an id is missing or not unique.
+  bool ids_valid_;
+
+  // Contains the id of each of the nodes found in the file. Used to determine
+  // if we have duplicates.
+  std::set<int64> ids_;
 
   // MD5 context used to compute MD5 hash of all bookmark data.
   MD5Context md5_context_;
diff --git a/chrome/browser/bookmarks/bookmark_model_unittest.cc b/chrome/browser/bookmarks/bookmark_model_unittest.cc
index 047ac25..8ae04ad 100644
--- a/chrome/browser/bookmarks/bookmark_model_unittest.cc
+++ b/chrome/browser/bookmarks/bookmark_model_unittest.cc
@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <set>
+
 #include "app/tree_node_iterator.h"
 #include "app/tree_node_model.h"
 #include "base/hash_tables.h"
@@ -764,6 +766,32 @@ class BookmarkModelTestWithProfile2 : public BookmarkModelTestWithProfile {
 
     ASSERT_TRUE(bb_model_->IsBookmarked(GURL("http://www.google.com")));
   }
+
+  void VerifyUniqueIDs() {
+    std::set<int64> ids;
+    bool has_unique = true;
+    VerifyUniqueIDImpl(bb_model_->GetBookmarkBarNode(), &ids, &has_unique);
+    VerifyUniqueIDImpl(bb_model_->other_node(), &ids, &has_unique);
+    ASSERT_TRUE(has_unique);
+  }
+
+ private:
+  void VerifyUniqueIDImpl(const BookmarkNode* node,
+                          std::set<int64>* ids,
+                          bool* has_unique) {
+    if (!*has_unique)
+      return;
+    if (ids->count(node->id()) != 0) {
+      *has_unique = false;
+      return;
+    }
+    ids->insert(node->id());
+    for (int i = 0; i < node->GetChildCount(); ++i) {
+      VerifyUniqueIDImpl(node->GetChild(i), ids, has_unique);
+      if (!*has_unique)
+        return;
+    }
+  }
 };
 
 // Tests migrating bookmarks from db into file. This copies an old history db
@@ -793,6 +821,11 @@ TEST_F(BookmarkModelTestWithProfile2, MigrateFromDBToFileTest) {
   if (HasFatalFailure())
     return;
 
+  // Make sure the ids are unique.
+  VerifyUniqueIDs();
+  if (HasFatalFailure())
+    return;
+
   // Create again. This time we shouldn't load from history at all.
   profile_->CreateBookmarkModel(false);
   BlockTillBookmarkModelLoaded();
@@ -802,12 +835,17 @@ TEST_F(BookmarkModelTestWithProfile2, MigrateFromDBToFileTest) {
   if (HasFatalFailure())
     return;
 
+  VerifyUniqueIDs();
+  if (HasFatalFailure())
+    return;
+
   // Recreate the history service (with a clean db). Do this just to make sure
   // we're loading correctly from the bookmarks file.
   profile_->CreateHistoryService(true);
   profile_->CreateBookmarkModel(false);
   BlockTillBookmarkModelLoaded();
   VerifyExpectedState();
+  VerifyUniqueIDs();
 }
 
 // Simple test that removes a bookmark. This test exercises the code paths in