[Sync] Add two more USS integration tests.

- Disabling and enabling the type, which revealed a bug in the DTC.
- Conflict resolution, which revealed a bug in the worker.

BUG=643269

Review-Url: https://codereview.chromium.org/2339403004
Cr-Commit-Position: refs/heads/master@{#419222}

chrome/browser/sync/test/integration/two_client_uss_sync_test.cc

@@ -12,6 +12,7 @@
 #include "chrome/browser/sync/test/integration/sync_integration_test_util.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "components/browser_sync/browser/profile_sync_components_factory_impl.h"
+#include "components/browser_sync/browser/profile_sync_service.h"
 #include "components/sync/api/fake_model_type_service.h"
 
 using browser_sync::ChromeSyncClient;
@@ -19,6 +20,11 @@ using syncer_v2::FakeModelTypeService;
 using syncer_v2::ModelTypeService;
 using syncer_v2::SharedModelTypeProcessor;
 
+const char kKey1[] = "key1";
+const char kValue1[] = "value1";
+const char kValue2[] = "value2";
+const char kResolutionValue[] = "RESOLVED";
+
 // A ChromeSyncClient that provides a ModelTypeService for PREFERENCES.
 class TestSyncClient : public ChromeSyncClient {
  public:
@@ -62,6 +68,13 @@ class TestModelTypeService : public FakeModelTypeService {
                                          db().CreateMetadataBatch());
   }
 
+  syncer_v2::ConflictResolution ResolveConflict(
+      const syncer_v2::EntityData& local_data,
+      const syncer_v2::EntityData& remote_data) const override {
+    return syncer_v2::ConflictResolution::UseNew(
+        GenerateEntityData(local_data.non_unique_name, kResolutionValue));
+  }
+
   void AddObserver(Observer* observer) { observers_.insert(observer); }
   void RemoveObserver(Observer* observer) { observers_.erase(observer); }
 
@@ -103,35 +116,73 @@ class KeyChecker : public StatusChangeChecker,
   const std::string key_;
 };
 
