Merge 120271 - [Sync] Fix writing of titles when encryption is enabled.

We would bypass idempotency checks if the datatype was encrypted. Now
we properly handle verifying that for bookmarks the underlying title is the same
and for other datatypes the NON_UNIQUE_NAME hasn't changed.

BUG=92850
TEST=sync_unit_tests

Review URL: https://chromiumcodereview.appspot.com/9320046

TBR=zea@chromium.org
Review URL: https://chromiumcodereview.appspot.com/9325037

git-svn-id: svn://svn.chromium.org/chrome/branches/1025/src@120390 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 276 insertions, 16 deletions

diff --git a/chrome/browser/sync/internal_api/base_node.h b/chrome/browser/sync/internal_api/base_node.h
index f61d0f7..70e0417 100644
--- a/chrome/browser/sync/internal_api/base_node.h
+++ b/chrome/browser/sync/internal_api/base_node.h
@@ -218,6 +218,10 @@ class BaseNode {
   FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, UpdatePasswordSetPasswordSpecifics);
   FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, UpdatePasswordNewPassphrase);
   FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, UpdatePasswordReencryptEverything);
+  FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, SetBookmarkTitle);
+  FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, SetBookmarkTitleWithEncryption);
+  FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, SetNonBookmarkTitle);
+  FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, SetNonBookmarkTitleWithEncryption);
 
   void* operator new(size_t size);  // Node is meant for stack use only.
 
diff --git a/chrome/browser/sync/internal_api/syncapi_unittest.cc b/chrome/browser/sync/internal_api/syncapi_unittest.cc
index fcbbf07..9a0b3d1 100644
--- a/chrome/browser/sync/internal_api/syncapi_unittest.cc
+++ b/chrome/browser/sync/internal_api/syncapi_unittest.cc
@@ -47,6 +47,7 @@
 #include "chrome/browser/sync/protocol/encryption.pb.h"
 #include "chrome/browser/sync/protocol/extension_specifics.pb.h"
 #include "chrome/browser/sync/protocol/password_specifics.pb.h"
+#include "chrome/browser/sync/protocol/preference_specifics.pb.h"
 #include "chrome/browser/sync/protocol/proto_value_conversions.h"
 #include "chrome/browser/sync/protocol/sync.pb.h"
 #include "chrome/browser/sync/sessions/sync_session.h"
@@ -798,6 +799,7 @@ class SyncManagerTest : public testing::Test,
     (*out)[syncable::THEMES] = browser_sync::GROUP_PASSIVE;
     (*out)[syncable::SESSIONS] = browser_sync::GROUP_PASSIVE;
     (*out)[syncable::PASSWORDS] = browser_sync::GROUP_PASSIVE;
+    (*out)[syncable::PREFERENCES] = browser_sync::GROUP_PASSIVE;
   }
 
   virtual void OnChangesApplied(
@@ -1159,7 +1161,7 @@ TEST_F(SyncManagerTest, GetChildNodeIds) {
   ListValue* nodes = NULL;
   ASSERT_TRUE(return_args.Get().GetList(0, &nodes));
   ASSERT_TRUE(nodes);
-  EXPECT_EQ(5u, nodes->GetSize());
+  EXPECT_EQ(6u, nodes->GetSize());
 }
 
 TEST_F(SyncManagerTest, GetChildNodeIdsFailure) {
@@ -1661,6 +1663,36 @@ TEST_F(SyncManagerTest, EncryptBookmarksWithLegacyData) {
   }
 }
 
+// Create a bookmark and set the title/url, then verify the data was properly
+// set. This replicates the unique way bookmarks have of creating sync nodes.
+// See BookmarkChangeProcessor::PlaceSyncNode(..).
+TEST_F(SyncManagerTest, CreateLocalBookmark) {
+  std::string title = "title";
+  GURL url("url");
+  {
+    WriteTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
+    ReadNode root_node(&trans);
+    root_node.InitByRootLookup();
+    WriteNode node(&trans);
+    ASSERT_TRUE(node.InitByCreation(syncable::BOOKMARKS, root_node, NULL));
+    node.SetIsFolder(false);
+    node.SetTitle(UTF8ToWide(title));
+    node.SetURL(url);
+  }
+  {
+    ReadTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
+    ReadNode root_node(&trans);
+    root_node.InitByRootLookup();
+    int64 child_id = root_node.GetFirstChildId();
+
+    ReadNode node(&trans);
+    ASSERT_TRUE(node.InitByIdLookup(child_id));
+    EXPECT_FALSE(node.GetIsFolder());
+    EXPECT_EQ(title, node.GetTitle());
+    EXPECT_EQ(url, node.GetURL());
+  }
+}
+
 // Verifies WriteNode::UpdateEntryWithEncryption does not make unnecessary
 // changes.
 TEST_F(SyncManagerTest, UpdateEntryWithEncryption) {
@@ -1697,7 +1729,6 @@ TEST_F(SyncManagerTest, UpdateEntryWithEncryption) {
           BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE)));
   ASSERT_TRUE(helper->Run());
   PumpLoop();
