HttpServerPropertiesManager: Remove use of two threads.

All consumers now use a single thread for prefs and the network stack,
so there's no need for it to have thread hopping logic.

This also makes HttpServerPropertiesManager better support the case
where prefs aren't loaded when it's created (Always the case in
consumers), and makes it flush to prefs on destruction, which can
now be done, since the net and prefs threads are the same.

This also fixes a crasher in the single-thread case, due to the use of
base::Unretained when calling from the prefs thread to the network
thread, which doesn't work when they're the same thread.

Bug: 768884, 770179
NOPRESUBMIT=true
(Unfortunately, the tests use banned APIs. This use predates this CL,
    and this CL doesn't introduce new uses of them. Fixing a crasher
    is more important than removing banned APIs).

Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I2692e3c02ff6028b585416808b60cabcf9d2e2e6
Reviewed-on: https://chromium-review.googlesource.com/693054Reviewed-by: Andrei Kapishnikov <kapishnikov@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Bence Béky <bnc@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506897}

components/cronet/android/cronet_url_request_context_adapter.h

@@ -224,18 +224,19 @@ class CronetURLRequestContextAdapter
   // that weakly references |network_quality_estimator_|.
   std::unique_ptr<net::NetworkQualityEstimator> network_quality_estimator_;
 
-  std::unique_ptr<net::URLRequestContext> context_;
-
   // Manages the PrefService and all associated persistence managers
   // such as NetworkQualityPrefsManager, HostCachePersistenceManager, etc.
   // It should be destroyed before |network_quality_estimator_| and
-  // |context_|.
+  // after |context_|.
   std::unique_ptr<CronetPrefsManager> cronet_prefs_manager_;
 
-  std::unique_ptr<net::ProxyConfigService> proxy_config_service_;
+  std::unique_ptr<net::URLRequestContext> context_;
 
   // Context config is only valid until context is initialized.
   std::unique_ptr<URLRequestContextConfig> context_config_;
+  // As is the proxy config service, as ownership is passed to the
+  // URLRequestContextBuilder.
+  std::unique_ptr<net::ProxyConfigService> proxy_config_service_;
 
   // Effective experimental options. Kept for NetLog.
   std::unique_ptr<base::DictionaryValue> effective_experimental_options_;

components/cronet/cronet_prefs_manager.cc

@@ -6,7 +6,9 @@
 
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
+#include "base/location.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "base/time/time.h"
 #include "components/cronet/host_cache_persistence_manager.h"
 #include "components/prefs/json_pref_store.h"
@@ -101,14 +103,8 @@ class PrefServiceAdapter
   ~PrefServiceAdapter() override {}
 
   // PrefDelegate implementation.
-  bool HasServerProperties() override {
-    return pref_service_->HasPrefPath(path_);
-  }
-
-  const base::DictionaryValue& GetServerProperties() const override {
-    // Guaranteed not to return null when the pref is registered
-    // (RegisterProfilePrefs was called).
-    return *pref_service_->GetDictionary(path_);
+  const base::DictionaryValue* GetServerProperties() const override {
+    return pref_service_->GetDictionary(path_);
   }
 
   void SetServerProperties(const base::DictionaryValue& value) override {
@@ -117,10 +113,9 @@ class PrefServiceAdapter
 
   void StartListeningForUpdates(const base::Closure& callback) override {
     pref_change_registrar_.Add(path_, callback);
-  }
-
-  void StopListeningForUpdates() override {
-    pref_change_registrar_.RemoveAll();
+    // Notify the pref manager that settings are already loaded, as a result
+    // of initializing the pref store synchornously.
+    base::SequencedTaskRunnerHandle::Get()->PostTask(FROM_HERE, callback);
   }
 
  private:
@@ -245,9 +240,7 @@ CronetPrefsManager::CronetPrefsManager(
   }
 
   http_server_properties_manager_ = new net::HttpServerPropertiesManager(
-      new PrefServiceAdapter(pref_service_.get()), network_task_runner,
-      network_task_runner, net_log);
-  http_server_properties_manager_->InitializeOnNetworkSequence();
+      std::make_unique<PrefServiceAdapter>(pref_service_.get()), net_log);
 
   // Passes |http_server_properties_manager_| ownership to |context_builder|.
   // The ownership will be subsequently passed to UrlRequestContext.
@@ -289,9 +282,10 @@ void CronetPrefsManager::PrepareForShutdown() {
     pref_service_->CommitPendingWrite();
 
   // Shutdown managers on the Pref sequence.
-  http_server_properties_manager_->ShutdownOnPrefSequence();
   if (network_qualities_prefs_manager_)
     network_qualities_prefs_manager_->ShutdownOnPrefSequence();
+
+  host_cache_persistence_manager_.reset();
 }
 
 }  // namespace cronet

