IOThread: Move more stuff into system context's URLRequestContextStore.

IOThread: Move more stuff into system context's URLRequestContextStore.

BUG=715695
Review-Url: https://codereview.chromium.org/2872133005
Cr-Commit-Position: refs/heads/master@{#475671}
Committed: https://chromium.googlesource.com/chromium/src/+/15992ce510e5b514646e60c0f5475e1414ecd212

[mmenke, 2017-05-10 19:13:25]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 19:14:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 19:21:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 19:21:47]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mmenke, 2017-05-10 19:43:12]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 19:44:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2017-05-10 20:34:13]

Description was changed from

==========
IOThread More more stuff into system context's URLRequestContextStore.

BUG=715695
==========

to

==========
IOThread: More more stuff into system context's URLRequestContextStore.

BUG=715695
==========

[mmenke, 2017-05-10 20:36:09]

mmenke@chromium.org changed reviewers:
+ rdsmith@chromium.org

[mmenke, 2017-05-10 20:36:10]

Another one of these - expect one or two more before I switch over to the
builder, though detouring to working on the builder side of things for the
moment.

[commit-bot: I haz the power, 2017-05-10 22:01:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 22:01:43]

Dry run: This issue passed the CQ dry run.

[Randy Smith (Not in Mondays), 2017-05-11 18:05:42]

https://codereview.chromium.org/2872133005/diff/20001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (left):

https://codereview.chromium.org/2872133005/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:635: params_.ct_policy_enforcer =
globals_->ct_policy_enforcer.get();
I spent some time trying to convince myself that this was safe and failed; want
to help me out?  The problems I see are:

* params_ is used in CreateHttpNetworkSession() in
ProfileIOData::InitializeInternal().  I'll admit I didn't check that the
HttpNetworkSession references HttpNetworkSession::Params::ct_policy_enforcer,
but if it doesn't, that seems like it's surprising enough to be unwise to rely
on.

* params_ is reconfigured after having various things set in in by
ConfigureParamsFromFieldTrialAndCommandLine().  Even if that function doesn't
alter the ct_policy_enforcer, it seems like it could easily in the future, and
avoiding using params for transmitting the ct_policy_enforcer seems a mistake.

[mmenke, 2017-05-11 18:15:31]

https://codereview.chromium.org/2872133005/diff/20001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (left):

https://codereview.chromium.org/2872133005/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:635: params_.ct_policy_enforcer =
globals_->ct_policy_enforcer.get();
On 2017/05/11 18:05:42, Randy Smith (Not in Mondays) wrote:
> I spent some time trying to convince myself that this was safe and failed;
want
> to help me out?  The problems I see are:
> 
> * params_ is used in CreateHttpNetworkSession() in
> ProfileIOData::InitializeInternal().  I'll admit I didn't check that the
> HttpNetworkSession references HttpNetworkSession::Params::ct_policy_enforcer,
> but if it doesn't, that seems like it's surprising enough to be unwise to rely
> on.

The last line in this method calls ConstructSystemRequestContext() (Note that
the first CL in this change, still out for review, moves the
ConstructSystemRequestContext call there.  On trunk, this CL would not be safe,
in isolation), and nothing in the method calls into ProfileIOData, since this
isn't a profile-scoped class.

> * params_ is reconfigured after having various things set in in by
> ConfigureParamsFromFieldTrialAndCommandLine().  Even if that function doesn't
> alter the ct_policy_enforcer, it seems like it could easily in the future, and
> avoiding using params for transmitting the ct_policy_enforcer seems a mistake.

It doesn't make any sense for a command line flag to affect only the IOThread's
CtPolicyEnforcer.  So it would affect the CtPolicyEnforcer globally, which is
the one that's owned by the SystemURLRequestContext.  Also note that the
net::URLRequestContextBuilder::SetHttpNetworkSessionComponents call overrides
that member of HttpNetworkSession params, anyways, so it's effectively ignored.

Regardless, that method won't exist in the future - I'm going to need to unify
the logic to set params so I can send the individual values we need over to the
network service, so that's going away (And we can't pass an entire
CtPolicyEnforcer across processes), anyways.  I'll have to clean this up before
I can switch over to a builder, anyways, since I can't pass it a raw
HttpNetworkSession::Params, since it creates half of the things that are needed
to populate them.

[mmenke, 2017-05-11 20:12:57]

Description was changed from

==========
IOThread: More more stuff into system context's URLRequestContextStore.

BUG=715695
==========

to

