Don't delay creation of system URLRequestContext until first use

Don't delay creation of system URLRequestContext until first use.

This also requires updating ProxyConfigServiceImpl to pass its status synchronously
to the ProxyConfig it creates, since there's not yet an IO Thread to post a task to
during IOThread creation.

BUG=715695
Review-Url: https://codereview.chromium.org/2860033003
Cr-Commit-Position: refs/heads/master@{#471791}
Committed: https://chromium.googlesource.com/chromium/src/+/02505dac8a35d644e38d27b01e48645ce39599bc

[mmenke, 2017-05-08 18:47:24]

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

[commit-bot: I haz the power, 2017-05-08 18:48:45]

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-08 18:50:10]

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

[commit-bot: I haz the power, 2017-05-08 18:50:37]

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-08 18:56:01]

Patchset #3 (id:40001) has been deleted

[mmenke, 2017-05-08 18:59:38]

Description was changed from

==========
Don't delay creation of system URLRequestContext

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

to

==========
Don't delay creation of system URLRequestContext until first use.

This also requires updating ProxyConfigServiceImpl to handle getting a
OnProxyConfigChanged call from the delegate proxy config service before it has
received its initiate settings.

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

[commit-bot: I haz the power, 2017-05-08 20:06:11]

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

[commit-bot: I haz the power, 2017-05-08 20:06:12]

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_...)
  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-08 21:33:12]

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

[commit-bot: I haz the power, 2017-05-08 21:33:51]

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-08 21:52:26]

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

[commit-bot: I haz the power, 2017-05-08 21:53:38]

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-08 22:52:55]

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

[commit-bot: I haz the power, 2017-05-08 22:52:56]

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...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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-09 03:06:23]

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

[commit-bot: I haz the power, 2017-05-09 03:08:14]

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-09 04:31:01]

Description was changed from

==========
Don't delay creation of system URLRequestContext until first use.

This also requires updating ProxyConfigServiceImpl to handle getting a
OnProxyConfigChanged call from the delegate proxy config service before it has
received its initiate settings.

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

to

==========
Don't delay creation of system URLRequestContext until first use.

This also requires updating ProxyConfigServiceImpl to pass its status
synchronously
to the ProxyConfig it creates, since there's not yet an IO Thread to post a task
to
during IOThread creation.

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

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

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

[commit-bot: I haz the power, 2017-05-09 05:06:22]

Dry run: Try jobs failed on following builders:
  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-09 15:25:34]

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

[commit-bot: I haz the power, 2017-05-09 15:26:07]

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-09 16:01:44]

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

[commit-bot: I haz the power, 2017-05-09 16:01:45]

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

[mmenke, 2017-05-09 17:17:42]

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

[commit-bot: I haz the power, 2017-05-09 17:18:45]

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-09 17:53:52]

mmenke@chromium.org changed reviewers:
+ eroman@chromium.org, stevenjb@chromium.org

[mmenke, 2017-05-09 17:53:53]

[eroman]:  Please review chrome/browser/io_thread and testing_io_thread_state. 
Sending this to you because, the main complexity is around creating the proxy
config service earlier (Before the IOThread is even running, actually, though
can still get its TaskRunner, just not post tasks to it yet).

[stevenjb]:  Please review components/proxy_config/

https://codereview.chromium.org/2860033003/diff/140001/chrome/test/base/testi...
File chrome/test/base/testing_io_thread_state.cc (right):

https://codereview.chromium.org/2860033003/diff/140001/chrome/test/base/testi...
chrome/test/base/testing_io_thread_state.cc:99: io_thread_state_->CleanUp();
This change is needed because now  system_proxy_config_service_ is initialized
in IOThread::Init, it's not destroyed when destroying globals, and on OSX the
proxy service DCHECKs that it's destroyed on the IOTHread, which normally
happens in IOThread::Cleanup.

[mmenke, 2017-05-09 17:54:31]

The CQ bit was unchecked by mmenke@chromium.org

[mmenke, 2017-05-10 16:04:08]

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

[commit-bot: I haz the power, 2017-05-10 16:04:56]

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 16:51:20]

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

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

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_...)

