Make UrlRequestContextBuilder take scoped_ptr's when it takes ownership

UrlRequestContextBuilder was already taking ownership in most cases, so
it should be taking scoped_ptr's instead of raw pointers.  This change
should help enforce proper ownership and has already identified two
ownership bugs.  I'm fixing the ownership bugs (a double-free of
ProxyConfigService in Cronet and of NetLog in AwURLRequestContextGetter)
in this change also.  I'm changing UrlRequestContextBuilder to not take
ownership of NetLog however as this conflicts with some other uses of
NetLog, like how it's exposed via ContentBrowserClient.

BUG=508553
TBR=jam

Review URL: https://codereview.chromium.org/1303493002

Cr-Commit-Position: refs/heads/master@{#346637}

android_webview/browser/net/aw_url_request_context_getter.cc

@@ -64,7 +64,7 @@ void ApplyCmdlineOverridesToURLRequestContextBuilder(
             net::HostResolver::CreateDefaultResolver(NULL)));
     host_resolver->SetRulesFromString(
         command_line.GetSwitchValueASCII(switches::kHostResolverRules));
-    builder->set_host_resolver(host_resolver.release());
+    builder->set_host_resolver(host_resolver.Pass());
   }
 }
 
@@ -205,9 +205,11 @@ void AwURLRequestContextGetter::InitializeURLRequestContext() {
   DCHECK(browser_context);
 
   builder.set_network_delegate(
-      browser_context->GetDataReductionProxyIOData()->CreateNetworkDelegate(
-          aw_network_delegate.Pass(),
-          false /* No UMA is produced to track bypasses. */ ).release());
+      browser_context->GetDataReductionProxyIOData()
+          ->CreateNetworkDelegate(
+              aw_network_delegate.Pass(),
+              false /* No UMA is produced to track bypasses. */)
+          .Pass());
 #if !defined(DISABLE_FTP_SUPPORT)
   builder.set_ftp_enabled(false);  // Android WebView does not support ftp yet.
 #endif
@@ -216,14 +218,13 @@ void AwURLRequestContextGetter::InitializeURLRequestContext() {
   // Create the proxy without a resolver since we rely on this local HTTP proxy.
   // TODO(sgurun) is this behavior guaranteed through SDK?
   builder.set_proxy_service(
-      net::ProxyService::CreateWithoutProxyResolver(
-          proxy_config_service_.release(),
-          net_log_.get()));
+      make_scoped_ptr(net::ProxyService::CreateWithoutProxyResolver(
+          proxy_config_service_.release(), net_log_.get())));
   builder.set_net_log(net_log_.get());
   builder.SetCookieAndChannelIdStores(cookie_store_, NULL);
   ApplyCmdlineOverridesToURLRequestContextBuilder(&builder);
 
-  url_request_context_.reset(builder.Build());
+  url_request_context_ = builder.Build().Pass();
   // TODO(mnaganov): Fix URLRequestContextBuilder to use proper threads.
   net::HttpNetworkSession::Params network_session_params;
 

chrome/service/net/service_url_request_context_getter.cc

@@ -108,9 +108,9 @@ ServiceURLRequestContextGetter::GetURLRequestContext() {
     net::URLRequestContextBuilder builder;
     builder.set_user_agent(user_agent_);
     builder.set_accept_language("en-us,fr");
-    builder.set_proxy_config_service(proxy_config_service_.release());
+    builder.set_proxy_config_service(proxy_config_service_.Pass());
     builder.set_throttling_enabled(true);
-    url_request_context_.reset(builder.Build());
+    url_request_context_ = builder.Build().Pass();
   }
   return url_request_context_.get();
 }

chrome/test/chromedriver/net/url_request_context_getter.cc

@@ -22,9 +22,9 @@ net::URLRequestContext* URLRequestContextGetter::GetURLRequestContext() {
     // net::HttpServer fails to parse headers if user-agent header is blank.
     builder.set_user_agent("chromedriver");
     builder.DisableHttpCache();
-    builder.set_proxy_config_service(
-        new net::ProxyConfigServiceFixed(net::ProxyConfig::CreateDirect()));
-    url_request_context_.reset(builder.Build());
+    builder.set_proxy_config_service(make_scoped_ptr(
+        new net::ProxyConfigServiceFixed(net::ProxyConfig::CreateDirect())));
+    url_request_context_ = builder.Build().Pass();
   }
   return url_request_context_.get();
 }

chromecast/net/connectivity_checker_impl.cc

