Reland (3rd time) "Persist broken and recently-broken alt-svcs to pre…

…fs 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. Original land: https://chromium-review.googlesource.com/c/562604/ 1st reland: https://chromium-review.googlesource.com/c/571044/ 2nd reland: https://chromium-review.googlesource.com/c/576349/ CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_ubsan_rel_ng BUG=705029 TBR=mpearson@chromium.org Change-Id: I26a720b21162253dfc0c3e3b2ec8b40ced1e962d Reviewed-on: https://chromium-review.googlesource.com/585782 Commit-Queue: Yixin Wang <wangyix@chromium.org> Reviewed-by: Ryan Hamilton <rch@chromium.org> Reviewed-by: Mark Pearson <mpearson@chromium.org> Reviewed-by: Zhongyi Shi <zhongyi@chromium.org> Cr-Commit-Position: refs/heads/master@{#490034}
Denger-Network · Jul 27, 2017 · 1c34cc3 · 1c34cc3
1 parent bea2d08
commit 1c34cc3
Show file tree

Hide file tree

Showing 10 changed files with 929 additions and 170 deletions.
diff --git a/net/http/broken_alternative_services.cc b/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,
@@ -132,21 +122,15 @@ void BrokenAlternativeServices::SetBrokenAndRecentlyBrokenAlternativeServices(
   DCHECK(broken_alternative_service_list);
   DCHECK(recently_broken_alternative_services);
 
-  // Make sure all alt svcs in |broken_alternative_service_list| has an entry
-  // in |recently_broken_alternative_services|
-  for (const auto& pair : *broken_alternative_service_list) {
-    DCHECK(recently_broken_alternative_services->Peek(pair.first) !=
-           recently_broken_alternative_services->end());
-  }
-
   base::TimeTicks next_expiration =
       broken_alternative_service_list_.empty()
           ? base::TimeTicks::Max()
           : broken_alternative_service_list_.front().second;
 
-  // Add recently broken alt svcs to |recently_broken_alternative_services_|.
-  // If an alt-svc already exists, update its broken-count to the one provided
-  // in |recently_broken_alternative_services|.
+  // Add |recently_broken_alternative_services| to
+  // |recently_broken_alternative_services_|.
+  // If an alt-svc already exists, overwrite its broken-count to the one in
+  // |recently_broken_alternative_services|.
 
   recently_broken_alternative_services_.Swap(
       *recently_broken_alternative_services);
@@ -160,30 +144,46 @@ void BrokenAlternativeServices::SetBrokenAndRecentlyBrokenAlternativeServices(
     }
   }
 
-  // Add broken alt svcs to |broken_alternative_service_map_|. If an entry
-  // already exists, delete its corresponding entry in
-  // |broken_alternative_service_list_| and update its map entry to point to
-  // its position in |broken_alternative_service_list|.
-  for (auto it = broken_alternative_service_list->begin();
-       it != broken_alternative_service_list->end(); ++it) {
-    const AlternativeService& alternative_service = it->first;
+  // Append |broken_alternative_service_list| to
+  // |broken_alternative_service_list_|
+  size_t num_broken_alt_svcs_added = broken_alternative_service_list->size();
+  broken_alternative_service_list_.splice(
+      broken_alternative_service_list_.begin(),
+      *broken_alternative_service_list);
+  // For each newly-appended alt svc in |broken_alternative_service_list_|,
+  // add an entry to |broken_alternative_service_map_| that points to its
+  // list iterator. Also, add an entry for that alt svc in
+  // |recently_broken_alternative_services_| if one doesn't exist.
+  auto list_it = broken_alternative_service_list_.begin();
+  for (size_t i = 0; i < num_broken_alt_svcs_added; ++i) {
+    const AlternativeService& alternative_service = list_it->first;
     auto map_it = broken_alternative_service_map_.find(alternative_service);
     if (map_it != broken_alternative_service_map_.end()) {
-      broken_alternative_service_list_.erase(map_it->second);
-      map_it->second = it;
+      // Implies this entry already exists somewhere else in
+      // |broken_alternative_service_list_|. Remove the existing entry from
+      // |broken_alternative_service_list_|, and update the
+      // |broken_alternative_service_map_| entry to point to this list entry
+      // instead.
+      auto list_existing_entry_it = map_it->second;
+      broken_alternative_service_list_.erase(list_existing_entry_it);
+      map_it->second = list_it;
     } else {
       broken_alternative_service_map_.insert(
-          std::make_pair(alternative_service, it));
+          std::make_pair(alternative_service, list_it));
+    }
+
+    if (recently_broken_alternative_services_.Peek(alternative_service) ==
+        recently_broken_alternative_services_.end()) {
+      recently_broken_alternative_services_.Put(alternative_service, 1);
     }
+
+    ++list_it;
   }
 
-  // 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,
+  // Sort |broken_alternative_service_list_| by expiration time. This operation
+  // does not invalidate list iterators, so |broken_alternative_service_map_|
+  // does not need to be updated.
+  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 +198,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,
@@ -262,4 +272,4 @@ void BrokenAlternativeServices ::
           weak_ptr_factory_.GetWeakPtr()));
 }
 
-}  // namespace net
+}  // namespace net
diff --git a/net/http/broken_alternative_services.h b/net/http/broken_alternative_services.h
@@ -80,10 +80,12 @@ class NET_EXPORT_PRIVATE BrokenAlternativeServices {
   void ConfirmAlternativeService(const AlternativeService& alternative_service);
 
   // Sets broken and recently broken alternative services.
-  // |broken_alternative_service_list| must be sorted from earliest to latest
-  // expiration time.
-  // All AlternativeServices in |broken_alternative_service_list| must exist in
-  // |recently_broken_alternative_services|.
+  // |broken_alternative_service_list|, |recently_broken_alternative_services|
+  // must not be nullptr.
+  //
+  // 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,

diff --git a/net/http/broken_alternative_services_unittest.cc b/net/http/broken_alternative_services_unittest.cc
@@ -411,12 +411,12 @@ TEST_F(BrokenAlternativeServicesTest,
 
   std::unique_ptr<BrokenAlternativeServiceList> broken_list =
       base::MakeUnique<BrokenAlternativeServiceList>();
-  broken_list->push_back(
-      {alternative_service3,
-       broken_services_clock_->NowTicks() + base::TimeDelta::FromMinutes(1)});
   broken_list->push_back(
       {alternative_service1,
        broken_services_clock_->NowTicks() + base::TimeDelta::FromMinutes(3)});
+  broken_list->push_back(
+      {alternative_service3,
+       broken_services_clock_->NowTicks() + base::TimeDelta::FromMinutes(1)});
 
   std::unique_ptr<RecentlyBrokenAlternativeServices> recently_broken_map =
       base::MakeUnique<RecentlyBrokenAlternativeServices>(

diff --git a/net/http/http_server_properties.h b/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(

diff --git a/net/http/http_server_properties_impl.cc b/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();

diff --git a/net/http/http_server_properties_impl.h b/net/http/http_server_properties_impl.h
@@ -26,16 +26,25 @@
 #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.
 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.