Plumb SSLKEYLOGFILE from Cronet to net

Plumb SSLKEYLOGFILE from Cronet to net

This will be useful to debug HTTP/2 traffic.

BUG=598002
Committed: https://crrev.com/da7f770292b2122e8d20386bbe286d9578a302b1
Cr-Commit-Position: refs/heads/master@{#383504}

[xunjieli, 2016-03-25 20:19:35]

Description was changed from

==========
Plumb SSLKEYLOGFILE from Cronet to net

This will be useful to debug HTTP/2 traffic in wireshark.

BUG=598002
==========

to

==========
Plumb SSLKEYLOGFILE from Cronet to net

This will be useful to debug HTTP/2 traffic.

BUG=598002
==========

[xunjieli, 2016-03-25 20:20:44]

xunjieli@chromium.org changed reviewers:
+ davidben@chromium.org, mef@chromium.org

[xunjieli, 2016-03-25 20:20:44]

PTAL. Thanks

[davidben, 2016-03-25 20:30:45]

https://codereview.chromium.org/1833203002/diff/20001/components/cronet/url_r...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/1833203002/diff/20001/components/cronet/url_r...
components/cronet/url_request_context_config.cc:208: file_task_runner);
Hrm. When can this method get called? It's not safe to call this function after
any SSLClientSockets have been created. I assume Cronet lets you create multiple
URLRequestContexts and such.

(We really should get to finishing the SSLClientContext refactor. Then this
nuisance will be resolved. :-/)

[xunjieli, 2016-03-25 20:45:35]

https://codereview.chromium.org/1833203002/diff/20001/components/cronet/url_r...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/1833203002/diff/20001/components/cronet/url_r...
components/cronet/url_request_context_config.cc:208: file_task_runner);
On 2016/03/25 20:30:45, davidben wrote:
> Hrm. When can this method get called? It's not safe to call this function
after
> any SSLClientSockets have been created. I assume Cronet lets you create
multiple
> URLRequestContexts and such.
> 
> (We really should get to finishing the SSLClientContext refactor. Then this
> nuisance will be resolved. :-/)

Acknowledged. Talked to David offline. Assuming clients don't create multiple
CronetEngines, we will be fine. Will wait for refactoring is done to expose this
as a stable API. Added a comment.

[davidben, 2016-03-25 20:51:03]

lgtm

