[CrOS MultiDevice] Proactively sync after FindEligibleDevices() call.

When a host verification is forced via the "Verify" button in settings,
HostVerifierImpl makes a call to FindEligibleDevices(), which should end
up resulting in a sync tickle being sent to the Chromebook. In practice,
we've found that this tickle is often missed by the device.

This CL ensures that a sync occurs by forcing a sync after a successful
FindEligibleDevices() call. The sync is scheduled for 5 seconds after
the FindEligibleDevices() call completes to ensure that the server has
had enough time to update its database before the sync.

TBR=khorimoto@chromium.org

(cherry picked from commit 790b6a7867682cdf632c1ac679894014301d7359)

Bug: 913816
Change-Id: Iaf8686ce4ac34e0e48a0613108949ba1b9a06cff
Reviewed-on: https://chromium-review.googlesource.com/c/1444053
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Jeremy Klein <jlklein@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#627240}
Reviewed-on: https://chromium-review.googlesource.com/c/1447256
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/branch-heads/3683@{#79}
Cr-Branched-From: e51029943e0a38dd794b73caaf6373d5496ae783-refs/heads/master@{#625896}
diff --git a/chromeos/services/multidevice_setup/host_verifier_impl.cc b/chromeos/services/multidevice_setup/host_verifier_impl.cc
index 147088c..df0897d65 100644
--- a/chromeos/services/multidevice_setup/host_verifier_impl.cc
+++ b/chromeos/services/multidevice_setup/host_verifier_impl.cc
@@ -4,6 +4,10 @@
 
 #include "chromeos/services/multidevice_setup/host_verifier_impl.h"
 
+#include <utility>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/no_destructor.h"
@@ -45,6 +49,10 @@
 constexpr const base::TimeDelta kFirstRetryDelta =
     base::TimeDelta::FromMinutes(10);
 
+// Delta for the time between a successful FindEligibleDevices call and a
+// request to sync devices.
+constexpr const base::TimeDelta kSyncDelay = base::TimeDelta::FromSeconds(5);
+
 // The multiplier for increasing the backoff timer between retries.
 const double kExponentialBackoffMultiplier = 1.5;
 
@@ -74,10 +82,11 @@
     device_sync::DeviceSyncClient* device_sync_client,
     PrefService* pref_service,
     base::Clock* clock,
-    std::unique_ptr<base::OneShotTimer> timer) {
-  return base::WrapUnique(new HostVerifierImpl(host_backend_delegate,
-                                               device_sync_client, pref_service,
-                                               clock, std::move(timer)));
+    std::unique_ptr<base::OneShotTimer> retry_timer,
+    std::unique_ptr<base::OneShotTimer> sync_timer) {
+  return base::WrapUnique(new HostVerifierImpl(
+      host_backend_delegate, device_sync_client, pref_service, clock,
+      std::move(retry_timer), std::move(sync_timer)));
 }
 
 // static
@@ -91,12 +100,15 @@
     device_sync::DeviceSyncClient* device_sync_client,
     PrefService* pref_service,
     base::Clock* clock,
-    std::unique_ptr<base::OneShotTimer> timer)
+    std::unique_ptr<base::OneShotTimer> retry_timer,
+    std::unique_ptr<base::OneShotTimer> sync_timer)
     : host_backend_delegate_(host_backend_delegate),
       device_sync_client_(device_sync_client),
       pref_service_(pref_service),
       clock_(clock),
