Moved data reduction proxy initialization logic to ProfileImplIOData

android_webview/browser/net/aw_url_request_context_getter.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 "android_webview/browser/net/aw_url_request_context_getter.h"
 
 #include <vector>
 
 #include "android_webview/browser/aw_browser_context.h"
 #include "android_webview/browser/aw_content_browser_client.h"
 #include "android_webview/browser/aw_request_interceptor.h"
 #include "android_webview/browser/net/aw_network_delegate.h"
 #include "android_webview/browser/net/aw_url_request_job_factory.h"
 #include "android_webview/browser/net/init_native_callback.h"
 #include "android_webview/common/aw_content_client.h"
 #include "base/command_line.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/threading/worker_pool.h"
+#include "components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.h"
 #include "components/data_reduction_proxy/browser/data_reduction_proxy_config_service.h"
 #include "components/data_reduction_proxy/browser/data_reduction_proxy_settings.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/cookie_store_factory.h"
 #include "content/public/common/content_client.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/url_constants.h"
 #include "net/base/cache_type.h"
 #include "net/cookies/cookie_store.h"
@@ skipping 133 matching lines @@
 
   return job_factory.Pass();
 }
 
 }  // namespace
 
 AwURLRequestContextGetter::AwURLRequestContextGetter(
     const base::FilePath& partition_path, net::CookieStore* cookie_store)
     : partition_path_(partition_path),
       cookie_store_(cookie_store),
-      proxy_config_service_(new DataReductionProxyConfigService(
-          scoped_ptr<net::ProxyConfigService>(
-              net::ProxyService::CreateSystemProxyConfigService(
-                  GetNetworkTaskRunner(),
-                  NULL /* Ignored on Android */)).Pass())) {
+      data_reduction_proxy_config_service_(
+          new data_reduction_proxy::DataReductionProxyConfigService(
+              scoped_ptr<net::ProxyConfigService>(
+                  net::ProxyService::CreateSystemProxyConfigService(
+                      GetNetworkTaskRunner(),
+                      NULL /* Ignored on Android */)).Pass())) {
   // CreateSystemProxyConfigService for Android must be called on main thread.
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 }
 
 AwURLRequestContextGetter::~AwURLRequestContextGetter() {
 }
 
 void AwURLRequestContextGetter::InitializeURLRequestContext() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   DCHECK(!url_request_context_);
 
   net::URLRequestContextBuilder builder;
   builder.set_user_agent(GetUserAgent());
   AwNetworkDelegate* aw_network_delegate = new AwNetworkDelegate();
   builder.set_network_delegate(aw_network_delegate);
 #if !defined(DISABLE_FTP_SUPPORT)
   builder.set_ftp_enabled(false);  // Android WebView does not support ftp yet.
 #endif
-  builder.set_proxy_config_service(proxy_config_service_.release());
+  builder.set_proxy_config_service(
+      data_reduction_proxy_config_service_.release());

[willchan no longer on Chromium, 2014/07/31 22:31:34]

This should get fixed. AFAICT, this is what happens:

(a) AwURLRequestContextGetter gets created on the UI thread, which creates
DataReductionProxyConfigService, which lives on the IO thread.
(b) Right afterward, AwBrowserContext extracts the
DataReductionProxyConfigService* on the UI thread via
AwURLRequestContextGetter::GetDataReductionProxyConfigService(), and it passes
the object to DataReductionProxyConfigTracker, which lives on the UI thread.
(c) When AwURLRequestContextGetter initializes on the IO thread, it'll release
the DataReductionProxyConfigService member variable into the
URLRequestContextBuilder.

There are two problems:
(1) AwURLRequestContextGetter::GetDataReductionProxyConfigService() now returns
NULL. It's only ever safe to call at one particular point in the initialization.
This is very fragile, because there's no way for a reader to understand this at
first glance.
(2) DataReductionProxyConfigTracker is a UI thread object with a raw pointer
(DataReductionProxyConfigService*) to an IO thread object. And there's no thread
safety checking.

Here are my proposed fixes:
(i) Create the DataReductionProxyConfigService in
AwBrowserContext::PreMainMessageLoopRun() and pass it in via the constructor to
AwURLRequestContextGetter. This solves problem (1), since now you don't need the
getter just to expose it back to AwBrowserContext::PreMainMessageLoopRun().
(ii) DataReductionProxyConfigTracker shouldn't take a
DataReductionProxyConfigService*. Instead, it should take a base::Callback<bool,
const net::ProxyConfig&>. AwURLRequestContextGetter::PreMainMessageLoopRun()
should pass in base::Bind(&DataReductionProxyConfigService::UpdateProxyConfig,
base::Unretained(data_reduction_proxy_config_service)) to
DataReductionProxyConfigTracker. This reduces the scope, and loosens the
coupling between DataReductionProxyConfigTracker and
DataReductionProxyConfigService, and improves testability since you don't need a
full DataReductionProxyConfigService now for testing.
(iii) For extra safety, you could wrap the callback passed to
DataReductionProxyConfigService inside another callback that does
DCHECK(content::BrowserThread::GetMessageLoopProxyForThread(content::BrowserThread::IO)->BelongsToCurrentThread()));
This would make sure it always runs on the correct thread.

