Update param types of HttpServerPropertiesImpl setters and getters. Fix MRU order when loading

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..4434ca0fac701bef4acc0e9235239c8eaa9df3c7 100644
--- a/net/http/http_server_properties_manager.cc
+++ b/net/http/http_server_properties_manager.cc
@@ -475,11 +475,12 @@ void HttpServerPropertiesManager::UpdateCacheFromPrefsOnPrefSequence() {
     }
   }
 
-  IPAddress* addr = new IPAddress;
-  ReadSupportsQuic(http_server_properties_dict, addr);
+  std::unique_ptr<IPAddress> addr = base::MakeUnique<IPAddress>();
+  ReadSupportsQuic(http_server_properties_dict, addr.get());
 
   // String is "scheme://host:port" tuple of spdy server.
-  std::unique_ptr<ServerList> spdy_servers(new ServerList);
+  std::unique_ptr<SpdyServersMap> spdy_servers_map(
+      new SpdyServersMap(SpdyServersMap::NO_AUTO_EVICT));
   std::unique_ptr<AlternativeServiceMap> alternative_service_map(
       new AlternativeServiceMap(kMaxAlternateProtocolHostsToPersist));
   std::unique_ptr<ServerNetworkStatsMap> server_network_stats_map(
@@ -488,20 +489,23 @@ void HttpServerPropertiesManager::UpdateCacheFromPrefsOnPrefSequence() {
       new QuicServerInfoMap(QuicServerInfoMap::NO_AUTO_EVICT));
 
   if (version < 4) {
-    if (!AddServersData(*servers_dict, spdy_servers.get(),
+    if (!AddServersData(*servers_dict, spdy_servers_map.get(),
                         alternative_service_map.get(),
                         server_network_stats_map.get(), version)) {
       detected_corrupted_prefs = true;
     }
   } else {
-    for (base::ListValue::const_iterator it = servers_list->begin();
-         it != servers_list->end(); ++it) {
+    /*for (base::ListValue::const_iterator it = servers_list->begin();
+         it != servers_list->end(); ++it) {*/

[Ryan Hamilton, 2017/06/22 02:45:52]

Remove?

[wangyix1, 2017/06/22 17:45:09]

Done.

+    for (base::ListValue::const_iterator it = servers_list->end();
+         it != servers_list->begin();) {

[Ryan Hamilton, 2017/06/22 02:45:52]

This appears to iterate in a different order than before. Does that represent a
behavior change?

[wangyix1, 2017/06/22 17:45:09]

No behavior change. This change is a result of Impl::SetSpdyServers() taking a
SpdyServersMap directly instead of a vector of servers. Before, the code did
this:

* In UpdateCacheFromPrefsOnPrefSequence(), |servers_list| iterated forwards,
AddServersData() called on each entry.

* AddServersData() appends entry to |spdy_servers|, which is a vector<string>

* |spdy_servers| is eventually passed to Impl::SetSpdyServers().

* Impl::SetSpdyServers() internally iterates |spdy_servers| backwards and adds
each entry to a SpdyServersMap


Now, the code does this:

* In UpdateCacheFromPrefsOnPrefSequence(), |servers_list| iterated backwards,
AddServersData() called on each entry.

* AddServersData() adds entry to |spdy_servers_map|, which is an MRUCache.

* |spdy_servers_map| is eventually passed to Impl::SetSpdyServers()



In summary, the step where a list of servers is iterated backwards to convert it
into an MRUCache of servers has moved from Impl::SetSpdyServers() to
Manager::UpdateCacheFromPrefsOnPrefSequence()

 

[Ryan Hamilton, 2017/06/22 22:10:30]

Ah! Thanks for the great summary.

[wangyix1, 2017/06/22 22:11:06]

I take that back, this does have a behavior change. AddServersData() adds stuff
to other maps too. Let me look into this first.

[wangyix1, 2017/06/23 19:06:06]

Discussed with rch. This behavior change actually fixes a bug where the
alternative service map when loaded from prefs is loaded with reverse MRU order.

+      --it;
       if (!it->GetAsDictionary(&servers_dict)) {
         DVLOG(1) << "Malformed http_server_properties for servers dictionary.";
         detected_corrupted_prefs = true;
         continue;
       }
-      if (!AddServersData(*servers_dict, spdy_servers.get(),
+      if (!AddServersData(*servers_dict, spdy_servers_map.get(),
                           alternative_service_map.get(),
                           server_network_stats_map.get(), version)) {
         detected_corrupted_prefs = true;
@@ -518,16 +522,15 @@ void HttpServerPropertiesManager::UpdateCacheFromPrefsOnPrefSequence() {
       FROM_HERE,
       base::Bind(
           &HttpServerPropertiesManager::UpdateCacheFromPrefsOnNetworkSequence,
-          base::Unretained(this), base::Owned(spdy_servers.release()),
-          base::Owned(alternative_service_map.release()), base::Owned(addr),
-          base::Owned(server_network_stats_map.release()),
-          base::Owned(quic_server_info_map.release()),
-          detected_corrupted_prefs));
+          base::Unretained(this), base::Passed(&spdy_servers_map),
+          base::Passed(&alternative_service_map), base::Passed(&addr),
+          base::Passed(&server_network_stats_map),
+          base::Passed(&quic_server_info_map), detected_corrupted_prefs));
 }
 
 bool HttpServerPropertiesManager::AddServersData(
     const base::DictionaryValue& servers_dict,
-    ServerList* spdy_servers,
+    SpdyServersMap* spdy_servers_map,
     AlternativeServiceMap* alternative_service_map,
     ServerNetworkStatsMap* network_stats_map,
     int version) {
@@ -554,9 +557,8 @@ bool HttpServerPropertiesManager::AddServersData(
 
     // Get if server supports Spdy.
     bool supports_spdy = false;
-    if ((server_pref_dict->GetBoolean(kSupportsSpdyKey, &supports_spdy)) &&
-        supports_spdy) {
-      spdy_servers->push_back(spdy_server.Serialize());
+    if (server_pref_dict->GetBoolean(kSupportsSpdyKey, &supports_spdy)) {
+      spdy_servers_map->Put(spdy_server.Serialize(), supports_spdy);
     }
 
     if (!AddToAlternativeServiceMap(spdy_server, *server_pref_dict,
@@ -775,34 +777,36 @@ bool HttpServerPropertiesManager::AddToQuicServerInfoMap(
 }
 
 void HttpServerPropertiesManager::UpdateCacheFromPrefsOnNetworkSequence(
-    ServerList* spdy_servers,
-    AlternativeServiceMap* alternative_service_map,
-    IPAddress* last_quic_address,
-    ServerNetworkStatsMap* server_network_stats_map,
-    QuicServerInfoMap* quic_server_info_map,
+    std::unique_ptr<SpdyServersMap> spdy_servers_map,
+    std::unique_ptr<AlternativeServiceMap> alternative_service_map,
+    std::unique_ptr<IPAddress> last_quic_address,
+    std::unique_ptr<ServerNetworkStatsMap> server_network_stats_map,
+    std::unique_ptr<QuicServerInfoMap> quic_server_info_map,
     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.
   DCHECK(network_task_runner_->RunsTasksInCurrentSequence());
 
-  UMA_HISTOGRAM_COUNTS("Net.CountOfSpdyServers", spdy_servers->size());
-  http_server_properties_impl_->SetSpdyServers(spdy_servers, true);
+  UMA_HISTOGRAM_COUNTS("Net.CountOfSpdyServers", spdy_servers_map->size());
+  http_server_properties_impl_->SetSpdyServers(std::move(spdy_servers_map));
 
   // Update the cached data and use the new alternative service list from
   // preferences.
   UMA_HISTOGRAM_COUNTS("Net.CountOfAlternateProtocolServers",
                        alternative_service_map->size());
   http_server_properties_impl_->SetAlternativeServiceServers(
-      alternative_service_map);
+      std::move(alternative_service_map));
 
-  http_server_properties_impl_->SetSupportsQuic(last_quic_address);
+  http_server_properties_impl_->SetSupportsQuic(*last_quic_address);
 
-  http_server_properties_impl_->SetServerNetworkStats(server_network_stats_map);
+  http_server_properties_impl_->SetServerNetworkStats(
+      std::move(server_network_stats_map));
 
   UMA_HISTOGRAM_COUNTS_1000("Net.CountOfQuicServerInfos",
                             quic_server_info_map->size());
 
-  http_server_properties_impl_->SetQuicServerInfoMap(quic_server_info_map);
+  http_server_properties_impl_->SetQuicServerInfoMap(
+      std::move(quic_server_info_map));
 
   // Update the prefs with what we have read (delete all corrupted prefs).
   if (detected_corrupted_prefs)
@@ -838,12 +842,14 @@ void HttpServerPropertiesManager::UpdatePrefsFromCacheOnNetworkSequence(
   DCHECK(network_task_runner_->RunsTasksInCurrentSequence());
 
   // It is in MRU order.
-  base::ListValue* spdy_server_list = new base::ListValue;
+  std::unique_ptr<std::vector<std::string>> spdy_servers =
+      base::MakeUnique<std::vector<std::string>>();
   http_server_properties_impl_->GetSpdyServerList(
-      spdy_server_list, kMaxSupportsSpdyServerHostsToPersist);
+      spdy_servers.get(), kMaxSupportsSpdyServerHostsToPersist);
 
-  AlternativeServiceMap* alternative_service_map =
-      new AlternativeServiceMap(kMaxAlternateProtocolHostsToPersist);
+  std::unique_ptr<AlternativeServiceMap> alternative_service_map =
+      base::MakeUnique<AlternativeServiceMap>(
+          kMaxAlternateProtocolHostsToPersist);
   const AlternativeServiceMap& map =
       http_server_properties_impl_->alternative_service_map();
   UMA_HISTOGRAM_COUNTS("Net.CountOfAlternateProtocolServers.Memory",
@@ -890,8 +896,9 @@ void HttpServerPropertiesManager::UpdatePrefsFromCacheOnNetworkSequence(
     ++count;
   }
 
-  ServerNetworkStatsMap* server_network_stats_map =
-      new ServerNetworkStatsMap(kMaxServerNetworkStatsHostsToPersist);
+  std::unique_ptr<ServerNetworkStatsMap> server_network_stats_map =
+      base::MakeUnique<ServerNetworkStatsMap>(
+          kMaxServerNetworkStatsHostsToPersist);
   const ServerNetworkStatsMap& network_stats_map =
       http_server_properties_impl_->server_network_stats_map();
   count = 0;
@@ -903,28 +910,29 @@ void HttpServerPropertiesManager::UpdatePrefsFromCacheOnNetworkSequence(
     server_network_stats_map->Put(it->first, it->second);
   }
 
-  QuicServerInfoMap* quic_server_info_map = nullptr;
+  std::unique_ptr<QuicServerInfoMap> quic_server_info_map;
   const QuicServerInfoMap& main_quic_server_info_map =
       http_server_properties_impl_->quic_server_info_map();
   if (main_quic_server_info_map.size() > 0) {
-    quic_server_info_map =
-        new QuicServerInfoMap(max_server_configs_stored_in_properties());
+    quic_server_info_map = base::MakeUnique<QuicServerInfoMap>(
+        max_server_configs_stored_in_properties());
     for (const std::pair<const QuicServerId, std::string>& entry :
          main_quic_server_info_map) {
       quic_server_info_map->Put(entry.first, entry.second);
     }
   }
 
-  IPAddress* last_quic_addr = new IPAddress;
-  http_server_properties_impl_->GetSupportsQuic(last_quic_addr);
+  std::unique_ptr<IPAddress> last_quic_addr = base::MakeUnique<IPAddress>();
+  http_server_properties_impl_->GetSupportsQuic(last_quic_addr.get());
   // Update the preferences on the pref thread.
   pref_task_runner_->PostTask(
       FROM_HERE,
-      base::Bind(
-          &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::Bind(&HttpServerPropertiesManager::UpdatePrefsOnPrefThread,
+                 pref_weak_ptr_, base::Passed(&spdy_servers),
+                 base::Passed(&alternative_service_map),
+                 base::Passed(&last_quic_addr),
+                 base::Passed(&server_network_stats_map),
+                 base::Passed(&quic_server_info_map), completion));
 }
 
 // A local or temporary data structure to hold |supports_spdy|, SpdySettings,
@@ -957,11 +965,11 @@ struct ServerPref {
 
 // All maps and lists are in MRU order.
 void HttpServerPropertiesManager::UpdatePrefsOnPrefThread(
-    base::ListValue* spdy_server_list,
-    AlternativeServiceMap* alternative_service_map,
-    IPAddress* last_quic_address,
-    ServerNetworkStatsMap* server_network_stats_map,
-    QuicServerInfoMap* quic_server_info_map,
+    std::unique_ptr<std::vector<std::string>> spdy_servers,
+    std::unique_ptr<AlternativeServiceMap> alternative_service_map,
+    std::unique_ptr<IPAddress> last_quic_address,
+    std::unique_ptr<ServerNetworkStatsMap> server_network_stats_map,
+    std::unique_ptr<QuicServerInfoMap> quic_server_info_map,
     const base::Closure& completion) {
   typedef base::MRUCache<url::SchemeHostPort, ServerPref> ServerPrefMap;
   ServerPrefMap server_pref_map(ServerPrefMap::NO_AUTO_EVICT);
@@ -969,22 +977,21 @@ void HttpServerPropertiesManager::UpdatePrefsOnPrefThread(
   DCHECK(pref_task_runner_->RunsTasksInCurrentSequence());
 
   // Add servers that support spdy to server_pref_map in the MRU order.
-  for (size_t index = spdy_server_list->GetSize(); index > 0; --index) {
-    std::string server_str;
-    if (spdy_server_list->GetString(index - 1, &server_str)) {
-      url::SchemeHostPort server((GURL(server_str)));
-      ServerPrefMap::iterator it = server_pref_map.Get(server);
-      if (it == server_pref_map.end()) {
-        ServerPref server_pref;
-        server_pref.supports_spdy = true;
-        server_pref_map.Put(server, server_pref);
-      } else {
-        it->second.supports_spdy = true;
-      }
+  DCHECK(spdy_servers);
+  for (size_t i = spdy_servers->size(); i > 0; --i) {
+    url::SchemeHostPort server((GURL(spdy_servers->at(i - 1))));
+    ServerPrefMap::iterator it = server_pref_map.Get(server);
+    if (it == server_pref_map.end()) {
+      ServerPref server_pref;
+      server_pref.supports_spdy = true;
+      server_pref_map.Put(server, server_pref);
+    } else {
+      it->second.supports_spdy = true;
     }
   }
 
   // Add alternative services to server_pref_map in the MRU order.
+  DCHECK(alternative_service_map);
   for (AlternativeServiceMap::const_reverse_iterator map_it =
            alternative_service_map->rbegin();
        map_it != alternative_service_map->rend(); ++map_it) {
@@ -1000,6 +1007,7 @@ void HttpServerPropertiesManager::UpdatePrefsOnPrefThread(
   }
 
   // Add ServerNetworkStats servers to server_pref_map in the MRU order.
+  DCHECK(server_network_stats_map);
   for (ServerNetworkStatsMap::const_reverse_iterator map_it =
            server_network_stats_map->rbegin();
        map_it != server_network_stats_map->rend(); ++map_it) {
@@ -1029,8 +1037,8 @@ void HttpServerPropertiesManager::UpdatePrefsOnPrefThread(
     if (server_pref.supports_spdy)
       server_pref_dict->SetBoolean(kSupportsSpdyKey, server_pref.supports_spdy);
     SaveAlternativeServiceToServerPrefs(
-        server_pref.alternative_service_info_vector, server_pref_dict.get());
-    SaveNetworkStatsToServerPrefs(server_pref.server_network_stats,
+        *server_pref.alternative_service_info_vector, server_pref_dict.get());
+    SaveNetworkStatsToServerPrefs(*server_pref.server_network_stats,
                                   server_pref_dict.get());
 
     servers_dict->SetWithoutPathExpansion(server.Serialize(),
@@ -1043,10 +1051,13 @@ void HttpServerPropertiesManager::UpdatePrefsOnPrefThread(
                                                       std::move(servers_list));
   SetVersion(&http_server_properties_dict, kVersionNumber);
 
-  SaveSupportsQuicToPrefs(last_quic_address, &http_server_properties_dict);
+  DCHECK(last_quic_address);
+  SaveSupportsQuicToPrefs(*last_quic_address, &http_server_properties_dict);
 
-  SaveQuicServerInfoMapToServerPrefs(quic_server_info_map,
-                                     &http_server_properties_dict);
+  if (quic_server_info_map) {
+    SaveQuicServerInfoMapToServerPrefs(*quic_server_info_map,
+                                       &http_server_properties_dict);
+  }
 
   setting_prefs_ = true;
   pref_delegate_->SetServerProperties(http_server_properties_dict);
@@ -1061,16 +1072,15 @@ void HttpServerPropertiesManager::UpdatePrefsOnPrefThread(
 }
 
 void HttpServerPropertiesManager::SaveAlternativeServiceToServerPrefs(
-    const AlternativeServiceInfoVector* alternative_service_info_vector,
+    const AlternativeServiceInfoVector& alternative_service_info_vector,
     base::DictionaryValue* server_pref_dict) {
-  if (!alternative_service_info_vector ||
-      alternative_service_info_vector->empty()) {
+  if (alternative_service_info_vector.empty()) {
     return;
   }
   std::unique_ptr<base::ListValue> alternative_service_list(
       new base::ListValue);
   for (const AlternativeServiceInfo& alternative_service_info :
-       *alternative_service_info_vector) {
+       alternative_service_info_vector) {
     const AlternativeService alternative_service =
         alternative_service_info.alternative_service();
     DCHECK(IsAlternateProtocolValid(alternative_service.protocol));
@@ -1096,28 +1106,25 @@ void HttpServerPropertiesManager::SaveAlternativeServiceToServerPrefs(
 }
 
 void HttpServerPropertiesManager::SaveSupportsQuicToPrefs(
-    const IPAddress* last_quic_address,
+    const IPAddress& last_quic_address,
     base::DictionaryValue* http_server_properties_dict) {
-  if (!last_quic_address || !last_quic_address->IsValid())
+  if (!last_quic_address.IsValid())
     return;
 
   auto supports_quic_dict = base::MakeUnique<base::DictionaryValue>();
   supports_quic_dict->SetBoolean(kUsedQuicKey, true);
-  supports_quic_dict->SetString(kAddressKey, last_quic_address->ToString());
+  supports_quic_dict->SetString(kAddressKey, last_quic_address.ToString());
   http_server_properties_dict->SetWithoutPathExpansion(
       kSupportsQuicKey, std::move(supports_quic_dict));
 }
 
 void HttpServerPropertiesManager::SaveNetworkStatsToServerPrefs(
-    const ServerNetworkStats* server_network_stats,
+    const ServerNetworkStats& server_network_stats,
     base::DictionaryValue* server_pref_dict) {
-  if (!server_network_stats)
-    return;
-
   auto server_network_stats_dict = base::MakeUnique<base::DictionaryValue>();
   // Becasue JSON doesn't support int64_t, persist int64_t as a string.
   server_network_stats_dict->SetInteger(
-      kSrttKey, static_cast<int>(server_network_stats->srtt.ToInternalValue()));
+      kSrttKey, static_cast<int>(server_network_stats.srtt.ToInternalValue()));
   // TODO(rtenneti): When QUIC starts using bandwidth_estimate, then persist
   // bandwidth_estimate.
   server_pref_dict->SetWithoutPathExpansion(
@@ -1125,14 +1132,11 @@ void HttpServerPropertiesManager::SaveNetworkStatsToServerPrefs(
 }
 
 void HttpServerPropertiesManager::SaveQuicServerInfoMapToServerPrefs(
-    QuicServerInfoMap* quic_server_info_map,
+    const QuicServerInfoMap& quic_server_info_map,
     base::DictionaryValue* http_server_properties_dict) {
-  if (!quic_server_info_map)
-    return;
-
   auto quic_servers_dict = base::MakeUnique<base::DictionaryValue>();
   for (const std::pair<QuicServerId, std::string>& entry :
-       *quic_server_info_map) {
+       quic_server_info_map) {
     const QuicServerId& server_id = entry.first;
     auto quic_server_pref_dict = base::MakeUnique<base::DictionaryValue>();
     quic_server_pref_dict->SetStringWithoutPathExpansion(kServerInfoKey,