Allow ProxyService to share URLRequestContext with everything else.

Allow ProxyService to share URLRequestContext with everything else.

This CL makes ProxyScriptFetcherImpl use highest priority and
LOAD_IGNORE_LIMITS, and adds an OnShutdown method to ProxyService,
which must be called before teardown when the ProxyService makes
requests using a URLRequestContext that it is also used with.

No consumer currently uses the new functionality.  I plan to
switch IOThread/ProfileIOData to using it in another CL, and
add a call in URLRequestContextStorage's destructor.  May also
be worth making it mandatory as well, though that will involve
more changes.

BUG=715697
Review-Url: https://codereview.chromium.org/2845643003
Cr-Commit-Position: refs/heads/master@{#469005}
Committed: https://chromium.googlesource.com/chromium/src/+/ed8d7e4383d5dbf7e94291b7960b5a4616b4a45e

[mmenke, 2017-04-26 19:03:24]

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

[commit-bot: I haz the power, 2017-04-26 19:04:43]

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-04-26 19:38:12]

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

[commit-bot: I haz the power, 2017-04-26 19:38:12]

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

[mmenke, 2017-04-26 19:42:59]

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

[commit-bot: I haz the power, 2017-04-26 19:44:08]

Dry run: CQ is trying da patch.

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

[mmenke, 2017-04-26 20:56:59]

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

[mmenke, 2017-04-26 20:56:59]

[eroman]:  Is this a reasonable approach?  Adding a new state mostly just
magically works, since the code is peppered with state checks that do nothing in
unexpected states (Since they expect the state to advance automatically, when
the the PAC logic completes).  Deleting the config_service_ did require a couple
new checks, though.

I didn't read through the entire 1600 line class before making this change, just
looked through state changes, state checks, and entry points, so could well be
missing something.

[commit-bot: I haz the power, 2017-04-26 21:23:25]

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

[commit-bot: I haz the power, 2017-04-26 21:23:26]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-04-27 18:37:32]

https://codereview.chromium.org/2845643003/diff/20001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/2845643003/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1454: std::unique_ptr<DhcpProxyScriptFetcher>
dhcp_proxy_script_fetcher) {
Another option would be to just call this method with null fetchers, and make it
safe to do that while it's working (move old fetchers to temporary locations
before calling ApplyProxyConfigIfAvailable), but I'm worried that would add more
weird behavior prior to shutdown.

[eroman, 2017-04-27 19:35:42]

https://codereview.chromium.org/2845643003/diff/20001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/2845643003/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1466: // Cancel up in progress work, and destroy all
objects that depend on the two
ProxyService is already a huge mess. I would rather not add extra stuff into it,
as it is already too complicated.

How about moving the notion of shutdown into to the leaf dependency
(ProxyScriptFetcher) ?

In particular, ProxyScriptFetcherImpl is the class which holds a pointer to the
URLRequestContext [1].

So we could solve this more directly by adding a
ProxyScriptFetcherImpl::Shutdown() method that:

  * Aborts any outstanding URLRequest
  * Nulls its URLRequestContext pointer
  * Marks itself as shutdown (such that any new requests will fail immediately
with ERR_ABORTED).

This means that ProxyService continues working as before, only its
ProxyScriptFetcher behaves as if it has no network connectivity.

As far as how to plumb that through I leave it up to you. As a strawman proposal
could do:

  ProxyService::DetachURLRequestContext(URLRequest* context)
      calls ProxyScriptFetcher::DetachURLRequestContext(context)

ProxyScriptFetcherImpl::DetachURLRequestContext(context) is the only
implementation that needs to do anything - it would enter shutdown mode per
previous description.

[1] OK so so I failed to describe ProxyScriptDecider, which unfortunately grew a
dependency on the URLRequestContext for the quick detect stuff. So the final
solution won't be as clean as I made it out to be, but should be able to adapt
it.

WDYT?

https://codereview.chromium.org/2845643003/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1470: // Prevent proxy service changes from waking up
the ProxyService.
config service

https://codereview.chromium.org/2845643003/diff/20001/net/proxy/proxy_service.h
File net/proxy/proxy_service.h (right):

https://codereview.chromium.org/2845643003/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.h:228: // in-progress resolutions to just hang. This is
needed to breat the circular
breat -> break

https://codereview.chromium.org/2845643003/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.h:229: // dependency when the ProxyService a
URLRequestContext is using also uses
wording

[mmenke, 2017-04-27 20:02:13]

