sync: Improve tracking of useless updates

Our counts of non-useless updates are wrong in part because of the way
we track deletions.  When an item is deleted, we flip the IS_DEL bit.
On restart, we purge all fully synced IS_DEL items from our directory.
However, we might still receive some re-deliveries of the tombstone for
that item.  The old algorithm would consider tombstones of non-existent
items to be "useful", despite the fact that they're often just
reflections of updates we've already received.

The updated algorithm isn't perfect.  There's no easy way to distinguish
between a tombstone created by another client where this client never
saw the original (which one could argue is useful, and definitely not a
reflection) and the re-delivery of a tombstone created by another client
which this client has already received and processed long ago (which is
definitely not useful).  Both scenarios look the same, and both will be
counted as 'reflections' under the new algorithm.

A similar issue arises with UNIQUE_CLIENT_TAG items.  When we delete
them we force their version to 0 locally.  This makes our regular
reflection detection algorithm ineffective.  This change updates the
algorithm to consider tombstones to items that have UNIQUE_CLIENT_TAG
and are locally deleted to be reflections.  It's not perfect, but it
should be good enough for our purposes.

BUG=139684

Review URL: https://chromiumcodereview.appspot.com/10834095

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

1 files changed, 20 insertions, 0 deletions

diff --git a/sync/engine/verify_updates_command.cc b/sync/engine/verify_updates_command.cc
index 4b4a85a..4989afb 100644
--- a/sync/engine/verify_updates_command.cc
+++ b/sync/engine/verify_updates_command.cc
@@ -56,6 +56,26 @@ bool UpdateContainsNewVersion(syncable::BaseTransaction *trans,
   if (existing_entry.good())
     existing_version = existing_entry.Get(syncable::BASE_VERSION);
 
+  if (!existing_entry.good() && update.deleted()) {
+    // There are several possible explanations for this.  The most common cases
+    // will be first time sync and the redelivery of deletions we've already
+    // synced, accepted, and purged from our database.  In either case, the
+    // update is useless to us.  Let's count them all as "not new", even though
+    // that may not always be entirely accurate.
+    return false;
+  }
+
+  if (existing_entry.good() &&
+      !existing_entry.Get(syncable::UNIQUE_CLIENT_TAG).empty() &&
+      existing_entry.Get(syncable::IS_DEL) &&
+      update.deleted()) {
+    // Unique client tags will have their version set to zero when they're
+    // deleted.  The usual version comparison logic won't be able to detect
+    // reflections of these items.  Instead, we assume any received tombstones
+    // are reflections.  That should be correct most of the time.
+    return false;
+  }
+
   return existing_version < update.version();
 }