Prevent accidental retries in WifiSyncFeatureManager success callback

The true state of the back-end cannot be determined at the time
of success callbacks to cryptauth through DeviceSyncClient because
the remote device cache is not yet updated. This caused a second
unnecessary network request to be sent out when the success callback
was triggered because the back-end state could not be verified.

This CL fixes the duplicate network request by changing the success
callback function to infer the state of the back-end based on whether
the network request attempted to enable or disable wifi sync.

Bug: 1117619
Change-Id: Icec19d21954c60a6d90603f346f7136bfd5b3d62
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2439518Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Jon Mann <jonmann@chromium.org>
Commit-Queue: Claude van der Merwe <cvandermerwe@google.com>
Cr-Commit-Position: refs/heads/master@{#813164}

chromeos/services/multidevice_setup/wifi_sync_feature_manager_impl.cc

@@ -101,10 +101,10 @@ void WifiSyncFeatureManagerImpl::OnNewDevicesSynced() {
 
 void WifiSyncFeatureManagerImpl::SetIsWifiSyncEnabled(bool enabled) {
   if (GetCurrentState() == CurrentState::kNoVerifiedHost) {
-    ResetPendingWifiSyncHostNetworkRequest();
     PA_LOG(ERROR)
         << "WifiSyncFeatureManagerImpl::SetIsWifiSyncEnabled:  Network request "
            "not attempted because there is No Verified Host";
+    ResetPendingWifiSyncHostNetworkRequest();
     return;
   }
 
@@ -161,7 +161,6 @@ WifiSyncFeatureManagerImpl::GetCurrentState() {
            ->GetSoftwareFeatureState(
                multidevice::SoftwareFeature::kWifiSyncHost) ==
        multidevice::SoftwareFeatureState::kEnabled);
-
   bool pending_enabled = (GetPendingState() == PendingState::kPendingEnable);
 
   if (pending_enabled == enabled_on_host) {
@@ -224,12 +223,6 @@ void WifiSyncFeatureManagerImpl::OnSetWifiSyncHostStateNetworkRequestFinished(
     device_sync::mojom::NetworkRequestResult result_code) {
   network_request_in_flight_ = false;
 
-  bool has_valid_pending_request =
-      (GetCurrentState() == CurrentState::kValidPendingRequest);
-  if (!has_valid_pending_request) {
-    ResetPendingWifiSyncHostNetworkRequest();
-  }
-
   bool success =
       (result_code == device_sync::mojom::NetworkRequestResult::kSuccess);
 
@@ -242,11 +235,17 @@ void WifiSyncFeatureManagerImpl::OnSetWifiSyncHostStateNetworkRequestFinished(
 
   if (success) {
     PA_LOG(VERBOSE) << ss.str();
+    PendingState pending_state = GetPendingState();
+    if (pending_state == PendingState::kPendingNone) {
+      return;
+    }
+
+    bool pending_enabled = (pending_state == PendingState::kPendingEnable);
     // If the network request was successful but there is still a pending
     // network request then trigger a network request immediately. This could
     // happen if there was a second attempt to set the backend while the first
     // one was still in progress.
-    if (has_valid_pending_request) {
+    if (attempted_to_enable != pending_enabled) {
       AttemptSetWifiSyncHostStateNetworkRequest(false /* is_retry */);
     }
     return;
@@ -257,7 +256,7 @@ void WifiSyncFeatureManagerImpl::OnSetWifiSyncHostStateNetworkRequestFinished(
 
   // If the network request failed and there is still a pending network request,
   // schedule a retry.
-  if (has_valid_pending_request) {
+  if (GetCurrentState() == CurrentState::kValidPendingRequest) {
     timer_->Start(FROM_HERE,
                   base::TimeDelta::FromMinutes(kNumMinutesBetweenRetries),
                   base::BindOnce(&WifiSyncFeatureManagerImpl::

chromeos/services/multidevice_setup/wifi_sync_feature_manager_impl_unittest.cc

@@ -288,12 +288,11 @@ TEST_P(MultiDeviceSetupWifiSyncFeatureManagerImplTest, Success) {
   EXPECT_EQ(test_devices()[0].GetSoftwareFeatureState(
                 multidevice::SoftwareFeature::kWifiSyncHost),
             multidevice::SoftwareFeatureState::kSupported);
-
-  SetWifiSyncHostInDeviceSyncClient(test_devices()[0], true /* enabled */);
   InvokePendingSetWifiSyncHostNetworkRequestCallback(
       device_sync::mojom::NetworkRequestResult::kSuccess,
       false /* expected_to_notify_observer_and_start_retry_timer */);
   EXPECT_EQ(0, GetSetHostNetworkRequestCallbackQueueSize());
+  SetWifiSyncHostInDeviceSyncClient(test_devices()[0], true /* enabled */);
 
   EXPECT_EQ(test_devices()[0].GetSoftwareFeatureState(
                 multidevice::SoftwareFeature::kWifiSyncHost),
@@ -307,16 +306,45 @@ TEST_P(MultiDeviceSetupWifiSyncFeatureManagerImplTest, Success) {
                 multidevice::SoftwareFeature::kWifiSyncHost),
             multidevice::SoftwareFeatureState::kEnabled);
 
-  SetWifiSyncHostInDeviceSyncClient(test_devices()[0], false /* enabled */);
   InvokePendingSetWifiSyncHostNetworkRequestCallback(
       device_sync::mojom::NetworkRequestResult::kSuccess,
       false /* expected_to_notify_observer_and_start_retry_timer */);
   EXPECT_EQ(0, GetSetHostNetworkRequestCallbackQueueSize());
+  SetWifiSyncHostInDeviceSyncClient(test_devices()[0], false /* enabled */);
   EXPECT_EQ(test_devices()[0].GetSoftwareFeatureState(
                 multidevice::SoftwareFeature::kWifiSyncHost),
             multidevice::SoftwareFeatureState::kSupported);
 }
 
+TEST_P(MultiDeviceSetupWifiSyncFeatureManagerImplTest,
+       NewDevicesSyncedBeforeCallback) {
+  CreateDelegate(test_devices()[0] /* initial_host */);
+
+  // Attempt to enable wifi sync on host device and succeed
+  EXPECT_FALSE(delegate()->IsWifiSyncEnabled());
+  EXPECT_EQ(test_devices()[0].GetSoftwareFeatureState(
+                multidevice::SoftwareFeature::kWifiSyncHost),
+            multidevice::SoftwareFeatureState::kSupported);
+
+  SetIsWifiSyncEnabled(true);
+  EXPECT_EQ(1, GetSetHostNetworkRequestCallbackQueueSize());
+  EXPECT_TRUE(delegate()->IsWifiSyncEnabled());
+  EXPECT_EQ(test_devices()[0].GetSoftwareFeatureState(
+                multidevice::SoftwareFeature::kWifiSyncHost),
+            multidevice::SoftwareFeatureState::kSupported);
+  // Triggers OnNewDevicesSynced
+  SetWifiSyncHostInDeviceSyncClient(test_devices()[0], true /* enabled */);
+  // Triggers Success Callback
+  InvokePendingSetWifiSyncHostNetworkRequestCallback(
+      device_sync::mojom::NetworkRequestResult::kSuccess,
+      false /* expected_to_notify_observer_and_start_retry_timer */);
+  EXPECT_EQ(0, GetSetHostNetworkRequestCallbackQueueSize());
+
+  EXPECT_EQ(test_devices()[0].GetSoftwareFeatureState(
+                multidevice::SoftwareFeature::kWifiSyncHost),
+            multidevice::SoftwareFeatureState::kEnabled);
+}
+
 TEST_P(MultiDeviceSetupWifiSyncFeatureManagerImplTest, Failure) {
   CreateDelegate(test_devices()[0] /* initial_host */);
 
@@ -394,10 +422,10 @@ TEST_P(MultiDeviceSetupWifiSyncFeatureManagerImplTest,
                 multidevice::SoftwareFeature::kWifiSyncHost),
             multidevice::SoftwareFeatureState::kSupported);
 
-  SetWifiSyncHostInDeviceSyncClient(test_devices()[0], true /* enabled */);
   InvokePendingSetWifiSyncHostNetworkRequestCallback(
       device_sync::mojom::NetworkRequestResult::kSuccess,
       false /* expected_to_notify_observer_and_start_retry_timer */);
+  SetWifiSyncHostInDeviceSyncClient(test_devices()[0], true /* enabled */);
   EXPECT_EQ(0, GetSetHostNetworkRequestCallbackQueueSize());
   EXPECT_EQ(test_devices()[0].GetSoftwareFeatureState(
                 multidevice::SoftwareFeature::kWifiSyncHost),
@@ -455,10 +483,10 @@ TEST_P(MultiDeviceSetupWifiSyncFeatureManagerImplTest,
                 multidevice::SoftwareFeature::kWifiSyncHost),
             multidevice::SoftwareFeatureState::kSupported);
 
-  SetWifiSyncHostInDeviceSyncClient(test_devices()[0], true /* enabled */);
   InvokePendingSetWifiSyncHostNetworkRequestCallback(
       device_sync::mojom::NetworkRequestResult::kSuccess,
       false /* expected_to_notify_observer_and_start_retry_timer */);
+  SetWifiSyncHostInDeviceSyncClient(test_devices()[0], true /* enabled */);
   EXPECT_EQ(0, GetSetHostNetworkRequestCallbackQueueSize());
   EXPECT_EQ(test_devices()[0].GetSoftwareFeatureState(
                 multidevice::SoftwareFeature::kWifiSyncHost),
@@ -608,10 +636,10 @@ TEST_P(MultiDeviceSetupWifiSyncFeatureManagerImplTest,
   EXPECT_EQ(1, GetSetHostNetworkRequestCallbackQueueSize());
 
   // Successfully enable on host
-  SetWifiSyncHostInDeviceSyncClient(test_devices()[0], true /* enabled */);
   InvokePendingSetWifiSyncHostNetworkRequestCallback(
       device_sync::mojom::NetworkRequestResult::kSuccess,
       false /* expected_to_notify_observer_and_start_retry_timer */);
+  SetWifiSyncHostInDeviceSyncClient(test_devices()[0], true /* enabled */);
   EXPECT_FALSE(delegate()->IsWifiSyncEnabled());
   EXPECT_EQ(test_devices()[0].GetSoftwareFeatureState(
                 multidevice::SoftwareFeature::kWifiSyncHost),
@@ -619,10 +647,10 @@ TEST_P(MultiDeviceSetupWifiSyncFeatureManagerImplTest,
 
   // A new network request should be scheduled to disable
   EXPECT_EQ(1, GetSetHostNetworkRequestCallbackQueueSize());
-  SetWifiSyncHostInDeviceSyncClient(test_devices()[0], false /* enabled */);
   InvokePendingSetWifiSyncHostNetworkRequestCallback(
       device_sync::mojom::NetworkRequestResult::kSuccess,
       false /* expected_to_notify_observer_and_start_retry_timer */);
+  SetWifiSyncHostInDeviceSyncClient(test_devices()[0], false /* enabled */);
   EXPECT_FALSE(delegate()->IsWifiSyncEnabled());
   EXPECT_EQ(test_devices()[0].GetSoftwareFeatureState(
                 multidevice::SoftwareFeature::kWifiSyncHost),