Augment the PendingNetworConfigurationTracker.

Implements support for tracking the number of attempts
to apply a given update as well as the ability to return a specific
PendingNetworkConfigurationUpdate.

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

chromeos/components/sync_wifi/BUILD.gn

@@ -28,6 +28,7 @@ source_set("unit_tests") {
   testonly = true
   sources = [
     "pending_network_configuration_tracker_impl_unittest.cc",
+    "test_specifics_generator.h",
     "wifi_configuration_bridge_unittest.cc",
   ]
   deps = [

chromeos/components/sync_wifi/pending_network_configuration_tracker.h

@@ -14,8 +14,8 @@
 
 namespace sync_wifi {
 
-// Tracks updates to the local network stack while they are pending.  Updates
-// are stored persistently.
+// Tracks updates to the local network stack while they are pending, including
+// how many attempts have been executed.  Updates are stored persistently.
 class PendingNetworkConfigurationTracker {
  public:
   PendingNetworkConfigurationTracker() = default;
@@ -35,16 +35,19 @@ class PendingNetworkConfigurationTracker {
   virtual void MarkComplete(const std::string& change_guid,
                             const std::string& ssid) = 0;
 
-  // Returns true if this change is still in the list.  If a change is in
-  // process but a new change comes in for the same ssid, the first one will
-  // no longer be tracked.
-  virtual bool IsChangeTracked(const std::string& change_guid,
-                               const std::string& ssid) = 0;
-
-  // Returns all in flight updates.
+  // Returns all pending updates.
   virtual std::vector<PendingNetworkConfigurationUpdate>
   GetPendingUpdates() = 0;
 
+  // Returns the requested pending update, if it exists.
+  virtual base::Optional<PendingNetworkConfigurationUpdate> GetPendingUpdate(
+      const std::string& change_guid,
+      const std::string& ssid) = 0;
+
+  // Increments the number of completed attempts for the given update.
+  virtual void IncrementCompletedAttempts(const std::string& change_guid,
+                                          const std::string& ssid) = 0;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(PendingNetworkConfigurationTracker);
 };

chromeos/components/sync_wifi/pending_network_configuration_tracker_impl.cc

@@ -14,12 +14,34 @@ namespace {
 const char kPendingNetworkConfigurationsPref[] =
     "sync_wifi.pending_network_configuration_updates";
 const char kChangeGuidKey[] = "ChangeGuid";
+const char kCompletedAttemptsKey[] = "CompletedAttempts";
 const char kSpecificsKey[] = "Specifics";
 
 std::string GeneratePath(const std::string& ssid, const std::string& subkey) {
   return base::StringPrintf("%s.%s", ssid.c_str(), subkey.c_str());
 }
 
+sync_wifi::PendingNetworkConfigurationUpdate ConvertToPendingUpdate(
+    base::Value* dict,
+    const std::string ssid) {
+  std::string* change_guid = dict->FindStringKey(kChangeGuidKey);
+  base::Optional<sync_pb::WifiConfigurationSpecificsData> specifics;
+  std::string* specifics_string = dict->FindStringKey(kSpecificsKey);
+  if (!specifics_string->empty()) {
+    sync_pb::WifiConfigurationSpecificsData data;
+    data.ParseFromString(*specifics_string);
+    specifics = data;
+  }
+  base::Optional<int> completed_attempts =
+      dict->FindIntPath(kCompletedAttemptsKey);
+
+  DCHECK(change_guid);
+  DCHECK(completed_attempts);
+
+  return sync_wifi::PendingNetworkConfigurationUpdate(
+      ssid, *change_guid, specifics, completed_attempts.value());
+}
+
 }  // namespace
 
 namespace sync_wifi {
@@ -52,42 +74,47 @@ void PendingNetworkConfigurationTrackerImpl::TrackPendingUpdate(
   dict_.SetPath(GeneratePath(ssid, kChangeGuidKey), base::Value(change_guid));
   dict_.SetPath(GeneratePath(ssid, kSpecificsKey),
                 base::Value(serialized_specifics));
+  dict_.SetPath(GeneratePath(ssid, kCompletedAttemptsKey), base::Value(0));
   pref_service_->Set(kPendingNetworkConfigurationsPref, dict_);
 }
 
 void PendingNetworkConfigurationTrackerImpl::MarkComplete(
     const std::string& change_guid,
     const std::string& ssid) {
-  if (!IsChangeTracked(change_guid, ssid))
+  if (!GetPendingUpdate(change_guid, ssid))
     return;
 
   dict_.RemovePath(ssid);
   pref_service_->Set(kPendingNetworkConfigurationsPref, dict_);
 }
 
-bool PendingNetworkConfigurationTrackerImpl::IsChangeTracked(
+void PendingNetworkConfigurationTrackerImpl::IncrementCompletedAttempts(
     const std::string& change_guid,
     const std::string& ssid) {
-  std::string* found_id =
-      dict_.FindStringPath(GeneratePath(ssid, kChangeGuidKey));
-  return found_id && *found_id == change_guid;
+  std::string path = GeneratePath(ssid, kCompletedAttemptsKey);
+  base::Optional<int> completed_attempts = dict_.FindIntPath(path);
+  dict_.SetIntPath(path, completed_attempts.value() + 1);
 }
 
 std::vector<PendingNetworkConfigurationUpdate>
 PendingNetworkConfigurationTrackerImpl::GetPendingUpdates() {
   std::vector<PendingNetworkConfigurationUpdate> list;
   for (const auto& entry : dict_.DictItems()) {
-    base::Optional<sync_pb::WifiConfigurationSpecificsData> specifics;
-    std::string* specifics_string = entry.second.FindStringKey(kSpecificsKey);
-    if (!specifics_string->empty()) {
-      sync_pb::WifiConfigurationSpecificsData data;
-      data.ParseFromString(*specifics_string);
-      specifics = data;
-    }
-    std::string* change_guid = entry.second.FindStringKey(kChangeGuidKey);
-    list.emplace_back(/*ssid=*/entry.first, *change_guid, specifics);
+    list.push_back(
+        ConvertToPendingUpdate(/*dict=*/&entry.second, /*ssid=*/entry.first));
   }
   return list;
 }
+base::Optional<PendingNetworkConfigurationUpdate>
+PendingNetworkConfigurationTrackerImpl::GetPendingUpdate(
+    const std::string& change_guid,
+    const std::string& ssid) {
+  std::string* found_id =
+      dict_.FindStringPath(GeneratePath(ssid, kChangeGuidKey));
+  if (!found_id || *found_id != change_guid)
+    return base::nullopt;
+
+  return ConvertToPendingUpdate(dict_.FindPath(ssid), ssid);
+}
 
 }  // namespace sync_wifi

chromeos/components/sync_wifi/pending_network_configuration_tracker_impl.h

@@ -33,9 +33,12 @@ class PendingNetworkConfigurationTrackerImpl
       override;
   void MarkComplete(const std::string& change_guid,
                     const std::string& ssid) override;
-  bool IsChangeTracked(const std::string& change_guid,
-                       const std::string& ssid) override;
+  void IncrementCompletedAttempts(const std::string& change_guid,
+                                  const std::string& ssid) override;
   std::vector<PendingNetworkConfigurationUpdate> GetPendingUpdates() override;
+  base::Optional<PendingNetworkConfigurationUpdate> GetPendingUpdate(
+      const std::string& change_guid,
+      const std::string& ssid) override;
 
  private:
   PrefService* pref_service_;

chromeos/components/sync_wifi/pending_network_configuration_tracker_impl_unittest.cc

@@ -2,14 +2,14 @@
 // 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/pending_network_configuration_tracker_impl.h"
-
 #include <memory>
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/optional.h"
+#include "chromeos/components/sync_wifi/pending_network_configuration_tracker_impl.h"
+#include "chromeos/components/sync_wifi/test_specifics_generator.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -59,6 +59,26 @@ class PendingNetworkConfigurationTrackerImplTest : public testing::Test {
     return found_guid && *found_guid == update_guid;
   }
 
+  void AssertTrackerHasMatchingUpdate(
+      const std::string& update_guid,
+      const std::string& ssid,
+      int completed_attempts = 0,
+      const base::Optional<sync_pb::WifiConfigurationSpecificsData> specifics =
+          base::nullopt) {
+    base::Optional<PendingNetworkConfigurationUpdate> update =
+        tracker()->GetPendingUpdate(update_guid, ssid);
+    ASSERT_TRUE(update);
+    ASSERT_EQ(ssid, update->ssid());
+    ASSERT_EQ(completed_attempts, update->completed_attempts());
+    std::string serialized_specifics_wants;
+    std::string serialized_specifics_has;
+    if (specifics)
+      specifics->SerializeToString(&serialized_specifics_wants);
+    if (update->specifics())
+      update->specifics()->SerializeToString(&serialized_specifics_has);
+    ASSERT_EQ(serialized_specifics_wants, serialized_specifics_has);
+  }
+
  private:
   std::unique_ptr<sync_preferences::TestingPrefServiceSyncable>
       test_pref_service_;
@@ -69,23 +89,32 @@ class PendingNetworkConfigurationTrackerImplTest : public testing::Test {
 TEST_F(PendingNetworkConfigurationTrackerImplTest, TestMarkComplete) {
   tracker()->TrackPendingUpdate(kChangeGuid1, kFredSsid,
                                 /*specifics=*/base::nullopt);
-  EXPECT_TRUE(tracker()->IsChangeTracked(kChangeGuid1, kFredSsid));
+  AssertTrackerHasMatchingUpdate(kChangeGuid1, kFredSsid);
   EXPECT_EQ(1u, GetPref()->DictSize());
   EXPECT_TRUE(DoesPrefContainPendingUpdate(kFredSsid, kChangeGuid1));
   tracker()->MarkComplete(kChangeGuid1, kFredSsid);
-  EXPECT_FALSE(tracker()->IsChangeTracked(kChangeGuid1, kFredSsid));
+  EXPECT_FALSE(tracker()->GetPendingUpdate(kChangeGuid1, kFredSsid));
   EXPECT_EQ(0u, GetPref()->DictSize());
 }
 
 TEST_F(PendingNetworkConfigurationTrackerImplTest, TestTwoChangesSameNetwork) {
   tracker()->TrackPendingUpdate(kChangeGuid1, kFredSsid,
                                 /*specifics=*/base::nullopt);
-  EXPECT_TRUE(tracker()->IsChangeTracked(kChangeGuid1, kFredSsid));
+  tracker()->IncrementCompletedAttempts(kChangeGuid1, kFredSsid);
+  AssertTrackerHasMatchingUpdate(kChangeGuid1, kFredSsid,
+                                 /*completed_attempts=*/1);
   EXPECT_EQ(1u, GetPref()->DictSize());
+  EXPECT_EQ(1, tracker()
+                   ->GetPendingUpdate(kChangeGuid1, kFredSsid)
+                   ->completed_attempts());
+
   tracker()->TrackPendingUpdate(kChangeGuid2, kFredSsid,
                                 /*specifics=*/base::nullopt);
-  EXPECT_FALSE(tracker()->IsChangeTracked(kChangeGuid1, kFredSsid));
-  EXPECT_TRUE(tracker()->IsChangeTracked(kChangeGuid2, kFredSsid));
+  EXPECT_FALSE(tracker()->GetPendingUpdate(kChangeGuid1, kFredSsid));
+  AssertTrackerHasMatchingUpdate(kChangeGuid2, kFredSsid);
+  EXPECT_EQ(0, tracker()
+                   ->GetPendingUpdate(kChangeGuid2, kFredSsid)
+                   ->completed_attempts());
   EXPECT_EQ(1u, GetPref()->DictSize());
 }
 
@@ -93,13 +122,13 @@ TEST_F(PendingNetworkConfigurationTrackerImplTest,
        TestTwoChangesDifferentNetworks) {
   tracker()->TrackPendingUpdate(kChangeGuid1, kFredSsid,
                                 /*specifics=*/base::nullopt);
-  EXPECT_TRUE(tracker()->IsChangeTracked(kChangeGuid1, kFredSsid));
+  AssertTrackerHasMatchingUpdate(kChangeGuid1, kFredSsid);
   EXPECT_TRUE(DoesPrefContainPendingUpdate(kFredSsid, kChangeGuid1));
   EXPECT_EQ(1u, GetPref()->DictSize());
   tracker()->TrackPendingUpdate(kChangeGuid2, kMangoSsid,
                                 /*specifics=*/base::nullopt);
-  EXPECT_TRUE(tracker()->IsChangeTracked(kChangeGuid1, kFredSsid));
-  EXPECT_TRUE(tracker()->IsChangeTracked(kChangeGuid2, kMangoSsid));
+  AssertTrackerHasMatchingUpdate(kChangeGuid1, kFredSsid);
+  AssertTrackerHasMatchingUpdate(kChangeGuid2, kMangoSsid);
   EXPECT_TRUE(DoesPrefContainPendingUpdate(kFredSsid, kChangeGuid1));
   EXPECT_TRUE(DoesPrefContainPendingUpdate(kMangoSsid, kChangeGuid2));
   EXPECT_EQ(2u, GetPref()->DictSize());
@@ -125,4 +154,36 @@ TEST_F(PendingNetworkConfigurationTrackerImplTest, TestGetPendingUpdates) {
   EXPECT_EQ(kMangoSsid, list[0].ssid());
 }
 
+TEST_F(PendingNetworkConfigurationTrackerImplTest, TestGetPendingUpdate) {
+  sync_pb::WifiConfigurationSpecificsData specifics =
+      CreateSpecifics(kFredSsid);
+  tracker()->TrackPendingUpdate(kChangeGuid1, kFredSsid, specifics);
+
+  AssertTrackerHasMatchingUpdate(kChangeGuid1, kFredSsid,
+                                 /*completed_attempts=*/0, specifics);
+
+  EXPECT_FALSE(tracker()->GetPendingUpdate(kChangeGuid2, kMangoSsid));
+}
+
+TEST_F(PendingNetworkConfigurationTrackerImplTest, TestRetryCounting) {
+  tracker()->TrackPendingUpdate(kChangeGuid1, kFredSsid,
+                                /*specifics=*/base::nullopt);
+  AssertTrackerHasMatchingUpdate(kChangeGuid1, kFredSsid);
+  EXPECT_EQ(1u, GetPref()->DictSize());
+  EXPECT_EQ(0, tracker()
+                   ->GetPendingUpdate(kChangeGuid1, kFredSsid)
+                   ->completed_attempts());
+  tracker()->IncrementCompletedAttempts(kChangeGuid1, kFredSsid);
+  tracker()->IncrementCompletedAttempts(kChangeGuid1, kFredSsid);
+  tracker()->IncrementCompletedAttempts(kChangeGuid1, kFredSsid);
+  EXPECT_EQ(3, tracker()
+                   ->GetPendingUpdate(kChangeGuid1, kFredSsid)
+                   ->completed_attempts());
+  tracker()->IncrementCompletedAttempts(kChangeGuid1, kFredSsid);
+  tracker()->IncrementCompletedAttempts(kChangeGuid1, kFredSsid);
+  EXPECT_EQ(5, tracker()
+                   ->GetPendingUpdate(kChangeGuid1, kFredSsid)
+                   ->completed_attempts());
+}
+
 }  // namespace sync_wifi

chromeos/components/sync_wifi/pending_network_configuration_update.cc

@@ -9,8 +9,12 @@ namespace sync_wifi {
 PendingNetworkConfigurationUpdate::PendingNetworkConfigurationUpdate(
     const std::string& ssid,
     const std::string& change_guid,
-    const base::Optional<sync_pb::WifiConfigurationSpecificsData>& specifics)
-    : ssid_(ssid), change_guid_(change_guid), specifics_(specifics) {}
+    const base::Optional<sync_pb::WifiConfigurationSpecificsData>& specifics,
+    int completed_attempts)
+    : ssid_(ssid),
+      change_guid_(change_guid),
+      specifics_(specifics),
+      completed_attempts_(completed_attempts) {}
 
 PendingNetworkConfigurationUpdate::PendingNetworkConfigurationUpdate(
     const PendingNetworkConfigurationUpdate& update) = default;

chromeos/components/sync_wifi/pending_network_configuration_update.h

@@ -14,13 +14,15 @@
 
 namespace sync_wifi {
 
-// Represents a change to the local network stack which hasn't been saved yet.
+// Represents a change to the local network stack which hasn't been saved yet,
+// including the number of completed attempts to save it.
 class PendingNetworkConfigurationUpdate {
  public:
   PendingNetworkConfigurationUpdate(
       const std::string& ssid,
       const std::string& change_guid,
-      const base::Optional<sync_pb::WifiConfigurationSpecificsData>& specifics);
+      const base::Optional<sync_pb::WifiConfigurationSpecificsData>& specifics,
+      int completed_attempts);
   PendingNetworkConfigurationUpdate(
       const PendingNetworkConfigurationUpdate& update);
   virtual ~PendingNetworkConfigurationUpdate();
@@ -38,6 +40,8 @@ class PendingNetworkConfigurationUpdate {
     return specifics_;
   }
 
+  int completed_attempts() { return completed_attempts_; }
+
   // Returns |true| if the update operation is deleting a network.
   bool IsDeleteOperation() const;
 
@@ -45,6 +49,7 @@ class PendingNetworkConfigurationUpdate {
   const std::string ssid_;
   const std::string change_guid_;
   const base::Optional<sync_pb::WifiConfigurationSpecificsData> specifics_;
+  int completed_attempts_;
 };
 
 }  // namespace sync_wifi

chromeos/components/sync_wifi/test_specifics_generator.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.
+
+#include "components/sync/protocol/wifi_configuration_specifics.pb.h"
+
+#ifndef CHROMEOS_COMPONENTS_SYNC_WIFI_TEST_SPECIFICS_GENERATOR_H_
+#define CHROMEOS_COMPONENTS_SYNC_WIFI_TEST_SPECIFICS_GENERATOR_H_
+
+namespace sync_wifi {
+
+namespace {
+
+sync_pb::WifiConfigurationSpecificsData CreateSpecifics(
+    const std::string& ssid) {
+  sync_pb::WifiConfigurationSpecificsData specifics;
+  specifics.set_ssid(ssid);
+  specifics.set_security_type(
+      sync_pb::WifiConfigurationSpecificsData::SECURITY_TYPE_PSK);
+  specifics.set_passphrase("password");
+  specifics.set_automatically_connect(
+      sync_pb::WifiConfigurationSpecificsData::AUTOMATICALLY_CONNECT_ENABLED);
+  specifics.set_is_preferred(
+      sync_pb::WifiConfigurationSpecificsData::IS_PREFERRED_ENABLED);
+  specifics.set_metered(
+      sync_pb::WifiConfigurationSpecificsData::METERED_OPTION_AUTO);
+  sync_pb::WifiConfigurationSpecificsData_ProxyConfiguration proxy_config;
+  proxy_config.set_proxy_option(sync_pb::WifiConfigurationSpecificsData::
+                                    ProxyConfiguration::PROXY_OPTION_DISABLED);
+  specifics.mutable_proxy_configuration()->CopyFrom(proxy_config);
+  return specifics;
+}
+
+}  // namespace
+
+}  // namespace sync_wifi
+
+#endif  // CHROMEOS_COMPONENTS_SYNC_WIFI_TEST_SPECIFICS_GENERATOR_H_

chromeos/components/sync_wifi/wifi_configuration_bridge_unittest.cc

@@ -12,6 +12,7 @@
 #include "base/run_loop.h"
 #include "base/test/task_environment.h"
 #include "chromeos/components/sync_wifi/synced_network_updater.h"
+#include "chromeos/components/sync_wifi/test_specifics_generator.h"
 #include "components/sync/model/entity_change.h"
 #include "components/sync/model/metadata_batch.h"
 #include "components/sync/model/mock_model_type_change_processor.h"
@@ -39,24 +40,6 @@ using testing::UnorderedElementsAre;
 const char kSsidMeow[] = "meow";
 const char kSsidWoof[] = "woof";
 
-WifiConfigurationSpecificsData CreateSpecifics(const std::string& ssid) {
-  WifiConfigurationSpecificsData specifics;
-  specifics.set_ssid(ssid);
-  specifics.set_security_type(
-      WifiConfigurationSpecificsData::SECURITY_TYPE_PSK);
-  specifics.set_passphrase("password");
-  specifics.set_automatically_connect(
-      WifiConfigurationSpecificsData::AUTOMATICALLY_CONNECT_ENABLED);
-  specifics.set_is_preferred(
-      WifiConfigurationSpecificsData::IS_PREFERRED_ENABLED);
-  specifics.set_metered(WifiConfigurationSpecificsData::METERED_OPTION_AUTO);
-  sync_pb::WifiConfigurationSpecificsData_ProxyConfiguration proxy_config;
-  proxy_config.set_proxy_option(WifiConfigurationSpecificsData::
-                                    ProxyConfiguration::PROXY_OPTION_DISABLED);
-  specifics.mutable_proxy_configuration()->CopyFrom(proxy_config);
-  return specifics;
-}
-
 std::unique_ptr<syncer::EntityData> GenerateWifiEntityData(
     const sync_pb::WifiConfigurationSpecificsData& data) {
   auto entity_data = std::make_unique<syncer::EntityData>();