Make URLRequestContextBuilder use base/task_scheduler/

net/url_request/url_request_context_builder.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/url_request/url_request_context_builder.h"
 
 #include <string>
 #include <utility>
 #include <vector>
 
 #include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/message_loop/message_loop.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_util.h"
-#include "base/threading/thread.h"
-#include "base/threading/thread_task_runner_handle.h"
+#include "base/task_scheduler/post_task.h"
 #include "net/base/cache_type.h"
 #include "net/base/net_errors.h"
 #include "net/base/network_delegate_impl.h"
 #include "net/base/sdch_manager.h"
 #include "net/cert/cert_verifier.h"
 #include "net/cert/ct_known_logs.h"
 #include "net/cert/ct_log_verifier.h"
 #include "net/cert/ct_policy_enforcer.h"
 #include "net/cert/ct_verifier.h"
 #include "net/cert/multi_log_ct_verifier.h"
@@ skipping 107 matching lines @@
   DISALLOW_COPY_AND_ASSIGN(BasicNetworkDelegate);
 };
 
 // A URLRequestContext subclass that owns most of its components
 // via a UrlRequestContextStorage object. When URLRequestContextBuilder::Build()
 // is called, ownership of all URLRequestContext components is passed to the
 // ContainerURLRequestContext. Since this cancels requests in its destructor,
 // it's not safe to subclass this.
 class ContainerURLRequestContext final : public URLRequestContext {
  public:
-  explicit ContainerURLRequestContext(
-      const scoped_refptr<base::SingleThreadTaskRunner>& file_task_runner)
-      : file_task_runner_(file_task_runner), storage_(this) {}
+  ContainerURLRequestContext() : storage_(this) {}
 
   ~ContainerURLRequestContext() override {
     // Destroy the ReportingService before the rest of the URLRequestContext, so
     // it cancels any pending requests it may have.
     storage_.set_reporting_service(nullptr);
 
     // Shut down the ProxyService, as it may have pending URLRequests using this
     // context. Since this cancels requests, it's not safe to subclass this, as
     // some parts of the URLRequestContext may then be torn down before this
     // cancels the ProxyService's URLRequests.
     proxy_service()->OnShutdown();
 
     AssertNoURLRequests();
   }
 
   URLRequestContextStorage* storage() {
     return &storage_;
   }
 
-  scoped_refptr<base::SingleThreadTaskRunner>& GetFileTaskRunner() {
-    // Create a new thread to run file tasks, if needed.
-    if (!file_task_runner_) {
-      DCHECK(!file_thread_);
-      file_thread_.reset(new base::Thread("Network File Thread"));
-      file_thread_->StartWithOptions(
-          base::Thread::Options(base::MessageLoop::TYPE_DEFAULT, 0));
-      file_task_runner_ = file_thread_->task_runner();
-    }
-    return file_task_runner_;
-  }
-
   void set_transport_security_persister(
       std::unique_ptr<TransportSecurityPersister>
           transport_security_persister) {
     transport_security_persister_ = std::move(transport_security_persister);
   }
 
  private:
-  // The thread should be torn down last.
-  std::unique_ptr<base::Thread> file_thread_;
-  scoped_refptr<base::SingleThreadTaskRunner> file_task_runner_;
-
   URLRequestContextStorage storage_;
   std::unique_ptr<TransportSecurityPersister> transport_security_persister_;
 
   DISALLOW_COPY_AND_ASSIGN(ContainerURLRequestContext);
 };
 
 }  // namespace
 
 URLRequestContextBuilder::HttpCacheParams::HttpCacheParams()
     : type(IN_MEMORY),
@@ skipping 113 matching lines @@
   http_auth_handler_factory_ = std::move(factory);
 }
 
 void URLRequestContextBuilder::SetHttpServerProperties(
     std::unique_ptr<HttpServerProperties> http_server_properties) {
   http_server_properties_ = std::move(http_server_properties);
 }
 
 std::unique_ptr<URLRequestContext> URLRequestContextBuilder::Build() {
   std::unique_ptr<ContainerURLRequestContext> context(
-      new ContainerURLRequestContext(file_task_runner_));
+      new ContainerURLRequestContext());
   URLRequestContextStorage* storage = context->storage();
 
   context->set_name(name_);
   context->set_enable_brotli(enable_brotli_);
   context->set_network_quality_estimator(network_quality_estimator_);
 
   storage->set_http_user_agent_settings(
       base::MakeUnique<StaticHttpUserAgentSettings>(accept_language_,
                                                     user_agent_));
 
