[DeviceInfo USS] Fix the bridge leaking metadata orphans

This CL makes the device info USS bridge more robust in handling
ApplySyncChanges(). Previously, in a shutdown race condition, the bridge
could receive such calls when the device info is not available any more.
This could lead to the bridge not acting on the ApplySyncChanges() call,
not deleting any metadata.

This CL caches the current guid (which is stable between OnSyncStarting
calls) in the bridge.

Bug: 871733
Change-Id: I18d0bdcbcd9677e21196ee986a6af40a5abb29d9
Reviewed-on: https://chromium-review.googlesource.com/c/1273042
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610548}

components/sync/device_info/device_info_sync_bridge.cc

@@ -120,11 +120,9 @@ base::Optional<ModelError> DeviceInfoSyncBridge::MergeSyncData(
   DCHECK(change_processor()->IsTrackingMetadata());
   const DeviceInfo* local_info =
       local_device_info_provider_->GetLocalDeviceInfo();
-  // If our dependency was yanked out from beneath us, we cannot correctly
-  // handle this request, and all our data will be deleted soon.
-  if (local_info == nullptr) {
-    return {};
-  }
+  // DEVICE_INFO sync is running; DeviceInfo thus exists (it gets cleared only
+  // synchronously with disabling DEVICE_INFO sync).
+  DCHECK(local_info);
 
   // Local data should typically be near empty, with the only possible value
   // corresponding to this device. This is because on signout all device info
@@ -173,11 +171,9 @@ base::Optional<ModelError> DeviceInfoSyncBridge::ApplySyncChanges(
   DCHECK(has_provider_initialized_);
   const DeviceInfo* local_info =
       local_device_info_provider_->GetLocalDeviceInfo();
-  // If our dependency was yanked out from beneath us, we cannot correctly
-  // handle this request, and all our data will be deleted soon.
-  if (local_info == nullptr) {
-    return {};
-  }
+  // DEVICE_INFO sync is running; DeviceInfo thus exists (it gets cleared only
+  // synchronously with disabling DEVICE_INFO sync).
+  DCHECK(local_info);
 
   std::unique_ptr<WriteBatch> batch = store_->CreateWriteBatch();
   bool has_changes = false;

components/sync/device_info/device_info_sync_bridge_unittest.cc

@@ -380,16 +380,6 @@ TEST_F(DeviceInfoSyncBridgeTest, LocalProviderInitRace) {
   EXPECT_TRUE(local_device()->GetLocalDeviceInfo()->Equals(*devices[0]));
 }
 
-// Simulate shutting down sync during the ModelTypeStore callbacks. The pulse
-// timer should still be initialized, even though reconcile never occurs.
-TEST_F(DeviceInfoSyncBridgeTest, ClearProviderDuringInit) {
-  InitializeBridge();
-  local_device()->Clear();
-  base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(0u, bridge()->GetAllDeviceInfo().size());
-  EXPECT_TRUE(bridge()->IsPulseTimerRunningForTest());
-}
-
 TEST_F(DeviceInfoSyncBridgeTest, GetClientTagNormal) {
   InitializeBridge();
   const std::string guid = "abc";
@@ -584,26 +574,6 @@ TEST_F(DeviceInfoSyncBridgeTest, ApplyDeleteNonexistent) {
   EXPECT_EQ(1, change_count());
 }
 
-TEST_F(DeviceInfoSyncBridgeTest, ClearProviderAndApply) {
-  // This will initialize the provider a first time.
-  InitializeAndPump();
-  EXPECT_EQ(1u, bridge()->GetAllDeviceInfo().size());
-
-  const DeviceInfoSpecifics specifics = CreateSpecifics(1);
-
-  local_device()->Clear();
-  auto error1 = bridge()->ApplySyncChanges(bridge()->CreateMetadataChangeList(),
-                                           EntityAddList({specifics}));
-  EXPECT_FALSE(error1);
-  EXPECT_EQ(1u, bridge()->GetAllDeviceInfo().size());
-
-  local_device()->Initialize(CreateModel(kDefaultLocalSuffix));
-  auto error2 = bridge()->ApplySyncChanges(bridge()->CreateMetadataChangeList(),
-                                           EntityAddList({specifics}));
-  EXPECT_FALSE(error2);
-  EXPECT_EQ(2u, bridge()->GetAllDeviceInfo().size());
-}
-
 TEST_F(DeviceInfoSyncBridgeTest, MergeEmpty) {
   InitializeAndPump();
 
@@ -700,26 +670,6 @@ TEST_F(DeviceInfoSyncBridgeTest, MergeLocalGuidBeforeReconcile) {
   EXPECT_EQ(0u, bridge()->GetAllDeviceInfo().size());
 }
 
-TEST_F(DeviceInfoSyncBridgeTest, ClearProviderAndMerge) {
-  // This will initialize the provider a first time.
-  InitializeAndPump();
-  EXPECT_EQ(1u, bridge()->GetAllDeviceInfo().size());
-
-  const DeviceInfoSpecifics specifics = CreateSpecifics(1);
-
-  local_device()->Clear();
-  auto error1 = bridge()->MergeSyncData(bridge()->CreateMetadataChangeList(),
-                                        EntityAddList({specifics}));
-  EXPECT_FALSE(error1);
-  EXPECT_EQ(1u, bridge()->GetAllDeviceInfo().size());
-
-  local_device()->Initialize(CreateModel(kDefaultLocalSuffix));
-  auto error2 = bridge()->MergeSyncData(bridge()->CreateMetadataChangeList(),
-                                        EntityAddList({specifics}));
-  EXPECT_FALSE(error2);
-  EXPECT_EQ(2u, bridge()->GetAllDeviceInfo().size());
-}
-
 TEST_F(DeviceInfoSyncBridgeTest, CountActiveDevices) {
   InitializeAndPump();
   EXPECT_EQ(1, bridge()->CountActiveDevices());
@@ -776,13 +726,6 @@ TEST_F(DeviceInfoSyncBridgeTest, SendLocalData) {
   EXPECT_CALL(*processor(), Put(_, HasSpecifics(HasLastUpdatedAboutNow()), _));
   ForcePulse();
   EXPECT_EQ(2, change_count());
-
-  // After clearing, pulsing should no-op and not result in a processor put or
-  // a notification to observers.
-  EXPECT_CALL(*processor(), Put(_, _, _)).Times(0);
-  local_device()->Clear();
-  ForcePulse();
-  EXPECT_EQ(2, change_count());
 }
 
 TEST_F(DeviceInfoSyncBridgeTest, ApplyStopSyncChanges) {