Change sync conflict resolution for extensions

For extensions and apps, if the server has seen a delete action
(corresponding to an uninstall) and the local state has a not-yet-synced
change, we would prefer to resolve this conflict by keeping the server
state and uninstalling locally. This helps to prevent uninstalled
extensions "coming back to life".

BUG=517910

Review URL: https://codereview.chromium.org/1664643003

Cr-Commit-Position: refs/heads/master@{#376603}

2 files changed, 118 insertions, 25 deletions

diff --git a/sync/engine/conflict_resolver.cc b/sync/engine/conflict_resolver.cc
index 0af3097..d32460c 100644
--- a/sync/engine/conflict_resolver.cc
+++ b/sync/engine/conflict_resolver.cc
@@ -116,6 +116,8 @@ void ConflictResolver::ProcessSimpleConflict(WriteTransaction* trans,
   // f) Otherwise, it's in general safer to ignore local changes, with the
   //    exception of deletion conflicts (choose to undelete) and conflicts
   //    where the non_unique_name or parent don't match.
+  // e) Except for the case of extensions and apps, where we want uninstalls to
+  //    win over local modifications to avoid "back from the dead" reinstalls.
   if (!entry.GetServerIsDel()) {
     // TODO(nick): The current logic is arbitrary; instead, it ought to be made
     // consistent with the ModelAssociator behavior for a datatype.  It would
@@ -228,28 +230,42 @@ void ConflictResolver::ProcessSimpleConflict(WriteTransaction* trans,
     // specifics.
     entry.PutBaseServerSpecifics(sync_pb::EntitySpecifics());
   } else {  // SERVER_IS_DEL is true
-    if (entry.GetIsDir()) {
-      Directory::Metahandles children;
-      trans->directory()->GetChildHandlesById(trans,
-                                              entry.GetId(),
-                                              &children);
-      // If a server deleted folder has local contents it should be a hierarchy
-      // conflict.  Hierarchy conflicts should not be processed by this
-      // function.
-      DCHECK(children.empty());
-    }
+    ModelType type = entry.GetModelType();
+    if (type == EXTENSIONS || type == APPS) {
+      // Ignore local changes for extensions/apps when server had a delete, to
+      // avoid unwanted reinstall of an uninstalled extension.
+      DVLOG(1) << "Resolving simple conflict, ignoring local changes for "
+               << "extension/app: " << entry;
+      conflict_util::IgnoreLocalChanges(&entry);
+      status->increment_num_local_overwrites();
+      counters->num_local_overwrites++;
+      UMA_HISTOGRAM_ENUMERATION("Sync.ResolveSimpleConflict",
+                                OVERWRITE_LOCAL,
+                                CONFLICT_RESOLUTION_SIZE);
+    } else {
+      if (entry.GetIsDir()) {
+        Directory::Metahandles children;
+        trans->directory()->GetChildHandlesById(trans,
+                                                entry.GetId(),
+                                                &children);
+        // If a server deleted folder has local contents it should be a
+        // hierarchy conflict.  Hierarchy conflicts should not be processed by
+        // this function.
+        DCHECK(children.empty());
+      }
 
-    // The entry is deleted on the server but still exists locally.
-    // We undelete it by overwriting the server's tombstone with the local
-    // data.
-    conflict_util::OverwriteServerChanges(&entry);
-    status->increment_num_server_overwrites();
-    counters->num_server_overwrites++;
-    DVLOG(1) << "Resolving simple conflict, undeleting server entry: "
-             << entry;
-    UMA_HISTOGRAM_ENUMERATION("Sync.ResolveSimpleConflict",
-                              UNDELETE,
-                              CONFLICT_RESOLUTION_SIZE);
+      // The entry is deleted on the server but still exists locally.
+      // We undelete it by overwriting the server's tombstone with the local
+      // data.
+      conflict_util::OverwriteServerChanges(&entry);
+      status->increment_num_server_overwrites();
+      counters->num_server_overwrites++;
+      DVLOG(1) << "Resolving simple conflict, undeleting server entry: "
+               << entry;
+      UMA_HISTOGRAM_ENUMERATION("Sync.ResolveSimpleConflict",
+                                UNDELETE,
+                                CONFLICT_RESOLUTION_SIZE);
+    }
   }
 }
 
diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc
index b2a92de..b968e92 100644
--- a/sync/engine/syncer_unittest.cc
+++ b/sync/engine/syncer_unittest.cc
@@ -283,6 +283,7 @@ class SyncerTest : public testing::Test,
                                                  &cancelation_signal_));
     debug_info_getter_.reset(new MockDebugInfoGetter);
     EnableDatatype(BOOKMARKS);
