Fix and refactor HttpServerPropertiesImpl's alternative services brokenness expiration behavior

net/http/broken_alternative_services.h

Index: net/http/broken_alternative_services.h
diff --git a/net/http/http_server_properties_impl.h b/net/http/broken_alternative_services.h
similarity index 11%
copy from net/http/http_server_properties_impl.h
copy to net/http/broken_alternative_services.h
index b483bb296b1f73898f5bf47b536a5f13f064f917..0663e5e1a530ef47e6b549804ca018ab117534bc 100644
--- a/net/http/http_server_properties_impl.h
+++ b/net/http/broken_alternative_services.h
@@ -1,31 +1,19 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2017 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 NET_HTTP_HTTP_SERVER_PROPERTIES_IMPL_H_
-#define NET_HTTP_HTTP_SERVER_PROPERTIES_IMPL_H_
+#ifndef NET_HTTP_BROKEN_ALTERNATIVE_SERVICES_H_
+#define NET_HTTP_BROKEN_ALTERNATIVE_SERVICES_H_
 
-#include <stddef.h>
-#include <stdint.h>
+#include <list>
+#include <unordered_map>
 
-#include <deque>
-#include <map>
-#include <set>
-#include <string>
-#include <vector>
-
-#include "base/macros.h"
 #include "base/memory/weak_ptr.h"
-#include "base/threading/non_thread_safe.h"
-#include "base/values.h"
-#include "net/base/host_port_pair.h"
-#include "net/base/ip_address.h"
-#include "net/base/linked_hash_map.h"
-#include "net/base/net_export.h"
+#include "base/timer/timer.h"
 #include "net/http/http_server_properties.h"
 
 namespace base {
-class ListValue;
+class TickClock;
 }
 
 namespace net {
@@ -36,150 +24,121 @@ struct AlternativeServiceHash {
   }
 };
 
-// The implementation for setting/retrieving the HTTP server properties.
-class NET_EXPORT HttpServerPropertiesImpl
-    : public HttpServerProperties,
-      NON_EXPORTED_BASE(public base::NonThreadSafe) {
+// This class tracks HTTP alternative services that have been marked as broken.
+// The brokenness of an alt-svc will expire after some time according to an
+// exponential back-off formula: each time an alt-svc is marked broken, the
+// expiration delay will be some constant multiple of its previous expiration
+// delay. This prevents broken alt-svcs from being retried too often by the
+// network stack.
+class NET_EXPORT_PRIVATE BrokenAlternativeServices {
  public:
-  HttpServerPropertiesImpl();
-  ~HttpServerPropertiesImpl() override;
-
-  // Sets |spdy_servers_map_| with the servers (host/port) from
-  // |spdy_servers| that either support SPDY or not.
-  void SetSpdyServers(std::vector<std::string>* spdy_servers,
-                      bool support_spdy);
-
-  void SetAlternativeServiceServers(
-      AlternativeServiceMap* alternate_protocol_servers);
-
-  void SetSupportsQuic(IPAddress* last_address);
-
-  void SetServerNetworkStats(ServerNetworkStatsMap* server_network_stats_map);
-
-  void SetQuicServerInfoMap(QuicServerInfoMap* quic_server_info_map);
-
-  // Get the list of servers (host/port) that support SPDY. The max_size is the
-  // number of MRU servers that support SPDY that are to be returned.
-  void GetSpdyServerList(base::ListValue* spdy_server_list,
-                         size_t max_size) const;
-
-  // Returns flattened string representation of the |host_port_pair|. Used by
-  // unittests.
-  static std::string GetFlattenedSpdyServer(const HostPortPair& host_port_pair);
-
-  // Returns the canonical host suffix for |host|, or nullptr if none
-  // exists.
-  const std::string* GetCanonicalSuffix(const std::string& host) const;
-
-  // -----------------------------
-  // HttpServerProperties methods:
-  // -----------------------------
-
-  void Clear() override;
-  bool SupportsRequestPriority(const url::SchemeHostPort& server) override;
-  bool GetSupportsSpdy(const url::SchemeHostPort& server) override;
-  void SetSupportsSpdy(const url::SchemeHostPort& server,
-                       bool support_spdy) override;
-  bool RequiresHTTP11(const HostPortPair& server) override;
-  void SetHTTP11Required(const HostPortPair& server) override;
-  void MaybeForceHTTP11(const HostPortPair& server,
-                        SSLConfig* ssl_config) override;
-  AlternativeServiceVector GetAlternativeServices(
-      const url::SchemeHostPort& origin) override;
-  bool SetAlternativeService(const url::SchemeHostPort& origin,
-                             const AlternativeService& alternative_service,
-                             base::Time expiration) override;
-  bool SetAlternativeServices(const url::SchemeHostPort& origin,
-                              const AlternativeServiceInfoVector&
-                                  alternative_service_info_vector) override;
+  // Delegate to be used by owner so it can notified when the brokenness of an
+  // AlternativeService expires.
+  class NET_EXPORT Delegate {
+   public:
+    // Will be called when a broken alternative service's expiration time is
+    // reached.
+    virtual void OnExpireBrokenAlternativeService(
+        const AlternativeService& expired_alternative_service) = 0;
+    virtual ~Delegate() {}
+  };
+
+  // |delegate| will be notified when a broken alternative service expires. It
+  // must not null.
+  // |clock| is used for setting expiration times and scheduling the
+  // expiration of broken alternative services. It must not be null.
+  // |delegate| and |clock| are both unowned and must outlive this.
+  BrokenAlternativeServices(Delegate* delegate, base::TickClock* clock);
+
+  BrokenAlternativeServices(const BrokenAlternativeServices&) = delete;
+  void operator=(const BrokenAlternativeServices&) = delete;
+
+  ~BrokenAlternativeServices();
+
+  // Marks |alternative_service| as broken until after some expiration delay

[Ryan Hamilton, 2017/05/27 13:28:16]

nit: newlines before each of these method comments.

[wangyix1, 2017/05/30 21:18:18]

Discussed; will ignore comment.

+  // (determined by how many times it's been marked broken before). Being broken
+  // will cause IsAlternativeServiceBroken(alternative_service) to return true
+  // until the expiration time is reached, or until
+  // ConfirmAlternativeService(alternative_service) is called.
   void MarkAlternativeServiceBroken(
-      const AlternativeService& alternative_service) override;
+      const AlternativeService& alternative_service);
+
+  // Marks |alternative_service| as recently broken. Being recently broken will
+  // cause WasAlternativeServiceRecentlyBroken(alternative_service) to return
+  // true until ConfirmAlternativeService(alternative_service) is called.
   void MarkAlternativeServiceRecentlyBroken(
-      const AlternativeService& alternative_service) override;
+      const AlternativeService& alternative_service);
+
+  // Returns true if MarkAlternativeServiceBroken(alternative_service) was
+  // called, the expiration time has not been reached, and
+  // ConfirmAlternativeService(alternative_service) has not been called
+  // afterwards.
   bool IsAlternativeServiceBroken(
-      const AlternativeService& alternative_service) const override;
+      const AlternativeService& alternative_service) const;
+
+  // Returns true MarkAlternativeServiceRecentlyBroken(alternative_service) was

