[Sync] USS: Fix race condition in DeviceInfoService + add error handling

components/sync_driver/device_info_service_unittest.cc

 // Copyright 2015 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.
 
 #include "components/sync_driver/device_info_service.h"
 
 #include <map>
 #include <memory>
 #include <set>
 #include <string>
@@ skipping 42 matching lines @@
 using sync_pb::EntitySpecifics;
 
 using ClientTagList = ModelTypeService::ClientTagList;
 using RecordList = ModelTypeStore::RecordList;
 using Result = ModelTypeStore::Result;
 using StartCallback = ModelTypeChangeProcessor::StartCallback;
 using WriteBatch = ModelTypeStore::WriteBatch;
 
 namespace {
 
+std::unique_ptr<DeviceInfo> CreateDeviceInfo() {
+  return base::MakeUnique<DeviceInfo>(
+      "guid_1", "client_1", "Chromium 10k", "Chrome 10k",
+      sync_pb::SyncEnums_DeviceType_TYPE_LINUX, "device_id");
+}
+
 void AssertResultIsSuccess(Result result) {
   ASSERT_EQ(Result::SUCCESS, result);
 }
 
 void AssertEqual(const DeviceInfoSpecifics& s1, const DeviceInfoSpecifics& s2) {
   ASSERT_EQ(s1.cache_guid(), s2.cache_guid());
   ASSERT_EQ(s1.client_name(), s2.client_name());
   ASSERT_EQ(s1.device_type(), s2.device_type());
   ASSERT_EQ(s1.sync_user_agent(), s2.sync_user_agent());
   ASSERT_EQ(s1.chrome_version(), s2.chrome_version());
   ASSERT_EQ(s1.signin_scoped_device_id(), s2.signin_scoped_device_id());
 }
 
 void AssertEqual(const DeviceInfoSpecifics& specifics,
                  const DeviceInfo& model) {
   ASSERT_EQ(specifics.cache_guid(), model.guid());
   ASSERT_EQ(specifics.client_name(), model.client_name());
   ASSERT_EQ(specifics.device_type(), model.device_type());
   ASSERT_EQ(specifics.sync_user_agent(), model.sync_user_agent());
   ASSERT_EQ(specifics.chrome_version(), model.chrome_version());
   ASSERT_EQ(specifics.signin_scoped_device_id(),
             model.signin_scoped_device_id());
 }
 
-void AssertErrorFromDataBatch(SyncError error,
-                              std::unique_ptr<DataBatch> batch) {
-  ASSERT_TRUE(error.IsSet());
-}
-
 void AssertExpectedFromDataBatch(
     std::map<std::string, DeviceInfoSpecifics> expected,
     SyncError error,
     std::unique_ptr<DataBatch> batch) {
   ASSERT_FALSE(error.IsSet());
   while (batch->HasNext()) {
     const TagAndData& pair = batch->Next();
     std::map<std::string, DeviceInfoSpecifics>::iterator iter =
         expected.find(pair.first);
     ASSERT_NE(iter, expected.end());
     AssertEqual(iter->second, pair.second->specifics.device_info());
     // Removing allows us to verify we don't see the same item multiple times,
     // and that we saw everything we expected.
     expected.erase(iter);
   }
   ASSERT_TRUE(expected.empty());
 }
 
-// Creats an EntityData/EntityDataPtr around a copy of the given specifics.
+// Creates an EntityData/EntityDataPtr around a copy of the given specifics.
 EntityDataPtr SpecificsToEntity(const DeviceInfoSpecifics& specifics) {
   EntityData data;
   // These tests do not care about the tag hash, but EntityData and friends
   // cannot differentiate between the default EntityData object if the hash
   // is unset, which causes pass/copy operations to no-op and things start to
   // break, so we throw in a junk value and forget about it.
   data.client_tag_hash = "junk";
   *data.specifics.mutable_device_info() = specifics;
   return data.PassToPtr();
 }
@@ skipping 41 matching lines @@
 };
 
 }  // namespace
 
 class DeviceInfoServiceTest : public testing::Test,
                               public DeviceInfoTracker::Observer {
  protected:
   DeviceInfoServiceTest()
       : change_count_(0),
         store_(ModelTypeStoreTestUtil::CreateInMemoryStoreForTest()),
-        local_device_(new LocalDeviceInfoProviderMock(
-            "guid_1",
-            "client_1",
-            "Chromium 10k",
-            "Chrome 10k",
-            sync_pb::SyncEnums_DeviceType_TYPE_LINUX,
-            "device_id")) {}
+        local_device_(new LocalDeviceInfoProviderMock()) {
+    local_device_->Initialize(CreateDeviceInfo());
+  }
 
   ~DeviceInfoServiceTest() override {
     // Some tests may never initialize the service.
     if (service_)
       service_->RemoveObserver(this);
 
     // Force all remaining (store) tasks to execute so we don't leak memory.
     base::RunLoop().RunUntilIdle();
   }
 
@@ skipping 174 matching lines @@
   base::RunLoop().RunUntilIdle();
   ScopedVector<DeviceInfo> all_device_info(service()->GetAllDeviceInfo());
   ASSERT_EQ(1u, all_device_info.size());
   ASSERT_TRUE(
       local_device()->GetLocalDeviceInfo()->Equals(*all_device_info[0]));
 }
 
 TEST_F(DeviceInfoServiceTest, LocalProviderSubscription) {
   set_local_device(base::WrapUnique(new LocalDeviceInfoProviderMock()));
   InitializeAndPumpAndStart();
+
   ASSERT_EQ(0u, service()->GetAllDeviceInfo().size());
-  local_device()->Initialize(base::WrapUnique(
-      new DeviceInfo("guid_1", "client_1", "Chromium 10k", "Chrome 10k",
-                     sync_pb::SyncEnums_DeviceType_TYPE_LINUX, "device_id")));
+  local_device()->Initialize(CreateDeviceInfo());
+  base::RunLoop().RunUntilIdle();
+
   ScopedVector<DeviceInfo> all_device_info(service()->GetAllDeviceInfo());
   ASSERT_EQ(1u, all_device_info.size());
   ASSERT_TRUE(
       local_device()->GetLocalDeviceInfo()->Equals(*all_device_info[0]));
 }
 