==========
IOThread: Move more stuff into system context's URLRequestContextStore.

BUG=715695
==========

[Randy Smith (Not in Mondays), 2017-05-11 21:06:10]

LGTM.

https://codereview.chromium.org/2872133005/diff/20001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (left):

https://codereview.chromium.org/2872133005/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:635: params_.ct_policy_enforcer =
globals_->ct_policy_enforcer.get();
On 2017/05/11 18:15:31, mmenke wrote:
> On 2017/05/11 18:05:42, Randy Smith (Not in Mondays) wrote:
> > I spent some time trying to convince myself that this was safe and failed;
> want
> > to help me out?  The problems I see are:
> > 
> > * params_ is used in CreateHttpNetworkSession() in
> > ProfileIOData::InitializeInternal().  I'll admit I didn't check that the
> > HttpNetworkSession references
HttpNetworkSession::Params::ct_policy_enforcer,
> > but if it doesn't, that seems like it's surprising enough to be unwise to
rely
> > on.
> 
> The last line in this method calls ConstructSystemRequestContext() (Note that
> the first CL in this change, still out for review, moves the
> ConstructSystemRequestContext call there.  On trunk, this CL would not be
safe,
> in isolation), and nothing in the method calls into ProfileIOData, since this
> isn't a profile-scoped class.

The above doesn't seem relevant to me, since
ProfileIOData::CreateHttpNetworkSession reaches into
io_thread->NetworkSessionParams(), which is params_.  But your comment below
about URLRequestContextBuilder::SetupHttpNetworkSessionComponents() nuking the
ct_policy_enforcer resolves my concerns about potential behavior change here.  
I'd rather not have the code this confusing and suggestive that it is a behavior
change, but given that you're heading for nuking the relevant pieces in the near
future, I won't worry about it in the short term.

> 
> > * params_ is reconfigured after having various things set in in by
> > ConfigureParamsFromFieldTrialAndCommandLine().  Even if that function
doesn't
> > alter the ct_policy_enforcer, it seems like it could easily in the future,
and
> > avoiding using params for transmitting the ct_policy_enforcer seems a
mistake.
> 
> It doesn't make any sense for a command line flag to affect only the
IOThread's
> CtPolicyEnforcer.  So it would affect the CtPolicyEnforcer globally, which is
> the one that's owned by the SystemURLRequestContext.  Also note that the
> net::URLRequestContextBuilder::SetHttpNetworkSessionComponents call overrides
> that member of HttpNetworkSession params, anyways, so it's effectively
ignored.

Ah!  Ok, I was assuming it was an IN parameter, and yep, it doesn't look like it
is.  

> Regardless, that method won't exist in the future - I'm going to need to unify
> the logic to set params so I can send the individual values we need over to
the
> network service, so that's going away (And we can't pass an entire
> CtPolicyEnforcer across processes), anyways.  I'll have to clean this up
before
> I can switch over to a builder, anyways, since I can't pass it a raw
> HttpNetworkSession::Params, since it creates half of the things that are
needed
> to populate them.

[mmenke, 2017-05-11 21:12:08]

https://codereview.chromium.org/2872133005/diff/20001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (left):

https://codereview.chromium.org/2872133005/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:635: params_.ct_policy_enforcer =
globals_->ct_policy_enforcer.get();
On 2017/05/11 21:06:10, Randy Smith (Not in Mondays) wrote:
> On 2017/05/11 18:15:31, mmenke wrote:
> > On 2017/05/11 18:05:42, Randy Smith (Not in Mondays) wrote:
> > > I spent some time trying to convince myself that this was safe and failed;
> > want
> > > to help me out?  The problems I see are:
> > > 
> > > * params_ is used in CreateHttpNetworkSession() in
> > > ProfileIOData::InitializeInternal().  I'll admit I didn't check that the
> > > HttpNetworkSession references
> HttpNetworkSession::Params::ct_policy_enforcer,
> > > but if it doesn't, that seems like it's surprising enough to be unwise to
> rely
> > > on.
> > 
> > The last line in this method calls ConstructSystemRequestContext() (Note
that
> > the first CL in this change, still out for review, moves the
> > ConstructSystemRequestContext call there.  On trunk, this CL would not be
> safe,
> > in isolation), and nothing in the method calls into ProfileIOData, since
this
> > isn't a profile-scoped class.
> 
> The above doesn't seem relevant to me, since
> ProfileIOData::CreateHttpNetworkSession reaches into
> io_thread->NetworkSessionParams(), which is params_.  But your comment below
> about URLRequestContextBuilder::SetupHttpNetworkSessionComponents() nuking the
> ct_policy_enforcer resolves my concerns about potential behavior change here. 