-// Wait for a key to be present.
-class KeyPresentChecker : public KeyChecker {
+// Wait for data for a key to have a certain value.
+class DataChecker : public KeyChecker {
  public:
-  KeyPresentChecker(TestModelTypeService* service, const std::string& key)
-      : KeyChecker(service, key) {}
-  ~KeyPresentChecker() override {}
+  DataChecker(TestModelTypeService* service,
+              const std::string& key,
+              const std::string& value)
+      : KeyChecker(service, key), value_(value) {}
+  ~DataChecker() override {}
 
   bool IsExitConditionSatisfied() override {
-    return service_->db().HasData(key_);
+    const auto& db = service_->db();
+    return db.HasData(key_) && db.GetValue(key_) == value_;
   }
 
   std::string GetDebugMessage() const override {
-    return "Waiting for key '" + key_ + "' to be present.";
+    return "Waiting for data for key '" + key_ + "' to be '" + value_ + "'.";
   }
+
+ private:
+  const std::string value_;
 };
 
-// Wait for a key to be absent.
-class KeyAbsentChecker : public KeyChecker {
+// Wait for data for a key to be absent.
+class DataAbsentChecker : public KeyChecker {
  public:
-  KeyAbsentChecker(TestModelTypeService* service, const std::string& key)
+  DataAbsentChecker(TestModelTypeService* service, const std::string& key)
       : KeyChecker(service, key) {}
-  ~KeyAbsentChecker() override {}
+  ~DataAbsentChecker() override {}
 
   bool IsExitConditionSatisfied() override {
     return !service_->db().HasData(key_);
   }
 
   std::string GetDebugMessage() const override {
-    return "Waiting for key '" + key_ + "' to be absent.";
+    return "Waiting for data for key '" + key_ + "' to be absent.";
+  }
+};
+
+// Wait for metadata for a key to be present.
+class MetadataPresentChecker : public KeyChecker {
+ public:
+  MetadataPresentChecker(TestModelTypeService* service, const std::string& key)
+      : KeyChecker(service, key) {}
+  ~MetadataPresentChecker() override {}
+
+  bool IsExitConditionSatisfied() override {
+    return service_->db().HasMetadata(key_);
+  }
+
+  std::string GetDebugMessage() const override {
+    return "Waiting for metadata for key '" + key_ + "' to be present.";
+  }
+};
+
+// Wait for metadata for a key to be absent.
+class MetadataAbsentChecker : public KeyChecker {
+ public:
+  MetadataAbsentChecker(TestModelTypeService* service, const std::string& key)
+      : KeyChecker(service, key) {}
+  ~MetadataAbsentChecker() override {}
+
+  bool IsExitConditionSatisfied() override {
+    return !service_->db().HasMetadata(key_);
+  }
+
+  std::string GetDebugMessage() const override {
+    return "Waiting for metadata for key '" + key_ + "' to be absent.";
   }
 };
 
@@ -176,6 +227,7 @@ class TwoClientUssSyncTest : public SyncTest {
   std::vector<TestSyncClient*> clients_;
   bool first_client_ignored_ = false;
 
+ private:
   DISALLOW_COPY_AND_ASSIGN(TwoClientUssSyncTest);
 };
 
@@ -186,10 +238,67 @@ IN_PROC_BROWSER_TEST_F(TwoClientUssSyncTest, Sanity) {
   TestModelTypeService* model1 = GetModelTypeService(0);
   TestModelTypeService* model2 = GetModelTypeService(1);
 
-  model1->WriteItem("foo", "bar");
-  ASSERT_TRUE(KeyPresentChecker(model2, "foo").Wait());
-  EXPECT_EQ("bar", model2->db().GetValue("foo"));
+  // Add an entity.
+  model1->WriteItem(kKey1, kValue1);
+  ASSERT_TRUE(DataChecker(model2, kKey1, kValue1).Wait());
+
+  // Update an entity.
+  model1->WriteItem(kKey1, kValue2);
+  ASSERT_TRUE(DataChecker(model2, kKey1, kValue2).Wait());
+
+  // Delete an entity.
+  model1->DeleteItem(kKey1);
+  ASSERT_TRUE(DataAbsentChecker(model2, kKey1).Wait());
+}
+
+IN_PROC_BROWSER_TEST_F(TwoClientUssSyncTest, DisableEnable) {
+  ASSERT_TRUE(SetupSync());
+  TestModelTypeService* model1 = GetModelTypeService(0);
+  TestModelTypeService* model2 = GetModelTypeService(1);
+
+  // Add an entity to test with.
+  model1->WriteItem(kKey1, kValue1);
+  ASSERT_TRUE(DataChecker(model2, kKey1, kValue1).Wait());
+  ASSERT_EQ(1U, model1->db().data_count());
+  ASSERT_EQ(1U, model1->db().metadata_count());
+  ASSERT_EQ(1U, model2->db().data_count());
+  ASSERT_EQ(1U, model2->db().metadata_count());
+
+  // Disable PREFERENCES.
+  syncer::ModelTypeSet types = syncer::UserSelectableTypes();
+  types.Remove(syncer::PREFERENCES);
+  GetSyncService(0)->OnUserChoseDatatypes(false, types);
+
+  // Wait for it to take effect and remove the metadata.
+  ASSERT_TRUE(MetadataAbsentChecker(model1, kKey1).Wait());
+  ASSERT_EQ(1U, model1->db().data_count());
+  ASSERT_EQ(0U, model1->db().metadata_count());
+  // Model 2 should not be affected.
+  ASSERT_EQ(1U, model2->db().data_count());
+  ASSERT_EQ(1U, model2->db().metadata_count());
+
+  // Re-enable PREFERENCES.
+  GetSyncService(0)->OnUserChoseDatatypes(true, syncer::UserSelectableTypes());
+
+  // Wait for metadata to be re-added.
+  ASSERT_TRUE(MetadataPresentChecker(model1, kKey1).Wait());
+  ASSERT_EQ(1U, model1->db().data_count());
+  ASSERT_EQ(1U, model1->db().metadata_count());
+  ASSERT_EQ(1U, model2->db().data_count());
+  ASSERT_EQ(1U, model2->db().metadata_count());
+}
+
+IN_PROC_BROWSER_TEST_F(TwoClientUssSyncTest, ConflictResolution) {
+  ASSERT_TRUE(SetupSync());
+  TestModelTypeService* model1 = GetModelTypeService(0);
+  TestModelTypeService* model2 = GetModelTypeService(1);
+
+  // Write conflicting entities.
+  model1->WriteItem(kKey1, kValue1);
+  model2->WriteItem(kKey1, kValue2);
 
-  model1->DeleteItem("foo");
-  ASSERT_TRUE(KeyAbsentChecker(model2, "foo").Wait());
+  // Wait for them to be resolved to kResolutionValue by the custom conflict
+  // resolution logic in TestModelTypeService.
+  ASSERT_TRUE(DataChecker(model1, kKey1, kResolutionValue).Wait());
+  ASSERT_TRUE(DataChecker(model2, kKey1, kResolutionValue).Wait());
 }

components/sync/driver/non_blocking_data_type_controller.cc

@@ -131,11 +131,14 @@ void NonBlockingDataTypeController::OnProcessorStarted(
 void NonBlockingDataTypeController::RegisterWithBackend(
     sync_driver::BackendDataTypeConfigurer* configurer) {
   DCHECK(CalledOnValidThread());
+  if (activated_)
+    return;
   DCHECK(configurer);
   DCHECK(activation_context_);
   DCHECK_EQ(MODEL_LOADED, state_);
   configurer->ActivateNonBlockingDataType(type(),
                                           std::move(activation_context_));
+  activated_ = true;
 }
 
 void NonBlockingDataTypeController::StartAssociating(
@@ -165,7 +168,9 @@ void NonBlockingDataTypeController::DeactivateDataType(
     sync_driver::BackendDataTypeConfigurer* configurer) {
   DCHECK(CalledOnValidThread());
   DCHECK(configurer);
+  DCHECK(activated_);
   configurer->DeactivateNonBlockingDataType(type());
+  activated_ = false;
 }
 
 void NonBlockingDataTypeController::Stop() {

components/sync/driver/non_blocking_data_type_controller.h

@@ -97,6 +97,12 @@ class NonBlockingDataTypeController : public sync_driver::DataTypeController {
   // callback and must temporarily own it until ActivateDataType is called.
   std::unique_ptr<syncer_v2::ActivationContext> activation_context_;
 
+  // This is a hack to prevent reconfigurations from crashing, because USS
+  // activation is not idempotent. RegisterWithBackend only needs to actually do
+  // something the first time after the type is enabled.
+  // TODO(crbug.com/647505): Remove this once the DTM handles things better.
+  bool activated_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(NonBlockingDataTypeController);
 };
 

components/sync/engine_impl/model_type_worker.cc

@@ -148,17 +148,17 @@ SyncerError ModelTypeWorker::ProcessGetUpdatesResponse(
     // Check if specifics are encrypted and try to decrypt if so.
     if (!specifics.has_encrypted()) {
       // No encryption.
-      entity->ReceiveUpdate(update_entity->version());
       data.specifics = specifics;
       response_data.entity = data.PassToPtr();
+      entity->ReceiveUpdate(response_data);
       pending_updates_.push_back(response_data);
     } else if (specifics.has_encrypted() && cryptographer_ &&
                cryptographer_->CanDecrypt(specifics.encrypted())) {
       // Encrypted, but we know the key.
       if (DecryptSpecifics(cryptographer_.get(), specifics, &data.specifics)) {
-        entity->ReceiveUpdate(update_entity->version());
         response_data.entity = data.PassToPtr();
         response_data.encryption_key_name = specifics.encrypted().key_name();
+        entity->ReceiveUpdate(response_data);
         pending_updates_.push_back(response_data);
       }
     } else if (specifics.has_encrypted() &&

components/sync/engine_impl/model_type_worker.h

@@ -162,7 +162,7 @@ class ModelTypeWorker : public syncer::UpdateHandler,
   // Interface used to access and send nudges to the sync scheduler. Not owned.
   syncer::NudgeHandler* nudge_handler_;
 
-  // A map of per-entity information known to this object.
+  // A map of per-entity information, keyed by client_tag_hash.
   //
   // When commits are pending, their information is stored here. This
   // information is dropped from memory when the commit succeeds or gets
@@ -170,9 +170,7 @@ class ModelTypeWorker : public syncer::UpdateHandler,
   //
   // This also stores some information related to received server state in
   // order to implement reflection blocking and conflict detection. This
-  // information is kept in memory indefinitely. With a bit more coordination
-  // with the model thread, we could optimize this to reduce memory usage in
-  // the steady state.
+  // information is kept in memory indefinitely.
   EntityMap entities_;
 
   // Accumulates all the updates from a single GetUpdates cycle in memory so

components/sync/engine_impl/worker_entity_tracker.cc

@@ -34,7 +34,6 @@ bool WorkerEntityTracker::HasPendingCommit() const {
 void WorkerEntityTracker::PopulateCommitProto(
     sync_pb::SyncEntity* commit_entity) const {
   DCHECK(HasPendingCommit());
-  DCHECK(!client_tag_hash_.empty());
 
   if (!id_.empty()) {
     commit_entity->set_id_string(id_);
@@ -125,11 +124,12 @@ void WorkerEntityTracker::ReceiveCommitResponse(CommitResponseData* ack) {
   ClearPendingCommit();
 }
 
-void WorkerEntityTracker::ReceiveUpdate(int64_t version) {
-  if (version <= highest_gu_response_version_)
+void WorkerEntityTracker::ReceiveUpdate(const UpdateResponseData& update) {
+  if (update.response_version <= highest_gu_response_version_)
     return;
 
-  highest_gu_response_version_ = version;
+  highest_gu_response_version_ = update.response_version;
+  id_ = update.entity->id;
 
   // Got an applicable update newer than any pending updates.  It must be safe
   // to discard the old encrypted update, if there was one.

components/sync/engine_impl/worker_entity_tracker.h

@@ -56,7 +56,7 @@ class WorkerEntityTracker {
   void ReceiveCommitResponse(CommitResponseData* ack);
 
   // Handles receipt of an update from the server.
-  void ReceiveUpdate(int64_t version);
+  void ReceiveUpdate(const UpdateResponseData& update);
 
   // Handles the receipt of an encrypted update from the server.
   //
@@ -92,7 +92,7 @@ class WorkerEntityTracker {
   std::string id_;
 
   // The hashed client tag for this entry.
-  std::string client_tag_hash_;
+  const std::string client_tag_hash_;
 
   // The highest version seen in a commit response for this entry.
   int64_t highest_commit_response_version_;

components/sync/engine_impl/worker_entity_tracker_unittest.cc

@@ -37,7 +37,7 @@ class WorkerEntityTrackerTest : public ::testing::Test {
         kSpecificsHash("somehash"),
         kCtime(base::Time::UnixEpoch() + base::TimeDelta::FromDays(10)),
         kMtime(base::Time::UnixEpoch() + base::TimeDelta::FromDays(20)),
-        entity_(new WorkerEntityTracker(kServerId, kClientTagHash)) {
+        entity_(new WorkerEntityTracker("", kClientTagHash)) {
     specifics.mutable_preference()->set_name(kClientTag);
     specifics.mutable_preference()->set_value("pref.value");
   }
@@ -86,8 +86,10 @@ class WorkerEntityTrackerTest : public ::testing::Test {
 // Construct a new entity from a server update.  Then receive another update.
 TEST_F(WorkerEntityTrackerTest, FromUpdateResponse) {
   EXPECT_FALSE(entity_->HasPendingCommit());
-  entity_->ReceiveUpdate(20);
+  EXPECT_EQ("", entity_->id());
+  entity_->ReceiveUpdate(MakeUpdateResponseData(20));
   EXPECT_FALSE(entity_->HasPendingCommit());
+  EXPECT_EQ(kServerId, entity_->id());
 }
 
 // Construct a new entity from a commit request.  Then serialize it.
@@ -96,11 +98,12 @@ TEST_F(WorkerEntityTrackerTest, FromCommitRequest) {
   const int64_t kBaseVersion = 33;
   CommitRequestData data = MakeCommitRequestData(kSequenceNumber, kBaseVersion);
   entity_->RequestCommit(data);
+  EXPECT_EQ("", entity_->id());
 
   ASSERT_TRUE(entity_->HasPendingCommit());
   sync_pb::SyncEntity pb_entity;
   entity_->PopulateCommitProto(&pb_entity);
-  EXPECT_EQ(kServerId, pb_entity.id_string());
+  EXPECT_EQ("", pb_entity.id_string());
   EXPECT_EQ(kClientTagHash, pb_entity.client_defined_unique_tag());
   EXPECT_EQ(kBaseVersion, pb_entity.version());
   EXPECT_EQ(kCtime, syncer::ProtoTimeToTime(pb_entity.ctime()));
@@ -119,6 +122,13 @@ TEST_F(WorkerEntityTrackerTest, FromCommitRequest) {
   EXPECT_EQ(kSequenceNumber, ack.sequence_number);
   EXPECT_EQ(kSpecificsHash, ack.specifics_hash);
   EXPECT_FALSE(entity_->HasPendingCommit());
+
+  EXPECT_EQ(kServerId, entity_->id());
+  CommitRequestData data2 =
+      MakeCommitRequestData(kSequenceNumber + 1, ack.response_version);
+  entity_->RequestCommit(data2);
+  entity_->PopulateCommitProto(&pb_entity);
+  EXPECT_EQ(kServerId, pb_entity.id_string());
 }
 
 // Start with a server initiated entity.  Commit over top of it.
@@ -130,7 +140,7 @@ TEST_F(WorkerEntityTrackerTest, RequestCommit) {
 // Start with a server initiated entity.  Fail to request a commit because of
 // an out of date base version.
 TEST_F(WorkerEntityTrackerTest, RequestCommitFailure) {
-  entity_->ReceiveUpdate(10);
+  entity_->ReceiveUpdate(MakeUpdateResponseData(10));
   EXPECT_FALSE(entity_->HasPendingCommit());
   entity_->RequestCommit(
       MakeCommitRequestData(23, 5 /* base_version 5 < 10 */));
@@ -144,7 +154,7 @@ TEST_F(WorkerEntityTrackerTest, UpdateClobbersCommit) {
 
   EXPECT_TRUE(entity_->HasPendingCommit());
 
-  entity_->ReceiveUpdate(400);  // Version 400 > 33.
+  entity_->ReceiveUpdate(MakeUpdateResponseData(400));  // Version 400 > 33.
   EXPECT_FALSE(entity_->HasPendingCommit());
 }
 
@@ -156,7 +166,7 @@ TEST_F(WorkerEntityTrackerTest, ReflectedUpdateDoesntClobberCommit) {
 
   EXPECT_TRUE(entity_->HasPendingCommit());
 
-  entity_->ReceiveUpdate(33);  // Version 33 == 33.
+  entity_->ReceiveUpdate(MakeUpdateResponseData(33));  // Version 33 == 33.
   EXPECT_TRUE(entity_->HasPendingCommit());
 }