Persist broken and recently-broken alt-svcs to prefs in HttpServerPropertiesManager

net/http/http_server_properties_manager.cc

Index: net/http/http_server_properties_manager.cc
diff --git a/net/http/http_server_properties_manager.cc b/net/http/http_server_properties_manager.cc
index 6a4aaaaf62691584e548501b3982f8b24ac08b47..81727a2f21b3c1cea2f1cb27154836be4c9c886d 100644
--- a/net/http/http_server_properties_manager.cc
+++ b/net/http/http_server_properties_manager.cc
@@ -49,6 +49,12 @@ const int kMaxSupportsSpdyServerHostsToPersist = 300;
 // Persist 200 ServerNetworkStats.
 const int kMaxServerNetworkStatsHostsToPersist = 200;
 
+// Persist 200 BrokenAlternativeServices
+const int kMaxBrokenAlternativeServicesToPersist = 200;

[Zhongyi Shi, 2017/06/16 04:17:09]

We used to decide the number of Alt Svc to persist based on data collected from
histograms. I would suggest adding a histogram as well to log how many
broken/recently broken AlternativeServices and are persisted in the real world
so as to tune the size.

[wangyix1, 2017/06/16 18:41:15]

Done.

+
+// Persist 200 RecentlyBrokenAlternativeServices
+const int kMaxRecentlyBrokenAlternativeServicesToPersist = 200;
+
 const char kVersionKey[] = "version";
 const char kServersKey[] = "servers";
 const char kSupportsSpdyKey[] = "supports_spdy";
@@ -64,6 +70,9 @@ const char kPortKey[] = "port";
 const char kExpirationKey[] = "expiration";
 const char kNetworkStatsKey[] = "network_stats";
 const char kSrttKey[] = "srtt";
+const char kBrokenAlternativeServicesKey[] = "broken_alternative_services";
+const char kRecentlyBrokenAlternativeServicesKey[] =
+    "recently_broken_alternative_services";
 
 }  // namespace
 
@@ -76,13 +85,25 @@ HttpServerPropertiesManager::HttpServerPropertiesManager(
     PrefDelegate* pref_delegate,
     scoped_refptr<base::SingleThreadTaskRunner> pref_task_runner,
     scoped_refptr<base::SingleThreadTaskRunner> network_task_runner)
+    : HttpServerPropertiesManager(pref_delegate,
+                                  pref_task_runner,
+                                  network_task_runner,
+                                  nullptr) {}
+
+HttpServerPropertiesManager::HttpServerPropertiesManager(
+    PrefDelegate* pref_delegate,
+    scoped_refptr<base::SingleThreadTaskRunner> pref_task_runner,
+    scoped_refptr<base::SingleThreadTaskRunner> network_task_runner,
+    base::TickClock* clock)
     : pref_task_runner_(std::move(pref_task_runner)),
       pref_delegate_(pref_delegate),
       setting_prefs_(false),
