Make URLRequestContextBuilder use base/task_scheduler/

net/url_request/url_request_context_builder.cc

Index: net/url_request/url_request_context_builder.cc
diff --git a/net/url_request/url_request_context_builder.cc b/net/url_request/url_request_context_builder.cc
index a1e384d9886375985e156cb7eda5cea2c6b9a808..3a42739a1a043a53f4ebc9134765c498e80f5d02 100644
--- a/net/url_request/url_request_context_builder.cc
+++ b/net/url_request/url_request_context_builder.cc
@@ -15,8 +15,7 @@
 #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"
@@ -144,9 +143,7 @@ class BasicNetworkDelegate : public NetworkDelegateImpl {
 // 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
@@ -166,18 +163,6 @@ class ContainerURLRequestContext final : public URLRequestContext {
     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) {
@@ -185,10 +170,6 @@ class ContainerURLRequestContext final : public URLRequestContext {
   }
 
  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_;
 
@@ -322,7 +303,7 @@ void URLRequestContextBuilder::SetHttpServerProperties(
 
 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_);
@@ -386,12 +367,18 @@ std::unique_ptr<URLRequestContext> URLRequestContextBuilder::Build() {
   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_) {
@@ -429,7 +416,9 @@ std::unique_ptr<URLRequestContext> URLRequestContextBuilder::Build() {
     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_ =
@@ -462,7 +451,10 @@ std::unique_ptr<URLRequestContext> URLRequestContextBuilder::Build() {
               : 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);
@@ -491,9 +483,15 @@ std::unique_ptr<URLRequestContext> URLRequestContextBuilder::Build() {
 
 #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)
 
@@ -535,4 +533,27 @@ std::unique_ptr<ProxyService> URLRequestContextBuilder::CreateProxyService(
       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