Use a custom URLRequestContextGetter for performing the secure proxy check.

Use a custom URLRequestContextGetter for performing the secure proxy check.

BUG=478610
Committed: https://crrev.com/cdd5a3de49c48a2ddaab69ff23e8542dd8deabec
Cr-Commit-Position: refs/heads/master@{#326138}

[jeremyim, 2015-04-21 06:07:40]

jeremyim@chromium.org changed reviewers:
+ bengr@chromium.org, mmenke@chromium.org

[jeremyim, 2015-04-21 06:07:40]

PTAL.

mmenke => The correct usage of URLRequestContext/ContextBuilder/ContextGetter +
URLFetcher
bengr => General DRP-ness

[mmenke, 2015-04-21 14:36:59]

The Builder usage LGTM

https://codereview.chromium.org/1066513005/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc
(right):

https://codereview.chromium.org/1066513005/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:35:
class SimpleURLRequestContextGetter : public net::URLRequestContextGetter {
Suggestion:  Think this is now more an internal detail of SecureProxyChecker
than something this class should be worrying about.

Also, if you have all the logic in there, you can optionally throw away the
getter and the URLRequestContext when you're not running a probe, to save a bit
of memory and avoid keeping open connections.

https://codereview.chromium.org/1066513005/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:35:
class SimpleURLRequestContextGetter : public net::URLRequestContextGetter {
Optional:  You could use TrivialURLRequestContextGetter instead, but if you do,
you'd still need something to own the context, so this approach may be simpler.

Been thinking we should make an option to have the URLRequestContextBuilder
return a getter instead, for consumers like this, but that's another issue
entirely.

[tbansal1, 2015-04-21 16:14:05]

tbansal@chromium.org changed reviewers:
+ tbansal@chromium.org

[tbansal1, 2015-04-21 16:14:06]

Thanks for taking care of this!

https://codereview.chromium.org/1066513005/diff/20001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/1066513005/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:56:
: url_request_context_getter_(url_request_context_getter) {}
May be add a DCHECK

DCHECK(!url_request_context_getter_->GetURLRequestContext()->GetNetworkSessionParams()->enable_quic);

https://codereview.chromium.org/1066513005/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc
(right):

https://codereview.chromium.org/1066513005/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:63:
builder.SetSpdyAndQuicEnabled(false, false);
May be set the user agent. It feels weird to send request with empty user agent:

if (url_request_context_->http_user_agent_settings())
       builder.set_user_agent(
       url_request_context_->http_user_agent_settings()->GetUserAgent());

https://codereview.chromium.org/1066513005/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc
(right):

https://codereview.chromium.org/1066513005/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:112:
++proto_index;
Not sure if this is really testing the code path. http_params->enable_quic is
false by default but gets enabled by a finch trial.

I thought the only way to test is to enable it in tests and then check
enable_quic in secure proxy checker params

[tbansal1, 2015-04-21 16:15:43]

https://codereview.chromium.org/1066513005/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc
(right):

https://codereview.chromium.org/1066513005/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:63:
builder.SetSpdyAndQuicEnabled(false, false);
Also not copying the net_log means there won't be any net log entries
corresponding to this. Might make things hard to debug.

[jeremyim, 2015-04-21 18:15:22]

https://codereview.chromium.org/1066513005/diff/20001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/1066513005/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:56:
: url_request_context_getter_(url_request_context_getter) {}
On 2015/04/21 16:14:05, tbansal1 wrote:
> May be add a DCHECK
> 
>
DCHECK(!url_request_context_getter_->GetURLRequestContext()->GetNetworkSessionParams()->enable_quic);

I don't think that necessarily holds in unit tests (and we shouldn't care if
it's enforced at a higher level).

https://codereview.chromium.org/1066513005/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc
(right):

https://codereview.chromium.org/1066513005/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:35:
class SimpleURLRequestContextGetter : public net::URLRequestContextGetter {
On 2015/04/21 14:36:59, mmenke wrote:
> Optional:  You could use TrivialURLRequestContextGetter instead, but if you
do,
> you'd still need something to own the context, so this approach may be
simpler.
> 
> Been thinking we should make an option to have the URLRequestContextBuilder
> return a getter instead, for consumers like this, but that's another issue
> entirely.

I think it's best to have the URLRequestContextGetter own the URLRequestContext,
since the Getter is a refcounted object, so the request context could be deleted
before the getter is.

Also, in regards to this specific crash, if the IOData is being deleted and we
try to shutdown the URLFetcher, it seems like it's still possible to have the
RequestContext deleted (if not owned by the Getter) and have bad dereferencing.

https://codereview.chromium.org/1066513005/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:63:
builder.SetSpdyAndQuicEnabled(false, false);
On 2015/04/21 16:15:43, tbansal1 wrote:
> Also not copying the net_log means there won't be any net log entries
> corresponding to this. Might make things hard to debug.

This is something mmenke can address, but builder.set_net_log() can't be used
with the existing net_log because the current implementation of set_net_log
means the request context takes ownership of the net_log and we get a crash on
shutdown from a double delete.

https://codereview.chromium.org/1066513005/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:63:
builder.SetSpdyAndQuicEnabled(false, false);
On 2015/04/21 16:14:05, tbansal1 wrote:
> May be set the user agent. It feels weird to send request with empty user
agent:
> 
> if (url_request_context_->http_user_agent_settings())
>        builder.set_user_agent(
>        url_request_context_->http_user_agent_settings()->GetUserAgent());

Done.

https://codereview.chromium.org/1066513005/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc
(right):

https://codereview.chromium.org/1066513005/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:112:
++proto_index;
On 2015/04/21 16:14:05, tbansal1 wrote:
> Not sure if this is really testing the code path. http_params->enable_quic is
> false by default but gets enabled by a finch trial.
> 
> I thought the only way to test is to enable it in tests and then check
> enable_quic in secure proxy checker params 

Since we build our own URLRequestContext, the finch trial doesn't make a
difference (as long as the behaviour of
URLRequestContextBuilder::SetSpdyAndQuicEnabled doesn't change).

[tbansal1, 2015-04-21 18:21:42]

lgtm

[bengr, 2015-04-21 18:50:53]

https://codereview.chromium.org/1066513005/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc
(right):

https://codereview.chromium.org/1066513005/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:37:
class SimpleURLRequestContextGetter : public net::URLRequestContextGetter {
Naming. "Simple" doesn't say enough. How about "NPNDisabled" or "BasicHttp"?
Also, add a class comment.

https://codereview.chromium.org/1066513005/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:39:
SimpleURLRequestContextGetter(
What happens on webview or cronet? Can't the UA be changed dynamically?

https://codereview.chromium.org/1066513005/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:51:
scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
Rename as network_task_runner_, to be consistent with GetNetworkTaskRunner?

https://codereview.chromium.org/1066513005/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:67:
if (!url_request_context_.get()) {
No need for .get()

https://codereview.chromium.org/1066513005/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc
(right):

https://codereview.chromium.org/1066513005/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:113:
++proto_index;
nit: you could do EXPECT_EQ(proto, http_params->next_protos[proto_index++]); and
remove the curlies.

[jeremyim, 2015-04-21 19:00:50]

https://codereview.chromium.org/1066513005/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc
(right):

https://codereview.chromium.org/1066513005/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:37:
class SimpleURLRequestContextGetter : public net::URLRequestContextGetter {
On 2015/04/21 18:50:53, bengr wrote:
> Naming. "Simple" doesn't say enough. How about "NPNDisabled" or "BasicHttp"?
> Also, add a class comment.

Done.

https://codereview.chromium.org/1066513005/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:39:
SimpleURLRequestContextGetter(
On 2015/04/21 18:50:53, bengr wrote:
> What happens on webview or cronet? Can't the UA be changed dynamically?

I don't know if there's much value in changing the UA for the secure proxy
check...

https://codereview.chromium.org/1066513005/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:51:
scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
On 2015/04/21 18:50:53, bengr wrote:
> Rename as network_task_runner_, to be consistent with GetNetworkTaskRunner?

Done.

https://codereview.chromium.org/1066513005/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:67:
if (!url_request_context_.get()) {
On 2015/04/21 18:50:53, bengr wrote:
> No need for .get()

Done.

https://codereview.chromium.org/1066513005/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc
(right):

https://codereview.chromium.org/1066513005/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:113:
++proto_index;
On 2015/04/21 18:50:53, bengr wrote:
> nit: you could do EXPECT_EQ(proto, http_params->next_protos[proto_index++]);
and
> remove the curlies.

Done.

[jeremyim, 2015-04-21 19:33:52]

Patchset #5 (id:80001) has been deleted

[bengr, 2015-04-21 19:40:08]

lgtm

[jeremyim, 2015-04-21 19:41:21]

jeremyim@chromium.org changed reviewers:
+ sgurun@chromium.org

[jeremyim, 2015-04-21 19:41:21]

sgurun => aw_browser_context.cc

Thanks!

[sgurun-gerrit only, 2015-04-21 19:44:28]

On 2015/04/21 19:41:21, jeremyim wrote:
> sgurun => aw_browser_context.cc
> 
> Thanks!

aw lgtm

[jeremyim, 2015-04-21 19:45:40]

The CQ bit was checked by jeremyim@chromium.org

[jeremyim, 2015-04-21 19:45:40]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org,
tbansal@chromium.org
Link to the patchset: https://codereview.chromium.org/1066513005/#ps100001
(title: "Rename a field")

[commit-bot: I haz the power, 2015-04-21 19:46:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1066513005/100001

[commit-bot: I haz the power, 2015-04-21 22:01:02]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-04-21 22:02:01]

Patchset 5 (id:??) landed as
https://crrev.com/cdd5a3de49c48a2ddaab69ff23e8542dd8deabec
Cr-Commit-Position: refs/heads/master@{#326138}