+      clock_(clock ? clock : &default_clock_),
       is_initialized_(false),
       network_task_runner_(std::move(network_task_runner)) {
   DCHECK(pref_task_runner_->RunsTasksInCurrentSequence());
   DCHECK(pref_delegate_);
+  DCHECK(clock_);
   pref_weak_ptr_factory_.reset(
       new base::WeakPtrFactory<HttpServerPropertiesManager>(this));
   pref_weak_ptr_ = pref_weak_ptr_factory_->GetWeakPtr();
@@ -103,7 +124,7 @@ void HttpServerPropertiesManager::InitializeOnNetworkSequence() {
 
   network_weak_ptr_factory_.reset(
       new base::WeakPtrFactory<HttpServerPropertiesManager>(this));
-  http_server_properties_impl_.reset(new HttpServerPropertiesImpl());
+  http_server_properties_impl_.reset(new HttpServerPropertiesImpl(clock_));
 
   network_prefs_update_timer_.reset(new base::OneShotTimer);
   network_prefs_update_timer_->SetTaskRunner(network_task_runner_);
@@ -514,6 +535,64 @@ void HttpServerPropertiesManager::UpdateCacheFromPrefsOnPrefSequence() {
     detected_corrupted_prefs = true;
   }
 
+  std::unique_ptr<RecentlyBrokenAlternativeServices>
+      recently_broken_alternative_services;
+  const base::ListValue* recently_broken_alt_svc_list = nullptr;
+
+  std::unique_ptr<BrokenAlternativeServiceList> broken_alternative_service_list;
+  const base::ListValue* broken_alt_svc_list = nullptr;
+
+  // Read recently broken alternative services if they exist.
+  if (http_server_properties_dict.GetListWithoutPathExpansion(
+          kRecentlyBrokenAlternativeServicesKey,
+          &recently_broken_alt_svc_list)) {
+    recently_broken_alternative_services.reset(
+        new RecentlyBrokenAlternativeServices(
+            RecentlyBrokenAlternativeServices::NO_AUTO_EVICT));
+
+    for (auto it = recently_broken_alt_svc_list->begin();
+         it != recently_broken_alt_svc_list->end(); ++it) {
+      const base::DictionaryValue* entry_dict = nullptr;
+      if (!it->GetAsDictionary(&entry_dict) ||
+          !AddRecentlyBrokenAlternativeServiceData(
+              entry_dict, recently_broken_alternative_services.get())) {
+        DVLOG(1) << "Malformed http_server_properties for recently broken "
+                 << "alternative services list.";
+        detected_corrupted_prefs = true;
+      }
+    }
+
+    // Read broken alternative services if they exist
+    if (http_server_properties_dict.GetListWithoutPathExpansion(
+            kBrokenAlternativeServicesKey, &broken_alt_svc_list)) {
+      broken_alternative_service_list.reset(new BrokenAlternativeServiceList());
+
+      for (auto it = broken_alt_svc_list->begin();
+           it != broken_alt_svc_list->end(); ++it) {
+        const base::DictionaryValue* entry_dict = nullptr;
+        AlternativeService alternative_service;
+        base::TimeTicks expiration_time_ticks;
+        if (!it->GetAsDictionary(&entry_dict) ||
+            !ExtractBrokenAlternativeServiceData(
+                entry_dict, &alternative_service, &expiration_time_ticks)) {
+          DVLOG(1) << "Malformed http_server_properties for broken alternative "
+                   << "services list.";
+          detected_corrupted_prefs = true;
+          continue;
+        }
+        if (recently_broken_alternative_services->Peek(alternative_service) !=
+            recently_broken_alternative_services->end()) {
+          broken_alternative_service_list->push_back(
+              std::make_pair(alternative_service, expiration_time_ticks));
+        } else {
+          DVLOG(1) << "Broken alternative service does not have an entry in "
+                   << "map of recently broken alternative services. "
+                   << "Will not be added.";
+        }
+      }
+    }
+  }
+
   network_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(
@@ -522,9 +601,65 @@ void HttpServerPropertiesManager::UpdateCacheFromPrefsOnPrefSequence() {
           base::Owned(alternative_service_map.release()), base::Owned(addr),
           base::Owned(server_network_stats_map.release()),
           base::Owned(quic_server_info_map.release()),
+          base::Passed(&broken_alternative_service_list),
+          base::Passed(&recently_broken_alternative_services),
           detected_corrupted_prefs));
 }
 
+bool HttpServerPropertiesManager::ExtractBrokenAlternativeServiceData(
+    const base::DictionaryValue* broken_alternative_service_dict,
+    AlternativeService* alternative_service,
+    base::TimeTicks* expiration_time_ticks) {

[Zhongyi Shi, 2017/06/16 04:17:09]

Can we just change this method to take DictionaryValue as input, and adds a
entry to broken_alternative_service_list if valid? 

[wangyix1, 2017/06/16 18:41:15]

The reason for this can be seen in how this function is used in
UpdateCacheFromPrefsOnPrefSequence(). I only want to add an entry to
|broken_alternative_service_list| if the broken alt-svc has an entry in
|recently_broken_alternative_services|. The logic to check for this is currently
not the responsibility of this function. Should I move that logic into this
function? The params would then be:

const DictionaryValue* broken_alternative_service_dict,

const RecentlyBrokenAlternativeServices& recently_broken_alternative_services,

BrokenAlternativeServiceList* broken_alternative_service_list

+  DCHECK(broken_alternative_service_dict);
+
+  if (broken_alternative_service_dict->size() != 1u)
+    return false;
+
+  base::DictionaryValue::Iterator it(*broken_alternative_service_dict);
+
+  const std::string& alternative_service_string = it.key();
+  if (!AlternativeService::FromString(alternative_service_string,
+                                      alternative_service))
+    return false;
+
+  int64_t expiration_int64;
+  std::string time_ticks_string;
+  if (!it.value().GetAsString(&time_ticks_string))
+    return false;
+  if (!base::StringToInt64(time_ticks_string, &expiration_int64))
+    return false;
+  time_t expiration_time_t = static_cast<time_t>(expiration_int64);
+  // Convert expiration from time_t to Time to TimeTicks
+  *expiration_time_ticks =
+      clock_->NowTicks() +
+      (base::Time::FromTimeT(expiration_time_t) - base::Time::Now());
+
+  return true;
+}
+
+bool HttpServerPropertiesManager::AddRecentlyBrokenAlternativeServiceData(
+    const base::DictionaryValue* recently_broken_alternative_service_dict,
+    RecentlyBrokenAlternativeServices* recently_broken_alternative_services) {
+  DCHECK(recently_broken_alternative_service_dict);
+
+  if (recently_broken_alternative_service_dict->size() != 1u)
+    return false;
+
+  base::DictionaryValue::Iterator it(*recently_broken_alternative_service_dict);
+
+  AlternativeService alt_service;
+  const std::string& alt_service_string = it.key();
+  if (!AlternativeService::FromString(alt_service_string, &alt_service))
+    return false;
+
+  int broken_count;
+  if (!it.value().GetAsInteger(&broken_count))
+    return false;
+
+  recently_broken_alternative_services->Put(alt_service, broken_count);
+  return true;
+}
+
 bool HttpServerPropertiesManager::AddServersData(
     const base::DictionaryValue& servers_dict,
     ServerList* spdy_servers,
@@ -780,6 +915,10 @@ void HttpServerPropertiesManager::UpdateCacheFromPrefsOnNetworkSequence(
     IPAddress* last_quic_address,
     ServerNetworkStatsMap* server_network_stats_map,
     QuicServerInfoMap* quic_server_info_map,
+    std::unique_ptr<BrokenAlternativeServiceList>
+        broken_alternative_service_list,
+    std::unique_ptr<RecentlyBrokenAlternativeServices>
+        recently_broken_alternative_services,
     bool detected_corrupted_prefs) {
   // Preferences have the master data because admins might have pushed new
   // preferences. Update the cached data with new data from preferences.
@@ -804,6 +943,13 @@ void HttpServerPropertiesManager::UpdateCacheFromPrefsOnNetworkSequence(
 
   http_server_properties_impl_->SetQuicServerInfoMap(quic_server_info_map);
 
+  if (recently_broken_alternative_services) {
+    DCHECK(broken_alternative_service_list);
+    http_server_properties_impl_->SetBrokenAndRecentlyBrokenAlternativeServices(
+        std::move(broken_alternative_service_list),
+        std::move(recently_broken_alternative_services));
+  }
+
   // Update the prefs with what we have read (delete all corrupted prefs).
   if (detected_corrupted_prefs)
     ScheduleUpdatePrefsOnNetworkSequence(DETECTED_CORRUPTED_PREFS);
@@ -869,9 +1015,6 @@ void HttpServerPropertiesManager::UpdatePrefsFromCacheOnNetworkSequence(
       if (alternative_service.host.empty()) {
         alternative_service.host = server.host();
       }
-      if (IsAlternativeServiceBroken(alternative_service)) {
-        continue;
-      }
       notbroken_alternative_service_info_vector.push_back(
           alternative_service_info);
     }
@@ -915,6 +1058,41 @@ void HttpServerPropertiesManager::UpdatePrefsFromCacheOnNetworkSequence(
     }
   }
 
+  // Make copy of list of broken alternative services stored in
+  // |http_server_properties_impl_|
+  BrokenAlternativeServiceList* broken_alt_svc_list_copy = nullptr;
+  const BrokenAlternativeServiceList& broken_alternative_service_list =
+      http_server_properties_impl_->broken_alternative_service_list();
+  if (broken_alternative_service_list.size() > 0) {
+    broken_alt_svc_list_copy = new BrokenAlternativeServiceList();
+    size_t count = 0;
+    for (auto it = broken_alternative_service_list.begin();
+         it != broken_alternative_service_list.end() &&
+         count < kMaxBrokenAlternativeServicesToPersist;
+         ++it) {
+      broken_alt_svc_list_copy->push_back(*it);
+      ++count;
+    }
+  }
+
+  // Make copy of RecentlyBrokenAternativeServices stored in
+  // |http_server_properties_impl_|.
+  RecentlyBrokenAlternativeServices* recently_broken_alt_svc_copy = nullptr;
+  const RecentlyBrokenAlternativeServices& recently_broken_alt_services =
+      http_server_properties_impl_->recently_broken_alternative_services();
+  if (recently_broken_alt_services.size() > 0) {
+    recently_broken_alt_svc_copy = new RecentlyBrokenAlternativeServices(
+        kMaxRecentlyBrokenAlternativeServicesToPersist);
+    size_t count = 0;
+    for (auto it = recently_broken_alt_services.rbegin();
+         it != recently_broken_alt_services.rend() &&
+         count < kMaxRecentlyBrokenAlternativeServicesToPersist;
+         ++it) {
+      recently_broken_alt_svc_copy->Put(it->first, it->second);
+      ++count;
+    }
+  }
+
   IPAddress* last_quic_addr = new IPAddress;
   http_server_properties_impl_->GetSupportsQuic(last_quic_addr);
   // Update the preferences on the pref thread.
@@ -924,7 +1102,9 @@ void HttpServerPropertiesManager::UpdatePrefsFromCacheOnNetworkSequence(
           &HttpServerPropertiesManager::UpdatePrefsOnPrefThread, pref_weak_ptr_,
           base::Owned(spdy_server_list), base::Owned(alternative_service_map),
           base::Owned(last_quic_addr), base::Owned(server_network_stats_map),
-          base::Owned(quic_server_info_map), completion));
+          base::Owned(quic_server_info_map),
+          base::Owned(broken_alt_svc_list_copy),
+          base::Owned(recently_broken_alt_svc_copy), completion));
 }
 
 // A local or temporary data structure to hold |supports_spdy|, SpdySettings,
