[Wifi Sync] Add support for retries to SyncedNetworkUpdater.

Bug: 966270
Change-Id: I193e7b33f60846d488c9d08b62f23d2432710a34
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1794147
Commit-Queue: Jon Mann <jonmann@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#729510}

chromeos/components/sync_wifi/BUILD.gn

@@ -16,6 +16,8 @@ static_library("sync_wifi") {
     "synced_network_updater.h",
     "synced_network_updater_impl.cc",
     "synced_network_updater_impl.h",
+    "timer_factory.cc",
+    "timer_factory.h",
     "wifi_configuration_bridge.cc",
     "wifi_configuration_bridge.h",
     "wifi_configuration_sync_service.cc",
@@ -38,8 +40,12 @@ static_library("sync_wifi") {
 source_set("test_support") {
   testonly = true
   sources = [
+    "fake_one_shot_timer.cc",
+    "fake_one_shot_timer.h",
     "fake_pending_network_configuration_tracker.cc",
     "fake_pending_network_configuration_tracker.h",
+    "fake_timer_factory.cc",
+    "fake_timer_factory.h",
     "test_data_generator.cc",
     "test_data_generator.h",
   ]

chromeos/components/sync_wifi/fake_one_shot_timer.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chromeos/components/sync_wifi/fake_one_shot_timer.h"
+
+#include "base/callback.h"
+#include "base/run_loop.h"
+#include "base/test/simple_test_tick_clock.h"
+
+namespace chromeos {
+
+namespace sync_wifi {
+
+FakeOneShotTimer::FakeOneShotTimer(
+    base::OnceCallback<void(const base::UnguessableToken&)> destructor_callback)
+    : destructor_callback_(std::move(destructor_callback)),
+      id_(base::UnguessableToken::Create()) {}
+
+FakeOneShotTimer::~FakeOneShotTimer() {
+  std::move(destructor_callback_).Run(id_);
+}
+
+}  // namespace sync_wifi
+
+}  // namespace chromeos

chromeos/components/sync_wifi/fake_one_shot_timer.h

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROMEOS_COMPONENTS_SYNC_WIFI_FAKE_ONE_SHOT_TIMER_H_
+#define CHROMEOS_COMPONENTS_SYNC_WIFI_FAKE_ONE_SHOT_TIMER_H_
+
+#include "base/callback_forward.h"
+#include "base/macros.h"
+#include "base/single_thread_task_runner.h"
+#include "base/timer/mock_timer.h"
+#include "base/unguessable_token.h"
+
+namespace chromeos {
+
+namespace sync_wifi {
+
+// Fake base::OneShotTimer implementation, which extends MockOneShotTimer and
+// provides a mechanism for alerting its creator when it's destroyed.
+class FakeOneShotTimer : public base::MockOneShotTimer {
+ public:
+  FakeOneShotTimer(base::OnceCallback<void(const base::UnguessableToken&)>
+                       destructor_callback);
+  ~FakeOneShotTimer() override;
+
+  const base::UnguessableToken& id() const { return id_; }
+
+ private:
+  base::OnceCallback<void(const base::UnguessableToken&)> destructor_callback_;
+  base::UnguessableToken id_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeOneShotTimer);
+};
+
+}  // namespace sync_wifi
+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_COMPONENTS_SYNC_WIFI_FAKE_ONE_SHOT_TIMER_H_

chromeos/components/sync_wifi/fake_pending_network_configuration_tracker.cc

@@ -40,14 +40,13 @@ void FakePendingNetworkConfigurationTracker::MarkComplete(
 }
 
 void FakePendingNetworkConfigurationTracker::IncrementCompletedAttempts(
-    const std::string& change_id,
+    const std::string& change_guid,
     const NetworkIdentifier& id) {
-  base::Optional<PendingNetworkConfigurationUpdate> existing_update =
-      GetPendingUpdate(change_id, id);
-  id_to_pending_update_map_.emplace(
-      std::piecewise_construct, std::forward_as_tuple(id),
-      std::forward_as_tuple(id, change_id, existing_update->specifics(),
-                            existing_update->completed_attempts() + 1));
+  PendingNetworkConfigurationUpdate& existing_update =
+      id_to_pending_update_map_.at(id);
+  existing_update.SetCompletedAttemptsForTesting(
+      existing_update.completed_attempts() + 1);
+
   id_to_completed_attempts_map_[id]++;
 }
 

chromeos/components/sync_wifi/fake_timer_factory.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chromeos/components/sync_wifi/fake_timer_factory.h"
+
+#include "base/logging.h"
+#include "base/unguessable_token.h"
+#include "chromeos/components/sync_wifi/fake_one_shot_timer.h"
+
+namespace chromeos {
+
+namespace sync_wifi {
+
+FakeTimerFactory::FakeTimerFactory() = default;
+
+FakeTimerFactory::~FakeTimerFactory() = default;
+
+std::unique_ptr<base::OneShotTimer> FakeTimerFactory::CreateOneShotTimer() {
+  auto mock_timer = std::make_unique<FakeOneShotTimer>(
+      base::BindOnce(&FakeTimerFactory::OnOneShotTimerDeleted,
+                     weak_ptr_factory_.GetWeakPtr()));
+  id_to_timer_map_[mock_timer->id()] = mock_timer.get();
+  return mock_timer;
+}
+
+void FakeTimerFactory::FireAll() {
+  // Make a copy because firing a timer will usually destroy it.  This calls
+  // OnOneShotTimerDeleted and removes it from |id_to_timer_map_|.
+  auto id_to_timer_map_copy = id_to_timer_map_;
+  for (auto it : id_to_timer_map_copy)
+    it.second->Fire();
+}
+
+void FakeTimerFactory::OnOneShotTimerDeleted(
+    const base::UnguessableToken& deleted_timer_id) {
+  id_to_timer_map_.erase(deleted_timer_id);
+}
+
+}  // namespace sync_wifi
+
+}  // namespace chromeos

chromeos/components/sync_wifi/fake_timer_factory.h

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROMEOS_COMPONENTS_SYNC_WIFI_FAKE_TIMER_FACTORY_H_
+#define CHROMEOS_COMPONENTS_SYNC_WIFI_FAKE_TIMER_FACTORY_H_
+
+#include <memory>
+#include <string>
+
+#include "base/containers/flat_map.h"
+#include "base/single_thread_task_runner.h"
+#include "base/timer/timer.h"
+#include "chromeos/components/sync_wifi/fake_one_shot_timer.h"
+#include "chromeos/components/sync_wifi/timer_factory.h"
+
+namespace base {
+class OneShotTimer;
+class UnguessableToken;
+}  // namespace base
+
+namespace chromeos {
+
+namespace sync_wifi {
+
+class FakeTimerFactory : public TimerFactory {
+ public:
+  FakeTimerFactory();
+  ~FakeTimerFactory() override;
+
+  // TimerFactory:
+  std::unique_ptr<base::OneShotTimer> CreateOneShotTimer() override;
+
+  void FireAll();
+
+ private:
+  void OnOneShotTimerDeleted(const base::UnguessableToken& deleted_timer_id);
+
+  base::flat_map<base::UnguessableToken, FakeOneShotTimer*> id_to_timer_map_;
+  base::WeakPtrFactory<FakeTimerFactory> weak_ptr_factory_{this};
+
+  DISALLOW_COPY_AND_ASSIGN(FakeTimerFactory);
+};
+
+}  // namespace sync_wifi
+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_COMPONENTS_SYNC_WIFI_FAKE_TIMER_FACTORY_H_
\ No newline at end of file

chromeos/components/sync_wifi/pending_network_configuration_tracker.h

@@ -47,7 +47,8 @@ class PendingNetworkConfigurationTracker {
       const std::string& change_guid,
       const NetworkIdentifier& id) = 0;
 
-  // Increments the number of completed attempts for the given update.
+  // Increments the number of completed attempts for the given update.  Be sure
+  // that the |change_guid| and |ssid| exist in the tracker before calling.
   virtual void IncrementCompletedAttempts(const std::string& change_guid,
                                           const NetworkIdentifier& id) = 0;
 

chromeos/components/sync_wifi/pending_network_configuration_update.h

@@ -51,6 +51,12 @@ class PendingNetworkConfigurationUpdate {
   bool IsDeleteOperation() const;
 
  private:
+  friend class FakePendingNetworkConfigurationTracker;
+
+  void SetCompletedAttemptsForTesting(int completed_attempts) {
+    completed_attempts_ = completed_attempts;
+  }
+
   NetworkIdentifier id_;
   std::string change_guid_;
   base::Optional<sync_pb::WifiConfigurationSpecificsData> specifics_;

chromeos/components/sync_wifi/synced_network_updater_impl.cc

@@ -8,6 +8,7 @@
 #include "base/guid.h"
 #include "base/values.h"
 #include "chromeos/components/sync_wifi/network_type_conversions.h"
+#include "chromeos/components/sync_wifi/timer_factory.h"
 #include "chromeos/network/network_configuration_handler.h"
 #include "chromeos/network/network_profile_handler.h"
 #include "chromeos/network/network_state.h"
@@ -19,14 +20,29 @@ namespace chromeos {
 
 namespace sync_wifi {
 
+namespace {
+
+const int kMaxRetries = 3;
+const char kTimedOutErrorMsg[] = "Timed out";
+constexpr base::TimeDelta kTimeout = base::TimeDelta::FromMinutes(1);
+
+}  // namespace
+
 SyncedNetworkUpdaterImpl::SyncedNetworkUpdaterImpl(
     std::unique_ptr<PendingNetworkConfigurationTracker> tracker,
-    network_config::mojom::CrosNetworkConfig* cros_network_config)
-    : tracker_(std::move(tracker)), cros_network_config_(cros_network_config) {
+    network_config::mojom::CrosNetworkConfig* cros_network_config,
+    std::unique_ptr<TimerFactory> timer_factory)
+    : tracker_(std::move(tracker)),
+      cros_network_config_(cros_network_config),
+      timer_factory_(std::move(timer_factory)) {
   cros_network_config_->AddObserver(
       cros_network_config_observer_receiver_.BindNewPipeAndPassRemote());
   // Load the current list of networks.
   OnNetworkStateListChanged();
+  std::vector<PendingNetworkConfigurationUpdate> updates =
+      tracker_->GetPendingUpdates();
+  for (const PendingNetworkConfigurationUpdate& update : updates)
+    Retry(update);
 }
 
 SyncedNetworkUpdaterImpl::~SyncedNetworkUpdaterImpl() = default;
@@ -34,12 +50,21 @@ SyncedNetworkUpdaterImpl::~SyncedNetworkUpdaterImpl() = default;
 void SyncedNetworkUpdaterImpl::AddOrUpdateNetwork(
     const sync_pb::WifiConfigurationSpecificsData& specifics) {
   auto id = NetworkIdentifier::FromProto(specifics);
-  network_config::mojom::NetworkStatePropertiesPtr existing_network =
-      FindLocalNetwork(id);
   std::string change_guid = tracker_->TrackPendingUpdate(id, specifics);
+  StartAddOrUpdateOperation(change_guid, id, specifics);
+}
+
+void SyncedNetworkUpdaterImpl::StartAddOrUpdateOperation(
+    const std::string& change_guid,
+    const NetworkIdentifier& id,
+    const sync_pb::WifiConfigurationSpecificsData& specifics) {
+  network_config::mojom::NetworkStatePropertiesPtr existing_network =
+      FindMojoNetwork(id);
   network_config::mojom::ConfigPropertiesPtr config =
       MojoNetworkConfigFromProto(specifics);
 
+  StartTimer(change_guid, id);
+
   if (existing_network) {
     cros_network_config_->SetProperties(
         existing_network->guid, std::move(config),
@@ -47,29 +72,36 @@ void SyncedNetworkUpdaterImpl::AddOrUpdateNetwork(
                        weak_ptr_factory_.GetWeakPtr(), change_guid, id));
     return;
   }
+
   cros_network_config_->ConfigureNetwork(
-      std::move(config), /* shared= */ false,
+      std::move(config), /*shared=*/false,
       base::BindOnce(&SyncedNetworkUpdaterImpl::OnConfigureNetworkResult,
                      weak_ptr_factory_.GetWeakPtr(), change_guid, id));
 }
 
 void SyncedNetworkUpdaterImpl::RemoveNetwork(const NetworkIdentifier& id) {
   network_config::mojom::NetworkStatePropertiesPtr network =
-      FindLocalNetwork(id);
+      FindMojoNetwork(id);
   if (!network)
     return;
 
   std::string change_guid =
       tracker_->TrackPendingUpdate(id, /*specifics=*/base::nullopt);
+  StartDeleteOperation(change_guid, id, network->guid);
+}
 
+void SyncedNetworkUpdaterImpl::StartDeleteOperation(
+    const std::string& change_guid,
+    const NetworkIdentifier& id,
+    std::string guid) {
+  StartTimer(change_guid, id);
   cros_network_config_->ForgetNetwork(
-      network->guid,
-      base::BindOnce(&SyncedNetworkUpdaterImpl::OnForgetNetworkResult,
-                     weak_ptr_factory_.GetWeakPtr(), change_guid, id));
+      guid, base::BindOnce(&SyncedNetworkUpdaterImpl::OnForgetNetworkResult,
+                           weak_ptr_factory_.GetWeakPtr(), change_guid, id));
 }
 
 network_config::mojom::NetworkStatePropertiesPtr
-SyncedNetworkUpdaterImpl::FindLocalNetwork(const NetworkIdentifier& id) {
+SyncedNetworkUpdaterImpl::FindMojoNetwork(const NetworkIdentifier& id) {
   for (const network_config::mojom::NetworkStatePropertiesPtr& network :
        networks_) {
     if (id == NetworkIdentifier::FromMojoNetwork(network))
@@ -83,7 +115,7 @@ void SyncedNetworkUpdaterImpl::OnNetworkStateListChanged() {
       network_config::mojom::NetworkFilter::New(
           network_config::mojom::FilterType::kConfigured,
           network_config::mojom::NetworkType::kWiFi,
-          /* limit= */ 0),
+          /*limit=*/0),
       base::BindOnce(&SyncedNetworkUpdaterImpl::OnGetNetworkList,
                      base::Unretained(this)));
 }
@@ -98,6 +130,7 @@ void SyncedNetworkUpdaterImpl::OnError(const std::string& change_guid,
                                        const std::string& error_name) {
   NET_LOG(ERROR) << "Failed to update id:" << id.SerializeToString()
                  << " error:" << error_name;
+  HandleShillResult(change_guid, id, /*is_success=*/false);
 }
 
 void SyncedNetworkUpdaterImpl::OnConfigureNetworkResult(
@@ -105,38 +138,73 @@ void SyncedNetworkUpdaterImpl::OnConfigureNetworkResult(
     const NetworkIdentifier& id,
     const base::Optional<std::string>& guid,
     const std::string& error_message) {
-  if (!guid) {
-    OnError(change_guid, id, "Failed to configure network.");
-    return;
+  if (guid) {
+    VLOG(1) << "Successfully configured network with id "
+            << id.SerializeToString();
+  } else {
+    NET_LOG(ERROR) << "Failed to configure network with id "
+                   << id.SerializeToString() << ". " << error_message;
   }
-  VLOG(1) << "Successfully updated network with id " << id.SerializeToString();
-  CleanupUpdate(change_guid, id);
+  HandleShillResult(change_guid, id, guid.has_value());
 }
 
 void SyncedNetworkUpdaterImpl::OnSetPropertiesResult(
     const std::string& change_guid,
     const NetworkIdentifier& id,
-    bool success,
+    bool is_success,
     const std::string& error_message) {
-  if (!success) {
-    OnError(change_guid, id, "Failed to update properties on network.");
-    return;
+  if (is_success) {
+    VLOG(1) << "Successfully updated network with id "
+            << id.SerializeToString();
+  } else {
+    NET_LOG(ERROR) << "Failed to update network with id "
+                   << id.SerializeToString();
   }
-  VLOG(1) << "Successfully updated network with id " << id.SerializeToString();
-  CleanupUpdate(change_guid, id);
+  HandleShillResult(change_guid, id, is_success);
 }
 
 void SyncedNetworkUpdaterImpl::OnForgetNetworkResult(
     const std::string& change_guid,
     const NetworkIdentifier& id,
-    bool success) {
-  if (!success) {
-    OnError(change_guid, id, "Failed to remove network.");
+    bool is_success) {
+  if (is_success)
+    VLOG(1) << "Successfully deleted network with id "
+            << id.SerializeToString();
+  else
+    NET_LOG(ERROR) << "Failed to remove network with id "
+                   << id.SerializeToString();
+
+  HandleShillResult(change_guid, id, is_success);
+}
+
+void SyncedNetworkUpdaterImpl::HandleShillResult(const std::string& change_guid,
+                                                 const NetworkIdentifier& id,
+                                                 bool is_success) {
+  change_guid_to_timer_map_.erase(change_guid);
+  if (is_success) {
+    tracker_->MarkComplete(change_guid, id);
+    return;
+  }
+
+  if (!tracker_->GetPendingUpdate(change_guid, id)) {
+    VLOG(1) << "Update to network " << id.SerializeToString()
+            << " with change_guid " << change_guid
+            << " is no longer pending.  This is usually because it was "
+               "preempted by another update to the same network.";
+    return;
+  }
+  tracker_->IncrementCompletedAttempts(change_guid, id);
+
+  base::Optional<PendingNetworkConfigurationUpdate> update =
+      tracker_->GetPendingUpdate(change_guid, id);
+  if (update->completed_attempts() >= kMaxRetries) {
+    LOG(ERROR) << "Ran out of retries updating network with id "
+               << id.SerializeToString();
+    tracker_->MarkComplete(change_guid, id);
     return;
   }
 
-  VLOG(1) << "Successfully deleted network with id " << id.SerializeToString();
-  CleanupUpdate(change_guid, id);
+  Retry(*update);
 }
 
 void SyncedNetworkUpdaterImpl::CleanupUpdate(const std::string& change_guid,
@@ -144,6 +212,33 @@ void SyncedNetworkUpdaterImpl::CleanupUpdate(const std::string& change_guid,
   tracker_->MarkComplete(change_guid, id);
 }
 
+void SyncedNetworkUpdaterImpl::Retry(
+    const PendingNetworkConfigurationUpdate& update) {
+  if (update.IsDeleteOperation()) {
+    network_config::mojom::NetworkStatePropertiesPtr network =
+        FindMojoNetwork(update.id());
+    if (!network) {
+      tracker_->MarkComplete(update.change_guid(), update.id());
+      return;
+    }
+
+    StartDeleteOperation(update.change_guid(), update.id(), network->guid);
+    return;
+  }
+
+  StartAddOrUpdateOperation(update.change_guid(), update.id(),
+                            update.specifics().value());
+}
+
+void SyncedNetworkUpdaterImpl::StartTimer(const std::string& change_guid,
+                                          const NetworkIdentifier& id) {
+  change_guid_to_timer_map_[change_guid] = timer_factory_->CreateOneShotTimer();
+  change_guid_to_timer_map_[change_guid]->Start(
+      FROM_HERE, kTimeout,
+      base::BindOnce(&SyncedNetworkUpdaterImpl::OnError, base::Unretained(this),
+                     change_guid, id, kTimedOutErrorMsg));
+}
+
 }  // namespace sync_wifi
 
 }  // namespace chromeos

chromeos/components/sync_wifi/synced_network_updater_impl.h

@@ -6,6 +6,7 @@
 #define CHROMEOS_COMPONENTS_SYNC_WIFI_SYNCED_NETWORK_UPDATER_IMPL_H_
 
 #include "base/memory/weak_ptr.h"
+#include "base/timer/timer.h"
 #include "base/values.h"
 #include "chromeos/components/sync_wifi/network_identifier.h"
 #include "chromeos/components/sync_wifi/pending_network_configuration_tracker.h"
@@ -18,9 +19,11 @@ namespace chromeos {
 
 namespace sync_wifi {
 
+class TimerFactory;
+
 // Implementation of SyncedNetworkUpdater. This class takes add/update/delete
 // requests from the sync backend and applies them to the local network stack
-// using chromeos::NetworkConfigurationHandler.
+// using mojom::CrosNetworkConfig.
 class SyncedNetworkUpdaterImpl
     : public SyncedNetworkUpdater,
       public chromeos::network_config::mojom::CrosNetworkConfigObserver {
@@ -28,7 +31,8 @@ class SyncedNetworkUpdaterImpl
   // |cros_network_config| must outlive this class.
   SyncedNetworkUpdaterImpl(
       std::unique_ptr<PendingNetworkConfigurationTracker> tracker,
-      network_config::mojom::CrosNetworkConfig* cros_network_config);
+      network_config::mojom::CrosNetworkConfig* cros_network_config,
+      std::unique_ptr<TimerFactory> timer_factory);
   ~SyncedNetworkUpdaterImpl() override;
 
   void AddOrUpdateNetwork(
@@ -50,9 +54,21 @@ class SyncedNetworkUpdaterImpl
   void OnNetworkCertificatesChanged() override {}
 
  private:
+  void StartAddOrUpdateOperation(
+      const std::string& change_guid,
+      const NetworkIdentifier& id,
+      const sync_pb::WifiConfigurationSpecificsData& specifics);
+  void StartDeleteOperation(const std::string& change_guid,
+                            const NetworkIdentifier& id,
+                            std::string guid);
+  void StartTimer(const std::string& change_guid, const NetworkIdentifier& id);
+  void Retry(const PendingNetworkConfigurationUpdate& update);
+  void HandleShillResult(const std::string& change_guid,
+                         const NetworkIdentifier& id,
+                         bool is_success);
   void CleanupUpdate(const std::string& change_guid,
                      const NetworkIdentifier& id);
-  network_config::mojom::NetworkStatePropertiesPtr FindLocalNetwork(
+  network_config::mojom::NetworkStatePropertiesPtr FindMojoNetwork(
       const NetworkIdentifier& id);
 
   base::Optional<base::DictionaryValue> ConvertToDictionary(
@@ -80,6 +96,9 @@ class SyncedNetworkUpdaterImpl
   mojo::Receiver<chromeos::network_config::mojom::CrosNetworkConfigObserver>
       cros_network_config_observer_receiver_{this};
   std::vector<network_config::mojom::NetworkStatePropertiesPtr> networks_;
+  std::unique_ptr<TimerFactory> timer_factory_;
+  base::flat_map<std::string, std::unique_ptr<base::OneShotTimer>>
+      change_guid_to_timer_map_;
 
   base::WeakPtrFactory<SyncedNetworkUpdaterImpl> weak_ptr_factory_{this};
 };

chromeos/components/sync_wifi/synced_network_updater_impl_unittest.cc

@@ -12,6 +12,7 @@
 #include "base/run_loop.h"
 #include "base/test/task_environment.h"
 #include "chromeos/components/sync_wifi/fake_pending_network_configuration_tracker.h"
+#include "chromeos/components/sync_wifi/fake_timer_factory.h"
 #include "chromeos/components/sync_wifi/network_identifier.h"
 #include "chromeos/components/sync_wifi/pending_network_configuration_tracker_impl.h"
 #include "chromeos/components/sync_wifi/synced_network_updater_impl.h"
@@ -134,9 +135,12 @@ class SyncedNetworkUpdaterImplTest : public testing::Test {
 
     auto tracker_unique_ptr =
         std::make_unique<FakePendingNetworkConfigurationTracker>();
+    auto timer_factory_unique_ptr = std::make_unique<FakeTimerFactory>();
     tracker_ = tracker_unique_ptr.get();
+    timer_factory_ = timer_factory_unique_ptr.get();
     updater_ = std::make_unique<SyncedNetworkUpdaterImpl>(
-        std::move(tracker_unique_ptr), remote_cros_network_config_.get());
+        std::move(tracker_unique_ptr), remote_cros_network_config_.get(),
+        std::move(timer_factory_unique_ptr));
   }
 
   void TearDown() override {
@@ -145,6 +149,7 @@ class SyncedNetworkUpdaterImplTest : public testing::Test {
   }
 
   FakePendingNetworkConfigurationTracker* tracker() { return tracker_; }
+  FakeTimerFactory* timer_factory() { return timer_factory_; }
   SyncedNetworkUpdaterImpl* updater() { return updater_.get(); }
   chromeos::NetworkStateTestHelper* network_state_helper() {
     return network_state_helper_.get();
@@ -154,6 +159,7 @@ class SyncedNetworkUpdaterImplTest : public testing::Test {
 
  private:
   base::test::TaskEnvironment task_environment_;
+  FakeTimerFactory* timer_factory_;
   FakePendingNetworkConfigurationTracker* tracker_;
   std::unique_ptr<NetworkStateTestHelper> network_state_helper_;
   std::unique_ptr<SyncedNetworkUpdaterImpl> updater_;
@@ -223,7 +229,68 @@ TEST_F(SyncedNetworkUpdaterImplTest, TestFailToAdd_Error) {
   base::RunLoop().RunUntilIdle();
 
   EXPECT_FALSE(FindLocalNetworkById(fred_network_id()));
+
+  // The tracker should no longer be tracking the update because it reached the
+  // max failed number of attempts.
+  EXPECT_FALSE(tracker()->GetPendingUpdateById(fred_network_id()));
+  // Our test tracker holds on to the number of completed attempts after an
+  // update has been removed, and that should be equal to kMaxRetries (3).
+  EXPECT_EQ(3, tracker()->GetCompletedAttempts(fred_network_id()));
+}
+
+TEST_F(SyncedNetworkUpdaterImplTest, TestFailToAdd_Timeout) {
+  network_state_helper()->manager_test()->SetSimulateConfigurationResult(
+      chromeos::FakeShillSimulatedResult::kTimeout);
+
+  updater()->AddOrUpdateNetwork(GenerateTestWifiSpecifics(fred_network_id()));
+  EXPECT_TRUE(tracker()->GetPendingUpdateById(fred_network_id()));
+
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_TRUE(tracker()->GetPendingUpdateById(fred_network_id()));
+  EXPECT_EQ(0, tracker()->GetCompletedAttempts(fred_network_id()));
+
+  timer_factory()->FireAll();
+
+  EXPECT_TRUE(tracker()->GetPendingUpdateById(fred_network_id()));
+  EXPECT_EQ(1, tracker()->GetCompletedAttempts(fred_network_id()));
+
+  timer_factory()->FireAll();
+
+  EXPECT_TRUE(tracker()->GetPendingUpdateById(fred_network_id()));
+  EXPECT_EQ(2, tracker()->GetCompletedAttempts(fred_network_id()));
+
+  timer_factory()->FireAll();
+
+  EXPECT_EQ(3, tracker()->GetCompletedAttempts(fred_network_id()));
+  EXPECT_FALSE(tracker()->GetPendingUpdateById(fred_network_id()));
+}
+
+TEST_F(SyncedNetworkUpdaterImplTest, TestFailToAdd_Timeout_ThenSucceed) {
+  network_state_helper()->manager_test()->SetSimulateConfigurationResult(
+      chromeos::FakeShillSimulatedResult::kTimeout);
+
+  updater()->AddOrUpdateNetwork(GenerateTestWifiSpecifics(fred_network_id()));
   EXPECT_TRUE(tracker()->GetPendingUpdateById(fred_network_id()));
+
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_TRUE(tracker()->GetPendingUpdateById(fred_network_id()));
+  EXPECT_EQ(0, tracker()->GetCompletedAttempts(fred_network_id()));
+
+  timer_factory()->FireAll();
+
+  EXPECT_TRUE(tracker()->GetPendingUpdateById(fred_network_id()));
+  EXPECT_EQ(1, tracker()->GetCompletedAttempts(fred_network_id()));
+
+  network_state_helper()->manager_test()->SetSimulateConfigurationResult(
+      chromeos::FakeShillSimulatedResult::kSuccess);
+
+  timer_factory()->FireAll();
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_FALSE(tracker()->GetPendingUpdateById(fred_network_id()));
+  EXPECT_TRUE(FindLocalNetworkById(fred_network_id()));
 }
 
 TEST_F(SyncedNetworkUpdaterImplTest, TestFailToRemove) {
@@ -240,7 +307,8 @@ TEST_F(SyncedNetworkUpdaterImplTest, TestFailToRemove) {
   base::RunLoop().RunUntilIdle();
 
   EXPECT_TRUE(FindLocalNetworkById(fred_network_id()));
-  EXPECT_TRUE(tracker()->GetPendingUpdateById(fred_network_id()));
+  EXPECT_FALSE(tracker()->GetPendingUpdateById(fred_network_id()));
+  EXPECT_EQ(3, tracker()->GetCompletedAttempts(fred_network_id()));
 }
 
 }  // namespace sync_wifi

chromeos/components/sync_wifi/timer_factory.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chromeos/components/sync_wifi/timer_factory.h"
+
+#include <memory>
+
+namespace chromeos {
+
+namespace sync_wifi {
+
+TimerFactory::~TimerFactory() = default;
+
+std::unique_ptr<base::OneShotTimer> TimerFactory::CreateOneShotTimer() {
+  return std::make_unique<base::OneShotTimer>();
+}
+
+}  // namespace sync_wifi
+
+}  // namespace chromeos

chromeos/components/sync_wifi/timer_factory.h

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROMEOS_COMPONENTS_SYNC_WIFI_TIMER_FACTORY_H_
+#define CHROMEOS_COMPONENTS_SYNC_WIFI_TIMER_FACTORY_H_
+
+#include <memory>
+
+#include "base/macros.h"
+#include "base/memory/ptr_util.h"
+#include "base/timer/timer.h"
+
+namespace chromeos {
+
+namespace sync_wifi {
+
+// Serves as a simple Timer creator, injected into classes that use Timers.
+// Is intended to be overridden during testing in order to stub or mock the
+// Timers used by the object under test.
+class TimerFactory {
+ public:
+  virtual ~TimerFactory();
+
+  virtual std::unique_ptr<base::OneShotTimer> CreateOneShotTimer();
+};
+
+}  // namespace sync_wifi
+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_COMPONENTS_SYNC_WIFI_TIMER_FACTORY_H_

chromeos/components/sync_wifi/wifi_configuration_sync_service.cc

@@ -11,6 +11,7 @@
 #include "base/time/default_clock.h"
 #include "chromeos/components/sync_wifi/pending_network_configuration_tracker_impl.h"
 #include "chromeos/components/sync_wifi/synced_network_updater_impl.h"
+#include "chromeos/components/sync_wifi/timer_factory.h"
 #include "chromeos/components/sync_wifi/wifi_configuration_bridge.h"
 #include "chromeos/services/network_config/public/mojom/cros_network_config.mojom.h"
 #include "components/sync/base/report_unrecoverable_error.h"
@@ -29,7 +30,7 @@ WifiConfigurationSyncService::WifiConfigurationSyncService(
       remote_cros_network_config_.BindNewPipeAndPassReceiver());
   updater_ = std::make_unique<SyncedNetworkUpdaterImpl>(
       std::make_unique<PendingNetworkConfigurationTrackerImpl>(pref_service),
-      remote_cros_network_config_.get());
+      remote_cros_network_config_.get(), std::make_unique<TimerFactory>());
   bridge_ = std::make_unique<sync_wifi::WifiConfigurationBridge>(
       updater_.get(),
       std::make_unique<syncer::ClientTagBasedModelTypeProcessor>(