+    EnableDatatype(EXTENSIONS);
     EnableDatatype(NIGORI);
     EnableDatatype(PREFERENCES);
     EnableDatatype(NIGORI);
@@ -2670,11 +2671,11 @@ TEST_F(SyncerTest, CommitsUpdateDoesntAlterEntry) {
 }
 
 TEST_F(SyncerTest, ParentAndChildBothMatch) {
-  // Disable PREFERENCES which is enabled at the setup step to avoid
-  // auto-creating
-  // PREFERENCES root folder and failing the test below that verifies the number
-  // of children at the root.
+  // Disable PREFERENCES and EXTENSIONS which are enabled at the setup step to
+  // avoid auto-creating root folders and failing the test below
+  // that verifies the number of children at the root.
   DisableDatatype(PREFERENCES);
+  DisableDatatype(EXTENSIONS);
 
   const FullModelTypeSet all_types = FullModelTypeSet::All();
   syncable::Id parent_id = ids_.NewServerId();
@@ -3755,6 +3756,82 @@ TEST_F(SyncerTest, ConflictResolverMergesLocalDeleteAndServerUpdate) {
   }
 }
 
+// This ensures that for extensions, we resolve the conflict of local updates
+// and server deletes in favor of the server, to prevent extensions from
+// being reinstalled after uninstall.
+TEST_F(SyncerTest, ConflictResolverAcceptsServerDeleteForExtensions) {
+  ASSERT_TRUE(context_->GetEnabledTypes().Has(EXTENSIONS));
+
+  // Create an extension entry.
+  int64_t metahandle;
+  {
+    WriteTransaction trans(FROM_HERE, UNITTEST, directory());
+    MutableEntry extension(
+        &trans, CREATE, EXTENSIONS, trans.root_id(), "extension_name");
+    ASSERT_TRUE(extension.good());
+    sync_pb::EntitySpecifics specifics;
+    AddDefaultFieldValue(EXTENSIONS, &specifics);
+    extension.PutSpecifics(specifics);
+    EXPECT_FALSE(extension.GetIsUnappliedUpdate());
+    EXPECT_FALSE(extension.GetId().ServerKnows());
+    metahandle = extension.GetMetahandle();
+    extension.PutIsUnsynced(true);
+  }
+
+  // Make sure the server has received the new item.
+  SyncShareNudge();
+  syncable::Id id;
+  {
+    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());
+    id = entry.GetId();
+  }
+
+
+  // Simulate another client deleting the item.
+  {
+    syncable::ReadTransaction trans(FROM_HERE, directory());
+    Entry entry(&trans, GET_BY_HANDLE, metahandle);
+    mock_server_->AddUpdateTombstone(id, EXTENSIONS);
+  }
+
+  // Create a local update, which should cause a conflict with the delete that
+  // we just pushed to the server.
+  {
+    WriteTransaction trans(FROM_HERE, UNITTEST, directory());
+    MutableEntry extension(&trans, GET_BY_HANDLE, metahandle);
+    ASSERT_TRUE(extension.good());
+    sync_pb::EntitySpecifics specifics;
+    AddDefaultFieldValue(EXTENSIONS, &specifics);
+    specifics.mutable_extension()->set_disable_reasons(2);
+    extension.PutSpecifics(specifics);
+    EXPECT_FALSE(extension.GetIsUnappliedUpdate());
+    extension.PutIsUnsynced(true);
+  }
+
+  // Run a sync, and expect the item to be deleted.
+  SyncShareNudge();
+  {
+    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);
+  }
+}
+
 // See what happens if the IS_DIR bit gets flipped.  This can cause us
 // all kinds of disasters.
 TEST_F(SyncerTest, UpdateFlipsTheFolderBit) {