-      timer_(std::move(timer)) {
+      retry_timer_(std::move(retry_timer)),
+      sync_timer_(std::move(sync_timer)),
+      weak_ptr_factory_(this) {
   host_backend_delegate_->AddObserver(this);
   device_sync_client_->AddObserver(this);
 
@@ -148,21 +160,22 @@
 void HostVerifierImpl::UpdateRetryState() {
   // If there is no host, verification is not applicable.
   if (!host_backend_delegate_->GetMultiDeviceHostFromBackend()) {
-    StopTimerAndClearPrefs();
+    StopRetryTimerAndClearPrefs();
     return;
   }
 
   // If there is a host and it is verified, verification is no longer necessary.
   if (IsHostVerified()) {
-    bool was_timer_running = timer_->IsRunning();
-    StopTimerAndClearPrefs();
-    if (was_timer_running)
+    sync_timer_->Stop();
+    bool was_retry_timer_running = retry_timer_->IsRunning();
+    StopRetryTimerAndClearPrefs();
+    if (was_retry_timer_running)
       NotifyHostVerified();
     return;
   }
 
-  // If |timer_| is running, an ongoing retry attempt is in progress.
-  if (timer_->IsRunning())
+  // If |retry_timer_| is running, an ongoing retry attempt is in progress.
+  if (retry_timer_->IsRunning())
     return;
 
   int64_t timestamp_from_prefs =
@@ -180,15 +193,15 @@
 
   // If a timeout value was set but has not yet occurred, start the timer.
   if (clock_->Now() < retry_time_from_prefs) {
-    StartTimer(retry_time_from_prefs);
+    StartRetryTimer(retry_time_from_prefs);
     return;
   }
 
   AttemptVerificationAfterInitialTimeout(retry_time_from_prefs);
 }
 
-void HostVerifierImpl::StopTimerAndClearPrefs() {
-  timer_->Stop();
+void HostVerifierImpl::StopRetryTimerAndClearPrefs() {
+  retry_timer_->Stop();
   pref_service_->SetInt64(kRetryTimestampPrefName, kTimestampNotSet);
   pref_service_->SetInt64(kLastUsedTimeDeltaMsPrefName, 0);
 }
@@ -200,7 +213,7 @@
   pref_service_->SetInt64(kLastUsedTimeDeltaMsPrefName,
                           kFirstRetryDelta.InMilliseconds());
 
-  StartTimer(retry_time);
+  StartRetryTimer(retry_time);
   AttemptHostVerification();
 }
 
@@ -218,15 +231,15 @@
   pref_service_->SetInt64(kRetryTimestampPrefName, retry_time.ToJavaTime());
   pref_service_->SetInt64(kLastUsedTimeDeltaMsPrefName, time_delta_ms);
 
-  StartTimer(retry_time);
+  StartRetryTimer(retry_time);
   AttemptHostVerification();
 }
 
-void HostVerifierImpl::StartTimer(const base::Time& time_to_fire) {
+void HostVerifierImpl::StartRetryTimer(const base::Time& time_to_fire) {
   base::Time now = clock_->Now();
   DCHECK(now < time_to_fire);
 
-  timer_->Start(
+  retry_timer_->Start(
       FROM_HERE, time_to_fire - now /* delay */,
       base::Bind(&HostVerifierImpl::UpdateRetryState, base::Unretained(this)));
 }
@@ -243,7 +256,35 @@
   PA_LOG(VERBOSE) << "HostVerifierImpl::AttemptHostVerification(): Attempting "
                   << "host verification now.";
   device_sync_client_->FindEligibleDevices(
-      multidevice::SoftwareFeature::kBetterTogetherHost, base::DoNothing());
+      multidevice::SoftwareFeature::kBetterTogetherHost,
+      base::BindOnce(&HostVerifierImpl::OnFindEligibleDevicesResult,
+                     weak_ptr_factory_.GetWeakPtr()));
+}
+
+void HostVerifierImpl::OnFindEligibleDevicesResult(
+    device_sync::mojom::NetworkRequestResult result,
+    multidevice::RemoteDeviceRefList eligible_devices,
+    multidevice::RemoteDeviceRefList ineligible_devices) {
+  if (result != device_sync::mojom::NetworkRequestResult::kSuccess) {
+    PA_LOG(WARNING) << "HostVerifierImpl::OnFindEligibleDevicesResult(): "
+                    << "FindEligibleDevices call failed. Retry is scheduled.";
+    return;
+  }
+
+  // Now that the FindEligibleDevices call was sent successfully, the host phone
+  // is expected to enable its supported features. This should trigger a push
+  // message asking this Chromebook to sync these updated features, but in
+  // practice it has been observed that the Chromebook sometimes does not
+  // receive this message (see https://crbug.com/913816). Thus, schedule a sync
+  // after the phone has had enough time to enable its features. Note that this
+  // sync is canceled if the Chromebook does receive the push message.
+  sync_timer_->Start(
+      FROM_HERE, kSyncDelay,
+      base::Bind(&HostVerifierImpl::OnSyncTimerFired, base::Unretained(this)));
+}
+
+void HostVerifierImpl::OnSyncTimerFired() {
+  device_sync_client_->ForceSyncNow(base::DoNothing());
 }
 
 }  // namespace multidevice_setup