+// Metadata shouldn't be loaded before the provider is initialized.
+TEST_F(DeviceInfoServiceTest, LocalProviderInitRace) {
+  set_local_device(base::WrapUnique(new LocalDeviceInfoProviderMock()));
+  InitializeAndPump();
+  OnSyncStarting();
+  EXPECT_FALSE(processor()->metadata());
+
+  ASSERT_EQ(0u, service()->GetAllDeviceInfo().size());
+  local_device()->Initialize(CreateDeviceInfo());
+  base::RunLoop().RunUntilIdle();
+
+  ScopedVector<DeviceInfo> all_device_info(service()->GetAllDeviceInfo());
+  ASSERT_EQ(1u, all_device_info.size());
+  ASSERT_TRUE(
+      local_device()->GetLocalDeviceInfo()->Equals(*all_device_info[0]));
+
+  EXPECT_TRUE(processor()->metadata());
+}
+
 TEST_F(DeviceInfoServiceTest, GetClientTagNormal) {
   InitializeService();
   const std::string guid = "abc";
   EntitySpecifics entity_specifics;
   entity_specifics.mutable_device_info()->set_cache_guid(guid);
   EntityData entity_data;
   entity_data.specifics = entity_specifics;
   EXPECT_EQ(CacheGuidToTag(guid), service()->GetClientTag(entity_data));
 }
 
@@ skipping 88 matching lines @@
 
 TEST_F(DeviceInfoServiceTest, GetDataMissing) {
   InitializeAndPump();
   std::map<std::string, DeviceInfoSpecifics> expected;
   ClientTagList client_tags;
   client_tags.push_back(CacheGuidToTag("tag1"));
   service()->GetData(client_tags,
                      base::Bind(&AssertExpectedFromDataBatch, expected));
 }
 
