[Sync] Actively guard against provider being cleared in DeviceInfoSyncBridge.

When a user signs out and Sync starts to shut down, we immediately
clear the LocalDeviceInfoProvider. Many methods in DeviceInfoSyncBridge
depend on being able to access information in the provider, and can
crash at this point. The bridge should be disabled momentarily, but
this will require threads hops UI->Sync->UI. Meanwhile, various other
posted tasks can call into the bridge and hit this race condition.

We were previously guarding against this scenario when the pulse timer
would fire, but we were not guarding against processor's Merge/Apply
calls or ModelTypeStore async initialization. I've added nullptr checks
to all async entry points that mostly no-op when they detect this
scenario.

One slight hiccup is that the bridge currently leaves the pulse timer
running even when disabled. My intention is that this change will be
merged back into M56 so I wanted to keep this CL as small as possible.
When crbug.com/672600 is addressed I think it will make the most sense
to disable the pulse timer while disabled, since it should always be
re-enabled with an accurate timer when things are working again.

Additionally, some state tracking such as has_provider_initialized_
should probably just be removed at this point. This should be caught in
crbug.com/659263

BUG=672534

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

components/sync/device_info/device_info_sync_bridge.cc

@@ -118,6 +118,13 @@ SyncError DeviceInfoSyncBridge::MergeSyncData(
     EntityDataMap entity_data_map) {
   DCHECK(has_provider_initialized_);
   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 SyncError();
+  }
 
   // Local data should typically be near empty, with the only possible value
   // corresponding to this device. This is because on signout all device info
@@ -130,8 +137,6 @@ SyncError DeviceInfoSyncBridge::MergeSyncData(
   }
 
   bool has_changes = false;
-  const DeviceInfo* local_info =
-      local_device_info_provider_->GetLocalDeviceInfo();
   std::string local_guid = local_info->guid();
   std::unique_ptr<WriteBatch> batch = store_->CreateWriteBatch();
   for (const auto& kv : entity_data_map) {
@@ -166,6 +171,13 @@ SyncError DeviceInfoSyncBridge::ApplySyncChanges(
     std::unique_ptr<MetadataChangeList> metadata_change_list,
     EntityChangeList entity_changes) {
   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 SyncError();
+  }
 
   std::unique_ptr<WriteBatch> batch = store_->CreateWriteBatch();
   bool has_changes = false;
@@ -173,7 +185,7 @@ SyncError DeviceInfoSyncBridge::ApplySyncChanges(
     const std::string guid = change.storage_key();
     // Each device is the authoritative source for itself, ignore any remote
     // changes that have our local cache guid.
-    if (guid == local_device_info_provider_->GetLocalDeviceInfo()->guid()) {
+    if (guid == local_info->guid()) {
       continue;
     }
 
@@ -315,6 +327,8 @@ void DeviceInfoSyncBridge::OnProviderInitialized() {
   // should only need to give the processor metadata upon initialization. If
   // sync is disabled and enabled, our provider will try to retrigger this
   // event, but we do not want to send any more metadata to the processor.
+  // TODO(skym, crbug.com/672600): Handle re-initialization and start the pulse
+  // timer.
   subscription_.reset();
 
   has_provider_initialized_ = true;
@@ -387,6 +401,15 @@ void DeviceInfoSyncBridge::ReconcileLocalAndStored() {
 
   const DeviceInfo* current_info =
       local_device_info_provider_->GetLocalDeviceInfo();
+  // Must ensure |pulse_timer_| is started even if sync is in the process of
+  // being disabled. TODO(skym, crbug.com/672600): Remove this timer Start(), as
+  // it should be started when the provider re-initializes instead.
+  if (current_info == nullptr) {
+    pulse_timer_.Start(FROM_HERE, DeviceInfoUtil::kPulseInterval,
+                       base::Bind(&DeviceInfoSyncBridge::SendLocalData,
+                                  base::Unretained(this)));
+    return;
+  }
   auto iter = all_data_.find(current_info->guid());
 
   // Convert to DeviceInfo for Equals function.

components/sync/device_info/device_info_sync_bridge_unittest.cc

@@ -26,6 +26,7 @@
 
 namespace syncer {
 
+using base::OneShotTimer;
 using base::Time;
 using base::TimeDelta;
 using sync_pb::DeviceInfoSpecifics;
@@ -287,6 +288,8 @@ class DeviceInfoSyncBridgeTest : public testing::Test,
     return processor_;
   }
 
+  const OneShotTimer& pulse_timer() { return bridge()->pulse_timer_; }
+
   // Should only be called after the bridge has been initialized. Will first
   // recover the bridge's store, so another can be initialized later, and then
   // deletes the bridge.
@@ -398,6 +401,16 @@ TEST_F(DeviceInfoSyncBridgeTest, LocalProviderInitRace) {
   EXPECT_TRUE(processor()->metadata());
 }
 
+// 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(pulse_timer().IsRunning());
+}
+
 TEST_F(DeviceInfoSyncBridgeTest, GetClientTagNormal) {
   InitializeBridge();
   const std::string guid = "abc";
@@ -587,6 +600,25 @@ 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, Time::Now());
+
+  local_device()->Clear();
+  SyncError error = bridge()->ApplySyncChanges(
+      bridge()->CreateMetadataChangeList(), EntityAddList({specifics}));
+  EXPECT_FALSE(error.IsSet());
+  EXPECT_EQ(1u, bridge()->GetAllDeviceInfo().size());
+
+  local_device()->Initialize(CreateModel(kDefaultLocalSuffix));
+  error = bridge()->ApplySyncChanges(bridge()->CreateMetadataChangeList(),
+                                     EntityAddList({specifics}));
+  EXPECT_EQ(2u, bridge()->GetAllDeviceInfo().size());
+}
+
 TEST_F(DeviceInfoSyncBridgeTest, MergeEmpty) {
   InitializeAndPump();
   EXPECT_EQ(1, change_count());
@@ -680,6 +712,24 @@ 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, Time::Now());
+
+  local_device()->Clear();
+  SyncError error = bridge()->MergeSyncData(
+      bridge()->CreateMetadataChangeList(), InlineEntityDataMap({specifics}));
+  EXPECT_FALSE(error.IsSet());
+  EXPECT_EQ(1u, bridge()->GetAllDeviceInfo().size());
+  local_device()->Initialize(CreateModel(kDefaultLocalSuffix));
+  error = bridge()->MergeSyncData(bridge()->CreateMetadataChangeList(),
+                                  InlineEntityDataMap({specifics}));
+  EXPECT_EQ(2u, bridge()->GetAllDeviceInfo().size());
+}
+
 TEST_F(DeviceInfoSyncBridgeTest, CountActiveDevices) {
   InitializeAndPump();
   EXPECT_EQ(1, bridge()->CountActiveDevices());