[Ryan Hamilton, 2017/05/27 13:28:16]

nit: I think "if" is missing from this comment?

[wangyix1, 2017/05/30 21:18:18]

Done.

+  // called and ConfirmAlternativeService(alternative_service) has not been
+  // called afterwards.
   bool WasAlternativeServiceRecentlyBroken(
-      const AlternativeService& alternative_service) override;
-  void ConfirmAlternativeService(
-      const AlternativeService& alternative_service) override;
-  const AlternativeServiceMap& alternative_service_map() const override;
-  std::unique_ptr<base::Value> GetAlternativeServiceInfoAsValue()
-      const override;
-  bool GetSupportsQuic(IPAddress* last_address) const override;
-  void SetSupportsQuic(bool used_quic, const IPAddress& address) override;
-  void SetServerNetworkStats(const url::SchemeHostPort& server,
-                             ServerNetworkStats stats) override;
-  void ClearServerNetworkStats(const url::SchemeHostPort& server) override;
-  const ServerNetworkStats* GetServerNetworkStats(
-      const url::SchemeHostPort& server) override;
-  const ServerNetworkStatsMap& server_network_stats_map() const override;
-  bool SetQuicServerInfo(const QuicServerId& server_id,
-                         const std::string& server_info) override;
-  const std::string* GetQuicServerInfo(const QuicServerId& server_id) override;
-  const QuicServerInfoMap& quic_server_info_map() const override;
-  size_t max_server_configs_stored_in_properties() const override;
-  void SetMaxServerConfigsStoredInProperties(
-      size_t max_server_configs_stored_in_properties) override;
-  bool IsInitialized() const override;
+      const AlternativeService& alternative_service);
+
+  // Marks |alternative_service| as not broken and not recently broken.
+  void ConfirmAlternativeService(const AlternativeService& alternative_service);
 
  private:
+  // TODO (wangyix): modify HttpServerPropertiesImpl unit tests so this
+  // friendness is no longer required.
   friend class HttpServerPropertiesImplPeer;
 