[stevenjb, 2017-05-10 18:46:17]

components/proxy_config lgtm w/ nits

https://codereview.chromium.org/2860033003/diff/160001/components/proxy_confi...
File components/proxy_config/pref_proxy_config_tracker_impl.h (right):

https://codereview.chromium.org/2860033003/diff/160001/components/proxy_confi...
components/proxy_config/pref_proxy_config_tracker_impl.h:42: const
net::ProxyConfig& initial_config);
No longer explicit
s/initial_config/initial_pref_config/

https://codereview.chromium.org/2860033003/diff/160001/components/proxy_confi...
File components/proxy_config/pref_proxy_config_tracker_impl_unittest.cc (right):

https://codereview.chromium.org/2860033003/diff/160001/components/proxy_confi...
components/proxy_config/pref_proxy_config_tracker_impl_unittest.cc:79: //
Initializes the proxy config service.  The delegate config service has the
single ' ' between sentences.

[mmenke, 2017-05-10 18:59:20]

Thanks for the feedback!  (And if you're wondering about the test...In an
earlier CL, I had run into issues with the case where it got a config from the
delegate service before the proxy service was notified of settings.  Eventually
changed code so that didn't happen, anyways, and made most of my new tests moot,
but thought I'd keep the one that made sense)

https://codereview.chromium.org/2860033003/diff/160001/components/proxy_confi...
File components/proxy_config/pref_proxy_config_tracker_impl.h (right):

https://codereview.chromium.org/2860033003/diff/160001/components/proxy_confi...
components/proxy_config/pref_proxy_config_tracker_impl.h:42: const
net::ProxyConfig& initial_config);
On 2017/05/10 18:46:16, stevenjb wrote:
> No longer explicit
> s/initial_config/initial_pref_config/

Done.

https://codereview.chromium.org/2860033003/diff/160001/components/proxy_confi...
File components/proxy_config/pref_proxy_config_tracker_impl_unittest.cc (right):

https://codereview.chromium.org/2860033003/diff/160001/components/proxy_confi...
components/proxy_config/pref_proxy_config_tracker_impl_unittest.cc:79: //
Initializes the proxy config service.  The delegate config service has the
On 2017/05/10 18:46:16, stevenjb wrote:
> single ' ' between sentences.

Done.

[mmenke, 2017-05-11 20:26:39]

eroman:  Ping!  If you're busy, I can look for someone else, though going to be
sending another CL your way shortly.

[eroman, 2017-05-12 01:17:42]

Sorry, I actually was out past 2 days but failed to updated my status.

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

On 2017/05/12 01:17:42, eroman wrote:
> Sorry, I actually was out past 2 days but failed to updated my status.

Not a problem at all.

[mmenke, 2017-05-12 20:19:40]

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

[commit-bot: I haz the power, 2017-05-12 20:20:26]

Dry run: CQ is trying da patch.

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

[eroman, 2017-05-12 21:12:59]

lgtm

https://codereview.chromium.org/2860033003/diff/220001/components/proxy_confi...
File components/proxy_config/pref_proxy_config_tracker_impl.cc (right):

https://codereview.chromium.org/2860033003/diff/220001/components/proxy_confi...
components/proxy_config/pref_proxy_config_tracker_impl.cc:134:
update_pending_(true),
Can you delete this variable?

Seems like the only places that cares about this is
PrefProxyConfigTrackerImpl::OnProxyConfigChanged() and it can check the nullity
of |proxy_config_service_impl_|.

