[CrOS Multidevice] Fix crash in Tether.

This issue manifested as all new account creations on Chrome OS crashing.
It was caused by the local device metadata being returned by DeviceSyncClient
not yet being initialized.

DeviceSyncClient now internally keeps track of whether it is "ready", that is,
whether its local device metadata and synced devices are available yet. Only
once it has loaded both sets of data will it mark itself as "ready", and inform
clients appropriately. This change also ensures that DeviceSyncClient's Observer
callbacks, OnEnrollmentFinished() and OnNewDevicesSynced(), are not called until
DeviceSyncClient is ready.

R=jhawkins@chromium.org, khorimoto@chromium.org

Bug: 856406
Change-Id: Idb6fdf123d4d90883f67bb1e88a617004926207e
Reviewed-on: https://chromium-review.googlesource.com/1121351Reviewed-by: James Hawkins <jhawkins@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572309}

chrome/browser/chromeos/login/easy_unlock/easy_unlock_service_regular.cc

@@ -130,16 +130,10 @@ EasyUnlockServiceRegular::EasyUnlockServiceRegular(
       shown_pairing_changed_notification_(false),
       weak_ptr_factory_(this) {
   if (base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi)) {
-    // If this is the first GetSyncedDevices() call on DeviceSyncClient, it
-    // won't have devices cached yet. |is_waiting_for_initial_sync_| is flipped
-    // to indicate that we are waiting for this initial sync, to be inspected
-    // in OnNewDevicesSynced(). OnNewDevicesSynced() needs to know that it is
-    // receiving the initial sync, not a newly forced one, in order to prevent
-    // it from running unrelated logic.
-    if (device_sync_client_->GetSyncedDevices().empty())
-      is_waiting_for_initial_sync_ = true;
-    else
-      remote_device_unlock_keys_before_sync_ = GetUnlockKeys();
+    // If |device_sync_client_| is not ready yet, wait for it to call back on
+    // OnReady().
+    if (device_sync_client_->is_ready())
+      OnReady();
 
     device_sync_client_->AddObserver(this);
   }
@@ -157,11 +151,11 @@ EasyUnlockServiceRegular::~EasyUnlockServiceRegular() {
 // explicit.
 void EasyUnlockServiceRegular::LoadRemoteDevices() {
   if (base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi) &&
-      !device_sync_client_->GetLocalDeviceMetadata()) {
-    // OnEnrollmentFinished() will call back on this method once the local
-    // device is ready.
-    PA_LOG(INFO)
-        << "Local device is not ready yet, delaying UseLoadedRemoteDevices().";
+      !device_sync_client_->is_ready()) {
+    // OnEnrollmentFinished() or OnNewDevicesSynced() will call back on this
+    // method once |device_sync_client_| is ready.
+    PA_LOG(INFO) << "DeviceSyncClient is not ready yet, delaying "
+                    "UseLoadedRemoteDevices().";
     return;
   }
 
@@ -622,9 +616,16 @@ void EasyUnlockServiceRegular::OnSyncFinished(
   LoadRemoteDevices();
 }
 
+void EasyUnlockServiceRegular::OnReady() {
+  // If the local device and synced devices are ready for the first time,
+  // establish what the unlock keys were before the next sync. This is necessary
+  // in order for OnNewDevicesSynced() to determine if new devices were added
+  // since the last sync.
+  remote_device_unlock_keys_before_sync_ = GetUnlockKeys();
+}
+
 void EasyUnlockServiceRegular::OnEnrollmentFinished() {
-  // If the local device is ready for the first time, or has been updated,
-  // reload devices.
+  // If the local device has been updated, reload devices.
   LoadRemoteDevices();
 }
 
@@ -635,12 +636,6 @@ void EasyUnlockServiceRegular::OnNewDevicesSynced() {
   DCHECK(base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi));
 
   std::set<std::string> public_keys_before_sync;
-  if (is_waiting_for_initial_sync_) {
-    is_waiting_for_initial_sync_ = false;
-
-    // Without this, |public_keys_before_sync| would be incorrectly empty.
-    remote_device_unlock_keys_before_sync_ = GetUnlockKeys();
-  }
   for (const auto& remote_device : remote_device_unlock_keys_before_sync_) {
     public_keys_before_sync.insert(remote_device.public_key());
   }

chrome/browser/chromeos/login/easy_unlock/easy_unlock_service_regular.h

@@ -115,6 +115,7 @@ class EasyUnlockServiceRegular
                           device_change_result) override;
 
   // device_sync::DeviceSyncClient::Observer:
+  void OnReady() override;
   void OnEnrollmentFinished() override;
   void OnNewDevicesSynced() override;
 
@@ -220,14 +221,6 @@ class EasyUnlockServiceRegular
   // notification.
   bool shown_pairing_changed_notification_;
 
-  // If this service is the first caller on DeviceSyncClient, it won't have
-  // devices cached yet. |is_waiting_for_initial_sync_| is set to true if
-  // DeviceSyncClient has no devices, to indicate that we are waiting for the
-  // initial sync, to be inspected in OnNewDevicesSynced(). OnNewDevicesSynced()
-  // needs to know that it is receiving the initial sync, not a newly forced
-  // one, in order to prevent it from running unrelated logic.
-  bool is_waiting_for_initial_sync_ = false;
-
   // Listens to pref changes.
   PrefChangeRegistrar registrar_;
 

chrome/browser/chromeos/tether/tether_service.cc

@@ -26,7 +26,6 @@
 #include "chromeos/network/device_state.h"
 #include "chromeos/network/network_connect.h"
 #include "chromeos/network/network_type_pattern.h"
-#include "chromeos/services/device_sync/public/cpp/device_sync_client.h"
 #include "chromeos/services/secure_channel/public/cpp/client/secure_channel_client.h"
 #include "components/cryptauth/cryptauth_enrollment_manager.h"
 #include "components/cryptauth/cryptauth_service.h"
@@ -163,6 +162,8 @@ TetherService::TetherService(
   tether_host_fetcher_->AddObserver(this);
   power_manager_client_->AddObserver(this);
   network_state_handler_->AddObserver(this, FROM_HERE);
+  if (base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi))
+    device_sync_client_->AddObserver(this);
 
   registrar_.Init(profile_->GetPrefs());
   registrar_.Add(prefs::kInstantTetheringAllowed,
@@ -174,6 +175,12 @@ TetherService::TetherService(
   PA_LOG(INFO) << "TetherService has started. Initial user preference value: "
                << IsEnabledbyPreference();
 
+  if (base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi) &&
+      !device_sync_client_->is_ready()) {
+    // Wait for OnReady() to be called. It will call GetAdapter().
+    return;
+  }
+
   // GetAdapter may call OnBluetoothAdapterFetched immediately which can cause
   // problems with the Fake implementation since the class is not fully
   // constructed yet. Post the GetAdapter call to avoid this.
@@ -276,6 +283,8 @@ void TetherService::Shutdown() {
   tether_host_fetcher_->RemoveObserver(this);
   power_manager_client_->RemoveObserver(this);
   network_state_handler_->RemoveObserver(this, FROM_HERE);
+  if (base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi))
+    device_sync_client_->RemoveObserver(this);
   if (adapter_)
     adapter_->RemoveObserver(this);
   registrar_.RemoveAll();
@@ -389,6 +398,15 @@ void TetherService::OnShutdownComplete() {
     StartTetherIfPossible();
 }
 
+void TetherService::OnReady() {
+  if (shut_down_)
+    return;
+
+  device::BluetoothAdapterFactory::GetAdapter(
+      base::Bind(&TetherService::OnBluetoothAdapterFetched,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
 void TetherService::OnPrefsChanged() {
   UpdateTetherTechnologyState();
 }

chrome/browser/chromeos/tether/tether_service.h

@@ -16,6 +16,7 @@
 #include "chromeos/dbus/power_manager_client.h"
 #include "chromeos/network/network_state_handler.h"
 #include "chromeos/network/network_state_handler_observer.h"
+#include "chromeos/services/device_sync/public/cpp/device_sync_client.h"
 #include "components/cryptauth/cryptauth_device_manager.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/prefs/pref_change_registrar.h"
@@ -25,9 +26,6 @@ class Profile;
 
 namespace chromeos {
 class NetworkStateHandler;
-namespace device_sync {
-class DeviceSyncClient;
-}  // namespace device_sync
 namespace secure_channel {
 class SecureChannelClient;
 }  // namespace secure_channel
@@ -62,7 +60,8 @@ class TetherService : public KeyedService,
                       public chromeos::tether::TetherHostFetcher::Observer,
                       public device::BluetoothAdapter::Observer,
                       public chromeos::NetworkStateHandlerObserver,
-                      public chromeos::tether::TetherComponent::Observer {
+                      public chromeos::tether::TetherComponent::Observer,
+                      public chromeos::device_sync::DeviceSyncClient::Observer {
  public:
   TetherService(
       Profile* profile,
@@ -116,6 +115,9 @@ class TetherService : public KeyedService,
   // chromeos::tether::TetherComponent::Observer:
   void OnShutdownComplete() override;
 
+  // chromeos::device_sync::DeviceSyncClient::Observer:
+  void OnReady() override;
+
   // Callback when the controlling pref changes.
   void OnPrefsChanged();
 
@@ -136,6 +138,7 @@ class TetherService : public KeyedService,
  private:
   friend class TetherServiceTest;
   FRIEND_TEST_ALL_PREFIXES(TetherServiceTest, TestSuspend);
+  FRIEND_TEST_ALL_PREFIXES(TetherServiceTest, TestDeviceSyncClientNotReady);
   FRIEND_TEST_ALL_PREFIXES(TetherServiceTest, TestBleAdvertisingNotSupported);
   FRIEND_TEST_ALL_PREFIXES(
       TetherServiceTest,

chrome/browser/chromeos/tether/tether_service_unittest.cc

@@ -261,7 +261,10 @@ class FakeDeviceSyncClientImplFactory
   // chromeos::device_sync::DeviceSyncClientImpl::Factory:
   std::unique_ptr<chromeos::device_sync::DeviceSyncClient> BuildInstance(
       service_manager::Connector* connector) override {
-    return std::make_unique<chromeos::device_sync::FakeDeviceSyncClient>();
+    auto fake_device_sync_client =
+        std::make_unique<chromeos::device_sync::FakeDeviceSyncClient>();
+    fake_device_sync_client->NotifyReady();
+    return fake_device_sync_client;
   }
 };
 
@@ -311,6 +314,8 @@ class TetherServiceTest : public chromeos::NetworkStateTest {
 
     fake_device_sync_client_ =
         std::make_unique<chromeos::device_sync::FakeDeviceSyncClient>();
+    fake_device_sync_client_->NotifyReady();
+
     fake_device_sync_client_impl_factory_ =
         std::make_unique<FakeDeviceSyncClientImplFactory>();
     chromeos::device_sync::DeviceSyncClientImpl::Factory::SetInstanceForTesting(
@@ -415,17 +420,19 @@ class TetherServiceTest : public chromeos::NetworkStateTest {
         base::WrapUnique(fake_notification_presenter_),
         base::WrapUnique(mock_timer_));
 
-    // Ensure that TetherService does not prematurely update its TechnologyState
-    // before it fetches the BluetoothAdapter.
-    EXPECT_EQ(
-        chromeos::NetworkStateHandler::TechnologyState::TECHNOLOGY_UNAVAILABLE,
-        network_state_handler()->GetTechnologyState(
-            chromeos::NetworkTypePattern::Tether()));
-    VerifyTetherActiveStatus(false /* expected_active */);
-
     SetPrimaryUserLoggedIn();
 
-    base::RunLoop().RunUntilIdle();
+    if (fake_device_sync_client_->is_ready()) {
+      // Ensure that TetherService does not prematurely update its
+      // TechnologyState before it fetches the BluetoothAdapter.
+      EXPECT_EQ(chromeos::NetworkStateHandler::TechnologyState::
+                    TECHNOLOGY_UNAVAILABLE,
+                network_state_handler()->GetTechnologyState(
+                    chromeos::NetworkTypePattern::Tether()));
+      VerifyTetherActiveStatus(false /* expected_active */);
+
+      base::RunLoop().RunUntilIdle();
+    }
   }
 
   void ShutdownTetherService() {
@@ -513,11 +520,11 @@ class TetherServiceTest : public chromeos::NetworkStateTest {
       fake_tether_host_fetcher_factory_;
   chromeos::tether::FakeNotificationPresenter* fake_notification_presenter_;
   base::MockOneShotTimer* mock_timer_;
-  std::unique_ptr<chromeos::device_sync::DeviceSyncClient>
+  std::unique_ptr<chromeos::device_sync::FakeDeviceSyncClient>
       fake_device_sync_client_;
   std::unique_ptr<FakeDeviceSyncClientImplFactory>
       fake_device_sync_client_impl_factory_;
-  std::unique_ptr<chromeos::secure_channel::SecureChannelClient>
+  std::unique_ptr<chromeos::secure_channel::FakeSecureChannelClient>
       fake_secure_channel_client_;
   std::unique_ptr<FakeSecureChannelClientImplFactory>
       fake_secure_channel_client_impl_factory_;
@@ -625,6 +632,28 @@ TEST_F(TetherServiceTest, TestSuspend) {
       chromeos::tether::TetherComponent::ShutdownReason::USER_CLOSED_LID);
 }
 
+TEST_F(TetherServiceTest, TestDeviceSyncClientNotReady) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(chromeos::features::kMultiDeviceApi);
+
+  fake_device_sync_client_ =
+      std::make_unique<chromeos::device_sync::FakeDeviceSyncClient>();
+
+  CreateTetherService();
+
+  fake_device_sync_client_->NotifyReady();
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(chromeos::NetworkStateHandler::TechnologyState::TECHNOLOGY_ENABLED,
+            network_state_handler()->GetTechnologyState(
+                chromeos::NetworkTypePattern::Tether()));
+  VerifyTetherActiveStatus(true /* expected_active */);
+
+  ShutdownTetherService();
+  VerifyLastShutdownReason(
+      chromeos::tether::TetherComponent::ShutdownReason::USER_LOGGED_OUT);
+}
+
 TEST_F(TetherServiceTest, TestBleAdvertisingNotSupported) {
   mock_adapter_->set_is_ble_advertising_supported(false);
 

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

@@ -20,6 +20,13 @@ void DeviceSyncClient::RemoveObserver(Observer* observer) {
   observer_list_.RemoveObserver(observer);
 }
 
+void DeviceSyncClient::NotifyReady() {
+  is_ready_ = true;
+
+  for (auto& observer : observer_list_)
+    observer.OnReady();
+}
+
 void DeviceSyncClient::NotifyEnrollmentFinished() {
   for (auto& observer : observer_list_)
     observer.OnEnrollmentFinished();

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

@@ -24,6 +24,13 @@ class DeviceSyncClient {
  public:
   class Observer {
    public:
+    // Called when the DeviceSyncClient is ready, i.e. local device metadata
+    // and synced devices are available.
+    virtual void OnReady() {}
+
+    // OnEnrollmentFinished() and OnNewDevicesSynced() will only be called once
+    // DeviceSyncClient is ready, i.e., OnReady() will always be the first
+    // callback called.
     virtual void OnEnrollmentFinished() {}
     virtual void OnNewDevicesSynced() {}
 
@@ -42,6 +49,11 @@ class DeviceSyncClient {
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
+  // Clients of DeviceSyncClient should ensure that this returns true before
+  // calling any methods. If false, clients should listen on and wait for the
+  // OnReady() callback.
+  bool is_ready() { return is_ready_; }
+
   virtual void ForceEnrollmentNow(
       mojom::DeviceSync::ForceEnrollmentNowCallback callback) = 0;
   virtual void ForceSyncNow(
@@ -65,10 +77,12 @@ class DeviceSyncClient {
       mojom::DeviceSync::GetDebugInfoCallback callback) = 0;
 
  protected:
+  void NotifyReady();
   void NotifyEnrollmentFinished();
   void NotifyNewDevicesSynced();
 
  private:
+  bool is_ready_ = false;
   base::ObserverList<Observer> observer_list_;
 
   DISALLOW_COPY_AND_ASSIGN(DeviceSyncClient);

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

@@ -97,11 +97,13 @@ void DeviceSyncClientImpl::ForceSyncNow(
 }
 
 cryptauth::RemoteDeviceRefList DeviceSyncClientImpl::GetSyncedDevices() {
+  DCHECK(is_ready());
   return expiring_device_cache_->GetNonExpiredRemoteDevices();
 }
 
 base::Optional<cryptauth::RemoteDeviceRef>
 DeviceSyncClientImpl::GetLocalDeviceMetadata() {
+  DCHECK(is_ready());
   return local_device_id_
              ? expiring_device_cache_->GetRemoteDevice(*local_device_id_)
              : base::nullopt;
@@ -131,6 +133,25 @@ void DeviceSyncClientImpl::GetDebugInfo(
   device_sync_ptr_->GetDebugInfo(std::move(callback));
 }
 
+void DeviceSyncClientImpl::AttemptToBecomeReady() {
+  if (is_ready())
+    return;
+
+  if (waiting_for_synced_devices_ || waiting_for_local_device_metadata_)
+    return;
+
+  NotifyReady();
+
+  if (pending_notify_enrollment_finished_)
+    NotifyEnrollmentFinished();
+
+  if (pending_notify_new_synced_devices_)
+    NotifyNewDevicesSynced();
+
+  pending_notify_enrollment_finished_ = false;
+  pending_notify_new_synced_devices_ = false;
+}
+
 void DeviceSyncClientImpl::LoadSyncedDevices() {
   device_sync_ptr_->GetSyncedDevices(
       base::BindOnce(&DeviceSyncClientImpl::OnGetSyncedDevicesCompleted,
@@ -153,15 +174,22 @@ void DeviceSyncClientImpl::OnGetSyncedDevicesCompleted(
     return;
   }
 
-  if (waiting_for_local_device_metadata_) {
-    waiting_for_local_device_metadata_ = false;
+  waiting_for_synced_devices_ = false;
+
+  if (waiting_for_local_device_metadata_)
     LoadLocalDeviceMetadata();
-  }
 
   expiring_device_cache_->SetRemoteDevicesAndInvalidateOldEntries(
       *remote_devices);
 
-  NotifyNewDevicesSynced();
+  // Don't yet notify observers that new devices have synced until the client
+  // is ready.
+  if (is_ready()) {
+    NotifyNewDevicesSynced();
+  } else {
+    pending_notify_new_synced_devices_ = true;
+    AttemptToBecomeReady();
+  }
 }
 
 void DeviceSyncClientImpl::OnGetLocalDeviceMetadataCompleted(
@@ -170,14 +198,22 @@ void DeviceSyncClientImpl::OnGetLocalDeviceMetadataCompleted(
     PA_LOG(INFO) << "Tried to get local device metadata before service was "
                     "fully initialized; waiting for enrollment to complete "
                     "before continuing.";
-    waiting_for_local_device_metadata_ = true;
     return;
   }
 
   local_device_id_ = local_device_metadata->GetDeviceId();
   expiring_device_cache_->UpdateRemoteDevice(*local_device_metadata);
 
-  NotifyEnrollmentFinished();
+  waiting_for_local_device_metadata_ = false;
+
+  // Don't yet notify observers that enrollment has finished until the client
+  // is ready.
+  if (is_ready()) {
+    NotifyEnrollmentFinished();
+  } else {
+    pending_notify_enrollment_finished_ = true;
+    AttemptToBecomeReady();
+  }
 }
 
 void DeviceSyncClientImpl::OnFindEligibleDevicesCompleted(

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

@@ -80,6 +80,8 @@ class DeviceSyncClientImpl : public DeviceSyncClient,
   DeviceSyncClientImpl(service_manager::Connector* connector,
                        scoped_refptr<base::TaskRunner> task_runner);
 
+  void AttemptToBecomeReady();
+
   void LoadSyncedDevices();
   void LoadLocalDeviceMetadata();
 
@@ -101,7 +103,12 @@ class DeviceSyncClientImpl : public DeviceSyncClient,
   mojo::Binding<mojom::DeviceSyncObserver> binding_;
   std::unique_ptr<cryptauth::ExpiringRemoteDeviceCache> expiring_device_cache_;
 
-  bool waiting_for_local_device_metadata_ = false;
+  bool waiting_for_synced_devices_ = true;
+  bool waiting_for_local_device_metadata_ = true;
+
+  bool pending_notify_enrollment_finished_ = false;
+  bool pending_notify_new_synced_devices_ = false;
+
   base::Optional<std::string> local_device_id_;
 
   base::WeakPtrFactory<DeviceSyncClientImpl> weak_ptr_factory_;

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

@@ -104,7 +104,20 @@ class TestDeviceSyncClientObserver : public DeviceSyncClient::Observer {
     closure_for_new_devices_synced_ = std::move(closure);
   }
 
+  void OnReady() override {
+    if (ready_count_ == 0u) {
+      // Ensure that OnReady() was called before the other callbacks.
+      EXPECT_FALSE(enrollment_finished_count_);
+      EXPECT_FALSE(new_devices_synced_count_);
+    }
+
+    ++ready_count_;
+  }
+
   void OnEnrollmentFinished() override {
+    // Ensure that OnReady() was called before the other callbacks.
+    EXPECT_TRUE(ready_count_);
+
     ++enrollment_finished_count_;
 
     EXPECT_TRUE(closure_for_enrollment_finished_);
@@ -112,16 +125,21 @@ class TestDeviceSyncClientObserver : public DeviceSyncClient::Observer {
   }
 
   void OnNewDevicesSynced() override {
+    // Ensure that OnReady() was called before the other callbacks.
+    EXPECT_TRUE(ready_count_);
+
     ++new_devices_synced_count_;
 
     EXPECT_TRUE(closure_for_new_devices_synced_);
     std::move(closure_for_new_devices_synced_).Run();
   }
 
+  size_t ready_count() { return ready_count_; }
   size_t enrollment_finished_count() { return enrollment_finished_count_; }
   size_t new_devices_synced_count() { return new_devices_synced_count_; }
 
  private:
+  size_t ready_count_ = 0u;
   size_t enrollment_finished_count_ = 0u;
   size_t new_devices_synced_count_ = 0u;
 
@@ -198,69 +216,97 @@ class DeviceSyncClientImplTest : public testing::Test {
   }
 
   void InvokeInitialGetLocalMetadataAndThenSync() {
-    EXPECT_FALSE(client_->GetLocalDeviceMetadata());
+    EXPECT_FALSE(client_->is_ready());
+    EXPECT_EQ(0u, test_observer_->ready_count());
     EXPECT_EQ(0u, test_observer_->enrollment_finished_count());
+    EXPECT_EQ(0u, test_observer_->new_devices_synced_count());
 
-    base::RunLoop enrollment_run_loop;
-    test_observer_->set_closure_for_enrollment_finished(
-        enrollment_run_loop.QuitClosure());
     fake_device_sync_->InvokePendingGetLocalDeviceMetadataCallback(
         test_remote_device_list_[0]);
-    enrollment_run_loop.Run();
-
-    // In the case where enrollment finishes before sync, the local device
-    // metadata must still be accessible.
-    EXPECT_EQ(test_remote_device_list_[0].public_key,
-              client_->GetLocalDeviceMetadata()->public_key());
-    EXPECT_EQ(1u, test_observer_->enrollment_finished_count());
 
-    EXPECT_EQ(1u, client_->GetSyncedDevices().size());
+    // Ensure that no Observer callbacks are called until both the local device
+    // metadata and the remote devices are supplied.
+    EXPECT_FALSE(client_->is_ready());
+    EXPECT_EQ(0u, test_observer_->ready_count());
+    EXPECT_EQ(0u, test_observer_->enrollment_finished_count());
     EXPECT_EQ(0u, test_observer_->new_devices_synced_count());
 
-    base::RunLoop sync_run_loop;
-    test_observer_->set_closure_for_new_devices_synced(
-        sync_run_loop.QuitClosure());
+    base::RunLoop run_loop;
+
     fake_device_sync_->InvokePendingGetSyncedDevicesCallback(
         test_remote_device_list_);
-    sync_run_loop.Run();
 
+    test_observer_->set_closure_for_enrollment_finished(
+        run_loop.QuitWhenIdleClosure());
+    test_observer_->set_closure_for_new_devices_synced(
+        run_loop.QuitWhenIdleClosure());
+
+    run_loop.Run();
+
+    EXPECT_TRUE(client_->is_ready());
+    EXPECT_EQ(1u, test_observer_->ready_count());
+    EXPECT_EQ(test_remote_device_list_[0].public_key,
+              client_->GetLocalDeviceMetadata()->public_key());
+    EXPECT_EQ(1u, test_observer_->enrollment_finished_count());
     VerifyRemoteDeviceRefListAndRemoteDeviceListAreEqual(
         client_->GetSyncedDevices(), test_remote_device_list_);
     EXPECT_EQ(1u, test_observer_->new_devices_synced_count());
   }
 
   void InvokeInitialSyncAndThenGetLocalMetadata() {
-    EXPECT_EQ(0u, client_->GetSyncedDevices().size());
+    EXPECT_FALSE(client_->is_ready());
+    EXPECT_EQ(0u, test_observer_->ready_count());
+    EXPECT_EQ(0u, test_observer_->enrollment_finished_count());
     EXPECT_EQ(0u, test_observer_->new_devices_synced_count());
 
-    base::RunLoop sync_run_loop;
-    test_observer_->set_closure_for_new_devices_synced(
-        sync_run_loop.QuitClosure());
+    // Since local device metadata has not yet been supplied at this point,
+    // |client_| will queue up another call to fetch it. The callback is handled
+    // at the end of this method.
     fake_device_sync_->InvokePendingGetSyncedDevicesCallback(
         test_remote_device_list_);
-    sync_run_loop.Run();
 
+    // Ensure that no Observer callbacks are called until both the local device
+    // metadata and the remote devices are supplied.
+    EXPECT_FALSE(client_->is_ready());
+    EXPECT_EQ(0u, test_observer_->ready_count());
+    EXPECT_EQ(0u, test_observer_->enrollment_finished_count());
+    EXPECT_EQ(0u, test_observer_->new_devices_synced_count());
+
+    base::RunLoop run_loop;
+
+    fake_device_sync_->InvokePendingGetLocalDeviceMetadataCallback(
+        test_remote_device_list_[0]);
+
+    test_observer_->set_closure_for_new_devices_synced(
+        run_loop.QuitWhenIdleClosure());
+    test_observer_->set_closure_for_enrollment_finished(
+        run_loop.QuitWhenIdleClosure());
+
+    run_loop.Run();
+
+    EXPECT_TRUE(client_->is_ready());
+    EXPECT_EQ(1u, test_observer_->ready_count());
+    EXPECT_EQ(test_remote_device_list_[0].public_key,
+              client_->GetLocalDeviceMetadata()->public_key());
+    EXPECT_EQ(1u, test_observer_->enrollment_finished_count());
     VerifyRemoteDeviceRefListAndRemoteDeviceListAreEqual(
         client_->GetSyncedDevices(), test_remote_device_list_);
     EXPECT_EQ(1u, test_observer_->new_devices_synced_count());
 
-    EXPECT_FALSE(client_->GetLocalDeviceMetadata());
-    EXPECT_EQ(0u, test_observer_->enrollment_finished_count());
+    base::RunLoop second_enrollment_run_loop;
 
-    base::RunLoop enrollment_run_loop;
-    test_observer_->set_closure_for_enrollment_finished(
-        enrollment_run_loop.QuitClosure());
     fake_device_sync_->InvokePendingGetLocalDeviceMetadataCallback(
         test_remote_device_list_[0]);
-    enrollment_run_loop.Run();
-    EXPECT_EQ(test_remote_device_list_[0].public_key,
-              client_->GetLocalDeviceMetadata()->public_key());
+    test_observer_->set_closure_for_enrollment_finished(
+        second_enrollment_run_loop.QuitWhenIdleClosure());
+
+    second_enrollment_run_loop.Run();
 
     // Ensure that the rest of the synced devices are not removed from the cache
     // when updating the local device metadata.
     VerifyRemoteDeviceRefListAndRemoteDeviceListAreEqual(
         client_->GetSyncedDevices(), test_remote_device_list_);
-    EXPECT_EQ(1u, test_observer_->enrollment_finished_count());
+    EXPECT_EQ(2u, test_observer_->enrollment_finished_count());
   }
 
   void TearDown() override {
@@ -465,43 +511,46 @@ TEST_F(
 
   SendPendingMojoMessages();
 
-  EXPECT_FALSE(client_->GetLocalDeviceMetadata());
+  EXPECT_FALSE(client_->is_ready());
+  EXPECT_EQ(0u, test_observer_->ready_count());
   EXPECT_EQ(0u, test_observer_->enrollment_finished_count());
+  EXPECT_EQ(0u, test_observer_->new_devices_synced_count());
 
   // Simulate local device metadata not being ready. It will be ready once
   // synced devices are returned, at which point |client_| should call
   // GetLocalMetadata() again.
   fake_device_sync_->InvokePendingGetLocalDeviceMetadataCallback(base::nullopt);
-
-  EXPECT_FALSE(client_->GetLocalDeviceMetadata());
-  EXPECT_EQ(0u, test_observer_->enrollment_finished_count());
-
-  EXPECT_EQ(0u, client_->GetSyncedDevices().size());
-  EXPECT_EQ(0u, test_observer_->new_devices_synced_count());
-
-  base::RunLoop sync_run_loop;
-  test_observer_->set_closure_for_new_devices_synced(
-      sync_run_loop.QuitClosure());
   fake_device_sync_->InvokePendingGetSyncedDevicesCallback(
       test_remote_device_list_);
-  sync_run_loop.Run();
 
-  VerifyRemoteDeviceRefListAndRemoteDeviceListAreEqual(
-      client_->GetSyncedDevices(), test_remote_device_list_);
-  EXPECT_EQ(1u, test_observer_->new_devices_synced_count());
+  EXPECT_FALSE(client_->is_ready());
+  EXPECT_EQ(0u, test_observer_->ready_count());
+  EXPECT_EQ(0u, test_observer_->enrollment_finished_count());
+  EXPECT_EQ(0u, test_observer_->new_devices_synced_count());
 
   SendPendingMojoMessages();
 
-  base::RunLoop get_local_metadata_run_loop;
-  test_observer_->set_closure_for_enrollment_finished(
-      get_local_metadata_run_loop.QuitClosure());
+  base::RunLoop run_loop;
+
+  // Simulate the local device metadata now being ready.
   fake_device_sync_->InvokePendingGetLocalDeviceMetadataCallback(
       test_remote_device_list_[0]);
-  get_local_metadata_run_loop.Run();
 
+  test_observer_->set_closure_for_enrollment_finished(
+      run_loop.QuitWhenIdleClosure());
+  test_observer_->set_closure_for_new_devices_synced(
+      run_loop.QuitWhenIdleClosure());
+
+  run_loop.Run();
+
+  EXPECT_TRUE(client_->is_ready());
+  EXPECT_EQ(1u, test_observer_->ready_count());
   EXPECT_EQ(test_remote_device_list_[0].public_key,
             client_->GetLocalDeviceMetadata()->public_key());
   EXPECT_EQ(1u, test_observer_->enrollment_finished_count());
+  VerifyRemoteDeviceRefListAndRemoteDeviceListAreEqual(
+      client_->GetSyncedDevices(), test_remote_device_list_);
+  EXPECT_EQ(1u, test_observer_->new_devices_synced_count());
 }
 
 TEST_F(DeviceSyncClientImplTest, TestOnEnrollmentFinished) {

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

@@ -52,6 +52,7 @@ class FakeDeviceSyncClient : public DeviceSyncClient {
     local_device_metadata_ = local_device_metadata;
   }
 
+  using DeviceSyncClient::NotifyReady;
   using DeviceSyncClient::NotifyEnrollmentFinished;
   using DeviceSyncClient::NotifyNewDevicesSynced;
 

chromeos/services/multidevice_setup/multidevice_setup_initializer.cc

@@ -52,7 +52,7 @@ MultiDeviceSetupInitializer::MultiDeviceSetupInitializer(
     : pref_service_(pref_service),
       device_sync_client_(device_sync_client),
       secure_channel_client_(secure_channel_client) {
-  if (device_sync_client_->GetLocalDeviceMetadata()) {
+  if (device_sync_client_->is_ready()) {
     InitializeImplementation();
     return;
   }
@@ -90,10 +90,7 @@ void MultiDeviceSetupInitializer::TriggerEventForDebugging(
   std::move(callback).Run(false /* success */);
 }
 
-void MultiDeviceSetupInitializer::OnEnrollmentFinished() {
-  if (!device_sync_client_->GetLocalDeviceMetadata())
-    return;
-
+void MultiDeviceSetupInitializer::OnReady() {
   device_sync_client_->RemoveObserver(this);
   InitializeImplementation();
 }

chromeos/services/multidevice_setup/multidevice_setup_initializer.h

@@ -57,7 +57,7 @@ class MultiDeviceSetupInitializer
       TriggerEventForDebuggingCallback callback) override;
 
   // device_sync::DeviceSyncClient::Observer:
-  void OnEnrollmentFinished() override;
+  void OnReady() override;
 
   void InitializeImplementation();
 

chromeos/services/multidevice_setup/multidevice_setup_service_unittest.cc

@@ -160,7 +160,7 @@ class MultiDeviceSetupServiceTest : public testing::Test {
   void FinishInitialization() {
     EXPECT_FALSE(fake_multidevice_setup());
     fake_device_sync_client_->set_local_device_metadata(test_device_);
-    fake_device_sync_client_->NotifyEnrollmentFinished();
+    fake_device_sync_client_->NotifyReady();
     EXPECT_TRUE(fake_multidevice_setup());
   }