> I'd rather not have the code this confusing and suggestive that it is a
behavior
> change, but given that you're heading for nuking the relevant pieces in the
near
> future, I won't worry about it in the short term.

We call IOThread::Init when we create the IO thread, when we start it.  So
ProfileIOData cannot do anything on the IOThread before IOThread::Init is
called, and IOThread::Init has always created globals, and now creates the
SystemURLRequestContext as well.

Anyhow, I agree that this is a mess.  Currently, URLRequestContextBuilders own
everything passed to them.  So when moving ProfileIOData over to using
URLRequestContextBuilder (After IOThread), it should become clearer what is
shared, and I'll expect in the network service for those to be globals.  I'm not
doing it here mostly because globals that are part of the system request context
and those that aren't are hopelessly intertwined, and I think it's simplest not
to worry about which is what (At least right now).

> 
> > 
> > > * params_ is reconfigured after having various things set in in by
> > > ConfigureParamsFromFieldTrialAndCommandLine().  Even if that function
> doesn't
> > > alter the ct_policy_enforcer, it seems like it could easily in the future,
> and
> > > avoiding using params for transmitting the ct_policy_enforcer seems a
> mistake.
> > 
> > It doesn't make any sense for a command line flag to affect only the
> IOThread's
> > CtPolicyEnforcer.  So it would affect the CtPolicyEnforcer globally, which
is
> > the one that's owned by the SystemURLRequestContext.  Also note that the
> > net::URLRequestContextBuilder::SetHttpNetworkSessionComponents call
overrides
> > that member of HttpNetworkSession params, anyways, so it's effectively
> ignored.
> 
> Ah!  Ok, I was assuming it was an IN parameter, and yep, it doesn't look like
it
> is.  
> 
> > Regardless, that method won't exist in the future - I'm going to need to
unify
> > the logic to set params so I can send the individual values we need over to
> the
> > network service, so that's going away (And we can't pass an entire
> > CtPolicyEnforcer across processes), anyways.  I'll have to clean this up
> before
> > I can switch over to a builder, anyways, since I can't pass it a raw
> > HttpNetworkSession::Params, since it creates half of the things that are
> needed
> > to populate them.

[mmenke, 2017-05-11 21:23:05]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 21:24:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 23:09:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-11 23:09:37]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[mmenke, 2017-05-15 18:27:32]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 18:28:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 18:44:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-15 18:44:30]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mmenke, 2017-05-15 18:49:10]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 18:50:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 20:38:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-15 20:38:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mmenke, 2017-05-15 20:58:37]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 20:59:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 22:52:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-15 22:52:31]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mmenke, 2017-05-17 18:00:03]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 18:01:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 22:22:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-17 22:22:48]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-05-23 17:30:29]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 17:30:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 20:45:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-23 20:45:04]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-05-30 17:18:41]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2017-05-30 17:18:42]

The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/2872133005/#ps220001
(title: "More upstream merge conflicts! Fun!")

[commit-bot: I haz the power, 2017-05-30 17:19:24]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-30 17:33:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-30 17:33:38]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[mmenke, 2017-05-30 17:52:09]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2017-05-30 17:52:10]

The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/2872133005/#ps260001
(title: "Fix merge")

[commit-bot: I haz the power, 2017-05-30 17:52:51]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-30 19:24:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-30 19:24:24]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[mmenke, 2017-05-30 20:24:38]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2017-05-30 20:25:20]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-30 21:05:05]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1496175878977410,
"parent_rev": "5554098dcecdd937d4bd96cc8017c38c9b0fecd9", "commit_rev":
"15992ce510e5b514646e60c0f5475e1414ecd212"}

[commit-bot: I haz the power, 2017-05-30 21:05:16]

Description was changed from

==========
IOThread: Move more stuff into system context's URLRequestContextStore.

BUG=715695
==========

to

==========
IOThread: Move more stuff into system context's URLRequestContextStore.

BUG=715695

Review-Url: https://codereview.chromium.org/2872133005
Cr-Commit-Position: refs/heads/master@{#475671}
Committed:
https://chromium.googlesource.com/chromium/src/+/15992ce510e5b514646e60c0f547...
==========

[commit-bot: I haz the power, 2017-05-30 21:05:20]

Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/15992ce510e5b514646e60c0f547...