Make ProfileIOData's ProxyService fetch PACs with the main URLRequest…

…Context.

This is intended to make servicification a little simpler, since we won't
have to configure a secret global URLRequestContext used just to fetch PAC
scripts.

The system URLRequestContext will be updated in a similar manner in a
followup CL.

BUG=715697

Review-Url: https://codereview.chromium.org/2841163002
Cr-Commit-Position: refs/heads/master@{#469044}

chrome/browser/net/proxy_browsertest.cc

@@ -4,27 +4,35 @@
 
 #include "base/command_line.h"
 #include "base/files/file_path.h"
+#include "base/location.h"
 #include "base/macros.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/login/login_handler.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/test/browser_test_utils.h"
+#include "net/base/load_flags.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
+#include "net/test/embedded_test_server/embedded_test_server_connection_listener.h"
 #include "net/test/spawned_test_server/spawned_test_server.h"
 #include "net/test/test_data_directory.h"
+#include "net/url_request/url_fetcher.h"
+#include "net/url_request/url_fetcher_delegate.h"
+#include "url/gurl.h"
 
 namespace {
 
@@ -275,4 +283,91 @@ IN_PROC_BROWSER_TEST_F(OutOfProcessProxyResolverBrowserTest, Verify) {
   VerifyProxyScript(browser());
 }
 
+// Waits for the one connection. It's fine if there are more.
+class WaitForConnectionsListener
+    : public net::test_server::EmbeddedTestServerConnectionListener {
+ public:
+  WaitForConnectionsListener() {}
+
+  void AcceptedSocket(const net::StreamSocket& socket) override {
+    content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+                                     run_loop_.QuitClosure());
+  }
+
+  void ReadFromSocket(const net::StreamSocket& socket, int rv) override {}
+
+  void Wait() { run_loop_.Run(); }
+
+ private:
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
+
+  base::RunLoop run_loop_;
+
+  DISALLOW_COPY_AND_ASSIGN(WaitForConnectionsListener);
+};
+
+// Fetch PAC script via a hanging http:// URL.
+class HangingPacRequestProxyScriptBrowserTest : public InProcessBrowserTest {
+ public:
+  HangingPacRequestProxyScriptBrowserTest() {}
+  ~HangingPacRequestProxyScriptBrowserTest() override {}
+
+  void SetUp() override {
+    // Must start listening (And get a port for the proxy) before calling
+    // SetUp().
+    ASSERT_TRUE(embedded_test_server()->InitializeAndListen());
+    InProcessBrowserTest::SetUp();
+  }
+
+  void SetUpOnMainThread() override {
+    // This must be created after the main message loop has been set up.
+    connection_listener_ = base::MakeUnique<WaitForConnectionsListener>();
+    embedded_test_server()->SetConnectionListener(connection_listener_.get());
+    embedded_test_server()->StartAcceptingConnections();
+
+    InProcessBrowserTest::SetUpOnMainThread();
+  }
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    command_line->AppendSwitchASCII(
+        switches::kProxyPacUrl, embedded_test_server()->GetURL("/hung").spec());
+  }
+
+ protected:
+  std::unique_ptr<WaitForConnectionsListener> connection_listener_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(HangingPacRequestProxyScriptBrowserTest);
+};
+
+// URLFetcherDelegate that expects a request to hang.
+class HangingURLFetcherDelegate : public net::URLFetcherDelegate {
+ public:
+  HangingURLFetcherDelegate() {}
+  ~HangingURLFetcherDelegate() override {}
+
+  void OnURLFetchComplete(const net::URLFetcher* source) override {
+    ADD_FAILURE() << "This request should never complete.";
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(HangingURLFetcherDelegate);
+};
+
+// Check that the URLRequest for a PAC that is still alive during shutdown is
+// safely cleaned up.  This test relies on AssertNoURLRequests being called on
+// the main URLRequestContext.
+IN_PROC_BROWSER_TEST_F(HangingPacRequestProxyScriptBrowserTest, Shutdown) {
+  // Request that should hang while trying to request the PAC script.
+  // Enough requests are created on startup that this probably isn't needed, but
+  // best to be safe.
+  HangingURLFetcherDelegate hanging_request_delegate;
+  std::unique_ptr<net::URLFetcher> hanging_fetcher = net::URLFetcher::Create(
+      GURL("http://blah/"), net::URLFetcher::GET, &hanging_request_delegate);
+  hanging_fetcher->SetRequestContext(browser()->profile()->GetRequestContext());
+  hanging_fetcher->Start();
+
+  connection_listener_->Wait();
+}
+
 }  // namespace