https://codereview.chromium.org/2860033003/diff/220001/components/proxy_confi...
components/proxy_config/pref_proxy_config_tracker_impl.cc:148:
PrefProxyConfigTrackerImpl::CreateTrackingProxyConfigService(
The interface for CreateTrackingProxyConfigService() doesn't document whether
this can be called multiple times. Presumably it can't.

Care to add a DCHECK() that proxy_config_service_impl_ is null here, and add a
comment to the interface?

https://codereview.chromium.org/2860033003/diff/220001/components/proxy_confi...
File components/proxy_config/pref_proxy_config_tracker_impl.h (right):

https://codereview.chromium.org/2860033003/diff/220001/components/proxy_confi...
components/proxy_config/pref_proxy_config_tracker_impl.h:40:
ProxyConfigServiceImpl(net::ProxyConfigService* base_service,
If you are changing the signature anyway, want to make this a unique_ptr?

[mmenke, 2017-05-12 21:29:43]

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

[mmenke, 2017-05-12 21:29:58]

https://codereview.chromium.org/2860033003/diff/220001/components/proxy_confi...
File components/proxy_config/pref_proxy_config_tracker_impl.cc (right):

https://codereview.chromium.org/2860033003/diff/220001/components/proxy_confi...
components/proxy_config/pref_proxy_config_tracker_impl.cc:134:
update_pending_(true),
On 2017/05/12 21:12:59, eroman wrote:
> Can you delete this variable?
> 
> Seems like the only places that cares about this is
> PrefProxyConfigTrackerImpl::OnProxyConfigChanged() and it can check the
nullity
> of |proxy_config_service_impl_|.

This is used in a subclass, chromeos::ProxyConfigServiceImpl, in a manner there
that implies its configuration updates can also fail.  I'm sufficiently nervous
about its state machinery and use in that class that I think fixing it is beyond
the scope of this CL.

I'll try and remove it in a followup CL.

https://codereview.chromium.org/2860033003/diff/220001/components/proxy_confi...
components/proxy_config/pref_proxy_config_tracker_impl.cc:148:
PrefProxyConfigTrackerImpl::CreateTrackingProxyConfigService(
On 2017/05/12 21:12:59, eroman wrote:
> The interface for CreateTrackingProxyConfigService() doesn't document whether
> this can be called multiple times. Presumably it can't.
> 
> Care to add a DCHECK() that proxy_config_service_impl_ is null here, and add a
> comment to the interface?

Done (Docs added to PrefProxyConfigTracker)

https://codereview.chromium.org/2860033003/diff/220001/components/proxy_confi...
File components/proxy_config/pref_proxy_config_tracker_impl.h (right):

https://codereview.chromium.org/2860033003/diff/220001/components/proxy_confi...
components/proxy_config/pref_proxy_config_tracker_impl.h:40:
ProxyConfigServiceImpl(net::ProxyConfigService* base_service,
On 2017/05/12 21:12:59, eroman wrote:
> If you are changing the signature anyway, want to make this a unique_ptr?

SGTM, done

[commit-bot: I haz the power, 2017-05-12 21:31:01]

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-12 21:44:05]

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

[commit-bot: I haz the power, 2017-05-12 21:44:06]

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

[mmenke, 2017-05-12 21:48:43]

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

[commit-bot: I haz the power, 2017-05-12 21:49:51]

Dry run: CQ is trying da patch.

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

[eroman, 2017-05-12 21:54:55]

lgtm

https://codereview.chromium.org/2860033003/diff/220001/components/proxy_confi...
File components/proxy_config/pref_proxy_config_tracker_impl.cc (right):

https://codereview.chromium.org/2860033003/diff/220001/components/proxy_confi...
components/proxy_config/pref_proxy_config_tracker_impl.cc:134:
update_pending_(true),
On 2017/05/12 21:29:57, mmenke wrote:
> On 2017/05/12 21:12:59, eroman wrote:
> > Can you delete this variable?
> > 
> > Seems like the only places that cares about this is
> > PrefProxyConfigTrackerImpl::OnProxyConfigChanged() and it can check the
> nullity
> > of |proxy_config_service_impl_|.
> 
> This is used in a subclass, chromeos::ProxyConfigServiceImpl, in a manner
there
> that implies its configuration updates can also fail.  I'm sufficiently
nervous
> about its state machinery and use in that class that I think fixing it is
beyond
> the scope of this CL.
> 
> I'll try and remove it in a followup CL.

No need to follow-up if it isn't simple.

Just figured if there was an opportunity to add small cleanups while the code
was paged into working memory, would be good.

https://codereview.chromium.org/2860033003/diff/260001/components/proxy_confi...
File components/proxy_config/pref_proxy_config_tracker_impl.h (right):

https://codereview.chromium.org/2860033003/diff/260001/components/proxy_confi...
components/proxy_config/pref_proxy_config_tracker_impl.h:39: // Takes ownership
of the passed |base_service|.
You can delete this comment now.

[mmenke, 2017-05-12 21:57:58]

Thanks!

https://codereview.chromium.org/2860033003/diff/220001/components/proxy_confi...
File components/proxy_config/pref_proxy_config_tracker_impl.cc (right):

https://codereview.chromium.org/2860033003/diff/220001/components/proxy_confi...
components/proxy_config/pref_proxy_config_tracker_impl.cc:134:
update_pending_(true),
On 2017/05/12 21:54:55, eroman wrote:
> On 2017/05/12 21:29:57, mmenke wrote:
> > On 2017/05/12 21:12:59, eroman wrote:
> > > Can you delete this variable?
> > > 
> > > Seems like the only places that cares about this is
> > > PrefProxyConfigTrackerImpl::OnProxyConfigChanged() and it can check the
> > nullity
> > > of |proxy_config_service_impl_|.
> > 
> > This is used in a subclass, chromeos::ProxyConfigServiceImpl, in a manner
> there
> > that implies its configuration updates can also fail.  I'm sufficiently
> nervous
> > about its state machinery and use in that class that I think fixing it is
> beyond
> > the scope of this CL.
> > 
> > I'll try and remove it in a followup CL.
> 
> No need to follow-up if it isn't simple.
> 
> Just figured if there was an opportunity to add small cleanups while the code
> was paged into working memory, would be good.

I think it's probably pretty simple, just didn't want to delve into yet another
weird proxy class.  I'll take a look, and if it seems at all complicated, I'll
skip it.

https://codereview.chromium.org/2860033003/diff/260001/components/proxy_confi...
File components/proxy_config/pref_proxy_config_tracker_impl.h (right):

https://codereview.chromium.org/2860033003/diff/260001/components/proxy_confi...
components/proxy_config/pref_proxy_config_tracker_impl.h:39: // Takes ownership
of the passed |base_service|.
On 2017/05/12 21:54:55, eroman wrote:
> You can delete this comment now.

Thanks, will do!

[commit-bot: I haz the power, 2017-05-13 01:23:17]

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

[commit-bot: I haz the power, 2017-05-13 01:23:18]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-05-13 02:45:32]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2017-05-13 02:45:32]

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

[commit-bot: I haz the power, 2017-05-13 02:45:48]

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-13 02:53:21]

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