diff --git a/chromeos/services/multidevice_setup/host_verifier_impl.h b/chromeos/services/multidevice_setup/host_verifier_impl.h
index 2fc3e92..5ccdf453 100644
--- a/chromeos/services/multidevice_setup/host_verifier_impl.h
+++ b/chromeos/services/multidevice_setup/host_verifier_impl.h
@@ -8,8 +8,10 @@
 #include <memory>
 
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "base/time/default_clock.h"
 #include "base/timer/timer.h"
+#include "chromeos/components/multidevice/remote_device_ref.h"
 #include "chromeos/services/device_sync/public/cpp/device_sync_client.h"
 #include "chromeos/services/multidevice_setup/host_backend_delegate.h"
 #include "chromeos/services/multidevice_setup/host_verifier.h"
@@ -42,7 +44,9 @@
         device_sync::DeviceSyncClient* device_sync_client,
         PrefService* pref_service,
         base::Clock* clock = base::DefaultClock::GetInstance(),
-        std::unique_ptr<base::OneShotTimer> timer =
+        std::unique_ptr<base::OneShotTimer> retry_timer =
+            std::make_unique<base::OneShotTimer>(),
+        std::unique_ptr<base::OneShotTimer> sync_timer =
             std::make_unique<base::OneShotTimer>());
 
    private:
@@ -58,7 +62,8 @@
                    device_sync::DeviceSyncClient* device_sync_client,
                    PrefService* pref_service,
                    base::Clock* clock,
-                   std::unique_ptr<base::OneShotTimer> timer);
+                   std::unique_ptr<base::OneShotTimer> retry_timer,
+                   std::unique_ptr<base::OneShotTimer> sync_timer);
 
   // HostVerifier:
   bool IsHostVerified() override;
@@ -71,18 +76,25 @@
   void OnNewDevicesSynced() override;
 
   void UpdateRetryState();
-  void StopTimerAndClearPrefs();
+  void StopRetryTimerAndClearPrefs();
   void AttemptVerificationWithInitialTimeout();
   void AttemptVerificationAfterInitialTimeout(
       const base::Time& retry_time_from_prefs);
-  void StartTimer(const base::Time& time_to_fire);
+  void StartRetryTimer(const base::Time& time_to_fire);
   void AttemptHostVerification();
+  void OnFindEligibleDevicesResult(
+      device_sync::mojom::NetworkRequestResult result,
+      multidevice::RemoteDeviceRefList eligible_devices,
+      multidevice::RemoteDeviceRefList ineligible_devices);
+  void OnSyncTimerFired();
 
   HostBackendDelegate* host_backend_delegate_;
   device_sync::DeviceSyncClient* device_sync_client_;
   PrefService* pref_service_;
   base::Clock* clock_;
-  std::unique_ptr<base::OneShotTimer> timer_;
+  std::unique_ptr<base::OneShotTimer> retry_timer_;
+  std::unique_ptr<base::OneShotTimer> sync_timer_;
+  base::WeakPtrFactory<HostVerifierImpl> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(HostVerifierImpl);
 };
diff --git a/chromeos/services/multidevice_setup/host_verifier_impl_unittest.cc b/chromeos/services/multidevice_setup/host_verifier_impl_unittest.cc
index 865aac5..84af9af0 100644
--- a/chromeos/services/multidevice_setup/host_verifier_impl_unittest.cc
+++ b/chromeos/services/multidevice_setup/host_verifier_impl_unittest.cc
@@ -5,6 +5,7 @@
 #include "chromeos/services/multidevice_setup/host_verifier_impl.h"
 
 #include <memory>
