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(
+        GetFileSequencedTaskRunner(
+            {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());
+          nullptr /* |file_task_runner| is only used on Linux */);
     }
 #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,
+          GetFileSingleThreadTaskRunner(
+              {base::MayBlock(), base::TaskPriority::USER_BLOCKING,
+               base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN})));

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

How confident are you that the cache is resilient against posted tasks being
killed on shutdown?  I could imagine consistency issues.  If you're matching
existing behavior, I won't block, but I'd be inclined to check with
Gavin/Maks--I could imagine this being relevant to stuff they're working on.

ETA: Ok, I went digging to see if I could confirm that this is existing
behavior, and I failed.  If the builder created its own file thread, that would
have been done with default Options::joinable, which is true, which I think is
BLOCK_SHUTDOWN.  What am I missing?

[Maks Orlovich, 2017/06/21 15:58:28]

In case of simple cache, it's "resilient" in the sense it might lose the index
and have to recover it. Don't know about block cache, but probably worse, that
one would have to be confirmed by Gavin (and might have been had I not butted in
into Matt's and his conversation..)

Though it's curious that this is passing in FileTaskRunner and not the browser
CACHE thread (which they are also trying to remove, but which is what most
clients of disk_cache::CreateCacheBackend use.

[mmenke, 2017/06/21 16:07:55]

So I think you're right about this.  I can't remember what I was thinking when I
made this CONTNIUE_ON_SHUTDOWN (May have been confused by the BLOCKING in
USER_BLOCKING or something.  Or may not have been thinking at all).
Note that the "file" TaskRunner is just the single task runner passed into the
URLRequestContextBuilder for file I/O.  It's currently nullptr in Chrome, and
we're falling back to using the task scheduler.  Of course, this isn't being
used at all currently, anyways, since this class is only being used with the
SystemURLRequestContext, which doesn't have a cache.

     } 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()));
@@ skipping 10 matching lines @@
   protocol_handlers_.clear();
 
   if (data_enabled_)
     job_factory->SetProtocolHandler(url::kDataScheme,
                                     base::WrapUnique(new DataProtocolHandler));
 
 #if !BUILDFLAG(DISABLE_FILE_SUPPORT)
   if (file_enabled_) {
     job_factory->SetProtocolHandler(
         url::kFileScheme,
-        base::MakeUnique<FileProtocolHandler>(context->GetFileTaskRunner()));
+        base::MakeUnique<FileProtocolHandler>(GetFileTaskRunner(
+            {base::MayBlock(), base::TaskPriority::USER_BLOCKING,
+             base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN})));
   }
 #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::GetFileTaskRunner(
+    const base::TaskTraits& traits) {
+  if (file_task_runner_)
+    return file_task_runner_;
+  return base::CreateTaskRunnerWithTraits(traits);
+}
+
+scoped_refptr<base::SequencedTaskRunner>
+URLRequestContextBuilder::GetFileSequencedTaskRunner(
+    const base::TaskTraits& traits) {
+  if (file_task_runner_)
+    return file_task_runner_;
+  return base::CreateSequencedTaskRunnerWithTraits(traits);
+}
+
+scoped_refptr<base::SingleThreadTaskRunner>
+URLRequestContextBuilder::GetFileSingleThreadTaskRunner(
+    const base::TaskTraits& traits) {
+  if (file_task_runner_)
+    return file_task_runner_;
+  return base::CreateSingleThreadTaskRunnerWithTraits(traits);
+}
+
 }  // namespace net