https://codereview.chromium.org/2845643003/diff/20001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/2845643003/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1466: // Cancel up in progress work, and destroy all
objects that depend on the two
On 2017/04/27 19:35:41, eroman wrote:
> ProxyService is already a huge mess. I would rather not add extra stuff into
it,
> as it is already too complicated.
> 
> How about moving the notion of shutdown into to the leaf dependency
> (ProxyScriptFetcher) ?
> 
> In particular, ProxyScriptFetcherImpl is the class which holds a pointer to
the
> URLRequestContext [1].
> 
> So we could solve this more directly by adding a
> ProxyScriptFetcherImpl::Shutdown() method that:
> 
>   * Aborts any outstanding URLRequest
>   * Nulls its URLRequestContext pointer
>   * Marks itself as shutdown (such that any new requests will fail immediately
> with ERR_ABORTED).
> 
> This means that ProxyService continues working as before, only its
> ProxyScriptFetcher behaves as if it has no network connectivity.
> 
> As far as how to plumb that through I leave it up to you. As a strawman
proposal
> could do:
> 
>   ProxyService::DetachURLRequestContext(URLRequest* context)
>       calls ProxyScriptFetcher::DetachURLRequestContext(context)
> 
> ProxyScriptFetcherImpl::DetachURLRequestContext(context) is the only
> implementation that needs to do anything - it would enter shutdown mode per
> previous description.
> 
> [1] OK so so I failed to describe ProxyScriptDecider, which unfortunately grew
a
> dependency on the URLRequestContext for the quick detect stuff. So the final
> solution won't be as clean as I made it out to be, but should be able to adapt
> it.
> 
> WDYT?

I agree that this class is a complete mess.  Unwrangling the ProxyService,
ProxyConfigService, ProxyResolver, ProxyResolverFactory, ProxyDecider,
DhcpThingy, ProxyScriptFetcher, and InitializerClassThingy wasn't a particularly
fun experience, and I still only have vague notions of how they all plug in to
each other.

Don't forget we'd also need to hook it up to the
Dhcp-Proxy-Script-Fetcher-Resolver-Service-Host-Decider thingy, too.

Why would we take the URLRequestContext as an argument?  Are you thinking we'd
hook it up to URLRequestContext's destructor, and then if it's not the one being
used by the URLRequest, it just does nothing?  I have been thinking we'd tie
teardown to the URLRequestContextStorage, not the URLRequestContext, because of
all the hacky component storage, but putting stuff in the URLRequestContext
destructor may be fine.

[eroman, 2017-04-27 20:07:40]

Passing the URLRequestContext parameter was just a defense in depth kind of
thing in case we get confused on who is using it (and don't break the cycles
rightly). A straight up Shutdown() is of course sufficient.

Right, so there should just the ProxyScriptFetcher interface that needs a
shutdown, and unfortunately also the ProxyScriptDecider.

[mmenke, 2017-04-27 20:10:40]

On 2017/04/27 20:07:40, eroman wrote:
> Passing the URLRequestContext parameter was just a defense in depth kind of
> thing in case we get confused on who is using it (and don't break the cycles
> rightly). A straight up Shutdown() is of course sufficient.
> 
> Right, so there should just the ProxyScriptFetcher interface that needs a
> shutdown, and unfortunately also the ProxyScriptDecider.

Ok, I'll try that.  (And note that the method should either be OnShutdown or
ShutDown - shutdown is a noun, and method names shouldn't generally be nouns. 
I'll switch to OnShutdown, to avoid confusion over grammar nit-pickiness).

[mmenke, 2017-04-27 22:24:52]

Patchset #3 (id:40001) has been deleted

[mmenke, 2017-04-27 22:32:54]

What do you think of this approach?  I haven't written tests for it, just want
to come to agreement about the general approach first.  I also haven't hooked up
the ChromeOS DhcpProxyScriptFilter to the new logic.

I'm not being very consistent about hang vs failure - in process things hang,
while new things fail.  I could make new network requests hang, instead - not
sure that's really better, but may be more consistent.  Could also just destroy
init_proxy_resolver_, but probably best not to mess with that particular state
machine, as you said.

[eroman, 2017-04-28 18:39:16]

Thanks! Yes, that approach looks better to me.

[mmenke, 2017-04-28 18:43:21]

On 2017/04/28 18:39:16, eroman wrote:
> Thanks! Yes, that approach looks better to me.

Thanks for the feedback!  Note that I've switched to hanging, for consistency. 
Anyhow, should have tests ready Monday or Tuesday.

[eroman, 2017-04-28 19:05:20]

I don't think consistency is terribly important for hang vs fail.

My preference is for things to fail (more predictable, easier to debug if for
some reason we actually encounter this).

