Moved data reduction proxy initialization logic to ProfileImplIOData

android_webview/browser/net/aw_url_request_context_getter.cc

Index: android_webview/browser/net/aw_url_request_context_getter.cc
diff --git a/android_webview/browser/net/aw_url_request_context_getter.cc b/android_webview/browser/net/aw_url_request_context_getter.cc
index 94da7366edbc5b2078a6871a7ee754b18dd5d6c0..e2b8f3d7f10915ea105f83bd7d4fa198db1f925e 100644
--- a/android_webview/browser/net/aw_url_request_context_getter.cc
+++ b/android_webview/browser/net/aw_url_request_context_getter.cc
@@ -17,6 +17,7 @@
 #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"
@@ -170,11 +171,12 @@ 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));
 }
@@ -193,7 +195,8 @@ void AwURLRequestContextGetter::InitializeURLRequestContext() {
 #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);
@@ -222,13 +225,18 @@ void AwURLRequestContextGetter::InitializeURLRequestContext() {
 #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
 
@@ -261,10 +269,15 @@ void AwURLRequestContextGetter::SetHandlersAndInterceptors(
   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