@@ -962,6 +1142,8 @@ void HttpServerPropertiesManager::UpdatePrefsOnPrefThread(
     IPAddress* last_quic_address,
     ServerNetworkStatsMap* server_network_stats_map,
     QuicServerInfoMap* quic_server_info_map,
+    BrokenAlternativeServiceList* broken_alternative_service_list,
+    RecentlyBrokenAlternativeServices* recently_broken_alternative_services,
     const base::Closure& completion) {
   typedef base::MRUCache<url::SchemeHostPort, ServerPref> ServerPrefMap;
   ServerPrefMap server_pref_map(ServerPrefMap::NO_AUTO_EVICT);
@@ -1048,6 +1230,12 @@ void HttpServerPropertiesManager::UpdatePrefsOnPrefThread(
   SaveQuicServerInfoMapToServerPrefs(quic_server_info_map,
                                      &http_server_properties_dict);
 
+  SaveBrokenAlternativeServiceListToPrefs(broken_alternative_service_list,
+                                          &http_server_properties_dict);
+
+  SaveRecentlyBrokenAlternativeServicesToPrefs(
+      recently_broken_alternative_services, &http_server_properties_dict);
+
   setting_prefs_ = true;
   pref_delegate_->SetServerProperties(http_server_properties_dict);
   setting_prefs_ = false;
@@ -1144,6 +1332,57 @@ void HttpServerPropertiesManager::SaveQuicServerInfoMapToServerPrefs(
       kQuicServers, std::move(quic_servers_dict));
 }
 
+void HttpServerPropertiesManager::SaveBrokenAlternativeServiceListToPrefs(
+    const BrokenAlternativeServiceList* broken_alternative_service_list,
+    base::DictionaryValue* http_server_properties_dict) {
+  if (!broken_alternative_service_list)
+    return;
+
+  base::ListValue* broken_alt_services_list = new base::ListValue;
+  for (const auto& entry : *broken_alternative_service_list) {
+    const AlternativeService& alt_service = entry.first;
+    const base::TimeTicks& expiration_time_ticks = entry.second;
+    // Convert expiration from TimeTicks to Time to time_t
+    time_t expiration_time_t =
+        (base::Time::Now() + (expiration_time_ticks - clock_->NowTicks()))
+            .ToTimeT();
+    int64_t expiration_int64 = static_cast<int64_t>(expiration_time_t);
+    std::unique_ptr<base::DictionaryValue> broken_alt_service_entry_dict =
+        base::MakeUnique<base::DictionaryValue>();
+    broken_alt_service_entry_dict->SetStringWithoutPathExpansion(
+        alt_service.ToString(), base::Int64ToString(expiration_int64));
+    broken_alt_services_list->Append(std::move(broken_alt_service_entry_dict));
+  }
+  http_server_properties_dict->SetWithoutPathExpansion(
+      kBrokenAlternativeServicesKey,
+      std::unique_ptr<base::Value>(broken_alt_services_list));
+}
+
+void HttpServerPropertiesManager::SaveRecentlyBrokenAlternativeServicesToPrefs(
+    const RecentlyBrokenAlternativeServices*
+        recently_broken_alternative_services,
+    base::DictionaryValue* http_server_properties_dict) {
+  if (!recently_broken_alternative_services)
+    return;
+
+  // JSON list will have entries ordered from least recent to most recent.
+  base::ListValue* recently_broken_alt_services_list = new base::ListValue;
+  for (auto it = recently_broken_alternative_services->rbegin();
+       it != recently_broken_alternative_services->rend(); ++it) {
+    const AlternativeService& alt_service = it->first;
+    int broken_count = it->second;
+    std::unique_ptr<base::DictionaryValue> broken_alt_service_entry_dict =
+        base::MakeUnique<base::DictionaryValue>();
+    broken_alt_service_entry_dict->SetIntegerWithoutPathExpansion(
+        alt_service.ToString(), broken_count);
+    recently_broken_alt_services_list->Append(
+        std::move(broken_alt_service_entry_dict));
+  }
+  http_server_properties_dict->SetWithoutPathExpansion(
+      kRecentlyBrokenAlternativeServicesKey,
+      std::unique_ptr<base::Value>(recently_broken_alt_services_list));
+}
+
 void HttpServerPropertiesManager::OnHttpServerPropertiesChanged() {
   DCHECK(pref_task_runner_->RunsTasksInCurrentSequence());
   if (!setting_prefs_)