I think it is fine for new requests to consistently fail, and don't mind if it
is possible for outstanding requests to hang rather than be aborted (if aborting
is more work).

I haven't dug into the code to see in which cases this is possible, but as a
generality when aborting outstanding requests should pay attention to marking
state as shutdown prior to aborting requests, if aborting may cause user
callback to be invoked (leading to possible re-entrancy if it starts a new
request).

But yeah, "meh" since this is only at shutdown anyway.

[mmenke, 2017-04-28 19:08:29]

On 2017/04/28 19:05:20, eroman wrote:
> I don't think consistency is terribly important for hang vs fail.
> 
> My preference is for things to fail (more predictable, easier to debug if for
> some reason we actually encounter this).
> 
> I think it is fine for new requests to consistently fail, and don't mind if it
> is possible for outstanding requests to hang rather than be aborted (if
aborting
> is more work).
> 
> I haven't dug into the code to see in which cases this is possible, but as a
> generality when aborting outstanding requests should pay attention to marking
> state as shutdown prior to aborting requests, if aborting may cause user
> callback to be invoked (leading to possible re-entrancy if it starts a new
> request).
> 
> But yeah, "meh" since this is only at shutdown anyway.

Ok, I'll defer to you, then, and hang pending requests but fail future one.  I
don't have strong feelings here, just have a bit of a preference for
consistency, and failing everything makes me concerned that we'll be doing a
bunch of extra silly work on shutdown.

[mmenke, 2017-04-28 21:40:08]

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

[commit-bot: I haz the power, 2017-04-28 21:40:34]

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-04-28 21:50:52]

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

[commit-bot: I haz the power, 2017-04-28 21:50:53]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[mmenke, 2017-04-28 21:54:56]

Patchset #6 (id:120001) has been deleted

[mmenke, 2017-04-28 21:55:03]

Patchset #5 (id:100001) has been deleted

[mmenke, 2017-04-28 21:55:10]

Patchset #4 (id:80001) has been deleted

[mmenke, 2017-04-28 21:55:16]

Patchset #3 (id:60001) has been deleted

[mmenke, 2017-04-28 21:57:34]

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

[commit-bot: I haz the power, 2017-04-28 21:58:33]

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-04-28 22:34:19]

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

[commit-bot: I haz the power, 2017-04-28 22:34:20]

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_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mmenke, 2017-05-01 16:47:31]

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

[commit-bot: I haz the power, 2017-05-01 16:47:52]

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-01 16:52:14]

Ok, think this is finally ready for review.

https://codereview.chromium.org/2845643003/diff/160001/net/proxy/proxy_script...
File net/proxy/proxy_script_decider.cc (right):

https://codereview.chromium.org/2845643003/diff/160001/net/proxy/proxy_script...
net/proxy/proxy_script_decider.cc:480: request_.reset();
This could just be put in OnShutdown(), but not having it here already seems
like a bug.

https://codereview.chromium.org/2845643003/diff/160001/net/proxy/proxy_servic...
File net/proxy/proxy_service_unittest.cc (right):

https://codereview.chromium.org/2845643003/diff/160001/net/proxy/proxy_servic...
net/proxy/proxy_service_unittest.cc:3688: TEST_F(ProxyServiceTest,
OnShutdownWithLiveRequest) {
The tests don't test all paths - this one depends on the decider shutting down
properly (If it fails, shutting down the fetcher properly may still make it
pass, though), and the next just tests that the fetcher is shut down properly. 
Nothing for the DHCP fetcher, which I don't believe is even tested at all in any
of these.

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

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

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

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

[mmenke, 2017-05-01 17:08:27]

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

[commit-bot: I haz the power, 2017-05-01 17:08:57]

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-01 19:19:58]

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

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

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-01 19:27:50]

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

[commit-bot: I haz the power, 2017-05-01 19:28:35]

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-01 21:49:42]

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

[commit-bot: I haz the power, 2017-05-01 21:49:43]

Dry run: This issue passed the CQ dry run.

[eroman, 2017-05-02 17:49:20]

lgtm

https://codereview.chromium.org/2845643003/diff/220001/net/proxy/proxy_servic...
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/2845643003/diff/220001/net/proxy/proxy_servic...
net/proxy/proxy_service.cc:1473: dhcp_proxy_script_fetcher_->OnShutdown();
Does the order of shutdown calls matter?

[mmenke, 2017-05-02 17:57:36]

