Change sync conflict resolution for extensions

sync/engine/syncer_unittest.cc

Index: sync/engine/syncer_unittest.cc
diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc
index b2a92dea45dfe1af28f934c1b00551fd6f87866d..713304f33e636fb70cc38e70be5408ae1d3f98d7 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,89 @@ 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);

[asargent_no_longer_on_chrome, 2016/02/11 00:59:37]

Note: a bunch of the code I use in this test to create/update the entry and
nudge the sync to happen is largely cribbed from SyncerBookmarksTest::Create /
SyncerBookmarksTest::Update and SyncerTest::SyncShareNudge, but those all assume
the ModelType is BOOKMARKS. Let me know if you think it would be worth trying to
generalize that logic into helper methods that let you parameterize the
ModelType. 

+  }
+
+  // Make sure the server has received the new item.
+  ResetSession();

[Nicolas Zea, 2016/02/11 23:41:50]

You should just be able to call SyncShareNudge() here and below (the fact that
it nudges for Bookmarks doesn't matter).

[asargent_no_longer_on_chrome, 2016/02/18 00:53:45]

Done.

+  nudge_tracker_.RecordLocalChange(ModelTypeSet(EXTENSIONS));
+  EXPECT_TRUE(syncer_->NormalSyncShare(context_->GetEnabledTypes(),
+                                       &nudge_tracker_, session_.get()));
+  {
+    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());
+  }
+
+
+  // Simulate another client deleting the item.
+  {
+    syncable::ReadTransaction trans(FROM_HERE, directory());
+    Entry entry(&trans, GET_BY_HANDLE, metahandle);
+    mock_server_->AddUpdateTombstone(entry.GetId(), EXTENSIONS);

[Nicolas Zea, 2016/02/11 23:41:50]

You can just save the id above when you create the item. That way you don't need
to open a transaction and look up the entry again.

[asargent_no_longer_on_chrome, 2016/02/18 00:53:45]

Done.

BTW, I found that this only worked if I saved the id I got from within the first
ReadTransaction after nudging the server (value was of the form
"smock_server:10000"), but if I saved the id from within the first
WriteTransaction but before nudging the server, it didn't work (value was of the
form "cAC89AF2E-F949-47EC-8E89-9ABF59481D56"). Hopefully the way I'm doing it
now was what you meant in your comment. 

+  }
+
+  // Create a local update

[Nicolas Zea, 2016/02/11 23:41:50]

nit: add a period. Also maybe call out that we're purposefully creating the item
before we do the sync to force a conflict.

[asargent_no_longer_on_chrome, 2016/02/18 00:53:45]

Done.

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

[Nicolas Zea, 2016/02/11 23:41:50]

you don't need to actually change the specifics at this layer. By setting
IsUnsynced to true, you'll force the conflict.

[asargent_no_longer_on_chrome, 2016/02/18 00:53:45]

Hmm, I found a subtle difference in the test behavior when I didn't modify the
specifics - it's the difference between specifics_match being true vs. false in
ConflictResolver::ProcessSimpleConflict. When the specifics do match (ie don't
change any specifics here in the test), then down in the final ReadTransaction
at the end of the test, I found that entry.GetIsDel() returns false instead of
true. Does this matter? I was worried it might mean the item wasn't actually
deleted locally.

In both cases in ConflictResolver::ProcessSimpleConflict, we end up calling  
conflict_util::IgnoreConflict - the only difference between the two cases is
whether we also call status->increment_num_server_overwrites() and
counters->num_server_overwrites++ or not.

When I first ran into this issue while writing the test, I figured that changing
the enable/disable state matches my manual test case, and also what I suspect
*might* be the source of this happening in the wild.

Let me know what you think I should do here:

a) Skip updating the specifics, and remove the check of entry.GetIsDel() below

b) Leave it as is

c) Something else - perhaps this has uncovered a real problem that still isn't
fixed?

[Nicolas Zea, 2016/02/18 19:28:55]

You're right, entry.GetIsDel should be true if the item was properly deleted
after the conflict resolved.
Ah, that's I think because of the condition you added actually. Previously we
would assume that if the local isn't deleted, then the server wouldn't be
deleted either (due to the original condition), and hence checking specifics
match would be enough. With the new condition for extensions/apps that isn't
true. This makes me think we should move the condition (see my other comment).

[asargent_no_longer_on_chrome, 2016/02/19 23:59:52]

With the modification you suggested in conflict_resolver.cc, I've verified that
this test now succeeds regardless of whether the specifics are actually changed
or not. 

For now I've left the test as-is (still modifying the specifics), but let me
know if you'd rather I changed it to not do so. 

+    EXPECT_FALSE(extension.GetIsUnappliedUpdate());
+    extension.PutIsUnsynced(true);
+    if (extension.GetSyncing())

[Nicolas Zea, 2016/02/11 23:41:50]

I don't think this condition or the PutDirtySync are necessary.

[asargent_no_longer_on_chrome, 2016/02/18 00:53:45]

Done.

+      extension.PutDirtySync(true);
+  }
+
+  // Run a sync.
+  ResetSession();
+  nudge_tracker_.RecordLocalChange(ModelTypeSet(EXTENSIONS));
+  EXPECT_TRUE(syncer_->NormalSyncShare(context_->GetEnabledTypes(),
+                                       &nudge_tracker_, session_.get()));
+
+  // Now expect the item to be deleted.
+  {
+    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) {