@@ skipping 43 matching lines @@
   }
 
   if (sdch_enabled_) {
     storage->set_sdch_manager(
         std::unique_ptr<net::SdchManager>(new SdchManager()));
   }
 
   storage->set_transport_security_state(
       base::MakeUnique<TransportSecurityState>());
   if (!transport_security_persister_path_.empty()) {
+    // Use a low priority because saving this should not block anything
+    // user-visible. Block shutdown to ensure it does get persisted to disk,
+    // since it contains security-relevant information.
+    scoped_refptr<base::SequencedTaskRunner> task_runner(GetSequencedTaskRunner(
+        {base::MayBlock(), base::TaskPriority::BACKGROUND,
+         base::TaskShutdownBehavior::BLOCK_SHUTDOWN}));
+
     context->set_transport_security_persister(
         base::WrapUnique<TransportSecurityPersister>(
             new TransportSecurityPersister(context->transport_security_state(),
                                            transport_security_persister_path_,
-                                           context->GetFileTaskRunner(),
-                                           false)));
+                                           task_runner, false)));
   }
 
   if (http_server_properties_) {
     storage->set_http_server_properties(std::move(http_server_properties_));
   } else {
     storage->set_http_server_properties(
         std::unique_ptr<HttpServerProperties>(new HttpServerPropertiesImpl()));
   }
 
   if (cert_verifier_) {
@@ skipping 17 matching lines @@
         base::MakeUnique<URLRequestThrottlerManager>());
   }
 
   if (!proxy_service_) {
 #if !defined(OS_LINUX) && !defined(OS_ANDROID)
     // TODO(willchan): Switch to using this code when
     // ProxyService::CreateSystemProxyConfigService()'s signature doesn't suck.
     if (!proxy_config_service_) {
       proxy_config_service_ = ProxyService::CreateSystemProxyConfigService(
           base::ThreadTaskRunnerHandle::Get().get(),
-          context->GetFileTaskRunner());
+          GetSingleThreadTaskRunner(

[Randy Smith (Not in Mondays), 2017/06/21 12:56:26]

When I first looked at this, I thought "Oh, of course it needs it's own thread",
but then I realized I was confusing it with the pac resolver.  What's the
rationale for the ProxyService having it's own thread?

[mmenke, 2017/06/21 15:13:55]

ProxyService::CreateSystemProxyConfigService requires a SingleThreadTaskRunner
is the only rationale.

Turns out this argument is only used on OS_LINUX, anyways (Which the above ifdef
avoids running this code on), so I think we can just use nullptr.  Problem
solved.  :)

(Note that this code doesn't seem to be used anywhere, anyways - since it
doesn't work on Linux and Android, and so anyone who wants proxy support on
those platforms has to set up their own ProxyConfigService).

+              {base::MayBlock(), base::TaskPriority::USER_BLOCKING,
+               base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN}));

[Randy Smith (Not in Mondays), 2017/06/21 12:56:26]

Why CONTINUE_ON_SHUTDOWN for the proxy service?

[mmenke, 2017/06/21 15:13:56]

Because we aren't writing anything to disk, so no reason to block shutdown on
getting the proxy configuration.  Moot now anyways.

     }
 #endif  // !defined(OS_LINUX) && !defined(OS_ANDROID)
     proxy_service_ =
         CreateProxyService(std::move(proxy_config_service_), context.get(),
                            context->host_resolver(),
                            context->network_delegate(), context->net_log());
     proxy_service_->set_quick_check_enabled(pac_quick_check_enabled_);
     proxy_service_->set_sanitize_url_policy(pac_sanitize_url_policy_);
   }
   storage->set_proxy_service(std::move(proxy_service_));