https://codereview.chromium.org/2845643003/diff/220001/net/proxy/proxy_servic...
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/2845643003/diff/220001/net/proxy/proxy_servic...
net/proxy/proxy_service.cc:1473: dhcp_proxy_script_fetcher_->OnShutdown();
On 2017/05/02 17:49:20, eroman wrote:
> Does the order of shutdown calls matter?

Not for correctness.  Just for test coverage, I put the |init_proxy_resolver_|
first, so we shutdown the decider directly in one of the ProxyServiceTests.

[mmenke, 2017-05-02 18:21:28]

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

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

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-02 18:24:51]

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

[commit-bot: I haz the power, 2017-05-02 18:25:33]

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-02 19:34:51]

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

[commit-bot: I haz the power, 2017-05-02 19:34:52]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-05-02 20:03:46]

Patchset #8 (id:240001) has been deleted

[mmenke, 2017-05-02 20:04:09]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2017-05-02 20:04:09]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2845643003/#ps260001
(title: "Fix fetchuer shutdown with no active request, add test, add comment")

[commit-bot: I haz the power, 2017-05-02 20:04:47]

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

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

[commit-bot: I haz the power, 2017-05-02 20:14:51]

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-02 20:26:17]

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

[mmenke, 2017-05-02 20:26:18]

[+stevenjb]:  Please review
chromeos/network/dhcp_proxy_script_fetcher_chromeos.*.  There aren't any tests
for that change, but I didn't see any pre-existing tests for that class's
behavior.

[stevenjb, 2017-05-02 20:28:28]

lgtm

[mmenke, 2017-05-02 20:29:59]

Description was changed from

==========
Allow ProxyService to share URLRequestContext with everything else.

This CL makes ProxyScriptFetcherImpl use highest priority and
LOAD_IGNORE_LIMITS, and adds a ShutDown method to ProxyService,
which must be called before teardown when the ProxyService makes
requests using a URLRequestContext that it is also used with.

No consumer currently uses the new functionality.  I plan to
switch IOThread/ProfileIOData to using it in another CL, and
add a call in URLRequestContextStorage's destructor.  May also
be worth making it mandatory as well, though that will involve
more changes.

BUG=715697
==========

to

==========
Allow ProxyService to share URLRequestContext with everything else.

This CL makes ProxyScriptFetcherImpl use highest priority and
LOAD_IGNORE_LIMITS, and adds an OnShutdown method to ProxyService,
which must be called before teardown when the ProxyService makes
requests using a URLRequestContext that it is also used with.

No consumer currently uses the new functionality.  I plan to
switch IOThread/ProfileIOData to using it in another CL, and
add a call in URLRequestContextStorage's destructor.  May also
be worth making it mandatory as well, though that will involve
more changes.

BUG=715697
==========

[mmenke, 2017-05-03 16:41:34]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2017-05-03 16:42:17]

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-03 16:48:39]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1493829694165830,
"parent_rev": "7ad61a7154af4788bcc4d1333698709bf1aa42ac", "commit_rev":
"ed8d7e4383d5dbf7e94291b7960b5a4616b4a45e"}

[commit-bot: I haz the power, 2017-05-03 16:48:52]

Description was changed from

==========
Allow ProxyService to share URLRequestContext with everything else.

This CL makes ProxyScriptFetcherImpl use highest priority and
LOAD_IGNORE_LIMITS, and adds an OnShutdown method to ProxyService,
which must be called before teardown when the ProxyService makes
requests using a URLRequestContext that it is also used with.

No consumer currently uses the new functionality.  I plan to
switch IOThread/ProfileIOData to using it in another CL, and
add a call in URLRequestContextStorage's destructor.  May also
be worth making it mandatory as well, though that will involve
more changes.

BUG=715697
==========

to

==========
Allow ProxyService to share URLRequestContext with everything else.

This CL makes ProxyScriptFetcherImpl use highest priority and
LOAD_IGNORE_LIMITS, and adds an OnShutdown method to ProxyService,
which must be called before teardown when the ProxyService makes
requests using a URLRequestContext that it is also used with.

No consumer currently uses the new functionality.  I plan to
switch IOThread/ProfileIOData to using it in another CL, and
add a call in URLRequestContextStorage's destructor.  May also
be worth making it mandatory as well, though that will involve
more changes.

BUG=715697

Review-Url: https://codereview.chromium.org/2845643003
Cr-Commit-Position: refs/heads/master@{#469005}
Committed:
https://chromium.googlesource.com/chromium/src/+/ed8d7e4383d5dbf7e94291b7960b...
==========

[commit-bot: I haz the power, 2017-05-03 16:48:56]

Committed patchset #8 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/ed8d7e4383d5dbf7e94291b7960b...