@@ -57,10 +57,10 @@ void ConnectivityCheckerImpl::Initialize() {
       check_url_str.empty() ? kDefaultConnectivityCheckUrl : check_url_str));
 
   net::URLRequestContextBuilder builder;
-  builder.set_proxy_config_service(
-      new net::ProxyConfigServiceFixed(net::ProxyConfig::CreateDirect()));
+  builder.set_proxy_config_service(make_scoped_ptr(
+      new net::ProxyConfigServiceFixed(net::ProxyConfig::CreateDirect())));
   builder.DisableHttpCache();
-  url_request_context_.reset(builder.Build());
+  url_request_context_ = builder.Build().Pass();
 
   net::NetworkChangeNotifier::AddNetworkChangeObserver(this);
   task_runner_->PostTask(FROM_HERE,

cloud_print/gcp20/prototype/cloud_print_url_request_context_getter.cc

@@ -23,9 +23,9 @@ CloudPrintURLRequestContextGetter::GetURLRequestContext() {
     net::URLRequestContextBuilder builder;
 #if defined(OS_LINUX) || defined(OS_ANDROID)
     builder.set_proxy_config_service(
-        new net::ProxyConfigServiceFixed(net::ProxyConfig()));
+        make_scoped_ptr(new net::ProxyConfigServiceFixed(net::ProxyConfig())));
 #endif  // defined(OS_LINUX) || defined(OS_ANDROID)
-    context_.reset(builder.Build());
+    context_ = builder.Build().Pass();
   }
   return context_.get();
 }

components/cronet/android/cronet_url_request_context_adapter.cc