+#include <utility>
 
 #include "base/macros.h"
 #include "base/test/simple_test_clock.h"
@@ -78,12 +79,16 @@
     test_pref_service_->SetInt64(kLastUsedTimeDeltaMsPrefName,
                                  initial_time_delta_pref_value);
 
-    auto mock_timer = std::make_unique<base::MockOneShotTimer>();
-    mock_timer_ = mock_timer.get();
+    auto mock_retry_timer = std::make_unique<base::MockOneShotTimer>();
+    mock_retry_timer_ = mock_retry_timer.get();
+
+    auto mock_sync_timer = std::make_unique<base::MockOneShotTimer>();
+    mock_sync_timer_ = mock_sync_timer.get();
 
     host_verifier_ = HostVerifierImpl::Factory::Get()->BuildInstance(
         fake_host_backend_delegate_.get(), fake_device_sync_client_.get(),
-        test_pref_service_.get(), test_clock_.get(), std::move(mock_timer));
+        test_pref_service_.get(), test_clock_.get(),
+        std::move(mock_retry_timer), std::move(mock_sync_timer));
 
     fake_observer_ = std::make_unique<FakeHostVerifierObserver>();
     host_verifier_->AddObserver(fake_observer_.get());
@@ -118,22 +123,33 @@
               test_pref_service_->GetInt64(kLastUsedTimeDeltaMsPrefName));
 
     // If a retry timestamp is set, the timer should be running.
-    EXPECT_EQ(expected_retry_timestamp_value != 0, mock_timer_->IsRunning());
+    EXPECT_EQ(expected_retry_timestamp_value != 0,
+              mock_retry_timer_->IsRunning());
   }
 
