Sync: set BASE_VERSION to entry.version() instead of 0 when changing

IDs, but also force IS_UNAPPLIED.

This allows BV to be valid in case the item is marked UNSYNCED and later commits,
but still allows the update application to proceed if it's not.

BUG=49715
TEST=See bug

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

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

3 files changed, 35 insertions, 26 deletions

diff --git a/chrome/browser/sync/engine/build_and_process_conflict_sets_command.cc b/chrome/browser/sync/engine/build_and_process_conflict_sets_command.cc
index ca27be0..2fd6b30 100644
--- a/chrome/browser/sync/engine/build_and_process_conflict_sets_command.cc
+++ b/chrome/browser/sync/engine/build_and_process_conflict_sets_command.cc
@@ -227,12 +227,12 @@ void BuildAndProcessConflictSetsCommand::BuildConflictSets(
   set<syncable::Id>::iterator i = conflict_progress->ConflictingItemsBegin();
   while (i != conflict_progress->ConflictingItemsEnd()) {
     syncable::Entry entry(trans, syncable::GET_BY_ID, *i);
-    CHECK(entry.good());
-    if (!entry.Get(syncable::IS_UNSYNCED) &&
-        !entry.Get(syncable::IS_UNAPPLIED_UPDATE)) {
+    if (!entry.good() ||
+        (!entry.Get(syncable::IS_UNSYNCED) &&
+         !entry.Get(syncable::IS_UNAPPLIED_UPDATE))) {
       // This can happen very rarely. It means we had a simply conflicting item
-      // that randomly committed. We drop the entry as it's no longer
-      // conflicting.
+      // that randomly committed; its ID could have changed during the commit.
+      // We drop the entry as it's no longer conflicting.
       conflict_progress->EraseConflictingItemById(*(i++));
       continue;
     }
diff --git a/chrome/browser/sync/engine/process_updates_command.cc b/chrome/browser/sync/engine/process_updates_command.cc
index 6e42b1b..d55582e 100644
--- a/chrome/browser/sync/engine/process_updates_command.cc
+++ b/chrome/browser/sync/engine/process_updates_command.cc
@@ -124,29 +124,38 @@ ServerUpdateProcessingResult ProcessUpdatesCommand::ProcessUpdate(
   // If we're repurposing an existing local entry with a new server ID,
   // change the ID now, after we're sure that the update can succeed.
   if (local_id != server_id) {
+    DCHECK(!update.deleted());
     SyncerUtil::ChangeEntryIDAndUpdateChildren(&trans, &target_entry,
         server_id);
     // When IDs change, versions become irrelevant.  Forcing BASE_VERSION
-    // to zero would ensure that this update gets applied, but historically,
-    // that's an illegal state unless the item is using the client tag.
-    // Alternatively, we can force BASE_VERSION to entry.version(), but
-    // this has the effect of suppressing update application.
-    // TODO(nick): Make the treatment of these two cases consistent.
-    int64 new_version = target_entry.Get(UNIQUE_CLIENT_TAG).empty() ?
-        update.version() : 0;
-    target_entry.Put(BASE_VERSION, new_version);
+    // to zero would ensure that this update gets applied, but would indicate
+    // creation or undeletion if it were committed that way.  Instead, prefer
+    // forcing BASE_VERSION to entry.version() while also forcing
+    // IS_UNAPPLIED_UPDATE to true.  If the item is UNSYNCED, it's committable
+    // from the new state; it may commit before the conflict resolver gets
+    // a crack at it.
+    if (target_entry.Get(IS_UNSYNCED) || target_entry.Get(BASE_VERSION) > 0) {
+      // If either of these conditions are met, then we can expect valid client
+      // fields for this entry.  When BASE_VERSION is positive, consistency is
+      // enforced on the client fields at update-application time.  Otherwise,
+      // we leave the BASE_VERSION field alone; it'll get updated the first time
+      // we successfully apply this update.
+      target_entry.Put(BASE_VERSION, update.version());
+    }
+    // Force application of this update, no matter what.
+    target_entry.Put(IS_UNAPPLIED_UPDATE, true);
   }
 
   SyncerUtil::UpdateServerFieldsFromUpdate(&target_entry, update, name);
 
   if (target_entry.Get(SERVER_VERSION) == target_entry.Get(BASE_VERSION) &&
-      !target_entry.Get(IS_UNSYNCED)) {
-      // It's largely OK if data doesn't match exactly since a future update
-      // will just clobber the data. Conflict resolution will overwrite and
-      // take one side as the winner and does not try to merge, so strict
-      // equality isn't necessary.
-      LOG_IF(ERROR, !SyncerUtil::ServerAndLocalEntriesMatch(&target_entry))
-          << target_entry;
+      !target_entry.Get(IS_UNSYNCED) &&
+      !target_entry.Get(IS_UNAPPLIED_UPDATE)) {
+    // If these don't match, it means that we have a different view of the
+    // truth from other clients.  That's a sync bug, though we may be able
+    // to recover the next time this item commits.
+    LOG_IF(ERROR, !SyncerUtil::ServerAndLocalEntriesMatch(&target_entry))
+        << target_entry;
   }
   return SUCCESS_PROCESSED;
 }
diff --git a/chrome/browser/sync/engine/syncer_unittest.cc b/chrome/browser/sync/engine/syncer_unittest.cc
index 70f25e0c..ae6e597 100644
--- a/chrome/browser/sync/engine/syncer_unittest.cc
+++ b/chrome/browser/sync/engine/syncer_unittest.cc
@@ -4363,8 +4363,8 @@ TEST_F(SyncerTest, ClientTagClashWithinBatchOfUpdates) {
 
     Entry tag_a(&trans, GET_BY_CLIENT_TAG, "tag a");
     ASSERT_TRUE(tag_a.good());
-    ASSERT_TRUE(tag_a.Get(ID).ServerKnows());
-    ASSERT_TRUE(ids_.FromNumber(1) == tag_a.Get(ID));
+    EXPECT_TRUE(tag_a.Get(ID).ServerKnows());
+    EXPECT_EQ(ids_.FromNumber(1), tag_a.Get(ID));
     EXPECT_FALSE(tag_a.Get(IS_DEL));
     EXPECT_FALSE(tag_a.Get(IS_UNAPPLIED_UPDATE));
     EXPECT_FALSE(tag_a.Get(IS_UNSYNCED));
@@ -4374,8 +4374,8 @@ TEST_F(SyncerTest, ClientTagClashWithinBatchOfUpdates) {
 
     Entry tag_b(&trans, GET_BY_CLIENT_TAG, "tag b");
     ASSERT_TRUE(tag_b.good());
-    ASSERT_TRUE(tag_b.Get(ID).ServerKnows());
-    ASSERT_TRUE(ids_.FromNumber(101) == tag_b.Get(ID));
+    EXPECT_TRUE(tag_b.Get(ID).ServerKnows());
+    EXPECT_EQ(ids_.FromNumber(101), tag_b.Get(ID));
     EXPECT_FALSE(tag_b.Get(IS_DEL));
     EXPECT_FALSE(tag_b.Get(IS_UNAPPLIED_UPDATE));
     EXPECT_FALSE(tag_b.Get(IS_UNSYNCED));
@@ -4385,8 +4385,8 @@ TEST_F(SyncerTest, ClientTagClashWithinBatchOfUpdates) {
 
     Entry tag_c(&trans, GET_BY_CLIENT_TAG, "tag c");
     ASSERT_TRUE(tag_c.good());
-    ASSERT_TRUE(tag_c.Get(ID).ServerKnows());
-    ASSERT_TRUE(ids_.FromNumber(201) == tag_c.Get(ID));
+    EXPECT_TRUE(tag_c.Get(ID).ServerKnows());
+    EXPECT_EQ(ids_.FromNumber(201), tag_c.Get(ID));
     EXPECT_FALSE(tag_c.Get(IS_DEL));
     EXPECT_FALSE(tag_c.Get(IS_UNAPPLIED_UPDATE));
     EXPECT_FALSE(tag_c.Get(IS_UNSYNCED));