-
   {
     ReadTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
     ReadNode node(&trans);
@@ -1944,4 +1975,192 @@ TEST_F(SyncManagerTest, UpdatePasswordReencryptEverything) {
   EXPECT_FALSE(ResetUnsyncedEntry(syncable::PASSWORDS, client_tag));
 }
 
+// Verify SetTitle(..) doesn't unnecessarily set IS_UNSYNCED for bookmarks
+// when we write the same data, but does set it when we write new data.
+TEST_F(SyncManagerTest, SetBookmarkTitle) {
+  std::string client_tag = "title";
+  sync_pb::EntitySpecifics entity_specifics;
+  entity_specifics.MutableExtension(sync_pb::bookmark)->set_url("url");
+  entity_specifics.MutableExtension(sync_pb::bookmark)->set_title("title");
+  MakeServerNode(sync_manager_.GetUserShare(), syncable::BOOKMARKS, client_tag,
+                 BaseNode::GenerateSyncableHash(syncable::BOOKMARKS,
+                                                client_tag),
+                 entity_specifics);
+  // New node shouldn't start off unsynced.
+  EXPECT_FALSE(ResetUnsyncedEntry(syncable::BOOKMARKS, client_tag));
+
+  // Manually change to the same title. Should not set is_unsynced.
+  {
+    WriteTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
+    WriteNode node(&trans);
+    EXPECT_TRUE(node.InitByClientTagLookup(syncable::BOOKMARKS, client_tag));
+    node.SetTitle(UTF8ToWide(client_tag));
+  }
+  EXPECT_FALSE(ResetUnsyncedEntry(syncable::BOOKMARKS, client_tag));
+
+  // Manually change to new title. Should set is_unsynced.
+  {
+    WriteTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
+    WriteNode node(&trans);
+    EXPECT_TRUE(node.InitByClientTagLookup(syncable::BOOKMARKS, client_tag));
+    node.SetTitle(UTF8ToWide("title2"));
+  }
+  EXPECT_TRUE(ResetUnsyncedEntry(syncable::BOOKMARKS, client_tag));
+}
+
+// Verify SetTitle(..) doesn't unnecessarily set IS_UNSYNCED for encrypted
+// bookmarks when we write the same data, but does set it when we write new
+// data.
+TEST_F(SyncManagerTest, SetBookmarkTitleWithEncryption) {
+  std::string client_tag = "title";
+  sync_pb::EntitySpecifics entity_specifics;
+  entity_specifics.MutableExtension(sync_pb::bookmark)->set_url("url");
+  entity_specifics.MutableExtension(sync_pb::bookmark)->set_title("title");
+  MakeServerNode(sync_manager_.GetUserShare(), syncable::BOOKMARKS, client_tag,
+                 BaseNode::GenerateSyncableHash(syncable::BOOKMARKS,
+                                                client_tag),
+                 entity_specifics);
+  // New node shouldn't start off unsynced.
+  EXPECT_FALSE(ResetUnsyncedEntry(syncable::BOOKMARKS, client_tag));
+
+  // Encrypt the datatatype, should set is_unsynced.
+  EXPECT_CALL(observer_,
+              OnEncryptedTypesChanged(
+                  HasModelTypes(syncable::ModelTypeSet::All()), true));
+  EXPECT_CALL(observer_, OnEncryptionComplete());
+  EXPECT_TRUE(SetUpEncryption(WRITE_TO_NIGORI, FULL_ENCRYPTION));
+  sync_manager_.RefreshNigori(base::Bind(&SyncManagerTest::EmptyClosure,
+                                         base::Unretained(this)));
+  scoped_refptr<base::ThreadTestHelper> helper(
+      new base::ThreadTestHelper(
+          BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE)));
+  ASSERT_TRUE(helper->Run());
+  PumpLoop();
+  EXPECT_TRUE(ResetUnsyncedEntry(syncable::BOOKMARKS, client_tag));
+
+  // Manually change to the same title. Should not set is_unsynced.
+  // NON_UNIQUE_NAME should be kEncryptedString.
+  {
+    WriteTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
+    WriteNode node(&trans);
+    EXPECT_TRUE(node.InitByClientTagLookup(syncable::BOOKMARKS, client_tag));
+    node.SetTitle(UTF8ToWide(client_tag));
+    const syncable::Entry* node_entry = node.GetEntry();
+    const sync_pb::EntitySpecifics& specifics = node_entry->Get(SPECIFICS);
+    EXPECT_TRUE(specifics.has_encrypted());
+    EXPECT_EQ(kEncryptedString, node_entry->Get(NON_UNIQUE_NAME));
+  }
+  EXPECT_FALSE(ResetUnsyncedEntry(syncable::BOOKMARKS, client_tag));
+
+  // Manually change to new title. Should set is_unsynced. NON_UNIQUE_NAME
+  // should still be kEncryptedString.
+  {
+    WriteTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
+    WriteNode node(&trans);
+    EXPECT_TRUE(node.InitByClientTagLookup(syncable::BOOKMARKS, client_tag));
+    node.SetTitle(UTF8ToWide("title2"));
+    const syncable::Entry* node_entry = node.GetEntry();
+    const sync_pb::EntitySpecifics& specifics = node_entry->Get(SPECIFICS);
+    EXPECT_TRUE(specifics.has_encrypted());
+    EXPECT_EQ(kEncryptedString, node_entry->Get(NON_UNIQUE_NAME));
+  }
+  EXPECT_TRUE(ResetUnsyncedEntry(syncable::BOOKMARKS, client_tag));
+}
+
+// Verify SetTitle(..) doesn't unnecessarily set IS_UNSYNCED for non-bookmarks
+// when we write the same data, but does set it when we write new data.
+TEST_F(SyncManagerTest, SetNonBookmarkTitle) {
+  std::string client_tag = "title";
+  sync_pb::EntitySpecifics entity_specifics;
+  entity_specifics.MutableExtension(sync_pb::preference)->set_name("name");
+  entity_specifics.MutableExtension(sync_pb::preference)->set_value("value");
+  MakeServerNode(sync_manager_.GetUserShare(),
+                 syncable::PREFERENCES,
+                 client_tag,
+                 BaseNode::GenerateSyncableHash(syncable::PREFERENCES,
+                                                client_tag),
+                 entity_specifics);
+  // New node shouldn't start off unsynced.
+  EXPECT_FALSE(ResetUnsyncedEntry(syncable::PREFERENCES, client_tag));
+
+  // Manually change to the same title. Should not set is_unsynced.
+  {
+    WriteTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
+    WriteNode node(&trans);
+    EXPECT_TRUE(node.InitByClientTagLookup(syncable::PREFERENCES, client_tag));
+    node.SetTitle(UTF8ToWide(client_tag));
+  }
+  EXPECT_FALSE(ResetUnsyncedEntry(syncable::PREFERENCES, client_tag));
+
+  // Manually change to new title. Should set is_unsynced.
+  {
+    WriteTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
+    WriteNode node(&trans);
+    EXPECT_TRUE(node.InitByClientTagLookup(syncable::PREFERENCES, client_tag));
+    node.SetTitle(UTF8ToWide("title2"));
+  }
+  EXPECT_TRUE(ResetUnsyncedEntry(syncable::PREFERENCES, client_tag));
+}
+
+// Verify SetTitle(..) doesn't unnecessarily set IS_UNSYNCED for encrypted
+// non-bookmarks when we write the same data or when we write new data
+// data (should remained kEncryptedString).
+TEST_F(SyncManagerTest, SetNonBookmarkTitleWithEncryption) {
+  std::string client_tag = "title";
+  sync_pb::EntitySpecifics entity_specifics;
+  entity_specifics.MutableExtension(sync_pb::preference)->set_name("name");
+  entity_specifics.MutableExtension(sync_pb::preference)->set_value("value");
+  MakeServerNode(sync_manager_.GetUserShare(),
+                 syncable::PREFERENCES,
+                 client_tag,
+                 BaseNode::GenerateSyncableHash(syncable::PREFERENCES,
+                                                client_tag),
+                 entity_specifics);
+  // New node shouldn't start off unsynced.
+  EXPECT_FALSE(ResetUnsyncedEntry(syncable::PREFERENCES, client_tag));
+
+  // Encrypt the datatatype, should set is_unsynced.
+  EXPECT_CALL(observer_,
+              OnEncryptedTypesChanged(
+                  HasModelTypes(syncable::ModelTypeSet::All()), true));
+  EXPECT_CALL(observer_, OnEncryptionComplete());
+  EXPECT_TRUE(SetUpEncryption(WRITE_TO_NIGORI, FULL_ENCRYPTION));
+  sync_manager_.RefreshNigori(base::Bind(&SyncManagerTest::EmptyClosure,
+                                         base::Unretained(this)));
+  scoped_refptr<base::ThreadTestHelper> helper(
+      new base::ThreadTestHelper(
+          BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE)));
+  ASSERT_TRUE(helper->Run());
+  PumpLoop();
+  EXPECT_TRUE(ResetUnsyncedEntry(syncable::PREFERENCES, client_tag));
+
+  // Manually change to the same title. Should not set is_unsynced.
+  // NON_UNIQUE_NAME should be kEncryptedString.
+  {
+    WriteTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
+    WriteNode node(&trans);
+    EXPECT_TRUE(node.InitByClientTagLookup(syncable::PREFERENCES, client_tag));
+    node.SetTitle(UTF8ToWide(client_tag));
+    const syncable::Entry* node_entry = node.GetEntry();
+    const sync_pb::EntitySpecifics& specifics = node_entry->Get(SPECIFICS);
+    EXPECT_TRUE(specifics.has_encrypted());
+    EXPECT_EQ(kEncryptedString, node_entry->Get(NON_UNIQUE_NAME));
+  }
+  EXPECT_FALSE(ResetUnsyncedEntry(syncable::PREFERENCES, client_tag));
+
+  // Manually change to new title. Should not set is_unsynced because the
+  // NON_UNIQUE_NAME should still be kEncryptedString.
+  {
+    WriteTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
+    WriteNode node(&trans);
+    EXPECT_TRUE(node.InitByClientTagLookup(syncable::PREFERENCES, client_tag));
+    node.SetTitle(UTF8ToWide("title2"));
+    const syncable::Entry* node_entry = node.GetEntry();
+    const sync_pb::EntitySpecifics& specifics = node_entry->Get(SPECIFICS);
+    EXPECT_TRUE(specifics.has_encrypted());
+    EXPECT_EQ(kEncryptedString, node_entry->Get(NON_UNIQUE_NAME));
+    EXPECT_FALSE(node_entry->Get(IS_UNSYNCED));
+  }
+}
+
 }  // namespace browser_sync
