Change sync conflict resolution for extensions

sync/engine/syncer_unittest.cc

 // Copyright 2012 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.
 //
 // Syncer unit tests. Unfortunately a lot of these tests
 // are outdated and need to be reworked and updated.
 
 #include <stddef.h>
 #include <stdint.h>
 
@@ skipping 265 matching lines @@
             sync_pb::GetUpdatesCallerInfo::RECONFIGURATION,
             session_.get());
   }
 
   void SetUp() override {
     dir_maker_.SetUp();
     mock_server_.reset(new MockConnectionManager(directory(),
                                                  &cancelation_signal_));
     debug_info_getter_.reset(new MockDebugInfoGetter);
     EnableDatatype(BOOKMARKS);
+    EnableDatatype(EXTENSIONS);
     EnableDatatype(NIGORI);
     EnableDatatype(PREFERENCES);
     EnableDatatype(NIGORI);
     workers_.push_back(scoped_refptr<ModelSafeWorker>(
         new FakeModelWorker(GROUP_PASSIVE)));
     std::vector<SyncEngineEventListener*> listeners;
     listeners.push_back(this);
 
     ModelSafeRoutingInfo routing_info;
     GetModelSafeRoutingInfo(&routing_info);
@@ skipping 2367 matching lines @@
   EXPECT_TRUE(SyncShareNudge());
   {
     syncable::ReadTransaction trans(FROM_HERE, directory());
     Entry entry(&trans, syncable::GET_BY_ID, id);
     ASSERT_TRUE(entry.good());
     EXPECT_EQ(test_time, entry.GetMtime());
   }
 }
 
 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();
   syncable::Id child_id = ids_.NewServerId();
   syncable::Id parent_local_id;
   syncable::Id child_local_id;
 
   {
     WriteTransaction wtrans(FROM_HERE, UNITTEST, directory());
     MutableEntry parent(&wtrans, CREATE, BOOKMARKS, root_id_, "Folder");
@@ skipping 1060 matching lines @@
     syncable::ReadTransaction trans(FROM_HERE, directory());
     Entry local_deleted(&trans, GET_BY_ID, ids_.FromNumber(1));
     EXPECT_EQ(10, local_deleted.GetBaseVersion());
     EXPECT_FALSE(local_deleted.GetIsUnappliedUpdate());
     EXPECT_TRUE(local_deleted.GetIsUnsynced());
     EXPECT_TRUE(local_deleted.GetIsDel());
     EXPECT_FALSE(local_deleted.GetIsDir());
   }
 }
 
+// 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) {
   // Local object: a deleted directory (container), revision 1, unsynced.
   {
     WriteTransaction trans(FROM_HERE, UNITTEST, directory());
 
     MutableEntry local_deleted(
         &trans, CREATE, BOOKMARKS, trans.root_id(), "name");
     local_deleted.PutId(ids_.FromNumber(1));
@@ skipping 2052 matching lines @@
     EXPECT_EQ("xyz", final_monitor_records["xyz"].extension_id);
     EXPECT_EQ(2049U, final_monitor_records["ABC"].bookmark_write_count);
     EXPECT_EQ(4U,    final_monitor_records["xyz"].bookmark_write_count);
   } else {
     EXPECT_TRUE(final_monitor_records.empty())
         << "Should not restore records after successful bookmark commit.";
   }
 }
 
 }  // namespace syncer