[sgurun-gerrit only, 2014/07/31 23:11:55]

agreed with Will.  (thanks for doing such a detailed review!)

On 2014/07/31 22:31:34, willchan wrote:

[bengr, 2014/08/02 01:10:31]

Thanks for going into so much detail. I didn't do iii. Where would I put the
definition of the callback that does the IO thread check? The config service is
in a component that doesn't depend on content.

On 2014/07/31 22:31:34, willchan wrote:

   builder.set_accept_language(net::HttpUtil::GenerateAcceptLanguageHeader(
       AwContentBrowserClient::GetAcceptLangsImpl()));
   ApplyCmdlineOverridesToURLRequestContextBuilder(&builder);
 
   url_request_context_.reset(builder.Build());
   channel_id_service_.reset(
       new net::ChannelIDService(
           new net::DefaultChannelIDStore(NULL),
           base::WorkerPool::GetTaskRunner(true)));
   url_request_context_->set_channel_id_service(channel_id_service_.get());
   // TODO(mnaganov): Fix URLRequestContextBuilder to use proper threads.
   net::HttpNetworkSession::Params network_session_params;
 
   PopulateNetworkSessionParams(url_request_context_.get(),
                                &network_session_params);
 
   net::HttpCache* main_cache = new net::HttpCache(
       network_session_params,
       new net::HttpCache::DefaultBackend(
           net::DISK_CACHE,
           net::CACHE_BACKEND_SIMPLE,
           partition_path_.Append(FILE_PATH_LITERAL("Cache")),
           20 * 1024 * 1024,  // 20M
           BrowserThread::GetMessageLoopProxyForThread(BrowserThread::CACHE)));
 
 #if defined(SPDY_PROXY_AUTH_ORIGIN)
   AwBrowserContext* browser_context = AwBrowserContext::GetDefault();
   DCHECK(browser_context);
-  DataReductionProxySettings* drp_settings =
+  DataReductionProxySettings* data_reduction_proxy_settings =
       browser_context->GetDataReductionProxySettings();
-  if (drp_settings) {
+  if (data_reduction_proxy_settings) {
+    data_reduction_proxy_auth_request_handler_.reset(
+        new data_reduction_proxy::DataReductionProxyAuthRequestHandler(
+            data_reduction_proxy_settings->params(),
+            BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO)));
+
     aw_network_delegate->set_data_reduction_proxy_params(
-        drp_settings->params());
+        data_reduction_proxy_settings->params());
     aw_network_delegate->set_data_reduction_proxy_auth_request_handler(
-        browser_context->GetDataReductionProxyAuthRequestHandler());
+        data_reduction_proxy_auth_request_handler_.get());
   }
 #endif
 
   main_http_factory_.reset(main_cache);
   url_request_context_->set_http_transaction_factory(main_cache);
   url_request_context_->set_cookie_store(cookie_store_);
 
   job_factory_ = CreateJobFactory(&protocol_handlers_,
                                   request_interceptors_.Pass());
   url_request_context_->set_job_factory(job_factory_.get());
@@ skipping 12 matching lines @@
   return BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO);
 }
 
 void AwURLRequestContextGetter::SetHandlersAndInterceptors(
     content::ProtocolHandlerMap* protocol_handlers,
     content::URLRequestInterceptorScopedVector request_interceptors) {
   std::swap(protocol_handlers_, *protocol_handlers);
   request_interceptors_.swap(request_interceptors);
 }
 
-DataReductionProxyConfigService*
-AwURLRequestContextGetter::proxy_config_service() {
+data_reduction_proxy::DataReductionProxyConfigService*
+AwURLRequestContextGetter::GetDataReductionProxyConfigService() const {
   // TODO(bengr): return system config if data reduction proxy is disabled.
-  return proxy_config_service_.get();
+  return data_reduction_proxy_config_service_.get();
+}
+
+data_reduction_proxy::DataReductionProxyAuthRequestHandler*
+AwURLRequestContextGetter::GetDataReductionProxyAuthRequestHandler() const {
+  return data_reduction_proxy_auth_request_handler_.get();
 }
 
 }  // namespace android_webview