diff --git a/chrome/browser/sync/internal_api/write_node.cc b/chrome/browser/sync/internal_api/write_node.cc
index 58bf04a..812c04d 100644
--- a/chrome/browser/sync/internal_api/write_node.cc
+++ b/chrome/browser/sync/internal_api/write_node.cc
@@ -106,6 +106,9 @@ bool WriteNode::UpdateEntryWithEncryption(
     }
   }
   entry->Put(syncable::SPECIFICS, generated_specifics);
+  DVLOG(1) << "Overwriting specifics of type "
+           << syncable::ModelTypeToString(type)
+           << " and marking for syncing.";
   syncable::MarkForSyncing(entry);
   return true;
 }
@@ -119,31 +122,65 @@ void WriteNode::SetIsFolder(bool folder) {
 }
 
 void WriteNode::SetTitle(const std::wstring& title) {
-  sync_pb::EntitySpecifics specifics = GetEntitySpecifics();
-  std::string server_legal_name;
-  SyncAPINameToServerName(WideToUTF8(title), &server_legal_name);
+  DCHECK_NE(GetModelType(), syncable::UNSPECIFIED);
+  syncable::ModelType type = GetModelType();
+  Cryptographer* cryptographer = GetTransaction()->GetCryptographer();
+  bool needs_encryption = cryptographer->GetEncryptedTypes().Has(type);
+
+  // If this datatype is encrypted and is not a bookmark, we disregard the
+  // specified title in favor of kEncryptedString. For encrypted bookmarks the
+  // NON_UNIQUE_NAME will still be kEncryptedString, but we store the real title
+  // into the specifics. All strings compared are server legal strings.
+  std::string new_legal_title;
+  if (type != syncable::BOOKMARKS && needs_encryption) {
+    new_legal_title = kEncryptedString;
+  } else {
+    SyncAPINameToServerName(WideToUTF8(title), &new_legal_title);
+  }
 
-  string old_name = entry_->Get(syncable::NON_UNIQUE_NAME);
+  std::string current_legal_title;
+  if (syncable::BOOKMARKS == type &&
+      entry_->Get(syncable::SPECIFICS).has_encrypted()) {
+    // Encrypted bookmarks only have their title in the unencrypted specifics.
+    current_legal_title = GetBookmarkSpecifics().title();
+  } else {
+    // Non-bookmarks and legacy bookmarks (those with no title in their
+    // specifics) store their title in NON_UNIQUE_NAME. Non-legacy bookmarks
+    // store their title in specifics as well as NON_UNIQUE_NAME.
+    current_legal_title = entry_->Get(syncable::NON_UNIQUE_NAME);
+  }
 
-  if (server_legal_name == old_name)
-    return;  // Skip redundant changes.
+  bool title_matches = (current_legal_title == new_legal_title);
+  bool encrypted_without_overwriting_name = (needs_encryption &&
+      entry_->Get(syncable::NON_UNIQUE_NAME) != kEncryptedString);
 
-  // Only set NON_UNIQUE_NAME to the title if we're not encrypted.
-  Cryptographer* cryptographer = GetTransaction()->GetCryptographer();
-  if (cryptographer->GetEncryptedTypes().Has(GetModelType())) {
-    if (old_name != kEncryptedString)
-      entry_->Put(syncable::NON_UNIQUE_NAME, kEncryptedString);
-  } else {
-    entry_->Put(syncable::NON_UNIQUE_NAME, server_legal_name);
+  // If the title matches and the NON_UNIQUE_NAME is properly overwritten as
+  // necessary, nothing needs to change.
+  if (title_matches && !encrypted_without_overwriting_name) {
+    DVLOG(2) << "Title matches, dropping change.";
+    return;
   }
 
   // For bookmarks, we also set the title field in the specifics.
   // TODO(zea): refactor bookmarks to not need this functionality.
   if (GetModelType() == syncable::BOOKMARKS) {
-    specifics.MutableExtension(sync_pb::bookmark)->set_title(server_legal_name);
+    sync_pb::EntitySpecifics specifics = GetEntitySpecifics();
+    specifics.MutableExtension(sync_pb::bookmark)->set_title(new_legal_title);
     SetEntitySpecifics(specifics);  // Does it's own encryption checking.
   }
 
+  // For bookmarks, this has to happen after we set the title in the specifics,
+  // because the presence of a title in the NON_UNIQUE_NAME is what controls
+  // the logic deciding whether this is an empty node or a legacy bookmark.
+  // See BaseNode::GetUnencryptedSpecific(..).
+  if (needs_encryption)
+    entry_->Put(syncable::NON_UNIQUE_NAME, kEncryptedString);
+  else
+    entry_->Put(syncable::NON_UNIQUE_NAME, new_legal_title);
+
+  DVLOG(1) << "Overwriting title of type "
+           << syncable::ModelTypeToString(type)
+           << " and marking for syncing.";
   MarkForSyncing();
 }