https://codereview.chromium.org/1833203002/diff/40001/components/cronet/url_r...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/1833203002/diff/40001/components/cronet/url_r...
components/cronet/url_request_context_config.cc:210: // resolved.
(Probably worth mirroring this into any relevant API docs. Though if this is
just going to be undocumented, there's probably nothing to do.)

[xunjieli, 2016-03-25 20:52:34]

https://codereview.chromium.org/1833203002/diff/40001/components/cronet/url_r...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/1833203002/diff/40001/components/cronet/url_r...
components/cronet/url_request_context_config.cc:210: // resolved.
On 2016/03/25 20:51:03, davidben wrote:
> (Probably worth mirroring this into any relevant API docs. Though if this is
> just going to be undocumented, there's probably nothing to do.)

Yep, we don't have any documentation for experimental APIs.

[xunjieli, 2016-03-25 20:53:07]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2016-03-25 20:53:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1833203002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1833203002/40001

[mef, 2016-03-25 20:53:47]

https://codereview.chromium.org/1833203002/diff/40001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/1833203002/diff/40001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:446:
context_builder.SetFileTaskRunner(GetFileThread()->task_runner());
this seems redundant now.

https://codereview.chromium.org/1833203002/diff/40001/components/cronet/url_r...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/1833203002/diff/40001/components/cronet/url_r...
components/cronet/url_request_context_config.cc:206: if
(!ssl_key_log_file.empty()) {
maybe set a static flag here, and don't do it again if it is already set?

[xunjieli, 2016-03-25 20:55:16]

The CQ bit was unchecked by xunjieli@chromium.org

[xunjieli, 2016-03-25 20:58:36]

https://codereview.chromium.org/1833203002/diff/40001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/1833203002/diff/40001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:446:
context_builder.SetFileTaskRunner(GetFileThread()->task_runner());
On 2016/03/25 20:53:47, mef wrote:
> this seems redundant now.

I think this is to replace the default one in ContainerURLRequestContext

https://codereview.chromium.org/1833203002/diff/40001/components/cronet/url_r...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/1833203002/diff/40001/components/cronet/url_r...
components/cronet/url_request_context_config.cc:206: if
(!ssl_key_log_file.empty()) {
On 2016/03/25 20:53:47, mef wrote:
> maybe set a static flag here, and don't do it again if it is already set?

Should we avoid static flags / globals ? That is the exact problem
crbug.com/458365 is solving.

[xunjieli, 2016-03-28 14:04:01]

Misha: ping?

[mef, 2016-03-28 14:37:57]

On 2016/03/28 14:04:01, xunjieli wrote:
> Misha: ping?

looking

[mef, 2016-03-28 14:37:59]

On 2016/03/28 14:04:01, xunjieli wrote:
> Misha: ping?

looking

[mef, 2016-03-28 15:14:25]

lgtm

https://codereview.chromium.org/1833203002/diff/40001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/1833203002/diff/40001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:446:
context_builder.SetFileTaskRunner(GetFileThread()->task_runner());
On 2016/03/25 20:58:36, xunjieli wrote:
> On 2016/03/25 20:53:47, mef wrote:
> > this seems redundant now.
> 
> I think this is to replace the default one in ContainerURLRequestContext

Acknowledged. I thought that it could make sense to move this into
ConfigureURLRequestContextBuilder, but now I don't see how it would simplify
anything, so I'm fine with this way for now.

https://codereview.chromium.org/1833203002/diff/40001/components/cronet/url_r...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/1833203002/diff/40001/components/cronet/url_r...
components/cronet/url_request_context_config.cc:206: if
(!ssl_key_log_file.empty()) {
On 2016/03/25 20:58:36, xunjieli wrote:
> On 2016/03/25 20:53:47, mef wrote:
> > maybe set a static flag here, and don't do it again if it is already set?
> 
> Should we avoid static flags / globals ? That is the exact problem
> crbug.com/458365 is solving. 

Acknowledged. Maybe add a check that |file_task_runner| is not null, so it
wouldn't crash if accidentally set from legacy api?

[xunjieli, 2016-03-28 15:51:16]

Thanks!

https://codereview.chromium.org/1833203002/diff/40001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/1833203002/diff/40001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:446:
context_builder.SetFileTaskRunner(GetFileThread()->task_runner());
On 2016/03/28 15:14:25, mef wrote:
> On 2016/03/25 20:58:36, xunjieli wrote:
> > On 2016/03/25 20:53:47, mef wrote:
> > > this seems redundant now.
> > 
> > I think this is to replace the default one in ContainerURLRequestContext
> 
> Acknowledged. I thought that it could make sense to move this into
> ConfigureURLRequestContextBuilder, but now I don't see how it would simplify
> anything, so I'm fine with this way for now.

Acknowledged. I agree. We should unify these two approaches before we expose
this more broadly.

https://codereview.chromium.org/1833203002/diff/40001/components/cronet/url_r...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/1833203002/diff/40001/components/cronet/url_r...
components/cronet/url_request_context_config.cc:206: if
(!ssl_key_log_file.empty()) {
On 2016/03/28 15:14:25, mef wrote:
> On 2016/03/25 20:58:36, xunjieli wrote:
> > On 2016/03/25 20:53:47, mef wrote:
> > > maybe set a static flag here, and don't do it again if it is already set?
> > 
> > Should we avoid static flags / globals ? That is the exact problem
> > crbug.com/458365 is solving. 
> 
> Acknowledged. Maybe add a check that |file_task_runner| is not null, so it
> wouldn't crash if accidentally set from legacy api?

Done.

[xunjieli, 2016-03-28 15:51:20]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2016-03-28 15:51:20]

The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org,
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1833203002/#ps60001
(title: "Address comment")

[commit-bot: I haz the power, 2016-03-28 15:51:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1833203002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1833203002/60001

[commit-bot: I haz the power, 2016-03-28 16:36:45]

Description was changed from

==========
Plumb SSLKEYLOGFILE from Cronet to net

This will be useful to debug HTTP/2 traffic.

BUG=598002
==========

to

==========
Plumb SSLKEYLOGFILE from Cronet to net

This will be useful to debug HTTP/2 traffic.

BUG=598002
==========

[commit-bot: I haz the power, 2016-03-28 16:36:46]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-28 16:38:16]

Description was changed from

==========
Plumb SSLKEYLOGFILE from Cronet to net

This will be useful to debug HTTP/2 traffic.

BUG=598002
==========

to

==========
Plumb SSLKEYLOGFILE from Cronet to net

This will be useful to debug HTTP/2 traffic.

BUG=598002

Committed: https://crrev.com/da7f770292b2122e8d20386bbe286d9578a302b1
Cr-Commit-Position: refs/heads/master@{#383504}
==========

[commit-bot: I haz the power, 2016-03-28 16:38:17]

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