Make NetworkContext wait until on-disk prefs have been cleared.

Previously, NetworkContext::ClearNetworkingHistorySince would tell
HttpServerProperties to clear data, and then immediately inform the
caller that the data was cleared. The data was deleted from memory by
that point, but hadn't been written to disk, and we didn't even trigger
a flush to disk, at that point.

This CL makes changes that. We now pass in the callback to the
HttpServerProperties, which writes the data to the pref store and
then tells the pref store to flush data to disk. Only once the newly
cleared data has been flushed do we invoke the callback passed to
NetworkContext::ClearNetworkingHistorySince.

Bug: 795877
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.linux:linux_mojo;master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I4b1ed23eb5bdbab77871c79cdd068b95c0972535
Reviewed-on: https://chromium-review.googlesource.com/834393
Reviewed-by: Andrei Kapishnikov <kapishnikov@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526762}

components/cronet/cronet_prefs_manager.cc

@@ -6,6 +6,7 @@
 
 #include <memory>
 
+#include "base/callback.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/location.h"
@@ -110,11 +111,15 @@ class PrefServiceAdapter
     return pref_service_->GetDictionary(path_);
   }
 
-  void SetServerProperties(const base::DictionaryValue& value) override {
-    return pref_service_->Set(path_, value);
+  void SetServerProperties(const base::DictionaryValue& value,
+                           base::OnceClosure callback) override {
+    pref_service_->Set(path_, value);
+    if (callback)
+      pref_service_->CommitPendingWrite(std::move(callback));
   }
 
-  void StartListeningForUpdates(const base::Closure& callback) override {
+  void StartListeningForUpdates(
+      const base::RepeatingClosure& callback) override {
     pref_change_registrar_.Add(path_, callback);
     // Notify the pref manager that settings are already loaded, as a result
     // of initializing the pref store synchornously.

content/network/http_server_properties_pref_delegate.cc

@@ -32,12 +32,15 @@ HttpServerPropertiesPrefDelegate::GetServerProperties() const {
 }
 
 void HttpServerPropertiesPrefDelegate::SetServerProperties(
-    const base::DictionaryValue& value) {
-  return pref_service_->Set(kPrefPath, value);
+    const base::DictionaryValue& value,
+    base::OnceClosure callback) {
+  pref_service_->Set(kPrefPath, value);
+  if (callback)
+    pref_service_->CommitPendingWrite(std::move(callback));
 }
 
 void HttpServerPropertiesPrefDelegate::StartListeningForUpdates(
-    const base::Closure& callback) {
+    const base::RepeatingClosure& callback) {
   pref_change_registrar_.Add(kPrefPath, callback);
   // PrefChangeRegistrar isn't notified of initial pref load, so watch for that,
   // too.

content/network/http_server_properties_pref_delegate.h

@@ -5,6 +5,7 @@
 #ifndef CONTENT_NETWORK_HTTP_SERVER_PROPERTIES_PREF_DELEGATE_H_
 #define CONTENT_NETWORK_HTTP_SERVER_PROPERTIES_PREF_DELEGATE_H_
 
+#include "base/callback.h"
 #include "base/macros.h"
 #include "components/prefs/pref_change_registrar.h"
 #include "net/http/http_server_properties.h"
@@ -26,8 +27,10 @@ class HttpServerPropertiesPrefDelegate
 
   // net::HttpServerPropertiesManager::PrefDelegate implementation.
   const base::DictionaryValue* GetServerProperties() const override;
-  void SetServerProperties(const base::DictionaryValue& value) override;
-  void StartListeningForUpdates(const base::Closure& callback) override;
+  void SetServerProperties(const base::DictionaryValue& value,
+                           base::OnceClosure callback) override;
+  void StartListeningForUpdates(
+      const base::RepeatingClosure& callback) override;
 
  private:
   PrefService* pref_service_;

content/network/network_context.cc

@@ -373,8 +373,8 @@ void NetworkContext::ClearNetworkingHistorySince(
   url_request_context_->transport_security_state()->DeleteAllDynamicDataSince(
       time);
 
-  url_request_context_->http_server_properties()->Clear();
-  std::move(completion_callback).Run();
+  url_request_context_->http_server_properties()->Clear(
+      std::move(completion_callback));
 }
 
 void NetworkContext::SetNetworkConditions(

ios/chrome/browser/browser_state/chrome_browser_state_impl_io_data.mm

@@ -351,6 +351,9 @@ new JsonPrefStore(network_json_store_filepath,
   DCHECK(transport_security_state());
   // Completes synchronously.
   transport_security_state()->DeleteAllDynamicDataSince(time);
-  http_server_properties()->Clear();
-  web::WebThread::PostTask(web::WebThread::UI, FROM_HERE, completion);
+  http_server_properties()->Clear(base::BindOnce(
+      [](const base::Closure& completion) {
+        web::WebThread::PostTask(web::WebThread::UI, FROM_HERE, completion);
+      },
+      completion));
 }

ios/chrome/browser/net/http_server_properties_manager_factory.cc

@@ -8,6 +8,7 @@
 
 #include "base/values.h"
 #include "components/pref_registry/pref_registry_syncable.h"
+#include "components/prefs/json_pref_store.h"
 #include "ios/chrome/browser/pref_names.h"
 #include "ios/web/public/web_thread.h"
 #include "net/http/http_server_properties_manager.h"
@@ -18,7 +19,7 @@ class PrefServiceAdapter
     : public net::HttpServerPropertiesManager::PrefDelegate,
       public PrefStore::Observer {
  public:
-  explicit PrefServiceAdapter(scoped_refptr<WriteablePrefStore> pref_store)
+  explicit PrefServiceAdapter(scoped_refptr<JsonPrefStore> pref_store)
       : pref_store_(std::move(pref_store)),
         path_(prefs::kHttpServerProperties) {
     pref_store_->AddObserver(this);
@@ -37,11 +38,15 @@ class PrefServiceAdapter
 
     return nullptr;
   }
-  void SetServerProperties(const base::DictionaryValue& value) override {
-    return pref_store_->SetValue(path_, value.CreateDeepCopy(),
-                                 WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS);
+  void SetServerProperties(const base::DictionaryValue& value,
+                           base::OnceClosure callback) override {
+    pref_store_->SetValue(path_, value.CreateDeepCopy(),
+                          WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS);
+    if (callback)
+      pref_store_->CommitPendingWrite(std::move(callback));
   }
-  void StartListeningForUpdates(const base::Closure& callback) override {
+  void StartListeningForUpdates(
+      const base::RepeatingClosure& callback) override {
     on_changed_callback_ = callback;
   }
 
@@ -56,7 +61,7 @@ class PrefServiceAdapter
   }
 
  private:
-  scoped_refptr<WriteablePrefStore> pref_store_;
+  scoped_refptr<JsonPrefStore> pref_store_;
   const std::string path_;
 
   base::Closure on_changed_callback_;
@@ -69,7 +74,7 @@ class PrefServiceAdapter
 // static
 std::unique_ptr<net::HttpServerPropertiesManager>
 HttpServerPropertiesManagerFactory::CreateManager(
-    scoped_refptr<WriteablePrefStore> pref_store,
+    scoped_refptr<JsonPrefStore> pref_store,
     net::NetLog* net_log) {
   DCHECK_CURRENTLY_ON(web::WebThread::IO);
   return std::make_unique<net::HttpServerPropertiesManager>(

ios/chrome/browser/net/http_server_properties_manager_factory.h

@@ -9,7 +9,8 @@
 
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
-#include "components/prefs/writeable_pref_store.h"
+
+class JsonPrefStore;
 
 namespace net {
 class HttpServerPropertiesManager;
@@ -21,7 +22,7 @@ class HttpServerPropertiesManagerFactory {
  public:
   // Create an instance of HttpServerPropertiesManager.
   static std::unique_ptr<net::HttpServerPropertiesManager> CreateManager(
-      scoped_refptr<WriteablePrefStore> pref_store,
+      scoped_refptr<JsonPrefStore> pref_store,
       net::NetLog* net_log);
 
  private:

net/http/http_server_properties.h

@@ -13,6 +13,7 @@
 #include <tuple>
 #include <vector>
 
+#include "base/callback.h"
 #include "base/containers/mru_cache.h"
 #include "base/macros.h"
 #include "base/time/time.h"
@@ -313,8 +314,10 @@ class NET_EXPORT HttpServerProperties {
   HttpServerProperties() {}
   virtual ~HttpServerProperties() {}
 
-  // Deletes all data.
-  virtual void Clear() = 0;
+  // Deletes all data. If |callback| is non-null, flushes data to disk
+  // and invokes the callback asynchronously once changes have been written to
+  // disk.
+  virtual void Clear(base::OnceClosure callback) = 0;
 
   // Returns true if |server| supports a network protocol which honors
   // request prioritization.

net/http/http_server_properties_impl.cc

@@ -177,7 +177,7 @@ HttpServerPropertiesImpl::recently_broken_alternative_services() const {
   return broken_alternative_services_.recently_broken_alternative_services();
 }
 
-void HttpServerPropertiesImpl::Clear() {
+void HttpServerPropertiesImpl::Clear(base::OnceClosure callback) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   spdy_servers_map_.Clear();
   alternative_service_map_.Clear();
@@ -187,6 +187,11 @@ void HttpServerPropertiesImpl::Clear() {
   server_network_stats_map_.Clear();
   quic_server_info_map_.Clear();
   canonical_server_info_map_.clear();
+
+  if (!callback.is_null()) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                  std::move(callback));
+  }
 }
 
 bool HttpServerPropertiesImpl::SupportsRequestPriority(

net/http/http_server_properties_impl.h

@@ -13,6 +13,7 @@
 #include <string>
 #include <vector>
 
+#include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/threading/thread_checker.h"
@@ -86,7 +87,7 @@ class NET_EXPORT HttpServerPropertiesImpl
   // HttpServerProperties methods:
   // -----------------------------
 
-  void Clear() override;
+  void Clear(base::OnceClosure callback) override;
   bool SupportsRequestPriority(const url::SchemeHostPort& server) override;
   bool GetSupportsSpdy(const url::SchemeHostPort& server) override;
   void SetSupportsSpdy(const url::SchemeHostPort& server,

net/http/http_server_properties_impl_unittest.cc

@@ -280,9 +280,21 @@ TEST_F(SpdyServerPropertiesTest, Clear) {
   EXPECT_TRUE(impl_.SupportsRequestPriority(spdy_server_google));
   EXPECT_TRUE(impl_.SupportsRequestPriority(spdy_server_mail));
 
-  impl_.Clear();
+  base::RunLoop run_loop;
+  bool callback_invoked_ = false;
+  impl_.Clear(base::BindOnce(
+      [](bool* callback_invoked, base::OnceClosure quit_closure) {
+        *callback_invoked = true;
+        std::move(quit_closure).Run();
+      },
+      &callback_invoked_, run_loop.QuitClosure()));
   EXPECT_FALSE(impl_.SupportsRequestPriority(spdy_server_google));
   EXPECT_FALSE(impl_.SupportsRequestPriority(spdy_server_mail));
+
+  // Callback should be run asynchronously.
+  EXPECT_FALSE(callback_invoked_);
+  run_loop.Run();
+  EXPECT_TRUE(callback_invoked_);
 }
 
 TEST_F(SpdyServerPropertiesTest, MRUOfSpdyServersMap) {
@@ -329,7 +341,7 @@ TEST_F(AlternateProtocolServerPropertiesTest, Basic) {
   EXPECT_EQ(alternative_service,
             alternative_service_info_vector[0].alternative_service());
 
-  impl_.Clear();
+  impl_.Clear(base::OnceClosure());
   EXPECT_FALSE(HasAlternativeService(test_server));
 }
 
@@ -933,7 +945,7 @@ TEST_F(AlternateProtocolServerPropertiesTest, ClearWithCanonical) {
                                                    "bar.c.youtube.com", 1234);
 
   SetAlternativeService(canonical_server, canonical_alternative_service);
-  impl_.Clear();
+  impl_.Clear(base::OnceClosure());
   EXPECT_FALSE(HasAlternativeService(test_server));
 }
 
@@ -1093,7 +1105,7 @@ TEST_F(SupportsQuicServerPropertiesTest, SetSupportsQuic) {
   EXPECT_TRUE(impl_.GetSupportsQuic(&address));
   EXPECT_EQ(actual_address, address);
 
-  impl_.Clear();
+  impl_.Clear(base::OnceClosure());
 
   EXPECT_FALSE(impl_.GetSupportsQuic(&address));
 }
@@ -1186,7 +1198,7 @@ TEST_F(ServerNetworkStatsServerPropertiesTest, SetServerNetworkStats) {
   // Https server should have nothing set for server network stats.
   EXPECT_EQ(NULL, impl_.GetServerNetworkStats(foo_https_server));
 
-  impl_.Clear();
+  impl_.Clear(base::OnceClosure());
   EXPECT_EQ(NULL, impl_.GetServerNetworkStats(foo_http_server));
   EXPECT_EQ(NULL, impl_.GetServerNetworkStats(foo_https_server));
 }
@@ -1305,7 +1317,7 @@ TEST_F(QuicServerInfoServerPropertiesTest, SetQuicServerInfo) {
   EXPECT_EQ(1u, impl_.quic_server_info_map().size());
   EXPECT_EQ(quic_server_info1, *(impl_.GetQuicServerInfo(quic_server_id)));
 
-  impl_.Clear();
+  impl_.Clear(base::OnceClosure());
   EXPECT_EQ(0u, impl_.quic_server_info_map().size());
   EXPECT_EQ(nullptr, impl_.GetQuicServerInfo(quic_server_id));
 }

net/http/http_server_properties_manager.cc

@@ -116,9 +116,9 @@ HttpServerPropertiesManager::HttpServerPropertiesManager(
   DCHECK(pref_delegate_);
   DCHECK(clock_);
 
-  pref_delegate_->StartListeningForUpdates(
-      base::Bind(&HttpServerPropertiesManager::OnHttpServerPropertiesChanged,
-                 base::Unretained(this)));
+  pref_delegate_->StartListeningForUpdates(base::BindRepeating(
+      &HttpServerPropertiesManager::OnHttpServerPropertiesChanged,
+      base::Unretained(this)));
   net_log_.BeginEvent(NetLogEventType::HTTP_SERVER_PROPERTIES_INITIALIZATION);
 
   http_server_properties_impl_.reset(new HttpServerPropertiesImpl(clock_));
@@ -127,7 +127,7 @@ HttpServerPropertiesManager::HttpServerPropertiesManager(
 HttpServerPropertiesManager::~HttpServerPropertiesManager() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   // Flush settings on destruction.
-  UpdatePrefsFromCache();
+  UpdatePrefsFromCache(base::OnceClosure());
 }
 
 // static
@@ -141,11 +141,11 @@ void HttpServerPropertiesManager::SetVersion(
     http_server_properties_dict->SetInteger(kVersionKey, version_number);
 }
 
-void HttpServerPropertiesManager::Clear() {
+void HttpServerPropertiesManager::Clear(base::OnceClosure callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  http_server_properties_impl_->Clear();
-  UpdatePrefsFromCache();
+  http_server_properties_impl_->Clear(base::OnceClosure());
+  UpdatePrefsFromCache(std::move(callback));
 }
 
 bool HttpServerPropertiesManager::SupportsRequestPriority(
@@ -978,15 +978,17 @@ void HttpServerPropertiesManager::ScheduleUpdatePrefs(Location location) {
     return;
 
   network_prefs_update_timer_.Start(
-      FROM_HERE, kUpdatePrefsDelay, this,
-      &HttpServerPropertiesManager::UpdatePrefsFromCache);
+      FROM_HERE, kUpdatePrefsDelay,
+      base::Bind(&HttpServerPropertiesManager::UpdatePrefsFromCache,
+                 base::Unretained(this), base::Passed(base::OnceClosure())));
 
   // TODO(rtenneti): Delete the following histogram after collecting some data.
   UMA_HISTOGRAM_ENUMERATION("Net.HttpServerProperties.UpdatePrefs", location,
                             HttpServerPropertiesManager::NUM_LOCATIONS);
 }
 
-void HttpServerPropertiesManager::UpdatePrefsFromCache() {
+void HttpServerPropertiesManager::UpdatePrefsFromCache(
+    base::OnceClosure callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   typedef base::MRUCache<url::SchemeHostPort, ServerPref> ServerPrefMap;
@@ -1114,7 +1116,8 @@ void HttpServerPropertiesManager::UpdatePrefsFromCache() {
       &http_server_properties_dict);
 
   setting_prefs_ = true;
-  pref_delegate_->SetServerProperties(http_server_properties_dict);
+  pref_delegate_->SetServerProperties(http_server_properties_dict,
+                                      std::move(callback));
   setting_prefs_ = false;
 
   net_log_.AddEvent(NetLogEventType::HTTP_SERVER_PROPERTIES_UPDATE_PREFS,