chrome/browser/profiles/off_the_record_profile_io_data.cc

@@ -192,7 +192,6 @@ OffTheRecordProfileIOData::~OffTheRecordProfileIOData() {
 }
 
 void OffTheRecordProfileIOData::InitializeInternal(
-    std::unique_ptr<ChromeNetworkDelegate> chrome_network_delegate,
     ProfileParams* profile_params,
     content::ProtocolHandlerMap* protocol_handlers,
     content::URLRequestInterceptorScopedVector request_interceptors) const {
@@ -213,11 +212,6 @@ void OffTheRecordProfileIOData::InitializeInternal(
 
   main_context->set_net_log(io_thread->net_log());
 
-  main_context_storage->set_network_delegate(
-      std::move(chrome_network_delegate));
-
-  main_context->set_host_resolver(
-      io_thread_globals->host_resolver.get());
   main_context->set_http_auth_handler_factory(
       io_thread_globals->http_auth_handler_factory.get());
   main_context->set_proxy_service(proxy_service());

chrome/browser/profiles/off_the_record_profile_io_data.h

@@ -108,7 +108,6 @@ class OffTheRecordProfileIOData : public ProfileIOData {
   ~OffTheRecordProfileIOData() override;
 
   void InitializeInternal(
-      std::unique_ptr<ChromeNetworkDelegate> chrome_network_delegate,
       ProfileParams* profile_params,
       content::ProtocolHandlerMap* protocol_handlers,
       content::URLRequestInterceptorScopedVector request_interceptors)

chrome/browser/profiles/profile_impl_io_data.cc

@@ -435,24 +435,16 @@ ProfileImplIOData::~ProfileImplIOData() {
     media_request_context_->AssertNoURLRequests();
 }
 
-void ProfileImplIOData::InitializeInternal(
-    std::unique_ptr<ChromeNetworkDelegate> chrome_network_delegate,
-    ProfileParams* profile_params,
-    content::ProtocolHandlerMap* protocol_handlers,
-    content::URLRequestInterceptorScopedVector request_interceptors) const {
-  net::URLRequestContext* main_context = main_request_context();
-  net::URLRequestContextStorage* main_context_storage =
-      main_request_context_storage();
-
-  IOThread* const io_thread = profile_params->io_thread;
-  IOThread::Globals* const io_thread_globals = io_thread->globals();
-
+std::unique_ptr<net::NetworkDelegate>
+ProfileImplIOData::ConfigureNetworkDelegate(
+    IOThread* io_thread,
+    std::unique_ptr<ChromeNetworkDelegate> chrome_network_delegate) const {
   if (lazy_params_->domain_reliability_monitor) {
     // Hold on to a raw pointer to call Shutdown() in ~ProfileImplIOData.
     domain_reliability_monitor_ =
         lazy_params_->domain_reliability_monitor.get();
 
-    domain_reliability_monitor_->InitURLRequestContext(main_context);
+    domain_reliability_monitor_->InitURLRequestContext(main_request_context());
     domain_reliability_monitor_->AddBakedInConfigs();
     domain_reliability_monitor_->SetDiscardUploads(
         !GetMetricsEnabledStateOnIOThread());
@@ -461,6 +453,24 @@ void ProfileImplIOData::InitializeInternal(
         std::move(lazy_params_->domain_reliability_monitor));
   }
 
+  return data_reduction_proxy_io_data()->CreateNetworkDelegate(
+      io_thread->globals()->data_use_ascriber->CreateNetworkDelegate(
+          std::move(chrome_network_delegate),
+          io_thread->GetMetricsDataUseForwarder()),
+      true);
+}
+
+void ProfileImplIOData::InitializeInternal(
+    ProfileParams* profile_params,
+    content::ProtocolHandlerMap* protocol_handlers,
+    content::URLRequestInterceptorScopedVector request_interceptors) const {
+  net::URLRequestContext* main_context = main_request_context();
+  net::URLRequestContextStorage* main_context_storage =
+      main_request_context_storage();
+
+  IOThread* const io_thread = profile_params->io_thread;
+  IOThread::Globals* const io_thread_globals = io_thread->globals();
+
   ApplyProfileParamsToContext(main_context);
 
   if (lazy_params_->http_server_properties_manager) {
@@ -475,16 +485,6 @@ void ProfileImplIOData::InitializeInternal(
 
   main_context->set_net_log(io_thread->net_log());
 
-  main_context_storage->set_network_delegate(
-      data_reduction_proxy_io_data()->CreateNetworkDelegate(
-          io_thread_globals->data_use_ascriber->CreateNetworkDelegate(
-              std::move(chrome_network_delegate),
-              io_thread->GetMetricsDataUseForwarder()),
-          true));
-
-  main_context->set_host_resolver(
-      io_thread_globals->host_resolver.get());
-
   main_context->set_http_auth_handler_factory(
       io_thread_globals->http_auth_handler_factory.get());
 

chrome/browser/profiles/profile_impl_io_data.h

@@ -156,8 +156,12 @@ class ProfileImplIOData : public ProfileIOData {
   ProfileImplIOData();
   ~ProfileImplIOData() override;
 
+  std::unique_ptr<net::NetworkDelegate> ConfigureNetworkDelegate(
+      IOThread* io_thread,
+      std::unique_ptr<ChromeNetworkDelegate> chrome_network_delegate)
+      const override;
+
   void InitializeInternal(
-      std::unique_ptr<ChromeNetworkDelegate> chrome_network_delegate,
       ProfileParams* profile_params,
       content::ProtocolHandlerMap* protocol_handlers,
       content::URLRequestInterceptorScopedVector request_interceptors)

chrome/browser/profiles/profile_io_data.cc

@@ -674,6 +674,11 @@ ProfileIOData::~ProfileIOData() {
         std::unique_ptr<net::ReportingService>());
   }
 
+  // This should be shut down last, as any other requests may initiate more
+  // activity when the ProxyService aborts lookups.
+  if (proxy_service_)
+    proxy_service_->OnShutdown();
+
   // TODO(ajwong): These AssertNoURLRequests() calls are unnecessary since they
   // are already done in the URLRequestContext destructor.
   if (main_request_context_)
@@ -991,7 +996,7 @@ void ProfileIOData::Init(
 
   main_request_context_->set_enable_brotli(io_thread_globals->enable_brotli);
 
-  std::unique_ptr<ChromeNetworkDelegate> network_delegate(
+  std::unique_ptr<ChromeNetworkDelegate> chrome_network_delegate(
       new ChromeNetworkDelegate(
 #if BUILDFLAG(ENABLE_EXTENSIONS)
           io_thread_globals->extension_event_router_forwarder.get(),
@@ -1000,34 +1005,47 @@ void ProfileIOData::Init(
 #endif
           &enable_referrers_));
 #if BUILDFLAG(ENABLE_EXTENSIONS)
-  network_delegate->set_extension_info_map(
+  chrome_network_delegate->set_extension_info_map(
       profile_params_->extension_info_map.get());
   if (!command_line.HasSwitch(switches::kDisableExtensionsHttpThrottling)) {
     extension_throttle_manager_.reset(
         new extensions::ExtensionThrottleManager());
   }
 #endif
 
-  network_delegate->set_url_blacklist_manager(url_blacklist_manager_.get());
-  network_delegate->set_profile(profile_params_->profile);
-  network_delegate->set_profile_path(profile_params_->path);
-  network_delegate->set_cookie_settings(profile_params_->cookie_settings.get());
-  network_delegate->set_enable_do_not_track(&enable_do_not_track_);
-  network_delegate->set_force_google_safe_search(&force_google_safesearch_);
-  network_delegate->set_force_youtube_restrict(&force_youtube_restrict_);
-  network_delegate->set_allowed_domains_for_apps(&allowed_domains_for_apps_);
-  network_delegate->set_data_use_aggregator(
+  chrome_network_delegate->set_url_blacklist_manager(
+      url_blacklist_manager_.get());
+  chrome_network_delegate->set_profile(profile_params_->profile);
+  chrome_network_delegate->set_profile_path(profile_params_->path);
+  chrome_network_delegate->set_cookie_settings(
+      profile_params_->cookie_settings.get());
+  chrome_network_delegate->set_enable_do_not_track(&enable_do_not_track_);
+  chrome_network_delegate->set_force_google_safe_search(
+      &force_google_safesearch_);
+  chrome_network_delegate->set_force_youtube_restrict(&force_youtube_restrict_);
+  chrome_network_delegate->set_allowed_domains_for_apps(
+      &allowed_domains_for_apps_);
+  chrome_network_delegate->set_data_use_aggregator(
       io_thread_globals->data_use_aggregator.get(), IsOffTheRecord());
 
+  std::unique_ptr<net::NetworkDelegate> network_delegate =
+      ConfigureNetworkDelegate(profile_params_->io_thread,
+                               std::move(chrome_network_delegate));
+
+  main_request_context_->set_host_resolver(
+      io_thread_globals->host_resolver.get());
+
   // NOTE: Proxy service uses the default io thread network delegate, not the
   // delegate just created.
   proxy_service_ = ProxyServiceFactory::CreateProxyService(
-      io_thread->net_log(),
-      io_thread_globals->proxy_script_fetcher_context.get(),
-      io_thread_globals->system_network_delegate.get(),
+      io_thread->net_log(), main_request_context_.get(), network_delegate.get(),
       std::move(profile_params_->proxy_config_service), command_line,
       io_thread->WpadQuickCheckEnabled(),
       io_thread->PacHttpsUrlStrippingEnabled());
+
+  main_request_context_storage_->set_network_delegate(
+      std::move(network_delegate));
+
   transport_security_state_.reset(new net::TransportSecurityState());
   base::SequencedWorkerPool* pool = BrowserThread::GetBlockingPool();
   transport_security_persister_.reset(
@@ -1119,8 +1137,8 @@ void ProfileIOData::Init(
       base::Bind(&IOThread::UnregisterSTHObserver, base::Unretained(io_thread),
                  ct_tree_tracker_.get());
 
-  InitializeInternal(std::move(network_delegate), profile_params_.get(),
-                     protocol_handlers, std::move(request_interceptors));
+  InitializeInternal(profile_params_.get(), protocol_handlers,
+                     std::move(request_interceptors));
 
   profile_params_.reset();
   initialized_ = true;
@@ -1310,6 +1328,13 @@ std::unique_ptr<net::HttpCache> ProfileIOData::CreateHttpFactory(
       std::move(backend), false /* is_main_cache */);
 }
 
+std::unique_ptr<net::NetworkDelegate> ProfileIOData::ConfigureNetworkDelegate(
+    IOThread* io_thread,
+    std::unique_ptr<ChromeNetworkDelegate> chrome_network_delegate) const {
+  return base::WrapUnique<net::NetworkDelegate>(
+      chrome_network_delegate.release());
+}
+
 void ProfileIOData::SetCookieSettingsForTesting(
     content_settings::CookieSettings* cookie_settings) {
   DCHECK(!cookie_settings_.get());

chrome/browser/profiles/profile_io_data.h

@@ -449,10 +449,16 @@ class ProfileIOData {
   // Virtual interface for subtypes to implement:
   // --------------------------------------------
 
+  // Does any necessary additional configuration of the network delegate,
+  // including composing it with other NetworkDelegates, if needed. By default,
+  // just returns the input NetworkDelegate.
+  virtual std::unique_ptr<net::NetworkDelegate> ConfigureNetworkDelegate(
+      IOThread* io_thread,
+      std::unique_ptr<ChromeNetworkDelegate> chrome_network_delegate) const;
+
   // Does the actual initialization of the ProfileIOData subtype. Subtypes
   // should use the static helper functions above to implement this.
   virtual void InitializeInternal(
-      std::unique_ptr<ChromeNetworkDelegate> chrome_network_delegate,
       ProfileParams* profile_params,
       content::ProtocolHandlerMap* protocol_handlers,
       content::URLRequestInterceptorScopedVector request_interceptors)

net/test/embedded_test_server/embedded_test_server.cc

@@ -78,7 +78,7 @@ EmbeddedTestServer::~EmbeddedTestServer() {
 
 void EmbeddedTestServer::SetConnectionListener(
     EmbeddedTestServerConnectionListener* listener) {
-  DCHECK(!Started());
+  DCHECK(!io_thread_.get());
   connection_listener_ = listener;
 }