[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}

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);