diff options
author | zea <zea@chromium.org> | 2014-11-17 12:03:42 -0800 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2014-11-17 20:04:07 +0000 |
commit | 87192fcf800c67404457a4bb20110c8f43724a38 (patch) | |
tree | df03d333b3e7603b84ffcecf719012930f5282f1 /sync | |
parent | 3a13d33628135098a284b37640958de9abefd1ac (diff) | |
download | chromium_src-87192fcf800c67404457a4bb20110c8f43724a38.zip chromium_src-87192fcf800c67404457a4bb20110c8f43724a38.tar.gz chromium_src-87192fcf800c67404457a4bb20110c8f43724a38.tar.bz2 |
[Sync] Fix deletion while uncommitted logic
If a deletion happens while a commit is in flight, we should make sure that the
deletion makes it to the server. Previously we were over-aggressively assuming
that the entity was unknown by the server and therefore the deletion was
unnecessary.
BUG=426865
Review URL: https://codereview.chromium.org/729523003
Cr-Commit-Position: refs/heads/master@{#304462}
Diffstat (limited to 'sync')
-rw-r--r-- | sync/engine/syncer_unittest.cc | 267 | ||||
-rw-r--r-- | sync/syncable/directory.cc | 13 | ||||
-rw-r--r-- | sync/syncable/mutable_entry.cc | 4 | ||||
-rw-r--r-- | sync/test/engine/mock_connection_manager.cc | 12 | ||||
-rw-r--r-- | sync/test/engine/mock_connection_manager.h | 2 |
5 files changed, 251 insertions, 47 deletions
diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc index 46fa871..8d97d33 100644 --- a/sync/engine/syncer_unittest.cc +++ b/sync/engine/syncer_unittest.cc @@ -4448,6 +4448,217 @@ TEST_F(SyncerTest, GetKeyEmpty) { } } +// Tests specifically related to bookmark (and therefore no client tags) sync +// logic. Entities without client tags have custom logic in parts of the code, +// and hence are not covered by e.g. the Undeletion tests below. +class SyncerBookmarksTest : public SyncerTest { + public: + SyncerBookmarksTest() : metahandle_(syncable::kInvalidMetaHandle) { + } + + void Create() { + WriteTransaction trans(FROM_HERE, UNITTEST, directory()); + MutableEntry bookmark( + &trans, CREATE, BOOKMARKS, ids_.root(), "clientname"); + ASSERT_TRUE(bookmark.good()); + bookmark.PutIsUnsynced(true); + bookmark.PutSyncing(false); + bookmark.PutSpecifics(DefaultBookmarkSpecifics()); + EXPECT_FALSE(bookmark.GetIsUnappliedUpdate()); + EXPECT_FALSE(bookmark.GetId().ServerKnows()); + metahandle_ = bookmark.GetMetahandle(); + local_id_ = bookmark.GetId(); + } + + void Delete() { + WriteTransaction trans(FROM_HERE, UNITTEST, directory()); + MutableEntry entry(&trans, GET_BY_HANDLE, metahandle_); + ASSERT_TRUE(entry.good()); + EXPECT_EQ(metahandle_, entry.GetMetahandle()); + // The order of setting IS_UNSYNCED vs IS_DEL matters. See + // WriteNode::Tombstone(). + entry.PutIsUnsynced(true); + entry.PutIsDel(true); + entry.PutSyncing(false); + } + + void Undelete() { + WriteTransaction trans(FROM_HERE, UNITTEST, directory()); + MutableEntry entry(&trans, GET_BY_HANDLE, metahandle_); + ASSERT_TRUE(entry.good()); + EXPECT_EQ(metahandle_, entry.GetMetahandle()); + EXPECT_TRUE(entry.GetIsDel()); + entry.PutIsDel(false); + entry.PutIsUnsynced(true); + entry.PutSyncing(false); + } + + int64 GetMetahandleOfTag() { + syncable::ReadTransaction trans(FROM_HERE, directory()); + Entry entry(&trans, GET_BY_HANDLE, metahandle_); + EXPECT_TRUE(entry.good()); + if (!entry.good()) { + return syncable::kInvalidMetaHandle; + } + return entry.GetMetahandle(); + } + + Id GetServerId() { + syncable::ReadTransaction trans(FROM_HERE, directory()); + Entry entry(&trans, GET_BY_HANDLE, metahandle_); + EXPECT_TRUE(entry.good()); + if (!entry.good()) { + return Id(); + } + return entry.GetId(); + } + + void ExpectUnsyncedCreation() { + syncable::ReadTransaction trans(FROM_HERE, directory()); + Entry entry(&trans, GET_BY_HANDLE, metahandle_); + + EXPECT_EQ(metahandle_, entry.GetMetahandle()); + EXPECT_FALSE(entry.GetIsDel()); + EXPECT_FALSE(entry.GetServerIsDel()); // Never been committed. + EXPECT_LT(entry.GetBaseVersion(), 0); + EXPECT_TRUE(entry.GetIsUnsynced()); + EXPECT_FALSE(entry.GetIsUnappliedUpdate()); + } + + void ExpectUnsyncedUndeletion() { + syncable::ReadTransaction trans(FROM_HERE, directory()); + Entry entry(&trans, GET_BY_HANDLE, metahandle_); + + EXPECT_EQ(metahandle_, entry.GetMetahandle()); + EXPECT_FALSE(entry.GetIsDel()); + EXPECT_TRUE(entry.GetServerIsDel()); + EXPECT_GE(entry.GetBaseVersion(), 0); + EXPECT_TRUE(entry.GetIsUnsynced()); + EXPECT_FALSE(entry.GetIsUnappliedUpdate()); + EXPECT_TRUE(entry.GetId().ServerKnows()); + } + + void ExpectUnsyncedEdit() { + syncable::ReadTransaction trans(FROM_HERE, directory()); + Entry entry(&trans, GET_BY_HANDLE, metahandle_); + + EXPECT_EQ(metahandle_, entry.GetMetahandle()); + EXPECT_FALSE(entry.GetIsDel()); + EXPECT_FALSE(entry.GetServerIsDel()); + EXPECT_GE(entry.GetBaseVersion(), 0); + EXPECT_TRUE(entry.GetIsUnsynced()); + EXPECT_FALSE(entry.GetIsUnappliedUpdate()); + EXPECT_TRUE(entry.GetId().ServerKnows()); + } + + void ExpectUnsyncedDeletion() { + syncable::ReadTransaction trans(FROM_HERE, directory()); + Entry entry(&trans, GET_BY_HANDLE, metahandle_); + + EXPECT_EQ(metahandle_, entry.GetMetahandle()); + EXPECT_TRUE(entry.GetIsDel()); + EXPECT_FALSE(entry.GetServerIsDel()); + EXPECT_TRUE(entry.GetIsUnsynced()); + EXPECT_FALSE(entry.GetIsUnappliedUpdate()); + EXPECT_GE(entry.GetBaseVersion(), 0); + EXPECT_GE(entry.GetServerVersion(), 0); + } + + void ExpectSyncedAndCreated() { + syncable::ReadTransaction trans(FROM_HERE, directory()); + Entry entry(&trans, GET_BY_HANDLE, metahandle_); + + EXPECT_EQ(metahandle_, entry.GetMetahandle()); + EXPECT_FALSE(entry.GetIsDel()); + EXPECT_FALSE(entry.GetServerIsDel()); + EXPECT_GE(entry.GetBaseVersion(), 0); + EXPECT_EQ(entry.GetBaseVersion(), entry.GetServerVersion()); + EXPECT_FALSE(entry.GetIsUnsynced()); + EXPECT_FALSE(entry.GetIsUnappliedUpdate()); + } + + void ExpectSyncedAndDeleted() { + syncable::ReadTransaction trans(FROM_HERE, directory()); + Entry entry(&trans, GET_BY_HANDLE, metahandle_); + + EXPECT_EQ(metahandle_, entry.GetMetahandle()); + EXPECT_TRUE(entry.GetIsDel()); + EXPECT_TRUE(entry.GetServerIsDel()); + EXPECT_FALSE(entry.GetIsUnsynced()); + EXPECT_FALSE(entry.GetIsUnappliedUpdate()); + EXPECT_GE(entry.GetBaseVersion(), 0); + EXPECT_GE(entry.GetServerVersion(), 0); + } + + protected: + syncable::Id local_id_; + int64 metahandle_; +}; + +TEST_F(SyncerBookmarksTest, CreateSyncThenDeleteSync) { + Create(); + ExpectUnsyncedCreation(); + SyncShareNudge(); + ExpectSyncedAndCreated(); + Delete(); + ExpectUnsyncedDeletion(); + SyncShareNudge(); + ExpectSyncedAndDeleted(); +} + +TEST_F(SyncerBookmarksTest, CreateThenDeleteBeforeSync) { + Create(); + ExpectUnsyncedCreation(); + Delete(); + + // Deleting before the initial commit should result in not needing to send + // the delete to the server. It will still be in an unsynced state, but with + // IS_UNSYNCED set to false. + { + syncable::ReadTransaction trans(FROM_HERE, directory()); + Entry entry(&trans, GET_BY_HANDLE, metahandle_); + + EXPECT_EQ(metahandle_, entry.GetMetahandle()); + EXPECT_TRUE(entry.GetIsDel()); + EXPECT_FALSE(entry.GetServerIsDel()); + EXPECT_FALSE(entry.GetIsUnsynced()); + EXPECT_FALSE(entry.GetIsUnappliedUpdate()); + EXPECT_EQ(entry.GetBaseVersion(), -1); + EXPECT_EQ(entry.GetServerVersion(), 0); + } +} + +TEST_F(SyncerBookmarksTest, LocalDeleteRemoteChangeConflict) { + Create(); + ExpectUnsyncedCreation(); + SyncShareNudge(); + ExpectSyncedAndCreated(); + Delete(); + ExpectUnsyncedDeletion(); + + // Trigger a getupdates that modifies the bookmark. The update should be + // clobbered by the local delete. + mock_server_->AddUpdateBookmark(GetServerId(), Id(), "dummy", 10, 10, + local_cache_guid(), local_id_.GetServerId()); + + SyncShareNudge(); + ExpectSyncedAndDeleted(); +} + +TEST_F(SyncerBookmarksTest, CreateThenDeleteDuringCommit) { + Create(); + ExpectUnsyncedCreation(); + + // In the middle of the initial creation commit, perform a deletion. + // This should trigger performing two consecutive commit cycles, resulting + // in the bookmark being both deleted and synced. + mock_server_->SetMidCommitCallback( + base::Bind(&SyncerBookmarksTest::Delete, base::Unretained(this))); + + SyncShareNudge(); + ExpectSyncedAndDeleted(); +} + // Test what happens if a client deletes, then recreates, an object very // quickly. It is possible that the deletion gets sent as a commit, and // the undelete happens during the commit request. The principle here @@ -4477,12 +4688,12 @@ class SyncerUndeletionTest : public SyncerTest { void Create() { WriteTransaction trans(FROM_HERE, UNITTEST, directory()); MutableEntry perm_folder( - &trans, CREATE, BOOKMARKS, ids_.root(), "clientname"); + &trans, CREATE, PREFERENCES, ids_.root(), "clientname"); ASSERT_TRUE(perm_folder.good()); perm_folder.PutUniqueClientTag(client_tag_); perm_folder.PutIsUnsynced(true); perm_folder.PutSyncing(false); - perm_folder.PutSpecifics(DefaultBookmarkSpecifics()); + perm_folder.PutSpecifics(DefaultPreferencesSpecifics()); EXPECT_FALSE(perm_folder.GetIsUnappliedUpdate()); EXPECT_FALSE(perm_folder.GetId().ServerKnows()); metahandle_ = perm_folder.GetMetahandle(); @@ -4494,8 +4705,10 @@ class SyncerUndeletionTest : public SyncerTest { MutableEntry entry(&trans, GET_BY_CLIENT_TAG, client_tag_); ASSERT_TRUE(entry.good()); EXPECT_EQ(metahandle_, entry.GetMetahandle()); - entry.PutIsDel(true); + // The order of setting IS_UNSYNCED vs IS_DEL matters. See + // WriteNode::Tombstone(). entry.PutIsUnsynced(true); + entry.PutIsDel(true); entry.PutSyncing(false); } @@ -4527,7 +4740,7 @@ class SyncerUndeletionTest : public SyncerTest { EXPECT_EQ(metahandle_, entry.GetMetahandle()); EXPECT_FALSE(entry.GetIsDel()); EXPECT_FALSE(entry.GetServerIsDel()); // Never been committed. - EXPECT_GE(0, entry.GetBaseVersion()); + EXPECT_LT(entry.GetBaseVersion(), 0); EXPECT_TRUE(entry.GetIsUnsynced()); EXPECT_FALSE(entry.GetIsUnappliedUpdate()); } @@ -4539,7 +4752,7 @@ class SyncerUndeletionTest : public SyncerTest { EXPECT_EQ(metahandle_, entry.GetMetahandle()); EXPECT_FALSE(entry.GetIsDel()); EXPECT_TRUE(entry.GetServerIsDel()); - EXPECT_EQ(0, entry.GetBaseVersion()); + EXPECT_GE(entry.GetBaseVersion(), 0); EXPECT_TRUE(entry.GetIsUnsynced()); EXPECT_FALSE(entry.GetIsUnappliedUpdate()); EXPECT_TRUE(entry.GetId().ServerKnows()); @@ -4552,7 +4765,7 @@ class SyncerUndeletionTest : public SyncerTest { EXPECT_EQ(metahandle_, entry.GetMetahandle()); EXPECT_FALSE(entry.GetIsDel()); EXPECT_FALSE(entry.GetServerIsDel()); - EXPECT_LT(0, entry.GetBaseVersion()); + EXPECT_GE(entry.GetBaseVersion(), 0); EXPECT_TRUE(entry.GetIsUnsynced()); EXPECT_FALSE(entry.GetIsUnappliedUpdate()); EXPECT_TRUE(entry.GetId().ServerKnows()); @@ -4567,8 +4780,8 @@ class SyncerUndeletionTest : public SyncerTest { EXPECT_FALSE(entry.GetServerIsDel()); EXPECT_TRUE(entry.GetIsUnsynced()); EXPECT_FALSE(entry.GetIsUnappliedUpdate()); - EXPECT_LT(0, entry.GetBaseVersion()); - EXPECT_LT(0, entry.GetServerVersion()); + EXPECT_GE(entry.GetBaseVersion(), 0); + EXPECT_GE(entry.GetServerVersion(), 0); } void ExpectSyncedAndCreated() { @@ -4578,7 +4791,7 @@ class SyncerUndeletionTest : public SyncerTest { EXPECT_EQ(metahandle_, entry.GetMetahandle()); EXPECT_FALSE(entry.GetIsDel()); EXPECT_FALSE(entry.GetServerIsDel()); - EXPECT_LT(0, entry.GetBaseVersion()); + EXPECT_GE(entry.GetBaseVersion(), 0); EXPECT_EQ(entry.GetBaseVersion(), entry.GetServerVersion()); EXPECT_FALSE(entry.GetIsUnsynced()); EXPECT_FALSE(entry.GetIsUnappliedUpdate()); @@ -4593,8 +4806,8 @@ class SyncerUndeletionTest : public SyncerTest { EXPECT_TRUE(entry.GetServerIsDel()); EXPECT_FALSE(entry.GetIsUnsynced()); EXPECT_FALSE(entry.GetIsUnappliedUpdate()); - EXPECT_GE(0, entry.GetBaseVersion()); - EXPECT_GE(0, entry.GetServerVersion()); + EXPECT_GE(entry.GetBaseVersion(), 0); + EXPECT_GE(entry.GetServerVersion(), 0); } protected: @@ -4793,7 +5006,7 @@ TEST_F(SyncerUndeletionTest, UndeleteAfterOtherClientDeletes) { { syncable::ReadTransaction trans(FROM_HERE, directory()); Entry entry(&trans, GET_BY_HANDLE, metahandle_); - mock_server_->AddUpdateTombstone(entry.GetId()); + mock_server_->AddUpdateTombstone(entry.GetId(), PREFERENCES); } SyncShareNudge(); @@ -4835,7 +5048,7 @@ TEST_F(SyncerUndeletionTest, UndeleteAfterOtherClientDeletesImmediately) { { syncable::ReadTransaction trans(FROM_HERE, directory()); Entry entry(&trans, GET_BY_HANDLE, metahandle_); - mock_server_->AddUpdateTombstone(entry.GetId()); + mock_server_->AddUpdateTombstone(entry.GetId(), PREFERENCES); } SyncShareNudge(); @@ -4903,22 +5116,16 @@ TEST_F(SyncerUndeletionTest, OtherClientUndeletes) { { syncable::ReadTransaction trans(FROM_HERE, directory()); Entry entry(&trans, GET_BY_HANDLE, metahandle_); - mock_server_->AddUpdateBookmark( - entry.GetId(), - entry.GetParentId(), - "Thadeusz", 100, 1000, - local_cache_guid(), local_id_.GetServerId()); + mock_server_->AddUpdatePref( + entry.GetId().GetServerId(), + entry.GetParentId().GetServerId(), + client_tag_, 100, 1000); } mock_server_->SetLastUpdateClientTag(client_tag_); SyncShareNudge(); EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems()); EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests()); ExpectSyncedAndCreated(); - { - syncable::ReadTransaction trans(FROM_HERE, directory()); - Entry entry(&trans, GET_BY_HANDLE, metahandle_); - EXPECT_EQ("Thadeusz", entry.GetNonUniqueName()); - } } TEST_F(SyncerUndeletionTest, OtherClientUndeletesImmediately) { @@ -4958,22 +5165,16 @@ TEST_F(SyncerUndeletionTest, OtherClientUndeletesImmediately) { { syncable::ReadTransaction trans(FROM_HERE, directory()); Entry entry(&trans, GET_BY_HANDLE, metahandle_); - mock_server_->AddUpdateBookmark( - entry.GetId(), - entry.GetParentId(), - "Thadeusz", 100, 1000, - local_cache_guid(), local_id_.GetServerId()); + mock_server_->AddUpdatePref( + entry.GetId().GetServerId(), + entry.GetParentId().GetServerId(), + client_tag_, 100, 1000); } mock_server_->SetLastUpdateClientTag(client_tag_); SyncShareNudge(); EXPECT_EQ(0, session_->status_controller().TotalNumConflictingItems()); EXPECT_EQ(1, mock_server_->GetAndClearNumGetUpdatesRequests()); ExpectSyncedAndCreated(); - { - syncable::ReadTransaction trans(FROM_HERE, directory()); - Entry entry(&trans, GET_BY_HANDLE, metahandle_); - EXPECT_EQ("Thadeusz", entry.GetNonUniqueName()); - } } enum { diff --git a/sync/syncable/directory.cc b/sync/syncable/directory.cc index 5d11b6f..9c59409 100644 --- a/sync/syncable/directory.cc +++ b/sync/syncable/directory.cc @@ -1280,14 +1280,11 @@ bool Directory::CheckTreeInvariants(syncable::BaseTransaction* trans, trans)) return false; } - // Server-unknown items that are locally deleted should not be sent up to - // the server. They must be !IS_UNSYNCED. - if (!SyncAssert(!(!id.ServerKnows() && e.GetIsDel() && e.GetIsUnsynced()), - FROM_HERE, - "Locally deleted item must not be unsynced.", - trans)) { - return false; - } + + // Previously we would assert that locally deleted items that have never + // been synced must not be sent to the server (IS_UNSYNCED must be false). + // This is not always true in the case that an item is deleted while the + // initial commit is in flight. See crbug.com/426865. } return true; } diff --git a/sync/syncable/mutable_entry.cc b/sync/syncable/mutable_entry.cc index b1c1ff4..0aa37e7 100644 --- a/sync/syncable/mutable_entry.cc +++ b/sync/syncable/mutable_entry.cc @@ -170,7 +170,9 @@ void MutableEntry::PutIsDel(bool value) { // - Let us delete this entry permanently through // DirectoryBackingStore::DropDeletedEntries() when we next restart sync. // This will save memory and avoid crbug.com/125381. - if (!GetId().ServerKnows()) { + // Note: do not unset IsUnsynced if the syncer is in the middle of + // attempting to commit this entity. + if (!GetId().ServerKnows() && !GetSyncing()) { PutIsUnsynced(false); } } diff --git a/sync/test/engine/mock_connection_manager.cc b/sync/test/engine/mock_connection_manager.cc index ef3022e..4e42f54 100644 --- a/sync/test/engine/mock_connection_manager.cc +++ b/sync/test/engine/mock_connection_manager.cc @@ -394,8 +394,9 @@ sync_pb::SyncEntity* MockConnectionManager::AddUpdateFromLastCommit() { last_commit_response().entryresponse(0).response_type()); if (last_sent_commit().entries(0).deleted()) { + ModelType type = GetModelType(last_sent_commit().entries(0)); AddUpdateTombstone(syncable::Id::CreateFromServerId( - last_sent_commit().entries(0).id_string())); + last_sent_commit().entries(0).id_string()), type); } else { sync_pb::SyncEntity* ent = GetUpdateResponse()->add_entries(); ent->CopyFrom(last_sent_commit().entries(0)); @@ -425,7 +426,9 @@ sync_pb::SyncEntity* MockConnectionManager::AddUpdateFromLastCommit() { return GetMutableLastUpdate(); } -void MockConnectionManager::AddUpdateTombstone(const syncable::Id& id) { +void MockConnectionManager::AddUpdateTombstone( + const syncable::Id& id, + ModelType type) { // Tombstones have only the ID set and dummy values for the required fields. sync_pb::SyncEntity* ent = GetUpdateResponse()->add_entries(); ent->set_id_string(id.GetServerId()); @@ -434,14 +437,15 @@ void MockConnectionManager::AddUpdateTombstone(const syncable::Id& id) { ent->set_deleted(true); // Make sure we can still extract the ModelType from this tombstone. - ent->mutable_specifics()->mutable_bookmark(); + AddDefaultFieldValue(type, ent->mutable_specifics()); } void MockConnectionManager::SetLastUpdateDeleted() { // Tombstones have only the ID set. Wipe anything else. string id_string = GetMutableLastUpdate()->id_string(); + ModelType type = GetModelType(*GetMutableLastUpdate()); GetUpdateResponse()->mutable_entries()->RemoveLast(); - AddUpdateTombstone(syncable::Id::CreateFromServerId(id_string)); + AddUpdateTombstone(syncable::Id::CreateFromServerId(id_string), type); } void MockConnectionManager::SetLastUpdateOriginatorFields( diff --git a/sync/test/engine/mock_connection_manager.h b/sync/test/engine/mock_connection_manager.h index 19e483b..611d949 100644 --- a/sync/test/engine/mock_connection_manager.h +++ b/sync/test/engine/mock_connection_manager.h @@ -145,7 +145,7 @@ class MockConnectionManager : public ServerConnectionManager { // Add a deleted item. Deletion records typically contain no // additional information beyond the deletion, and no specifics. // The server may send the originator fields. - void AddUpdateTombstone(const syncable::Id& id); + void AddUpdateTombstone(const syncable::Id& id, ModelType type); void SetLastUpdateDeleted(); void SetLastUpdateServerTag(const std::string& tag); |