[DeviceSync v2] Fix local device metadata race condition

Do not assume the local device has an Instance ID in the DeviceSync
client if v2 DeviceSync is enabled. The local device will always have
an Instance ID with CryptAuth, but there might be a race condition
while v1 and v2 DeviceSync are running in parallel; the v1 local
device data--which does not contain an Instance ID--might be returned
before the v2 local device data.

Bug: 951969, 1019206
Change-Id: I46882202d99cde87efc60cdd60628053a96c74f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2078749
Commit-Queue: James Vecore <vecore@google.com>
Reviewed-by: James Vecore <vecore@google.com>
Auto-Submit: Josh Nohle <nohle@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746427}

chromeos/services/device_sync/public/cpp/device_sync_client_impl.cc

@@ -259,12 +259,18 @@ void DeviceSyncClientImpl::OnGetLocalDeviceMetadataCompleted(
     return;
   }
 
-  if (features::ShouldUseV2DeviceSync()) {
+  if (features::ShouldUseV1DeviceSync()) {
+    local_instance_id_ = local_device_metadata->instance_id.empty()
+                             ? base::nullopt
+                             : base::make_optional<std::string>(
+                                   local_device_metadata->instance_id);
+    local_legacy_device_id_ = local_device_metadata->GetDeviceId().empty()
+                                  ? base::nullopt
+                                  : base::make_optional<std::string>(
+                                        local_device_metadata->GetDeviceId());
+  } else {
     DCHECK(!local_device_metadata->instance_id.empty());
     local_instance_id_ = local_device_metadata->instance_id;
-  } else {
-    DCHECK(!local_device_metadata->GetDeviceId().empty());
-    local_legacy_device_id_ = local_device_metadata->GetDeviceId();
   }
 
   expiring_device_cache_->UpdateRemoteDevice(*local_device_metadata);

chromeos/services/device_sync/public/cpp/device_sync_client_impl.h

@@ -125,9 +125,13 @@ class DeviceSyncClientImpl : public DeviceSyncClient,
 
   base::Optional<std::string> local_instance_id_;
 
-  // TODO(https://crbug.com/951969): Only track local Instance ID after v2
-  // DeviceSync is launched, when the local device is guaranteed to have an
-  // Instance ID.
+  // TODO(https://crbug.com/1019206): Track only the local Instance ID after v1
+  // DeviceSync is disabled, when the local device is guaranteed to have an
+  // Instance ID. Note: When v1 and v2 DeviceSync are running in parallel, if we
+  // are still waiting for the first v2 DeviceSync to successfully complete, it
+  // is possible that only v1 device data--which does not contain Instance
+  // IDs--is loaded by the RemoteDeviceProvider. In that case, the local device
+  // will not have an Instance ID until the very first v2 DeviceSync succeeds.
   base::Optional<std::string> local_legacy_device_id_;
 
   base::WeakPtrFactory<DeviceSyncClientImpl> weak_ptr_factory_{this};