components/cronet/ios/cronet_environment.mm

@@ -272,14 +272,14 @@ void CronetEnvironment::CleanUpOnNetworkThread() {
     cronet_prefs_manager_->PrepareForShutdown();
   }
 
+  // TODO(lilyhoughton) this should be smarter about making sure there are no
+  // pending requests, etc.
+  main_context_.reset();
+
   // cronet_prefs_manager_ should be deleted on the network thread.
   cronet_prefs_manager_.reset();
 
   file_thread_.reset();
-
-  // TODO(lilyhoughton) this should be smarter about making sure there are no
-  // pending requests, etc.
-  main_context_.reset();
 }
 
 CronetEnvironment::~CronetEnvironment() {

content/network/http_server_properties_pref_delegate.cc

@@ -26,15 +26,9 @@ void HttpServerPropertiesPrefDelegate::RegisterPrefs(
   pref_registry->RegisterDictionaryPref(kPrefPath);
 }
 
-bool HttpServerPropertiesPrefDelegate::HasServerProperties() {
-  return pref_service_->HasPrefPath(kPrefPath);
-}
-
-const base::DictionaryValue&
+const base::DictionaryValue*
 HttpServerPropertiesPrefDelegate::GetServerProperties() const {
-  // Guaranteed not to return null when the pref is registered
-  // (RegisterProfilePrefs was called).
-  return *pref_service_->GetDictionary(kPrefPath);
+  return pref_service_->GetDictionary(kPrefPath);
 }
 
 void HttpServerPropertiesPrefDelegate::SetServerProperties(
@@ -54,8 +48,4 @@ void HttpServerPropertiesPrefDelegate::StartListeningForUpdates(
   }
 }
 
-void HttpServerPropertiesPrefDelegate::StopListeningForUpdates() {
-  pref_change_registrar_.RemoveAll();
-}
-
 }  // namespace content

content/network/http_server_properties_pref_delegate.h

@@ -25,11 +25,9 @@ class HttpServerPropertiesPrefDelegate
   static void RegisterPrefs(PrefRegistrySimple* pref_registry);
 
   // net::HttpServerPropertiesManager::PrefDelegate implementation.
-  bool HasServerProperties() override;
-  const base::DictionaryValue& GetServerProperties() const override;
+  const base::DictionaryValue* GetServerProperties() const override;
   void SetServerProperties(const base::DictionaryValue& value) override;
   void StartListeningForUpdates(const base::Closure& callback) override;
-  void StopListeningForUpdates() override;
 
  private:
   PrefService* pref_service_;

content/network/network_context.cc

@@ -13,7 +13,6 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/task_scheduler/post_task.h"
 #include "base/task_scheduler/task_traits.h"
-#include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "components/network_session_configurator/browser/network_session_configurator.h"
 #include "components/network_session_configurator/common/network_switches.h"