@@ skipping 12 matching lines @@
   std::unique_ptr<HttpTransactionFactory> http_transaction_factory;
   if (http_cache_enabled_) {
     std::unique_ptr<HttpCache::BackendFactory> http_cache_backend;
     if (http_cache_params_.type != HttpCacheParams::IN_MEMORY) {
       BackendType backend_type =
           http_cache_params_.type == HttpCacheParams::DISK
               ? CACHE_BACKEND_DEFAULT
               : CACHE_BACKEND_SIMPLE;
       http_cache_backend.reset(new HttpCache::DefaultBackend(
           DISK_CACHE, backend_type, http_cache_params_.path,
-          http_cache_params_.max_size, context->GetFileTaskRunner()));
+          http_cache_params_.max_size,
+          GetSingleThreadTaskRunner(
+              {base::MayBlock(), base::TaskPriority::USER_BLOCKING,
+               base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN})));
     } else {
       http_cache_backend =
           HttpCache::DefaultBackend::InMemory(http_cache_params_.max_size);
     }
 
     http_transaction_factory.reset(new HttpCache(
         storage->http_network_session(), std::move(http_cache_backend), true));
   } else {
     http_transaction_factory.reset(
         new HttpNetworkLayer(storage->http_network_session()));
   }
   storage->set_http_transaction_factory(std::move(http_transaction_factory));
 
   URLRequestJobFactoryImpl* job_factory = new URLRequestJobFactoryImpl;
   // Adds caller-provided protocol handlers first so that these handlers are
   // used over data/file/ftp handlers below.
   for (auto& scheme_handler : protocol_handlers_) {
     job_factory->SetProtocolHandler(scheme_handler.first,
                                     std::move(scheme_handler.second));
   }
   protocol_handlers_.clear();
 
   if (data_enabled_)
     job_factory->SetProtocolHandler(url::kDataScheme,
                                     base::WrapUnique(new DataProtocolHandler));
 
 #if !BUILDFLAG(DISABLE_FILE_SUPPORT)
   if (file_enabled_) {
+    scoped_refptr<base::SequencedTaskRunner> task_runner(GetSequencedTaskRunner(
+        {base::MayBlock(), base::TaskPriority::BACKGROUND,
+         base::TaskShutdownBehavior::BLOCK_SHUTDOWN}));

[Randy Smith (Not in Mondays), 2017/06/21 12:56:26]

It's not doubt due to my mis-reading, but where is this variable used?  It looks
like it's constructed and then thrown away.

[mmenke, 2017/06/21 15:13:56]

Oops...Copy-paste error.

+
     job_factory->SetProtocolHandler(
         url::kFileScheme,
-        base::MakeUnique<FileProtocolHandler>(context->GetFileTaskRunner()));
+        base::MakeUnique<FileProtocolHandler>(
+            GetTaskRunner({base::MayBlock(), base::TaskPriority::USER_VISIBLE,
+                           base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN})));

[Randy Smith (Not in Mondays), 2017/06/21 12:56:26]

Hmmm.  Everything else was USER_BLOCKING, which made sense to me because it was
in the user request resolution path.  But this is as well, but it's only
USER_VISIBLE.  What's the rationale?

[mmenke, 2017/06/21 15:13:55]

So, actually, despite the CL description, this was copying the existing
behavior.  Using the description, though, USER_BLOCKING does seem accurate here.

Copying is also why this is SKIP_ON_SHUTDOWN - I'd tend to think
CONTINUE_ON_SHUTDOWN (Which means we don't block shutdown on completing pending
tasks) would be more appropriate, but leaving as-is for now.

[Randy Smith (Not in Mondays), 2017/06/21 15:48:52]

Ah, I missed that.  I'd suggest being pretty clear in the CL description as to
what behaviors are expected to change and what not as a result of this CL--the
regression investigator you'll be helping is unlikely to be yourself, but might
appreciate it anyway :-}.
Agreed now that I've mapped in the constant definitions, but if you want to keep
it to match existing behavior, that's also ok by me.

   }
 #endif  // !BUILDFLAG(DISABLE_FILE_SUPPORT)
 
 #if !BUILDFLAG(DISABLE_FTP_SUPPORT)
   if (ftp_enabled_) {
     job_factory->SetProtocolHandler(
         url::kFtpScheme, FtpProtocolHandler::Create(context->host_resolver()));
   }
 #endif  // !BUILDFLAG(DISABLE_FTP_SUPPORT)
 
@@ skipping 21 matching lines @@
 std::unique_ptr<ProxyService> URLRequestContextBuilder::CreateProxyService(
     std::unique_ptr<ProxyConfigService> proxy_config_service,
     URLRequestContext* url_request_context,
     HostResolver* host_resolver,
     NetworkDelegate* network_delegate,
     NetLog* net_log) {
   return ProxyService::CreateUsingSystemProxyResolver(
       std::move(proxy_config_service), net_log);
 }
 
+scoped_refptr<base::TaskRunner> URLRequestContextBuilder::GetTaskRunner(
+    const base::TaskTraits& traits) {
+  if (file_task_runner_)
+    return file_task_runner_;
+  return base::CreateTaskRunnerWithTraits(traits);
+}
+
+scoped_refptr<base::SequencedTaskRunner>
+URLRequestContextBuilder::GetSequencedTaskRunner(
+    const base::TaskTraits& traits) {
+  if (file_task_runner_)
+    return file_task_runner_;
+  return base::CreateSequencedTaskRunnerWithTraits(traits);
+}
+
+scoped_refptr<base::SingleThreadTaskRunner>
+URLRequestContextBuilder::GetSingleThreadTaskRunner(
+    const base::TaskTraits& traits) {
+  if (file_task_runner_)
+    return file_task_runner_;
+  return base::CreateSingleThreadTaskRunnerWithTraits(traits);
+}
+
 }  // namespace net