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

This is a reland of a66ebc8a
Original change's description:
> Persist broken and recently-broken alt-svcs to prefs in HttpServerPropertiesManager
> 
> Modify TickClock dependency injection for BrokenAlternativeServices to use a setter instead of a constructor param.
> Add TickClock dependency injection for HttpServerPropertiesImpl and HttpServerPropertiesManager for testing.
> 
> Add BrokenAlternativeService::Clear() and update HttpServerPropertiesImpl::Clear() to call that.
> 
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_ubsan_rel_ng
> 
> BUG=705029
> 
> Change-Id: Idb411192e47d275cde3362b479a6b9e9fa773a17
> Reviewed-on: https://chromium-review.googlesource.com/562604
> Reviewed-by: Zhongyi Shi <zhongyi@chromium.org>
> Reviewed-by: Steven Holte <holte@chromium.org>
> Commit-Queue: Yixin Wang <wangyix@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#486519}

Bug: 705029
Change-Id: I14f36cc6014f001d9aefa4678a7bfa8f621b2834
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_ubsan_rel_ng
Reviewed-on: https://chromium-review.googlesource.com/571044Reviewed-by: Zhongyi Shi <zhongyi@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Commit-Queue: Yixin Wang <wangyix@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487342}

net/http/broken_alternative_services.cc

@@ -114,16 +114,6 @@ void BrokenAlternativeServices::ConfirmAlternativeService(
   }
 }
 
-const BrokenAlternativeServiceList&
-BrokenAlternativeServices::broken_alternative_service_list() const {
-  return broken_alternative_service_list_;
-}
-
-const RecentlyBrokenAlternativeServices&
-BrokenAlternativeServices::recently_broken_alternative_services() const {
-  return recently_broken_alternative_services_;
-}
-
 void BrokenAlternativeServices::SetBrokenAndRecentlyBrokenAlternativeServices(
     std::unique_ptr<BrokenAlternativeServiceList>
         broken_alternative_service_list,
@@ -177,13 +167,14 @@ void BrokenAlternativeServices::SetBrokenAndRecentlyBrokenAlternativeServices(
     }
   }
 