@@ -97,9 +96,6 @@ NetworkContext::~NetworkContext() {
   while (!url_loaders_.empty())
     (*url_loaders_.begin())->Cleanup();
 
-  if (http_server_properties_manager_)
-    http_server_properties_manager_->ShutdownOnPrefSequence();
-
   // May be nullptr in tests.
   if (network_service_)
     network_service_->DeregisterNetworkContext(this);
@@ -260,17 +256,11 @@ void NetworkContext::ApplyContextParamsToBuilder(
     HttpServerPropertiesPrefDelegate::RegisterPrefs(pref_registry.get());
     pref_service_ = pref_service_factory.Create(pref_registry.get());
 
-    std::unique_ptr<net::HttpServerPropertiesManager> http_server_properties =
+    builder->SetHttpServerProperties(
         std::make_unique<net::HttpServerPropertiesManager>(
-            // HttpServerPropertiesManager implicitly takes ownership of the
-            // PrefDelegate.
-            new HttpServerPropertiesPrefDelegate(pref_service_.get()),
-            base::ThreadTaskRunnerHandle::Get(),
-            base::ThreadTaskRunnerHandle::Get(), builder->net_log());
-    http_server_properties_manager_ = http_server_properties.get();
-    http_server_properties_manager_->InitializeOnNetworkSequence();
-
-    builder->SetHttpServerProperties(std::move(http_server_properties));
+            std::make_unique<HttpServerPropertiesPrefDelegate>(
+                pref_service_.get()),
+            builder->net_log()));
   }
 
   builder->set_data_enabled(network_context_params->enable_data_url_support);
@@ -311,12 +301,8 @@ void NetworkContext::ClearNetworkingHistorySince(
   url_request_context_->transport_security_state()->DeleteAllDynamicDataSince(
       time);
 
-  if (http_server_properties_manager_) {
-    http_server_properties_manager_->Clear(std::move(completion_callback));
-  } else {
-    url_request_context_->http_server_properties()->Clear();
-    std::move(completion_callback).Run();
-  }
+  url_request_context_->http_server_properties()->Clear();
+  std::move(completion_callback).Run();
 }
 
 }  // namespace content

content/network/network_context.h

