diff options
Diffstat (limited to 'chrome/browser/sync/engine')
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)); |