@@ -166,22 +166,18 @@ void CronetURLRequestContextAdapter::InitializeOnNetworkThread(
   // TODO(mmenke):  Add method to have the builder enable SPDY.
   net::URLRequestContextBuilder context_builder;
 
-  scoped_ptr<net::NetLog> net_log(new net::NetLog);
+  net_log_.reset(new net::NetLog);
   scoped_ptr<net::NetworkDelegate> network_delegate(new BasicNetworkDelegate());
 #if defined(DATA_REDUCTION_PROXY_SUPPORT)
   DCHECK(!data_reduction_proxy_);
   // For now, the choice to enable the data reduction proxy happens once,
   // at initialization. It cannot be disabled thereafter.
   if (!config->data_reduction_proxy_key.empty()) {
-    data_reduction_proxy_.reset(
-        new CronetDataReductionProxy(
-            config->data_reduction_proxy_key,
-            config->data_reduction_primary_proxy,
-            config->data_reduction_fallback_proxy,
-            config->data_reduction_secure_proxy_check_url,
-            config->user_agent,
-            GetNetworkTaskRunner(),
-            net_log.get()));
+    data_reduction_proxy_.reset(new CronetDataReductionProxy(
+        config->data_reduction_proxy_key, config->data_reduction_primary_proxy,
+        config->data_reduction_fallback_proxy,
+        config->data_reduction_secure_proxy_check_url, config->user_agent,
+        GetNetworkTaskRunner(), net_log_.get()));
     network_delegate =
         data_reduction_proxy_->CreateNetworkDelegate(network_delegate.Pass());
     ScopedVector<net::URLRequestInterceptor> interceptors;
@@ -189,9 +185,9 @@ void CronetURLRequestContextAdapter::InitializeOnNetworkThread(
     context_builder.SetInterceptors(interceptors.Pass());
   }
 #endif  // defined(DATA_REDUCTION_PROXY_SUPPORT)
-  context_builder.set_network_delegate(network_delegate.release());
-  context_builder.set_net_log(net_log.release());
-  context_builder.set_proxy_config_service(proxy_config_service_.release());
+  context_builder.set_network_delegate(network_delegate.Pass());
+  context_builder.set_net_log(net_log_.get());
+  context_builder.set_proxy_config_service(proxy_config_service_.Pass());
   config->ConfigureURLRequestContextBuilder(&context_builder);
 
   // Set up pref file if storage path is specified.
@@ -220,7 +216,7 @@ void CronetURLRequestContextAdapter::InitializeOnNetworkThread(
         http_server_properties_manager.Pass());
   }
 
-  context_.reset(context_builder.Build());
+  context_ = context_builder.Build().Pass();
 
   default_load_flags_ = net::LOAD_DO_NOT_SAVE_COOKIES |
                         net::LOAD_DO_NOT_SEND_COOKIES;

components/cronet/android/cronet_url_request_context_adapter.h

@@ -26,6 +26,7 @@ class SingleThreadTaskRunner;
 
 namespace net {
 class HttpServerPropertiesManager;
+class NetLog;
 class ProxyConfigService;
 class SdchOwner;
 class URLRequestContext;
@@ -111,6 +112,7 @@ class CronetURLRequestContextAdapter {
   // |pref_service_| should outlive the HttpServerPropertiesManager owned by
   // |context_|.
   scoped_ptr<PrefService> pref_service_;
+  scoped_ptr<net::NetLog> net_log_;
   scoped_ptr<net::URLRequestContext> context_;
   scoped_ptr<net::ProxyConfigService> proxy_config_service_;
   scoped_refptr<JsonPrefStore> json_pref_store_;

components/cronet/android/url_request_context_adapter.cc

@@ -139,11 +139,12 @@ void URLRequestContextAdapter::InitRequestContextOnNetworkThread() {
   DCHECK(config_);
   // TODO(mmenke):  Add method to have the builder enable SPDY.
   net::URLRequestContextBuilder context_builder;
-  context_builder.set_network_delegate(new BasicNetworkDelegate());
-  context_builder.set_proxy_config_service(proxy_config_service_.get());
+  context_builder.set_network_delegate(
+      make_scoped_ptr(new BasicNetworkDelegate()));
+  context_builder.set_proxy_config_service(proxy_config_service_.Pass());
   config_->ConfigureURLRequestContextBuilder(&context_builder);
 
-  context_.reset(context_builder.Build());
+  context_ = context_builder.Build().Pass();
 
   if (config_->enable_sdch) {
     DCHECK(context_->sdch_manager());

components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc

@@ -70,9 +70,10 @@ net::URLRequestContext*
 BasicHTTPURLRequestContextGetter::GetURLRequestContext() {
   if (!url_request_context_) {
     net::URLRequestContextBuilder builder;
-    builder.set_proxy_service(net::ProxyService::CreateDirect());
+    builder.set_proxy_service(
+        make_scoped_ptr(net::ProxyService::CreateDirect()));
     builder.SetSpdyAndQuicEnabled(false, false);
-    url_request_context_.reset(builder.Build());
+    url_request_context_ = builder.Build().Pass();
   }
 
   return url_request_context_.get();

device/test/usb_test_gadget_impl.cc

@@ -143,8 +143,13 @@ class URLRequestContextGetter : public net::URLRequestContextGetter {
 
   // net::URLRequestContextGetter implementation
   net::URLRequestContext* GetURLRequestContext() override {
-    context_builder_.set_proxy_service(net::ProxyService::CreateDirect());
-    return context_builder_.Build();
+    if (!context_) {
+      net::URLRequestContextBuilder context_builder;
+      context_builder.set_proxy_service(
+          make_scoped_ptr(net::ProxyService::CreateDirect()));
+      context_ = context_builder.Build().Pass();
+    }
+    return context_.get();
   }
 
   scoped_refptr<base::SingleThreadTaskRunner> GetNetworkTaskRunner()
@@ -152,7 +157,7 @@ class URLRequestContextGetter : public net::URLRequestContextGetter {
     return network_task_runner_;
   }
 
-  net::URLRequestContextBuilder context_builder_;
+  scoped_ptr<net::URLRequestContext> context_;
   scoped_refptr<base::SingleThreadTaskRunner> network_task_runner_;
 };
 

mojo/services/network/network_context.cc

@@ -168,13 +168,12 @@ scoped_ptr<net::URLRequestContext> NetworkContext::MakeURLRequestContext(
         new net::MappedHostResolver(host_resolver.Pass()));
     remapped_host_resolver->SetRulesFromString(
         command_line->GetSwitchValueASCII(kHostResolverRules));
-    host_resolver.reset(remapped_host_resolver.release());
-    builder.set_host_resolver(host_resolver.release());
+    builder.set_host_resolver(remapped_host_resolver.Pass());
   }
 
   builder.set_accept_language("en-us,en");
   builder.set_user_agent(mojo::common::GetUserAgent());
-  builder.set_proxy_service(net::ProxyService::CreateDirect());
+  builder.set_proxy_service(make_scoped_ptr(net::ProxyService::CreateDirect()));
   builder.set_transport_security_persister_path(base_path);
 
   net::URLRequestContextBuilder::HttpCacheParams cache_params;
@@ -210,7 +209,7 @@ scoped_ptr<net::URLRequestContext> NetworkContext::MakeURLRequestContext(
         new net::CookieMonster(cookie_store, nullptr), nullptr);
   }
 
-  return make_scoped_ptr(builder.Build());
+  return builder.Build().Pass();
 }
 
 }  // namespace mojo

net/tools/get_server_time/get_server_time.cc

@@ -144,7 +144,7 @@ BuildURLRequestContext(net::NetLog* net_log) {
   //
   // TODO(akalin): Remove this once http://crbug.com/146421 is fixed.
   builder.set_proxy_config_service(
-      new net::ProxyConfigServiceFixed(net::ProxyConfig()));
+      make_scoped_ptr(new net::ProxyConfigServiceFixed(net::ProxyConfig())));
 #endif
   scoped_ptr<net::URLRequestContext> context(builder.Build());
   context->set_net_log(net_log);

net/url_request/url_request_context_builder.cc

@@ -129,6 +129,7 @@ class ContainerURLRequestContext : public URLRequestContext {
   explicit ContainerURLRequestContext(
       const scoped_refptr<base::SingleThreadTaskRunner>& file_task_runner)
       : file_task_runner_(file_task_runner), storage_(this) {}
+  ~ContainerURLRequestContext() override { AssertNoURLRequests(); }
 
   URLRequestContextStorage* storage() {
     return &storage_;
@@ -151,9 +152,6 @@ class ContainerURLRequestContext : public URLRequestContext {
     transport_security_persister = transport_security_persister.Pass();
   }
 
- protected:
-  ~ContainerURLRequestContext() override { AssertNoURLRequests(); }
-
  private:
   // The thread should be torn down last.
   scoped_ptr<base::Thread> file_thread_;
@@ -205,7 +203,8 @@ URLRequestContextBuilder::URLRequestContextBuilder()
       http_cache_enabled_(true),
       throttling_enabled_(false),
       backoff_enabled_(false),
-      sdch_enabled_(false) {
+      sdch_enabled_(false),
+      net_log_(nullptr) {
 }
 
 URLRequestContextBuilder::~URLRequestContextBuilder() {}
@@ -266,9 +265,9 @@ void URLRequestContextBuilder::SetHttpServerProperties(
   http_server_properties_ = http_server_properties.Pass();
 }
 
-URLRequestContext* URLRequestContextBuilder::Build() {
-  ContainerURLRequestContext* context =
-      new ContainerURLRequestContext(file_task_runner_);
+scoped_ptr<URLRequestContext> URLRequestContextBuilder::Build() {
+  scoped_ptr<ContainerURLRequestContext> context(
+      new ContainerURLRequestContext(file_task_runner_));
   URLRequestContextStorage* storage = context->storage();
 
   storage->set_http_user_agent_settings(new StaticHttpUserAgentSettings(
@@ -280,7 +279,9 @@ URLRequestContext* URLRequestContextBuilder::Build() {
   storage->set_network_delegate(network_delegate);
 
   if (net_log_) {
-    storage->set_net_log(net_log_.release());
+    // Unlike the other builder parameters, |net_log_| is not owned by the
+    // builder or resulting context.
+    context->set_net_log(net_log_);
   } else {
     storage->set_net_log(new NetLog);
   }
@@ -365,7 +366,7 @@ URLRequestContext* URLRequestContextBuilder::Build() {
     storage->set_backoff_manager(new URLRequestBackoffManager());
 
   HttpNetworkSession::Params network_session_params;
-  SetHttpNetworkSessionComponents(context, &network_session_params);
+  SetHttpNetworkSessionComponents(context.get(), &network_session_params);
 
   network_session_params.ignore_certificate_errors =
       http_network_session_params_.ignore_certificate_errors;
@@ -446,7 +447,7 @@ URLRequestContext* URLRequestContextBuilder::Build() {
   storage->set_job_factory(top_job_factory.release());
   // TODO(willchan): Support sdch.
 
-  return context;
+  return context.Pass();
 }
 
 }  // namespace net

net/url_request/url_request_context_builder.h

@@ -100,11 +100,12 @@ class NET_EXPORT URLRequestContextBuilder {
 
   // These functions are mutually exclusive.  The ProxyConfigService, if
   // set, will be used to construct a ProxyService.
-  void set_proxy_config_service(ProxyConfigService* proxy_config_service) {
-    proxy_config_service_.reset(proxy_config_service);
+  void set_proxy_config_service(
+      scoped_ptr<ProxyConfigService> proxy_config_service) {
+    proxy_config_service_ = proxy_config_service.Pass();
   }
-  void set_proxy_service(ProxyService* proxy_service) {
-    proxy_service_.reset(proxy_service);
+  void set_proxy_service(scoped_ptr<ProxyService> proxy_service) {
+    proxy_service_ = proxy_service.Pass();
   }
 
   // Call these functions to specify hard-coded Accept-Language
@@ -136,22 +137,22 @@ class NET_EXPORT URLRequestContextBuilder {
   }
 #endif
 
+  // Unlike the other setters, the builder does not take ownership of the
+  // NetLog.
   // TODO(mmenke):  Probably makes sense to get rid of this, and have consumers
   // set their own NetLog::Observers instead.
-  void set_net_log(NetLog* net_log) {
-    net_log_.reset(net_log);
-  }
+  void set_net_log(NetLog* net_log) { net_log_ = net_log; }
 
   // By default host_resolver is constructed with CreateDefaultResolver.
-  void set_host_resolver(HostResolver* host_resolver) {
-    host_resolver_.reset(host_resolver);
+  void set_host_resolver(scoped_ptr<HostResolver> host_resolver) {
+    host_resolver_ = host_resolver.Pass();
   }
 
   // Uses BasicNetworkDelegate by default. Note that calling Build will unset
   // any custom delegate in builder, so this must be called each time before
   // Build is called.
-  void set_network_delegate(NetworkDelegate* delegate) {
-    network_delegate_.reset(delegate);
+  void set_network_delegate(scoped_ptr<NetworkDelegate> delegate) {
+    network_delegate_ = delegate.Pass();
   }
 
   // Adds additional auth handler factories to be used in addition to what is
@@ -231,7 +232,7 @@ class NET_EXPORT URLRequestContextBuilder {
   void SetHttpServerProperties(
       scoped_ptr<HttpServerProperties> http_server_properties);
 
-  URLRequestContext* Build();
+  scoped_ptr<URLRequestContext> Build();
 
  private:
   struct NET_EXPORT SchemeFactory {
@@ -263,7 +264,7 @@ class NET_EXPORT URLRequestContextBuilder {
   HttpCacheParams http_cache_params_;
   HttpNetworkSessionParams http_network_session_params_;
   base::FilePath transport_security_persister_path_;
-  scoped_ptr<NetLog> net_log_;
+  NetLog* net_log_;
   scoped_ptr<HostResolver> host_resolver_;
   scoped_ptr<ChannelIDService> channel_id_service_;
   scoped_ptr<ProxyConfigService> proxy_config_service_;

net/url_request/url_request_context_builder_unittest.cc

@@ -53,8 +53,8 @@ class URLRequestContextBuilderTest : public PlatformTest {
                      base::FilePath(
                          FILE_PATH_LITERAL("net/data/url_request_unittest"))) {
 #if defined(OS_LINUX) || defined(OS_ANDROID)
-    builder_.set_proxy_config_service(
-        new ProxyConfigServiceFixed(ProxyConfig::CreateDirect()));
+    builder_.set_proxy_config_service(make_scoped_ptr(
+        new ProxyConfigServiceFixed(ProxyConfig::CreateDirect())));
 #endif  // defined(OS_LINUX) || defined(OS_ANDROID)
   }
 

remoting/base/url_request_context_getter.cc

@@ -25,10 +25,11 @@ net::URLRequestContext* URLRequestContextGetter::GetURLRequestContext() {
   if (!url_request_context_.get()) {
     net::URLRequestContextBuilder builder;
     builder.SetFileTaskRunner(file_task_runner_);
-    builder.set_net_log(new VlogNetLog());
+    net_log_.reset(new VlogNetLog());
+    builder.set_net_log(net_log_.get());
     builder.DisableHttpCache();
-    builder.set_proxy_config_service(proxy_config_service_.release());
-    url_request_context_.reset(builder.Build());
+    builder.set_proxy_config_service(proxy_config_service_.Pass());
+    url_request_context_ = builder.Build().Pass();
   }
   return url_request_context_.get();
 }

remoting/base/url_request_context_getter.h

@@ -14,6 +14,7 @@ class SingleThreadTaskRunner;
 }  // namespace base
 
 namespace net {
+class NetLog;
 class ProxyConfigService;
 }  // namespace net
 
@@ -37,6 +38,7 @@ class URLRequestContextGetter : public net::URLRequestContextGetter {
   scoped_refptr<base::SingleThreadTaskRunner> network_task_runner_;
   scoped_refptr<base::SingleThreadTaskRunner> file_task_runner_;
   scoped_ptr<net::ProxyConfigService> proxy_config_service_;
+  scoped_ptr<net::NetLog> net_log_;
   scoped_ptr<net::URLRequestContext> url_request_context_;
 
   DISALLOW_COPY_AND_ASSIGN(URLRequestContextGetter);