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

net/http/broken_alternative_services.cc

Index: net/http/broken_alternative_services.cc
diff --git a/net/http/broken_alternative_services.cc b/net/http/broken_alternative_services.cc
index ae7f7c4a2bd110ce35f11a10e3e03288aab1b853..33237adcaacbb6227454a7392f6b9f9b0421284f 100644
--- a/net/http/broken_alternative_services.cc
+++ b/net/http/broken_alternative_services.cc
@@ -30,19 +30,24 @@ base::TimeDelta ComputeBrokenAlternativeServiceExpirationDelay(
 
 }  // namespace
 
-BrokenAlternativeServices::BrokenAlternativeServices(Delegate* delegate,
-                                                     base::TickClock* clock)
+BrokenAlternativeServices::BrokenAlternativeServices(Delegate* delegate)
     : delegate_(delegate),
-      clock_(clock),
+      clock_(&default_clock_),
       recently_broken_alternative_services_(
           RecentlyBrokenAlternativeServices::NO_AUTO_EVICT),
       weak_ptr_factory_(this) {
   DCHECK(delegate_);
-  DCHECK(clock_);
 }
 
 BrokenAlternativeServices::~BrokenAlternativeServices() {}
 
+void BrokenAlternativeServices::Clear() {
+  expiration_timer_.Stop();
+  broken_alternative_service_list_.clear();
+  broken_alternative_service_map_.clear();
+  recently_broken_alternative_services_.Clear();
+}
+
 void BrokenAlternativeServices::MarkAlternativeServiceBroken(
     const AlternativeService& alternative_service) {
   // Empty host means use host of origin, callers are supposed to substitute.
@@ -69,7 +74,8 @@ void BrokenAlternativeServices::MarkAlternativeServiceBroken(
   // If this is now the first entry in the list (i.e. |alternative_service| is
   // the next alt svc to expire), schedule an expiration task for it.
   if (list_it == broken_alternative_service_list_.begin()) {
-    ScheduleBrokenAlternateProtocolMappingsExpiration();
+    ScheduleBrokenAlternateProtocolMappingsExpiration(
+        broken_alternative_service_list_.front().second);
   }
 }
 
@@ -114,17 +120,12 @@ void BrokenAlternativeServices::ConfirmAlternativeService(
   }
 }
 
-const BrokenAlternativeServiceList&
-BrokenAlternativeServices::broken_alternative_service_list() const {
-  return broken_alternative_service_list_;
+void BrokenAlternativeServices::SetTickClockForTesting(base::TickClock* clock) {
+  DCHECK(clock);
+  clock_ = clock;

[Ryan Hamilton, 2017/06/12 18:57:32]

I don't think I understand the clock-related changes in this file. What's the
motivation? I assume they were necessitated by something that I may have missed.

[wangyix1, 2017/06/14 00:01:25]

Acknowledged. Will go back to clock being set using a TickClock* param in the
constructor. 

 }
 
-const RecentlyBrokenAlternativeServices&
-BrokenAlternativeServices::recently_broken_alternative_services() const {
-  return recently_broken_alternative_services_;
-}
-
-bool BrokenAlternativeServices::AddBrokenAndRecentlyBrokenAlternativeServices(
+void BrokenAlternativeServices::AddBrokenAndRecentlyBrokenAlternativeServices(
     std::unique_ptr<BrokenAlternativeServiceList>
         broken_alternative_service_list,
     std::unique_ptr<RecentlyBrokenAlternativeServices>
@@ -139,48 +140,49 @@ bool BrokenAlternativeServices::AddBrokenAndRecentlyBrokenAlternativeServices(
            recently_broken_alternative_services->end());
   }
 
-  bool added = false;
+  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|.
 
   recently_broken_alternative_services_.Swap(
       *recently_broken_alternative_services);
   // Add back all existing recently broken alt svcs to cache so they're at
-  // front of recency list.
+  // front of recency list (MRUCache::Get() does this automatically).
   for (auto it = recently_broken_alternative_services->rbegin();
        it != recently_broken_alternative_services->rend(); ++it) {
     if (recently_broken_alternative_services_.Get(it->first) ==
         recently_broken_alternative_services_.end()) {
       recently_broken_alternative_services_.Put(it->first, it->second);
-      added = true;
     }
   }
 
-  // Add broken alt svcs to |broken_alternative_service_map_|. Remove all
-  // that already exist in the map.
-  auto it = broken_alternative_service_list->begin();
-  while (it != broken_alternative_service_list->end()) {
+  // 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;
-    if (broken_alternative_service_map_.find(alternative_service) ==
-        broken_alternative_service_map_.end()) {
+    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;
+    } else {
       broken_alternative_service_map_.insert(
           std::make_pair(alternative_service, it));
-      ++it;
-    } else {
-      broken_alternative_service_list->erase(it++);
     }
   }
 
-  if (!broken_alternative_service_list->empty())
-    added = true;
-
   // Merge |broken_alternative_service_list| with
   // |broken_alternative_service_list_|. Both should already be sorted by
-  // expiration time.
-  bool schedule_expiration =
-      (!broken_alternative_service_list->empty() &&
-       (broken_alternative_service_list_.empty() ||
-        broken_alternative_service_list->front().second <
-            broken_alternative_service_list_.front().second));
-
+  // 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,
       [](const std::pair<AlternativeService, base::TimeTicks>& lhs,
@@ -188,10 +190,23 @@ bool BrokenAlternativeServices::AddBrokenAndRecentlyBrokenAlternativeServices(
         return lhs.second < rhs.second;
       });
 
-  if (schedule_expiration)
-    ScheduleBrokenAlternateProtocolMappingsExpiration();
+  base::TimeTicks new_next_expiration =
+      broken_alternative_service_list_.empty()
+          ? base::TimeTicks::Max()
+          : broken_alternative_service_list_.front().second;
 
-  return added;
+  if (new_next_expiration != next_expiration)
+    ScheduleBrokenAlternateProtocolMappingsExpiration(new_next_expiration);
+}
+
+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(
@@ -241,14 +256,14 @@ void BrokenAlternativeServices::ExpireBrokenAlternateProtocolMappings() {
   }
 
   if (!broken_alternative_service_list_.empty())
-    ScheduleBrokenAlternateProtocolMappingsExpiration();
+    ScheduleBrokenAlternateProtocolMappingsExpiration(
+        broken_alternative_service_list_.front().second);
 }
 
 void BrokenAlternativeServices ::
-    ScheduleBrokenAlternateProtocolMappingsExpiration() {
+    ScheduleBrokenAlternateProtocolMappingsExpiration(base::TimeTicks when) {
   DCHECK(!broken_alternative_service_list_.empty());
   base::TimeTicks now = clock_->NowTicks();
-  base::TimeTicks when = broken_alternative_service_list_.front().second;
   base::TimeDelta delay = when > now ? when - now : base::TimeDelta();
   expiration_timer_.Stop();
   expiration_timer_.Start(