[commit-bot: I haz the power, 2017-05-13 02:53:22]

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

[mmenke, 2017-05-13 03:54:57]

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

[mmenke, 2017-05-13 03:54:58]

[+sky] For chrome/test/base/testing_io_thread_state.cc

[sky, 2017-05-14 16:27:27]

LGTM

[mmenke, 2017-05-15 15:03:59]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2017-05-15 15:04:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, eroman@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2860033003/#ps280001
(title: "Remove comment")

[commit-bot: I haz the power, 2017-05-15 15:04:37]

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 16:37:37]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1494860639618980,
"parent_rev": "eb2f886f0b9491f8e01ee197cd82a909c11102a6", "commit_rev":
"02505dac8a35d644e38d27b01e48645ce39599bc"}

[commit-bot: I haz the power, 2017-05-15 16:37:45]

Description was changed from

==========
Don't delay creation of system URLRequestContext until first use.

This also requires updating ProxyConfigServiceImpl to pass its status
synchronously
to the ProxyConfig it creates, since there's not yet an IO Thread to post a task
to
during IOThread creation.

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

to

==========
Don't delay creation of system URLRequestContext until first use.

This also requires updating ProxyConfigServiceImpl to pass its status
synchronously
to the ProxyConfig it creates, since there's not yet an IO Thread to post a task
to
during IOThread creation.

BUG=715695

Review-Url: https://codereview.chromium.org/2860033003
Cr-Commit-Position: refs/heads/master@{#471791}
Committed:
https://chromium.googlesource.com/chromium/src/+/02505dac8a35d644e38d27b01e48...
==========

[commit-bot: I haz the power, 2017-05-15 16:37:46]

Committed patchset #14 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/02505dac8a35d644e38d27b01e48...