-  void VerifyFindEligibleDevicesCalled() {
+  void InvokePendingFindEligibleDevicesCall(bool success = true) {
     fake_device_sync_client_->InvokePendingFindEligibleDevicesCallback(
-        device_sync::mojom::NetworkRequestResult::kSuccess,
+        success
+            ? device_sync::mojom::NetworkRequestResult::kSuccess
+            : device_sync::mojom::NetworkRequestResult::kInternalServerError,
         multidevice::RemoteDeviceRefList() /* eligible_devices */,
         multidevice::RemoteDeviceRefList() /* ineligible_devices */);
   }
 
-  void SimulateTimePassing(const base::TimeDelta& delta,
-                           bool simulate_timeout = false) {
+  void SimulateRetryTimePassing(const base::TimeDelta& delta,
+                                bool simulate_timeout = false) {
     test_clock_->Advance(delta);
 
     if (simulate_timeout)
-      mock_timer_->Fire();
+      mock_retry_timer_->Fire();
+  }
+
+  void FireSyncTimerAndVerifySyncOccurred() {
+    EXPECT_TRUE(mock_sync_timer_->IsRunning());
+    mock_sync_timer_->Fire();
+    fake_device_sync_client_->InvokePendingForceSyncNowCallback(
+        true /* success */);
+    SetHostState(HostState::kHostVerified);
   }
 
   FakeHostBackendDelegate* fake_host_backend_delegate() {
@@ -149,7 +165,8 @@
   std::unique_ptr<sync_preferences::TestingPrefServiceSyncable>
       test_pref_service_;
   std::unique_ptr<base::SimpleTestClock> test_clock_;
-  base::MockOneShotTimer* mock_timer_ = nullptr;
+  base::MockOneShotTimer* mock_retry_timer_ = nullptr;
+  base::MockOneShotTimer* mock_sync_timer_ = nullptr;
 
   std::unique_ptr<HostVerifier> host_verifier_;
 
@@ -160,13 +177,13 @@
   CreateVerifier(HostState::kHostNotSet);
 
   SetHostState(HostState::kHostSetButNotVerified);
-  VerifyFindEligibleDevicesCalled();
+  InvokePendingFindEligibleDevicesCall();
   VerifyState(
       false /* expected_is_verified */, 0u /* expected_num_verified_events */,
       kTestTimeMs + kFirstRetryDeltaMs /* expected_retry_timestamp_value */,
       kFirstRetryDeltaMs /* expected_retry_delta_value */);
 
-  SimulateTimePassing(base::TimeDelta::FromMinutes(1));
+  SimulateRetryTimePassing(base::TimeDelta::FromMinutes(1));
   SetHostState(HostState::kHostVerified);
   VerifyState(true /* expected_is_verified */,
               1u /* expected_num_verified_events */,
@@ -174,20 +191,51 @@
               0 /* expected_retry_delta_value */);
 }
 
+TEST_F(MultiDeviceSetupHostVerifierImplTest,
+       StartWithoutHost_FindEligibleDevicesFails) {
+  CreateVerifier(HostState::kHostNotSet);
+  SetHostState(HostState::kHostSetButNotVerified);
+
+  // If the FindEligibleDevices call fails, a retry should still be scheduled.
+  InvokePendingFindEligibleDevicesCall(false /* success */);
+  VerifyState(
+      false /* expected_is_verified */, 0u /* expected_num_verified_events */,
+      kTestTimeMs + kFirstRetryDeltaMs /* expected_retry_timestamp_value */,
+      kFirstRetryDeltaMs /* expected_retry_delta_value */);
+}
+
+TEST_F(MultiDeviceSetupHostVerifierImplTest, SyncAfterFindEligibleDevices) {
+  CreateVerifier(HostState::kHostNotSet);
+
+  SetHostState(HostState::kHostSetButNotVerified);
+  InvokePendingFindEligibleDevicesCall();
+  VerifyState(
+      false /* expected_is_verified */, 0u /* expected_num_verified_events */,
+      kTestTimeMs + kFirstRetryDeltaMs /* expected_retry_timestamp_value */,
+      kFirstRetryDeltaMs /* expected_retry_delta_value */);
+
+  FireSyncTimerAndVerifySyncOccurred();
+  VerifyState(true /* expected_is_verified */,
+              1u /* expected_num_verified_events */,
+              0 /* expected_retry_timestamp_value */,
+              0 /* expected_retry_delta_value */);
+}
+
 TEST_F(MultiDeviceSetupHostVerifierImplTest, StartWithoutHost_Retry) {
   CreateVerifier(HostState::kHostNotSet);
 
   SetHostState(HostState::kHostSetButNotVerified);
-  VerifyFindEligibleDevicesCalled();
+  InvokePendingFindEligibleDevicesCall();
   VerifyState(
       false /* expected_is_verified */, 0u /* expected_num_verified_events */,
       kTestTimeMs + kFirstRetryDeltaMs /* expected_retry_timestamp_value */,
       kFirstRetryDeltaMs /* expected_retry_delta_value */);
 
   // Simulate enough time pasing to time out and retry.
-  SimulateTimePassing(base::TimeDelta::FromMilliseconds(kFirstRetryDeltaMs),
-                      true /* simulate_timeout */);
-  VerifyFindEligibleDevicesCalled();
+  SimulateRetryTimePassing(
+      base::TimeDelta::FromMilliseconds(kFirstRetryDeltaMs),
+      true /* simulate_timeout */);
+  InvokePendingFindEligibleDevicesCall();
   VerifyState(false /* expected_is_verified */,
               0u /* expected_num_verified_events */,
               kTestTimeMs + kFirstRetryDeltaMs +
@@ -196,11 +244,12 @@
               kFirstRetryDeltaMs * kExponentialBackoffMultiplier
               /* expected_retry_delta_value */);
 
-  // Simulate the next retry timeout passing..
-  SimulateTimePassing(base::TimeDelta::FromMilliseconds(
-                          kFirstRetryDeltaMs * kExponentialBackoffMultiplier),
-                      true /* simulate_timeout */);
-  VerifyFindEligibleDevicesCalled();
+  // Simulate the next retry timeout passing.
+  SimulateRetryTimePassing(
+      base::TimeDelta::FromMilliseconds(kFirstRetryDeltaMs *
+                                        kExponentialBackoffMultiplier),
+      true /* simulate_timeout */);
+  InvokePendingFindEligibleDevicesCall();
   VerifyState(false /* expected_is_verified */,
               0u /* expected_num_verified_events */,
               kTestTimeMs + kFirstRetryDeltaMs +
@@ -224,7 +273,7 @@
        StartWithUnverifiedHost_NoInitialPrefs) {
   CreateVerifier(HostState::kHostSetButNotVerified);
 
-  VerifyFindEligibleDevicesCalled();
+  InvokePendingFindEligibleDevicesCall();
   VerifyState(
       false /* expected_is_verified */, 0u /* expected_num_verified_events */,
       kTestTimeMs + kFirstRetryDeltaMs /* expected_retry_timestamp_value */,
@@ -240,9 +289,9 @@
                  /* initial_timer_pref_value */,
                  kFirstRetryDeltaMs /* initial_time_delta_pref_value */);
 
-  SimulateTimePassing(base::TimeDelta::FromMinutes(5),
-                      true /* simulate_timeout */);
-  VerifyFindEligibleDevicesCalled();
+  SimulateRetryTimePassing(base::TimeDelta::FromMinutes(5),
+                           true /* simulate_timeout */);
+  InvokePendingFindEligibleDevicesCall();
   VerifyState(false /* expected_is_verified */,
               0u /* expected_num_verified_events */,
               kTestTimeMs + base::TimeDelta::FromMinutes(5).InMilliseconds() +
@@ -261,7 +310,7 @@
                  /* initial_timer_pref_value */,
                  kFirstRetryDeltaMs /* initial_time_delta_pref_value */);
 
-  VerifyFindEligibleDevicesCalled();
+  InvokePendingFindEligibleDevicesCall();
   VerifyState(false /* expected_is_verified */,
               0u /* expected_num_verified_events */,
               kTestTimeMs - base::TimeDelta::FromMinutes(5).InMilliseconds() +
@@ -283,7 +332,7 @@
   // Because the first delta is 10 minutes, the second delta is 10 * 1.5 = 15
   // minutes. In this case, that means that *two* previous timeouts were missed,
   // so the third one should be scheduled.
-  VerifyFindEligibleDevicesCalled();
+  InvokePendingFindEligibleDevicesCall();
   VerifyState(false /* expected_is_verified */,
               0u /* expected_num_verified_events */,
               kTestTimeMs - base::TimeDelta::FromMinutes(20).InMilliseconds() +
@@ -311,7 +360,7 @@
               0 /* expected_retry_delta_value */);
 
   SetHostState(HostState::kHostSetButNotVerified);
-  VerifyFindEligibleDevicesCalled();
+  InvokePendingFindEligibleDevicesCall();
   VerifyState(
       false /* expected_is_verified */, 0u /* expected_num_verified_events */,
       kTestTimeMs + kFirstRetryDeltaMs /* expected_retry_timestamp_value */,
diff --git a/chromeos/services/multidevice_setup/multidevice_setup_impl_unittest.cc b/chromeos/services/multidevice_setup/multidevice_setup_impl_unittest.cc
index 3efe506..4d68f420 100644
--- a/chromeos/services/multidevice_setup/multidevice_setup_impl_unittest.cc
+++ b/chromeos/services/multidevice_setup/multidevice_setup_impl_unittest.cc
@@ -165,7 +165,8 @@
       device_sync::DeviceSyncClient* device_sync_client,
       PrefService* pref_service,
       base::Clock* clock,
-      std::unique_ptr<base::OneShotTimer> timer) override {
+      std::unique_ptr<base::OneShotTimer> retry_timer,
+      std::unique_ptr<base::OneShotTimer> sync_timer) override {
     EXPECT_FALSE(instance_);
     EXPECT_EQ(fake_host_backend_delegate_factory_->instance(),
               host_backend_delegate);