Created new URLRequestContext for secure proxy check

components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc

Index: components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
index 29458b46555fe88e57e8ff928cc12b376e076192..24f15f47584a87cf659fc688a4d2d98688dad1a9 100644
--- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
+++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
@@ -15,8 +15,10 @@
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_params.h"
 #include "net/base/load_flags.h"
+#include "net/http/http_network_layer.h"
 #include "net/proxy/proxy_server.h"
 #include "net/url_request/url_fetcher.h"
+#include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "net/url_request/url_request_status.h"
 
@@ -48,6 +50,7 @@ void RecordNetworkChangeEvent(DataReductionProxyNetworkChangeEvent event) {
 namespace data_reduction_proxy {
 
 DataReductionProxyConfig::DataReductionProxyConfig(
+    net::URLRequestContextGetter* request_context_getter,
     scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
     scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner,
     net::NetLog* net_log,
@@ -55,7 +58,8 @@ DataReductionProxyConfig::DataReductionProxyConfig(
     DataReductionProxyConfigurator* configurator,
     DataReductionProxyEventStore* event_store,
     bool enable_quic)
-    : restricted_by_carrier_(false),
+    : url_request_context_getter_(request_context_getter),
+      restricted_by_carrier_(false),
       disabled_on_vpn_(false),
       unreachable_(false),
       enabled_by_user_(false),
@@ -191,7 +195,7 @@ void DataReductionProxyConfig::SetProxyConfigOnIOThread(
   // Check if the proxy has been restricted explicitly by the carrier.
   if (enabled &&
       !(alternative_enabled && !params_->alternative_fallback_allowed())) {
-    ui_task_runner_->PostTask(
+    io_task_runner_->PostTask(
         FROM_HERE, base::Bind(&DataReductionProxyConfig::StartSecureProxyCheck,
                               base::Unretained(this)));
   }
@@ -243,16 +247,7 @@ void DataReductionProxyConfig::LogProxyState(bool enabled,
 
 void DataReductionProxyConfig::HandleSecureProxyCheckResponse(

[bengr, 2015/03/24 15:39:12]

It would probably be better to create a new class called SecureProxyChecker and
have DRPConfig own it. It could have a method CheckIfSecureProxyIsAllowed, that
takes a callback. 

[tbansal1, 2015/04/01 19:58:58]

Done.

     const std::string& response, const net::URLRequestStatus& status) {
-  DCHECK(ui_task_runner_->BelongsToCurrentThread());
-  io_task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(
-          &DataReductionProxyConfig::HandleSecureProxyCheckResponseOnIOThread,
-          base::Unretained(this), response, status));
-}
-
-void DataReductionProxyConfig::HandleSecureProxyCheckResponseOnIOThread(
-    const std::string& response, const net::URLRequestStatus& status) {
+  DCHECK(io_task_runner_->BelongsToCurrentThread());
   if (event_store_) {
     event_store_->EndSecureProxyCheck(bound_net_log_, status.error());
   }
@@ -315,7 +310,7 @@ void DataReductionProxyConfig::OnIPAddressChanged() {
       return;
     }
 
-    ui_task_runner_->PostTask(
+    io_task_runner_->PostTask(
         FROM_HERE, base::Bind(&DataReductionProxyConfig::StartSecureProxyCheck,
                               base::Unretained(this)));
   }
@@ -369,7 +364,7 @@ void DataReductionProxyConfig::RecordSecureProxyCheckFetchResult(
 }
 
 void DataReductionProxyConfig::StartSecureProxyCheck() {
-  DCHECK(ui_task_runner_->BelongsToCurrentThread());
+  DCHECK(io_task_runner_->BelongsToCurrentThread());
   bound_net_log_ = net::BoundNetLog::Make(
       net_log_, net::NetLog::SOURCE_DATA_REDUCTION_PROXY);
   if (data_reduction_proxy_service_) {
@@ -378,7 +373,7 @@ void DataReductionProxyConfig::StartSecureProxyCheck() {
                                           params_->secure_proxy_check_url());
     }
 
-    data_reduction_proxy_service_->SecureProxyCheck(
+    SecureProxyCheck(
         params_->secure_proxy_check_url(),
         base::Bind(&DataReductionProxyConfig::HandleSecureProxyCheckResponse,
                    base::Unretained(this)));
@@ -418,4 +413,72 @@ bool DataReductionProxyConfig::DisableIfVPN() {
   return false;
 }
 
+void DataReductionProxyConfig::OnURLFetchComplete(
+    const net::URLFetcher* source) {
+  DCHECK(source == fetcher_.get());

[bengr, 2015/03/24 15:39:12]

DCHECK_EQ?

[tbansal1, 2015/04/01 19:58:59]

Done.

+  net::URLRequestStatus status = source->GetStatus();
+
+  std::string response;
+  source->GetResponseAsString(&response);
+
+  fetcher_callback_.Run(response, status);
+}
+
+net::URLFetcher* DataReductionProxyConfig::GetURLFetcherForSecureProxyCheck(
+    const GURL& secure_proxy_check_url) {
+  DCHECK(url_request_context_getter_);

[bengr, 2015/03/24 15:39:12]

Can we DCHECK this on construction instead?

[tbansal1, 2015/04/01 19:58:58]

Done.

+
+  url_request_context_.reset(new net::URLRequestContext());

[sclittle, 2015/03/20 19:28:46]

This could possibly destroy the URLRequestContext for a secure proxy check
that's in progress if the user toggles Data Saver on and off repeatedly really
fast. The URLRequestContext would also be destroyed prematurely if the DRPConfig
is destroyed before the URLFetcher gets destroyed.

I'd suggest that you create your own SecureProxyCheckURLRequestContextGetter
class in this file that takes ownership of the URLRequestContext and the
HttpNetworkLayer, and lazily initialize that URLRequestContextGetter, having
DRPConfig hold on to a scoped_refptr of it, and reuse it for later secure proxy
checks.

[tbansal1, 2015/04/01 19:58:58]

Done except that DRPConfig holds on the scoped_ptr of SecureProxyChecker
instead of scoped_refptr

+  url_request_context_->CopyFrom(
+      url_request_context_getter_->GetURLRequestContext());

[sclittle, 2015/03/20 19:28:46]

I think it's only safe to use the profile's URLRequestContextGetter
GetURLRequestContext() on the UI thread, see:

https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...

[tbansal1, 2015/04/01 19:58:58]

GetURLRequestContext() can only be used on IO thread.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/io_...

+
+  const net::HttpNetworkSession::Params* params_original =

[sclittle, 2015/03/20 19:28:46]

This variable is unnecessary. You could just have:

DCHECK(url_request_context->GetNetworkSessionParams());
net::HttpNetworkSession::Params params =
*url_request_context->GetNetworkSessionParams();

or something like that.

[tbansal1, 2015/04/01 19:58:58]

Done.

+      url_request_context_->GetNetworkSessionParams();
+  DCHECK(params_original);
+
+  net::HttpNetworkSession::Params params_modified = *params_original;
+  params_modified.enable_quic = false;
+  params_modified.next_protos = net::NextProtosWithSpdyAndQuic(false, false);
+
+  scoped_refptr<net::HttpNetworkSession> network_session(
+      new net::HttpNetworkSession(params_modified));
+
+  http_network_layer.reset(new net::HttpNetworkLayer(network_session.get()));

[sclittle, 2015/03/20 19:28:46]

You could get rid of the network_session variable and just have "new
net::HttpNetworkLayer(new net::HttpNetworkSession(params))"

[tbansal1, 2015/04/01 19:58:58]

Done.

+  url_request_context_->set_http_transaction_factory(
+      http_network_layer.get());
+
+  // URLRequestContextGetter to be used for the secure proxy check.
+  scoped_refptr<net::URLRequestContextGetter> getter_secure_proxy_check_;
+  getter_secure_proxy_check_ = new net::TrivialURLRequestContextGetter(

[sclittle, 2015/03/20 19:28:46]

You're constructing a new URLRequestContext and URLRequestContextGetter for
every secure proxy check; why not just lazily construct it once and re-use it?

[tbansal1, 2015/04/01 19:58:58]

Done.

+      url_request_context_.get(),
+      url_request_context_getter_->GetNetworkTaskRunner());
+
+  net::URLFetcher* fetcher = net::URLFetcher::Create(
+      secure_proxy_check_url, net::URLFetcher::GET, this);
+  fetcher->SetLoadFlags(net::LOAD_DISABLE_CACHE | net::LOAD_BYPASS_PROXY);
+  fetcher->SetRequestContext(getter_secure_proxy_check_.get());
+  // Configure max retries to be at most kMaxRetries times for 5xx errors.
+  static const int kMaxRetries = 5;
+  fetcher->SetMaxRetriesOn5xx(kMaxRetries);
+  fetcher->SetAutomaticallyRetryOnNetworkChanges(kMaxRetries);
+  // The secure proxy check should not be redirected. Since the secure proxy
+  // check will inevitably fail if it gets redirected somewhere else (e.g. by a
+  // captive portal), short circuit that by giving up on the secure proxy check
+  // if it gets redirected.
+  fetcher->SetStopOnRedirect(true);
+  return fetcher;
+}
+
+void DataReductionProxyConfig::SecureProxyCheck(
+    const GURL& secure_proxy_check_url,
+    FetcherResponseCallback fetcher_callback) {
+  net::URLFetcher* fetcher =
+      GetURLFetcherForSecureProxyCheck(secure_proxy_check_url);
+  if (!fetcher)
+    return;
+  fetcher_.reset(fetcher);
+  fetcher_callback_ = fetcher_callback;
+  fetcher_->Start();
+}
+
 }  // namespace data_reduction_proxy