@@ -96,11 +96,6 @@ class CONTENT_EXPORT NetworkContext : public mojom::NetworkContext {
   // Disables use of QUIC by the NetworkContext.
   void DisableQuic();
 
-  net::HttpServerPropertiesManager* http_server_properties_manager_for_testing()
-      const {
-    return http_server_properties_manager_;
-  }
-
  private:
   NetworkContext();
 
@@ -142,9 +137,6 @@ class CONTENT_EXPORT NetworkContext : public mojom::NetworkContext {
 
   std::unique_ptr<CookieManagerImpl> cookie_manager_;
 
-  // Owned by |owned_url_request_context_|. May be nullptr.
-  net::HttpServerPropertiesManager* http_server_properties_manager_ = nullptr;
-
   // Temporary class to help diagnose the impact of https://crbug.com/711579.
   // Every 24-hours, measures the size of the network cache and emits an UMA
   // metric.

content/network/network_context_unittest.cc

@@ -428,15 +428,10 @@ TEST_F(NetworkContextTest, HttpServerPropertiesToDisk) {
   network_context->url_request_context()
       ->http_server_properties()
       ->SetSupportsSpdy(kSchemeHostPort, true);
-  network_context->http_server_properties_manager_for_testing()
-      ->UpdatePrefsForTesting();
-  // Wait HttpServerProperties manager to flush to the pref service.
-  scoped_task_environment_.RunUntilIdle();
-
-  // Delete the NetworkContext and wait for its JsonPrefStore to flush to disk.
+  // Deleting the context will cause it to flush state. Wait for the pref
+  // service to flush to disk.
   network_context.reset();
   scoped_task_environment_.RunUntilIdle();
-  EXPECT_TRUE(base::PathExists(file_path));
 
   // Create a new NetworkContext using the same path for HTTP server properties.
   context_params = mojom::NetworkContextParams::New();
@@ -446,18 +441,6 @@ TEST_F(NetworkContextTest, HttpServerPropertiesToDisk) {
   // Wait for properties to load from disk.
   scoped_task_environment_.RunUntilIdle();
 
-  // Wait for net::HttpServerPropertiesManager to decide to read the prefs again
-  // from the pref file, now that it's been read.
-  base::RunLoop run_loop;
-  base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE, run_loop.QuitClosure(),
-      net::HttpServerPropertiesManager::GetUpdateCacheDelayForTesting());
-  run_loop.Run();
-
-  // Wait for the net::HttpServerPropertiesManager to actually read the prefs.
-  // TODO(mmenke): All this waiting is rather silly. Fix it.
-  scoped_task_environment_.RunUntilIdle();
-
   EXPECT_TRUE(network_context->url_request_context()
                   ->http_server_properties()
                   ->GetSupportsSpdy(kSchemeHostPort));

ios/chrome/browser/browser_state/chrome_browser_state_impl_io_data.h

@@ -21,7 +21,6 @@ class ChromeBrowserState;
 namespace net {
 class CookieStore;
 class HttpNetworkSession;
-class HttpServerPropertiesManager;
 class HttpTransactionFactory;
 }  // namespace net
 
@@ -137,10 +136,6 @@ class ChromeBrowserStateImplIOData : public ChromeBrowserStateIOData {
   mutable std::unique_ptr<net::HttpNetworkSession> http_network_session_;
   mutable std::unique_ptr<net::HttpTransactionFactory> main_http_factory_;
 
-  // Same as |ChromeBrowserState::http_server_properties_|, owned there to
-  // maintain destruction ordering.
-  mutable net::HttpServerPropertiesManager* http_server_properties_manager_;
-
   mutable std::unique_ptr<net::CookieStore> main_cookie_store_;
 
   mutable std::unique_ptr<net::URLRequestJobFactory> main_job_factory_;

ios/chrome/browser/browser_state/chrome_browser_state_impl_io_data.mm

@@ -176,13 +176,9 @@ ChromeBrowserStateImplIOData::LazyParams::~LazyParams() {}
 ChromeBrowserStateImplIOData::ChromeBrowserStateImplIOData()
     : ChromeBrowserStateIOData(
           ios::ChromeBrowserStateType::REGULAR_BROWSER_STATE),
-      http_server_properties_manager_(nullptr),
       app_cache_max_size_(0) {}
 
-ChromeBrowserStateImplIOData::~ChromeBrowserStateImplIOData() {
-  if (http_server_properties_manager_)
-    http_server_properties_manager_->ShutdownOnPrefSequence();
-}
+ChromeBrowserStateImplIOData::~ChromeBrowserStateImplIOData() {}
 
 void ChromeBrowserStateImplIOData::InitializeInternal(
     std::unique_ptr<IOSChromeNetworkDelegate> chrome_network_delegate,
@@ -206,11 +202,8 @@ void ChromeBrowserStateImplIOData::InitializeInternal(
 
   ApplyProfileParamsToContext(main_context);
 
-  http_server_properties_manager_ =
-      HttpServerPropertiesManagerFactory::CreateManager(network_json_store_,
-                                                        io_thread->net_log());
-  set_http_server_properties(base::WrapUnique(http_server_properties_manager_));
-  http_server_properties_manager_->InitializeOnNetworkSequence();
+  set_http_server_properties(HttpServerPropertiesManagerFactory::CreateManager(
+      network_json_store_, io_thread->net_log()));
 
   main_context->set_transport_security_state(transport_security_state());
 
@@ -353,8 +346,6 @@ void ChromeBrowserStateImplIOData::ClearNetworkingHistorySinceOnIOThread(
   DCHECK(transport_security_state());
   // Completes synchronously.
   transport_security_state()->DeleteAllDynamicDataSince(time);
-  DCHECK(http_server_properties_manager_);
-  http_server_properties_manager_->Clear(
-      base::BindOnce(base::IgnoreResult(&web::WebThread::PostTask),
-                     web::WebThread::UI, FROM_HERE, completion));
+  http_server_properties()->Clear();
+  web::WebThread::PostTask(web::WebThread::UI, FROM_HERE, completion);
 }

ios/chrome/browser/net/http_server_properties_manager_factory.cc

@@ -27,18 +27,15 @@ class PrefServiceAdapter
   ~PrefServiceAdapter() override { pref_store_->RemoveObserver(this); }
 
   // PrefDelegate implementation.
-  bool HasServerProperties() override {
-    return pref_store_->GetValue(path_, nullptr);
-  }
-  const base::DictionaryValue& GetServerProperties() const override {
+  const base::DictionaryValue* GetServerProperties() const override {
     const base::Value* value;
     if (pref_store_->GetValue(path_, &value)) {
       const base::DictionaryValue* dict;
       if (value->GetAsDictionary(&dict))
-        return *dict;
+        return dict;
     }
 
-    return empty_dictionary_;
+    return nullptr;
   }
   void SetServerProperties(const base::DictionaryValue& value) override {
     return pref_store_->SetValue(path_, value.CreateDeepCopy(),
@@ -47,9 +44,6 @@ class PrefServiceAdapter
   void StartListeningForUpdates(const base::Closure& callback) override {
     on_changed_callback_ = callback;
   }
-  void StopListeningForUpdates() override {
-    on_changed_callback_ = base::Closure();
-  }
 
   // PrefStore::Observer implementation.
   void OnPrefValueChanged(const std::string& key) override {
@@ -57,7 +51,7 @@ class PrefServiceAdapter
       on_changed_callback_.Run();
   }
   void OnInitializationCompleted(bool succeeded) override {
-    if (succeeded && on_changed_callback_ && HasServerProperties())
+    if (succeeded && on_changed_callback_)
       on_changed_callback_.Run();
   }
 
@@ -65,10 +59,6 @@ class PrefServiceAdapter
   scoped_refptr<WriteablePrefStore> pref_store_;
   const std::string path_;
 
-  // Returned when the pref is not set. Since the method returns a const
-  // net::DictionaryValue&, can't just create one on the stack.
-  base::DictionaryValue empty_dictionary_;
-
   base::Closure on_changed_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(PrefServiceAdapter);
@@ -77,13 +67,11 @@ class PrefServiceAdapter
 }  // namespace
 
 // static
-net::HttpServerPropertiesManager*
+std::unique_ptr<net::HttpServerPropertiesManager>
 HttpServerPropertiesManagerFactory::CreateManager(
     scoped_refptr<WriteablePrefStore> pref_store,
     net::NetLog* net_log) {
   DCHECK_CURRENTLY_ON(web::WebThread::IO);
-  return new net::HttpServerPropertiesManager(
-      new PrefServiceAdapter(std::move(pref_store)),
-      web::WebThread::GetTaskRunnerForThread(web::WebThread::IO),
-      web::WebThread::GetTaskRunnerForThread(web::WebThread::IO), net_log);
+  return std::make_unique<net::HttpServerPropertiesManager>(
+      std::make_unique<PrefServiceAdapter>(std::move(pref_store)), net_log);
 }

ios/chrome/browser/net/http_server_properties_manager_factory.h

@@ -5,6 +5,8 @@
 #ifndef IOS_CHROME_BROWSER_NET_HTTP_SERVER_PROPERTIES_MANAGER_FACTORY_H_
 #define IOS_CHROME_BROWSER_NET_HTTP_SERVER_PROPERTIES_MANAGER_FACTORY_H_
 
+#include <memory>
+
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "components/prefs/writeable_pref_store.h"
@@ -18,7 +20,7 @@ class NetLog;
 class HttpServerPropertiesManagerFactory {
  public:
   // Create an instance of HttpServerPropertiesManager.
-  static net::HttpServerPropertiesManager* CreateManager(
+  static std::unique_ptr<net::HttpServerPropertiesManager> CreateManager(
       scoped_refptr<WriteablePrefStore> pref_store,
       net::NetLog* net_log);