-  // |spdy_servers_map_| has flattened representation of servers
-  // (scheme, host, port) that either support or not support SPDY protocol.
-  typedef base::MRUCache<std::string, bool> SpdyServersMap;
-  typedef std::map<url::SchemeHostPort, url::SchemeHostPort> CanonicalHostMap;
-  typedef std::vector<std::string> CanonicalSufficList;
-  typedef std::set<HostPortPair> Http11ServerHostPortSet;
-
-  // Linked hash map from AlternativeService to expiration time.  This container
-  // is a queue with O(1) enqueue and dequeue, and a hash_map with O(1) lookup
-  // at the same time.
-  typedef linked_hash_map<AlternativeService,
-                          base::TimeTicks,
-                          AlternativeServiceHash>
-      BrokenAlternativeServices;
-
-  // Return the iterator for |server|, or for its canonical host, or end.
-  AlternativeServiceMap::const_iterator GetAlternateProtocolIterator(
-      const url::SchemeHostPort& server);
-
-  // Return the canonical host for |server|, or end if none exists.
-  CanonicalHostMap::const_iterator GetCanonicalHost(
-      const url::SchemeHostPort& server) const;
-
-  // Remove the cononical host for |server|.
-  void RemoveCanonicalHost(const url::SchemeHostPort& server);
+  // A pair containing an AlternativeService and its expiration time.
+  struct BrokenAltSvcExpireInfo {
+    BrokenAltSvcExpireInfo(const AlternativeService& alt_svc,
+                           base::TimeTicks expire)
+        : alternative_service(alt_svc), expiration(expire) {}
+
+    AlternativeService alternative_service;
+    base::TimeTicks expiration;
+  };
+
+  typedef std::list<BrokenAltSvcExpireInfo> BrokenAlternativeServiceList;
+
+  typedef std::unordered_map<AlternativeService,
+                             BrokenAlternativeServiceList::iterator,
+                             AlternativeServiceHash>
+      BrokenAlternativeServiceMap;
+
+  // Inserts |alternative_service| and its |expiration| time into

[Ryan Hamilton, 2017/05/27 13:28:16]

nit: newlines before these comments. a bit more vertical whitespace would be
nice. (Here and below)

[wangyix1, 2017/05/30 21:18:18]

Discussed; will ignore comment.

+  // |broken_alternative_service_list_| and |broken_alternative_service_map_|.
+  // |it| is the position in |broken_alternative_service_list_| where it was
+  // inserted.
+  bool AddToBrokenAlternativeServiceListAndMap(
+      const AlternativeService& alternative_service,
+      base::TimeTicks expiration,
+      BrokenAlternativeServiceList::iterator* it);
+
   void ExpireBrokenAlternateProtocolMappings();
   void ScheduleBrokenAlternateProtocolMappingsExpiration();
 
-  SpdyServersMap spdy_servers_map_;
-  Http11ServerHostPortSet http11_servers_;
+  Delegate* delegate_;      // Unowned
+  base::TickClock* clock_;  // Unowned
 
-  AlternativeServiceMap alternative_service_map_;
-  BrokenAlternativeServices broken_alternative_services_;
-  // Class invariant:  Every alternative service in broken_alternative_services_
-  // must also be in recently_broken_alternative_services_.
-  RecentlyBrokenAlternativeServices recently_broken_alternative_services_;
-
-  IPAddress last_quic_address_;
-  ServerNetworkStatsMap server_network_stats_map_;
-  // Contains a map of servers which could share the same alternate protocol.
-  // Map from a Canonical scheme/host/port (host is some postfix of host names)
-  // to an actual origin, which has a plausible alternate protocol mapping.
-  CanonicalHostMap canonical_host_to_origin_map_;
-  // Contains list of suffixes (for exmaple ".c.youtube.com",
-  // ".googlevideo.com", ".googleusercontent.com") of canonical hostnames.
-  CanonicalSufficList canonical_suffixes_;
+  // List of <broken alt svc, expiration time> pairs sorted by expiration time.
+  BrokenAlternativeServiceList broken_alternative_service_list_;
+  // A map from broken alt-svcs to their iterator pointing to that alt-svc's
+  // position in |broken_alternative_service_list_|.
+  BrokenAlternativeServiceMap broken_alternative_service_map_;
 
-  QuicServerInfoMap quic_server_info_map_;
-  size_t max_server_configs_stored_in_properties_;
+  // Maps broken alternative services to how many times they've been marked
+  // broken.
+  RecentlyBrokenAlternativeServices recently_broken_alternative_services_;
 
-  base::WeakPtrFactory<HttpServerPropertiesImpl> weak_ptr_factory_;
+  // Used for scheduling the task that expires the brokenness of alternative
+  // services.
+  base::OneShotTimer expiration_timer_;
 
-  DISALLOW_COPY_AND_ASSIGN(HttpServerPropertiesImpl);
+  base::WeakPtrFactory<BrokenAlternativeServices> weak_ptr_factory_;
 };
 
 }  // namespace net
 
-#endif  // NET_HTTP_HTTP_SERVER_PROPERTIES_IMPL_H_
+#endif  // NET_HTTP_BROKEN_ALTERNATIVE_SERVICES_H_