Fix handling of undeletion within the syncer.

chrome/browser/sync/engine/syncer_util.cc

 // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/sync/engine/syncer_util.h"
 
 #include <set>
 #include <string>
 #include <vector>
 
@@ skipping 114 matching lines @@
     syncable::WriteTransaction* trans,
     syncable::MutableEntry* entry,
     const syncable::Id& new_id) {
   syncable::Directory::ChildHandles children;
   ChangeEntryIDAndUpdateChildren(trans, entry, new_id, &children);
 }
 
 // static
 void SyncerUtil::AttemptReuniteClientTag(syncable::WriteTransaction* trans,
                                          const SyncEntity& server_entry) {
+  if (!server_entry.has_client_defined_unique_tag() ||
+      server_entry.client_defined_unique_tag().empty()) {
+    return;
+  }
 
   // Expected entry points of this function:
   // SyncEntity has NOT been applied to SERVER fields.
   // SyncEntity has NOT been applied to LOCAL fields.
   // DB has not yet been modified, no entries created for this update.
 
   // When a server sends down a client tag, the following cases can occur:
   // 1) Client has entry for tag already, ID is server style, matches
   // 2) Client has entry for tag already, ID is server, doesn't match.
   // 3) Client has entry for tag already, ID is local, (never matches)
   // 4) Client has no entry for tag
 
   // Case 1, we don't have to do anything since the update will
   // work just fine. Update will end up in the proper entry, via ID lookup.
   // Case 2 - Should never happen. We'd need to change the
   // ID of the local entry, we refuse. We'll skip this in VERIFY.
-  // Case 3 - We need to replace the local ID with the server ID. Conflict
-  // resolution must occur, but this is prior to update application! This case
-  // should be rare. For now, clobber client changes entirely.
+  // Case 3 - We need to replace the local ID with the server ID so that
+  // this update gets targeted at the correct local entry; we expect conflict
+  // resolution to occur.
   // Case 4 - Perfect. Same as case 1.
 
   syncable::MutableEntry local_entry(trans, syncable::GET_BY_CLIENT_TAG,
                                      server_entry.client_defined_unique_tag());
 
   // The SyncAPI equivalent of this function will return !good if IS_DEL.
   // The syncable version will return good even if IS_DEL.
   // TODO(chron): Unit test the case with IS_DEL and make sure.
   if (local_entry.good()) {
     if (local_entry.Get(ID).ServerKnows()) {
       // In release config, this will just continue and we'll
       // throw VERIFY_FAIL later.
       // This is Case 1 on success, Case 2 if it fails.
       DCHECK(local_entry.Get(ID) == server_entry.id());
     } else {
       // Case 3: We have a local entry with the same client tag.
-      // We can't have two updates with the same client tag though.
-      // One of these has to go. Let's delete the client entry and move it
-      // aside. This will cause a delete + create. The client API user must
-      // handle this correctly. In this situation the client must have created
-      // this entry but not yet committed it for the first time. Usually the
-      // client probably wants the server data for this instead.
-      // Other strategies to handle this are a bit flaky.
-      DCHECK(local_entry.Get(IS_UNAPPLIED_UPDATE) == false);
-      local_entry.Put(IS_UNSYNCED, false);
-      local_entry.Put(IS_DEL, true);
-      // Needs to get out of the index before our update can be put in.
-      local_entry.Put(UNIQUE_CLIENT_TAG, "");
+      // We should change the ID of the local entry to the server entry.
+      // This will result in an server ID with base version == 0, but that's
+      // a legal state for an item with a client tag.  By changing the ID,
+      // server_entry will now be applied to local_entry.
+      ChangeEntryIDAndUpdateChildren(trans, &local_entry, server_entry.id());
+      DCHECK(0 == local_entry.Get(BASE_VERSION) ||
+             CHANGES_VERSION == local_entry.Get(BASE_VERSION));
     }
   }
   // Case 4: Client has no entry for tag, all green.
 }
 
 // static
 void SyncerUtil::AttemptReuniteLostCommitResponses(
     syncable::WriteTransaction* trans,
     const SyncEntity& server_entry,
     const string& client_id) {
   // If a commit succeeds, but the response does not come back fast enough
   // then the syncer might assume that it was never committed.
   // The server will track the client that sent up the original commit and
   // return this in a get updates response. When this matches a local
   // uncommitted item, we must mutate our local item and version to pick up
   // the committed version of the same item whose commit response was lost.
   // There is however still a race condition if the server has not
   // completed the commit by the time the syncer tries to get updates
   // again. To mitigate this, we need to have the server time out in
   // a reasonable span, our commit batches have to be small enough
   // to process within our HTTP response "assumed alive" time.
 
-  // We need to check if we have a  that didn't get its server
+  // We need to check if we have an entry that didn't get its server
   // id updated correctly. The server sends down a client ID
   // and a local (negative) id. If we have a entry by that
   // description, we should update the ID and version to the
   // server side ones to avoid multiple commits to the same name.
   if (server_entry.has_originator_cache_guid() &&
       server_entry.originator_cache_guid() == client_id) {
     syncable::Id server_id = syncable::Id::CreateFromClientString(
         server_entry.originator_client_item_id());
-    CHECK(!server_id.ServerKnows());
+    DCHECK(!server_id.ServerKnows());
     syncable::MutableEntry local_entry(trans, GET_BY_ID, server_id);
 
     // If it exists, then our local client lost a commit response.
     if (local_entry.good() && !local_entry.Get(IS_DEL)) {
       int64 old_version = local_entry.Get(BASE_VERSION);
       int64 new_version = server_entry.version();
-      CHECK(old_version <= 0);
-      CHECK(new_version > 0);
+      DCHECK(old_version <= 0);
+      DCHECK(new_version > 0);
       // Otherwise setting the base version could cause a consistency failure.
       // An entry should never be version 0 and SYNCED.
-      CHECK(local_entry.Get(IS_UNSYNCED));
+      DCHECK(local_entry.Get(IS_UNSYNCED));
 
       // Just a quick sanity check.
-      CHECK(!local_entry.Get(ID).ServerKnows());
+      DCHECK(!local_entry.Get(ID).ServerKnows());
 
       LOG(INFO) << "Reuniting lost commit response IDs" <<
         " server id: " << server_entry.id() << " local id: " <<
         local_entry.Get(ID) << " new version: " << new_version;
 
       local_entry.Put(BASE_VERSION, new_version);
 
       ChangeEntryIDAndUpdateChildren(trans, &local_entry, server_entry.id());
 
       // We need to continue normal processing on this update after we
       // reunited its ID.
     }
     // !local_entry.Good() means we don't have a left behind entry for this
-    // ID. We successfully committed before. In the future we should get rid
-    // of this system and just have client side generated IDs as a whole.
+    // ID in sync database.  This could happen if we crashed after successfully
+    // committing an item that never was flushed to disk.
   }
 }
 
 // static
 UpdateAttemptResponse SyncerUtil::AttemptToUpdateEntry(
     syncable::WriteTransaction* const trans,
     syncable::MutableEntry* const entry,
     ConflictResolver* resolver,
     Cryptographer* cryptographer) {
 
@@ skipping 92 matching lines @@
 
 }  // namespace
 
 // Pass in name and checksum because of UTF8 conversion.
 // static
 void SyncerUtil::UpdateServerFieldsFromUpdate(
     MutableEntry* local_entry,
     const SyncEntity& server_entry,
     const string& name) {
   if (server_entry.deleted()) {
+    if (local_entry->Get(SERVER_IS_DEL)) {
+      // If we already think the item is server-deleted, we're done.
+      // Skipping these cases prevents our committed deletions from coming
+      // back and overriding subsequent undeletions.  For non-deleted items,
+      // the version number check has a similar effect.
+      return;
+    }
     // The server returns very lightweight replies for deletions, so we don't
     // clobber a bunch of fields on delete.
     local_entry->Put(SERVER_IS_DEL, true);
-    local_entry->Put(SERVER_VERSION,
-        std::max(local_entry->Get(SERVER_VERSION),
-          local_entry->Get(BASE_VERSION)) + 1L);
+    if (!local_entry->Get(UNIQUE_CLIENT_TAG).empty()) {
+      // Items identified by the client unique tag are undeletable; when
+      // they're deleted, they go back to version 0.
+      local_entry->Put(SERVER_VERSION, 0);
+    } else {
+      // Otherwise, fake a server version by bumping the local number.
+      local_entry->Put(SERVER_VERSION,
+          std::max(local_entry->Get(SERVER_VERSION),
+                   local_entry->Get(BASE_VERSION)) + 1);
+    }
     local_entry->Put(IS_UNAPPLIED_UPDATE, true);
     return;
   }
 
-  CHECK(local_entry->Get(ID) == server_entry.id())
+  DCHECK(local_entry->Get(ID) == server_entry.id())
       << "ID Changing not supported here";
   local_entry->Put(SERVER_PARENT_ID, server_entry.parent_id());
   local_entry->Put(SERVER_NON_UNIQUE_NAME, name);
   local_entry->Put(SERVER_VERSION, server_entry.version());
   local_entry->Put(SERVER_CTIME,
       ServerTimeToClientTime(server_entry.ctime()));
   local_entry->Put(SERVER_MTIME,
       ServerTimeToClientTime(server_entry.mtime()));
   local_entry->Put(SERVER_IS_DIR, server_entry.IsFolder());
   if (server_entry.has_server_defined_unique_tag()) {
@@ skipping 122 matching lines @@
   LOG(INFO) << "Splitting server information, local entry: " << *entry <<
     " server entry: " << new_entry;
 }
 
 // This function is called on an entry when we can update the user-facing data
 // from the server data.
 // static
 void SyncerUtil::UpdateLocalDataFromServerData(
     syncable::WriteTransaction* trans,
     syncable::MutableEntry* entry) {
-  CHECK(!entry->Get(IS_UNSYNCED));
-  CHECK(entry->Get(IS_UNAPPLIED_UPDATE));
+  DCHECK(!entry->Get(IS_UNSYNCED));
+  DCHECK(entry->Get(IS_UNAPPLIED_UPDATE));
 
   LOG(INFO) << "Updating entry : " << *entry;
   // Start by setting the properties that determine the model_type.
   entry->Put(SPECIFICS, entry->Get(SERVER_SPECIFICS));
   entry->Put(IS_DIR, entry->Get(SERVER_IS_DIR));
   // This strange dance around the IS_DEL flag avoids problems when setting
   // the name.
   // TODO(chron): Is this still an issue? Unit test this codepath.
   if (entry->Get(SERVER_IS_DEL)) {
     entry->Put(IS_DEL, true);
@@ skipping 196 matching lines @@
         return VERIFY_FAIL;
       }
     }
 
     if (!deleted &&
         (same_id->Get(SERVER_IS_DEL) ||
          (!same_id->Get(IS_UNSYNCED) && same_id->Get(IS_DEL) &&
           same_id->Get(BASE_VERSION) > 0))) {
       // An undelete. The latter case in the above condition is for
       // when the server does not give us an update following the
-      // commit of a delete, before undeleting.  Undeletion is possible
-      // in the server's storage backend, so it's possible on the client,
-      // though not expected to be something that is commonly possible.
+      // commit of a delete, before undeleting.
+      // Undeletion is common for items that reuse the client-unique tag.
       VerifyResult result =
           SyncerUtil::VerifyUndelete(trans, entry, same_id);
       if (VERIFY_UNDECIDED != result)
         return result;
     }
   }
   if (same_id->Get(BASE_VERSION) > 0) {
     // We've committed this entry in the past.
     if (is_directory != same_id->Get(IS_DIR) ||
         model_type != same_id->GetModelType()) {
@@ skipping 23 matching lines @@
   }
   return VERIFY_SUCCESS;
 }
 
 // Assumes we have an existing entry; verify an update that seems to be
 // expressing an 'undelete'
 // static
 VerifyResult SyncerUtil::VerifyUndelete(syncable::WriteTransaction* trans,
                                         const SyncEntity& entry,
                                         syncable::MutableEntry* same_id) {
+  // TODO(nick): We hit this path for items deleted items that the server
+  // tells us to re-create; only deleted items with positive base versions
+  // will hit this path.  However, it's not clear how such an undeletion
+  // would actually succeed on the server; in the protocol, a base
+  // version of 0 is required to undelete an object.  This codepath
+  // should be deprecated in favor of client-tag style undeletion
+  // (where items go to version 0 when they're deleted), or else
+  // removed entirely (if this type of undeletion is indeed impossible).
   CHECK(same_id->good());
   LOG(INFO) << "Server update is attempting undelete. " << *same_id
             << "Update:" << SyncerProtoUtil::SyncEntityDebugString(entry);
   // Move the old one aside and start over.  It's too tricky to get the old one
   // back into a state that would pass CheckTreeInvariants().
   if (same_id->Get(IS_DEL)) {
+    DCHECK(same_id->Get(UNIQUE_CLIENT_TAG).empty())
+        << "Doing move-aside undeletion on client-tagged item.";
     same_id->Put(ID, trans->directory()->NextId());
     same_id->Put(UNIQUE_CLIENT_TAG, "");
     same_id->Put(BASE_VERSION, CHANGES_VERSION);
     same_id->Put(SERVER_VERSION, 0);
     return VERIFY_SUCCESS;
   }
   if (entry.version() < same_id->Get(SERVER_VERSION)) {
     LOG(WARNING) << "Update older than current server version for" <<
         *same_id << "Update:" << SyncerProtoUtil::SyncEntityDebugString(entry);
     return VERIFY_SUCCESS;  // Expected in new sync protocol.
@@ skipping 53 matching lines @@
 
     // |update_entry| is considered to be somewhere after |candidate|, so store
     // it as the upper bound.
     closest_sibling = candidate.Get(ID);
   }
 
   return closest_sibling;
 }
 
 }  // namespace browser_sync