-  // Merge |broken_alternative_service_list| with
-  // |broken_alternative_service_list_|. Both should already be sorted by
-  // expiration time. std::list::merge() will not invalidate any iterators
-  // of either list, so all iterators in |broken_alternative_service_map_|
-  // remain valid.
-  broken_alternative_service_list_.merge(
-      *broken_alternative_service_list,
+  // Append |broken_alternative_service_list| to
+  // |broken_alternative_service_list_|, then sort the resulting list.
+  // Neither operations will invalidate any iterators of either list,
+  // so all iterators in |broken_alternative_service_map_| remain valid.
+  broken_alternative_service_list_.splice(
+      broken_alternative_service_list_.end(), *broken_alternative_service_list);
+
+  broken_alternative_service_list_.sort(
       [](const std::pair<AlternativeService, base::TimeTicks>& lhs,
          const std::pair<AlternativeService, base::TimeTicks>& rhs) -> bool {
         return lhs.second < rhs.second;
@@ -198,6 +189,16 @@ void BrokenAlternativeServices::SetBrokenAndRecentlyBrokenAlternativeServices(
     ScheduleBrokenAlternateProtocolMappingsExpiration();
 }
 
+const BrokenAlternativeServiceList&
+BrokenAlternativeServices::broken_alternative_service_list() const {
+  return broken_alternative_service_list_;
+}
+
+const RecentlyBrokenAlternativeServices&
+BrokenAlternativeServices::recently_broken_alternative_services() const {
+  return recently_broken_alternative_services_;
+}
+
 bool BrokenAlternativeServices::AddToBrokenAlternativeServiceListAndMap(
     const AlternativeService& alternative_service,
     base::TimeTicks expiration,

net/http/broken_alternative_services.h

@@ -84,6 +84,10 @@ class NET_EXPORT_PRIVATE BrokenAlternativeServices {
   // expiration time.
   // All AlternativeServices in |broken_alternative_service_list| must exist in
   // |recently_broken_alternative_services|.
+  //
+  // If a broken/recently-broken alt svc that's being added is already stored,
+  // the stored expiration/broken-count for that alt svc will be overwritten
+  // with the new value.
   void SetBrokenAndRecentlyBrokenAlternativeServices(
       std::unique_ptr<BrokenAlternativeServiceList>
           broken_alternative_service_list,

net/http/http_server_properties.h

@@ -104,6 +104,7 @@ struct NET_EXPORT AlternativeService {
            std::tie(other.protocol, other.host, other.port);
   }
 
+  // Output format: "protocol host:port", e.g. "h2 www.google.com:1234".
   std::string ToString() const;
 
   NextProto protocol;
@@ -115,6 +116,12 @@ NET_EXPORT_PRIVATE std::ostream& operator<<(
     std::ostream& os,
     const AlternativeService& alternative_service);
 
+struct AlternativeServiceHash {
+  size_t operator()(const net::AlternativeService& entry) const {
+    return entry.protocol ^ std::hash<std::string>()(entry.host) ^ entry.port;
+  }
+};
+
 class NET_EXPORT_PRIVATE AlternativeServiceInfo {
  public:
   static AlternativeServiceInfo CreateHttp2AlternativeServiceInfo(

net/http/http_server_properties_impl.cc

@@ -20,17 +20,10 @@
 
 namespace net {
 
-HttpServerPropertiesImpl::HttpServerPropertiesImpl()
-    : HttpServerPropertiesImpl(nullptr) {}
-
-HttpServerPropertiesImpl::HttpServerPropertiesImpl(
-    base::TickClock* broken_alternative_services_clock)
-    : broken_alternative_services_(this,
-                                   broken_alternative_services_clock
-                                       ? broken_alternative_services_clock
-                                       : &broken_alternative_services_clock_),
-      spdy_servers_map_(SpdyServersMap::NO_AUTO_EVICT),
+HttpServerPropertiesImpl::HttpServerPropertiesImpl(base::TickClock* clock)
+    : spdy_servers_map_(SpdyServersMap::NO_AUTO_EVICT),
       alternative_service_map_(AlternativeServiceMap::NO_AUTO_EVICT),
+      broken_alternative_services_(this, clock ? clock : &default_clock_),
       server_network_stats_map_(ServerNetworkStatsMap::NO_AUTO_EVICT),
       quic_server_info_map_(QuicServerInfoMap::NO_AUTO_EVICT),
       max_server_configs_stored_in_properties_(kMaxQuicServersToPersist) {
@@ -40,6 +33,9 @@ HttpServerPropertiesImpl::HttpServerPropertiesImpl(
   canonical_suffixes_.push_back(".googleusercontent.com");
 }
 
+HttpServerPropertiesImpl::HttpServerPropertiesImpl()
+    : HttpServerPropertiesImpl(nullptr) {}
+
 HttpServerPropertiesImpl::~HttpServerPropertiesImpl() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
 }
@@ -165,6 +161,26 @@ void HttpServerPropertiesImpl::GetSpdyServerList(
   }
 }
 
+void HttpServerPropertiesImpl::SetBrokenAndRecentlyBrokenAlternativeServices(
+    std::unique_ptr<BrokenAlternativeServiceList>
+        broken_alternative_service_list,
+    std::unique_ptr<RecentlyBrokenAlternativeServices>
+        recently_broken_alternative_services) {
+  broken_alternative_services_.SetBrokenAndRecentlyBrokenAlternativeServices(
+      std::move(broken_alternative_service_list),
+      std::move(recently_broken_alternative_services));
+}
+
+const BrokenAlternativeServiceList&
+HttpServerPropertiesImpl::broken_alternative_service_list() const {
+  return broken_alternative_services_.broken_alternative_service_list();
+}
+
+const RecentlyBrokenAlternativeServices&
+HttpServerPropertiesImpl::recently_broken_alternative_services() const {
+  return broken_alternative_services_.recently_broken_alternative_services();
+}
+
 void HttpServerPropertiesImpl::Clear() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   spdy_servers_map_.Clear();

net/http/http_server_properties_impl.h

@@ -26,6 +26,10 @@
 #include "net/http/broken_alternative_services.h"
 #include "net/http/http_server_properties.h"
 
+namespace base {
+class TickClock;
+}
+
 namespace net {
 
 // The implementation for setting/retrieving the HTTP server properties.
@@ -33,9 +37,14 @@ class NET_EXPORT HttpServerPropertiesImpl
     : public HttpServerProperties,
       public BrokenAlternativeServices::Delegate {
  public:
+  // |clock| is used for setting expiration times and scheduling the
+  // expiration of broken alternative services. If null, default clock will be
+  // used.
+  explicit HttpServerPropertiesImpl(base::TickClock* clock);
+
+  // Default clock will be used.
   HttpServerPropertiesImpl();
-  explicit HttpServerPropertiesImpl(
-      base::TickClock* broken_alternative_services_clock);
+
   ~HttpServerPropertiesImpl() override;
 
   // Sets |spdy_servers_map_| with the servers (host/port) from
@@ -58,6 +67,17 @@ class NET_EXPORT HttpServerPropertiesImpl
   void GetSpdyServerList(std::vector<std::string>* spdy_servers,
                          size_t max_size) const;
 
+  void SetBrokenAndRecentlyBrokenAlternativeServices(
+      std::unique_ptr<BrokenAlternativeServiceList>
+          broken_alternative_service_list,
+      std::unique_ptr<RecentlyBrokenAlternativeServices>
+          recently_broken_alternative_services);
+
+  const BrokenAlternativeServiceList& broken_alternative_service_list() const;
+
+  const RecentlyBrokenAlternativeServices&
+  recently_broken_alternative_services() const;
+
   // Returns flattened string representation of the |host_port_pair|. Used by
   // unittests.
   static std::string GetFlattenedSpdyServer(const HostPortPair& host_port_pair);
@@ -146,14 +166,15 @@ class NET_EXPORT HttpServerPropertiesImpl
   // Remove the cononical host for |server|.
   void RemoveCanonicalHost(const url::SchemeHostPort& server);
 
-  base::DefaultTickClock broken_alternative_services_clock_;
-  BrokenAlternativeServices broken_alternative_services_;
+  base::DefaultTickClock default_clock_;
 
   SpdyServersMap spdy_servers_map_;
   Http11ServerHostPortSet http11_servers_;
 
   AlternativeServiceMap alternative_service_map_;
 
+  BrokenAlternativeServices broken_alternative_services_;
+
   IPAddress last_quic_address_;
   ServerNetworkStatsMap server_network_stats_map_;
   // Contains a map of servers which could share the same alternate protocol.

net/http/http_server_properties_manager.h

@@ -15,6 +15,7 @@
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/time/default_tick_clock.h"
 #include "base/timer/timer.h"
 #include "base/values.h"
 #include "net/base/host_port_pair.h"
@@ -93,11 +94,24 @@ class NET_EXPORT HttpServerPropertiesManager : public HttpServerProperties {
   // cache update to the pref thread;
   // |network_task_runner| should be bound with the network thread and is used
   // to post pref update to the cache thread.
+  //
+  // |clock| is used for setting expiration times and scheduling the
+  // expiration of broken alternative services. If null, the default clock will
+  // be used.
+  HttpServerPropertiesManager(
+      PrefDelegate* pref_delegate,
+      scoped_refptr<base::SingleThreadTaskRunner> pref_task_runner,
+      scoped_refptr<base::SingleThreadTaskRunner> network_task_runner,
+      NetLog* net_log,
+      base::TickClock* clock);
+
+  // Default clock will be used.
   HttpServerPropertiesManager(
       PrefDelegate* pref_delegate,
       scoped_refptr<base::SingleThreadTaskRunner> pref_task_runner,
       scoped_refptr<base::SingleThreadTaskRunner> network_task_runner,
       NetLog* net_log);
+
   ~HttpServerPropertiesManager() override;
 
   // Initialize on Network thread.
@@ -218,6 +232,10 @@ class NET_EXPORT HttpServerPropertiesManager : public HttpServerProperties {
       std::unique_ptr<IPAddress> last_quic_address,
       std::unique_ptr<ServerNetworkStatsMap> server_network_stats_map,
       std::unique_ptr<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);
 
   // These are used to delay updating the preferences when cached data in
@@ -245,6 +263,10 @@ class NET_EXPORT HttpServerPropertiesManager : public HttpServerProperties {
       std::unique_ptr<IPAddress> last_quic_address,
       std::unique_ptr<ServerNetworkStatsMap> server_network_stats_map,
       std::unique_ptr<QuicServerInfoMap> quic_server_info_map,
+      std::unique_ptr<BrokenAlternativeServiceList>
+          broken_alternative_service_list,
+      std::unique_ptr<RecentlyBrokenAlternativeServices>
+          recently_broken_alternative_services,
       const base::Closure& completion);
 
  private:
@@ -263,8 +285,21 @@ class NET_EXPORT HttpServerPropertiesManager : public HttpServerProperties {
                       AlternativeServiceMap* alternative_service_map,
                       ServerNetworkStatsMap* network_stats_map,
                       int version);
-  bool ParseAlternativeServiceDict(
-      const base::DictionaryValue& alternative_service_dict,
+  // Helper method used for parsing an alternative service from JSON.
+  // |dict| is the JSON dictionary to be parsed. It should contain fields
+  // corresponding to members of AlternativeService.
+  // |host_optional| determines whether or not the "host" field is optional. If
+  // optional, the default value is empty string.
+  // |parsing_under| is used only for debug log outputs in case of error; it
+  // should describe what section of the JSON prefs is currently being parsed.
+  // |alternative_service| is the output of parsing |dict|.
+  // Return value is true if parsing is successful.
+  bool ParseAlternativeServiceDict(const base::DictionaryValue& dict,
+                                   bool host_optional,
+                                   const std::string& parsing_under,
+                                   AlternativeService* alternative_service);
+  bool ParseAlternativeServiceInfoDictOfServer(
+      const base::DictionaryValue& dict,
       const std::string& server_str,
       AlternativeServiceInfo* alternative_service_info);
   bool AddToAlternativeServiceMap(
@@ -278,6 +313,10 @@ class NET_EXPORT HttpServerPropertiesManager : public HttpServerProperties {
                             ServerNetworkStatsMap* network_stats_map);
   bool AddToQuicServerInfoMap(const base::DictionaryValue& server_dict,
                               QuicServerInfoMap* quic_server_info_map);
+  bool AddToBrokenAlternativeServices(
+      const base::DictionaryValue& broken_alt_svc_entry_dict,
+      BrokenAlternativeServiceList* broken_alternative_service_list,
+      RecentlyBrokenAlternativeServices* recently_broken_alternative_services);
 
   void SaveAlternativeServiceToServerPrefs(
       const AlternativeServiceInfoVector& alternative_service_info_vector,
@@ -291,8 +330,16 @@ class NET_EXPORT HttpServerPropertiesManager : public HttpServerProperties {
   void SaveQuicServerInfoMapToServerPrefs(
       const QuicServerInfoMap& quic_server_info_map,
       base::DictionaryValue* http_server_properties_dict);
+  void SaveBrokenAlternativeServicesToPrefs(
+      const BrokenAlternativeServiceList* broken_alternative_service_list,
+      const RecentlyBrokenAlternativeServices*
+          recently_broken_alternative_services,
+      base::DictionaryValue* http_server_properties_dict);
+
   void SetInitialized();
 
+  base::DefaultTickClock default_clock_;
+
   // -----------
   // Pref thread
   // -----------
@@ -307,6 +354,8 @@ class NET_EXPORT HttpServerPropertiesManager : public HttpServerProperties {
   std::unique_ptr<PrefDelegate> pref_delegate_;
   bool setting_prefs_;
 
+  base::TickClock* clock_;  // Unowned
+
   // --------------
   // Network thread
   // --------------

tools/metrics/histograms/histograms.xml

@@ -36518,6 +36518,14 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram name="Net.CountOfBrokenAlternativeServices" units="services">
+  <owner>wangyix@chromium.org</owner>
+  <summary>
+    This counts the number of broken alternative services persisted to prefs
+    file.
+  </summary>
+</histogram>
+
 <histogram name="Net.CountOfPipelineCapableServers" units="servers">
   <obsolete>
     Deprecated 05/2014, related field trial already long expired.
@@ -36536,6 +36544,14 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram name="Net.CountOfRecentlyBrokenAlternativeServices" units="services">
+  <owner>wangyix@chromium.org</owner>
+  <summary>
+    This counts the number of recently broken alternative services persisted to
+    prefs file.
+  </summary>
+</histogram>
+
 <histogram name="Net.CountOfSpdyServers">
   <owner>bnc@chromium.org</owner>
   <owner>rch@chromium.org</owner>