Do not persist HttpServerProperties to disk that often.

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 467c5b804545251f3a0dfde3271165d2be0c2e18..75edfc499575d94691479db9a98dfe60f50e3679 100644
--- a/net/http/http_server_properties_manager.cc
+++ b/net/http/http_server_properties_manager.cc
@@ -32,6 +32,10 @@ const int64_t kUpdateCacheDelayMs = 1000;
 // period will reset the timer.
 const int64_t kUpdatePrefsDelayMs = 60000;
 
+// Maximum time to wait between ScheduleUpdatePrefsOnNetworkThread() calls from
+// SetAlternativeService() and SetAlternativeServices(), in seconds.
+const int64_t kAlternativeServicesUpdatePrefsS = 3600;

[Ryan Hamilton, 2016/07/21 22:27:05]

This is one hour, right? That seems like a LONG time. On mobile, that's forever.

[Bence, 2016/07/22 16:28:54]

Well, then how about five minutes?  Then I can change it to milliseconds to
match the other two.

+
 // "version" 0 indicates, http_server_properties doesn't have "version"
 // property.
 const int kMissingVersion = 0;
@@ -81,7 +85,8 @@ HttpServerPropertiesManager::HttpServerPropertiesManager(
     : pref_task_runner_(base::ThreadTaskRunnerHandle::Get()),
       pref_delegate_(pref_delegate),
       setting_prefs_(false),
-      network_task_runner_(network_task_runner) {
+      network_task_runner_(network_task_runner),
+      last_update_prefs_from_cache_(base::Time::Now()) {

[Bence, 2016/07/22 16:28:54]

Oops.

   DCHECK(pref_delegate_);
   pref_weak_ptr_factory_.reset(
       new base::WeakPtrFactory<HttpServerPropertiesManager>(this));
@@ -195,8 +200,12 @@ bool HttpServerPropertiesManager::SetAlternativeService(
     const AlternativeService& alternative_service,
     base::Time expiration) {
   DCHECK(network_task_runner_->RunsTasksOnCurrentThread());
-  const bool changed = http_server_properties_impl_->SetAlternativeService(
-      origin, alternative_service, expiration);
+  const bool changed =
+      http_server_properties_impl_->SetAlternativeService(
+          origin, alternative_service, expiration) ||
+      (last_update_prefs_from_cache_ +
+           base::TimeDelta::FromSeconds(kAlternativeServicesUpdatePrefsS) <
+       base::Time::Now());
   if (changed) {
     ScheduleUpdatePrefsOnNetworkThread(SET_ALTERNATIVE_SERVICES);
   }
@@ -207,8 +216,12 @@ bool HttpServerPropertiesManager::SetAlternativeServices(
     const url::SchemeHostPort& origin,
     const AlternativeServiceInfoVector& alternative_service_info_vector) {
   DCHECK(network_task_runner_->RunsTasksOnCurrentThread());
-  const bool changed = http_server_properties_impl_->SetAlternativeServices(
-      origin, alternative_service_info_vector);
+  const bool changed =
+      http_server_properties_impl_->SetAlternativeServices(
+          origin, alternative_service_info_vector) ||
+      (last_update_prefs_from_cache_ +
+           base::TimeDelta::FromSeconds(kAlternativeServicesUpdatePrefsS) <
+       base::Time::Now());

[Ryan Hamilton, 2016/07/21 22:27:05]

Hm. If nothing has changed, why bother persisting again?

[Bence, 2016/07/22 16:28:54]

Because old_time in HttpServerPropertiesImpl is the expiration time of the entry
in memory, not of the one on disk.  Imagine a server advertising max-age of one
week.  If there is a request every three days, old_time is four days in the
future, new_time is seven, so |changed| is false, and we will never ever ever
persist to disk.  On desktop, Chrome could be running for months.  Then there is
a crash or someone trips in the power chord, and we do not have a valid Alt-Svc
entry on disk for the next startup.

[Ryan Hamilton, 2016/07/22 23:31:35]

I'm not sure this bothers me. If the user only hits this site every 3 days, and
hits *no other* sites which might cause HttpServerProperties to be persisted,
then it's really no big deal if this happens. That being said, in this example,
won't we persist on the first request (day 0), and then we crash on day 3, and
then write again on day 6?

[Bence, 2016/07/27 16:30:55]

All right then.  I think you convinced me that the added complexity is not worth
it.  I'm reverting this timing logic.

   if (changed) {
     ScheduleUpdatePrefsOnNetworkThread(SET_ALTERNATIVE_SERVICES);
   }
@@ -896,6 +909,7 @@ void HttpServerPropertiesManager::UpdatePrefsFromCacheOnNetworkThread() {
 void HttpServerPropertiesManager::UpdatePrefsFromCacheOnNetworkThread(
     const base::Closure& completion) {
   DCHECK(network_task_runner_->RunsTasksOnCurrentThread());
+  last_update_prefs_from_cache_ = base::Time::Now();
 
   // It is in MRU order.
   base::ListValue* spdy_server_list = new base::ListValue;