-TEST_F(DeviceInfoServiceTest, GetDataNotInitialized) {
-  InitializeService();
-  ClientTagList client_tags;
-  service()->GetData(client_tags, base::Bind(&AssertErrorFromDataBatch));
-}
-
 TEST_F(DeviceInfoServiceTest, GetAllData) {
   std::unique_ptr<WriteBatch> batch = store()->CreateWriteBatch();
   DeviceInfoSpecifics specifics1(GenerateTestSpecifics());
   DeviceInfoSpecifics specifics2(GenerateTestSpecifics());
   const std::string& tag1 = CacheGuidToTag(specifics1.cache_guid());
   const std::string& tag2 = CacheGuidToTag(specifics2.cache_guid());
   store()->WriteData(batch.get(), specifics1.cache_guid(),
                      specifics1.SerializeAsString());
   store()->WriteData(batch.get(), specifics2.cache_guid(),
                      specifics2.SerializeAsString());
   store()->CommitWriteBatch(std::move(batch),
                             base::Bind(&AssertResultIsSuccess));
 
   InitializeAndPump();
 
   std::map<std::string, DeviceInfoSpecifics> expected;
   expected[tag1] = specifics1;
   expected[tag2] = specifics2;
   ClientTagList client_tags;
   client_tags.push_back(tag1);
   client_tags.push_back(tag2);
   service()->GetData(client_tags,
                      base::Bind(&AssertExpectedFromDataBatch, expected));
 }
 
-TEST_F(DeviceInfoServiceTest, GetAllDataNotInitialized) {
-  InitializeService();
-  service()->GetAllData(base::Bind(&AssertErrorFromDataBatch));
-}
-
-TEST_F(DeviceInfoServiceTest, ApplySyncChangesBeforeInit) {
-  InitializeService();
-  const SyncError error = service()->ApplySyncChanges(
-      service()->CreateMetadataChangeList(), EntityChangeList());
-  EXPECT_TRUE(error.IsSet());
-  EXPECT_EQ(0, change_count());
-}
-
 TEST_F(DeviceInfoServiceTest, ApplySyncChangesEmpty) {
   InitializeAndPump();
   const SyncError error = service()->ApplySyncChanges(
       service()->CreateMetadataChangeList(), EntityChangeList());
   EXPECT_FALSE(error.IsSet());
   EXPECT_EQ(0, change_count());
 }
 
 TEST_F(DeviceInfoServiceTest, ApplySyncChangesInMemory) {
   InitializeAndPump();
@@ skipping 94 matching lines @@
   InitializeAndPumpAndStart();
   EXPECT_EQ(1, change_count());
   EntityChangeList delete_changes;
   delete_changes.push_back(EntityChange::CreateDelete(CacheGuidToTag("tag")));
   const SyncError error = service()->ApplySyncChanges(
       service()->CreateMetadataChangeList(), delete_changes);
   EXPECT_FALSE(error.IsSet());
   EXPECT_EQ(1, change_count());
 }
 
-TEST_F(DeviceInfoServiceTest, MergeWithoutProcessor) {
-  InitializeService();
-  const SyncError error = service()->MergeSyncData(
-      service()->CreateMetadataChangeList(), EntityDataMap());
-  EXPECT_TRUE(error.IsSet());
-  EXPECT_EQ(0, change_count());
-}
-
 TEST_F(DeviceInfoServiceTest, MergeEmpty) {
   InitializeAndPumpAndStart();
   EXPECT_EQ(1, change_count());
   const SyncError error = service()->MergeSyncData(
       service()->CreateMetadataChangeList(), EntityDataMap());
   EXPECT_FALSE(error.IsSet());
   EXPECT_EQ(1, change_count());
   EXPECT_EQ(1u, processor()->put_map().size());
   EXPECT_EQ(0u, processor()->delete_set().size());
 }
@@ skipping 110 matching lines @@
   specifics.set_cache_guid("non-local");
   PushBackEntityChangeAdd(specifics, &change_list);
   service()->ApplySyncChanges(service()->CreateMetadataChangeList(),
                               change_list);
   EXPECT_EQ(1, service()->CountActiveDevices());
 }
 
 }  // namespace
 
 }  // namespace sync_driver_v2