Shut down proxy resolver utility processes when no longer needed.

Shut down proxy resolver utility processes when no longer needed.

This adds tracking of ProxyResolverMojo instances in the browser
process when running with an out-of-process v8 proxy resolver. When no
longer needed, the utility process is shut down following an idle
timeout.

BUG=467832
Committed: https://crrev.com/1d5df4d2c461802b42ce92fcab3ae5450890b1dc
Cr-Commit-Position: refs/heads/master@{#328279}

[Sam McNally, 2015-04-10 03:41:05]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2015-04-10 03:41:18]

Patchset #1 (id:20001) has been deleted

[Sam McNally, 2015-04-10 04:08:58]

Patchset #1 (id:40001) has been deleted

[Sam McNally, 2015-04-10 04:19:19]

sammc@chromium.org changed reviewers:
+ amistry@chromium.org, eroman@chromium.org

[eroman, 2015-04-15 00:46:44]

The general approach to recycling the utility process after a few seconds of
there being no mojo proxy resolvers looksgood.

But as with the other mojo-wrapping code, I am finding the ownership semantics
very difficult to follow.

https://codereview.chromium.org/1076083002/diff/60001/chrome/browser/net/util...
File chrome/browser/net/utility_process_mojo_proxy_resolver_factory.cc (right):

https://codereview.chromium.org/1076083002/diff/60001/chrome/browser/net/util...
chrome/browser/net/utility_process_mojo_proxy_resolver_factory.cc:79:
base::Unretained(this))));
What makes the use of unretained safe here?

https://codereview.chromium.org/1076083002/diff/60001/chrome/browser/net/util...
chrome/browser/net/utility_process_mojo_proxy_resolver_factory.cc:104: delete
utility_process_host_.get();
Why is it written like this, rather than just a call to reset?

https://codereview.chromium.org/1076083002/diff/60001/net/proxy/mojo_proxy_re...
File net/proxy/mojo_proxy_resolver_factory.h (right):

https://codereview.chromium.org/1076083002/diff/60001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_factory.h:20: // |host_resolver| as the DNS
resolver. The return value should be deleted
nit: deleted -> released (deleted sounds too much like calling "delete" in my
mind).

https://codereview.chromium.org/1076083002/diff/60001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_mojo.h (right):

https://codereview.chromium.org/1076083002/diff/60001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo.h:42: // using |mojo_proxy_resolver| that will
delete |runner| when it is deleted.
this comment is hard to parse

https://codereview.chromium.org/1076083002/diff/60001/net/proxy/proxy_service...
File net/proxy/proxy_service_mojo.cc (right):

https://codereview.chromium.org/1076083002/diff/60001/net/proxy/proxy_service...
net/proxy/proxy_service_mojo.cc:27: static scoped_ptr<base::ScopedClosureRunner>
Create(
This code is hard to read and the ownerships hard to follow.

[Sam McNally, 2015-04-15 06:04:22]

https://codereview.chromium.org/1076083002/diff/60001/chrome/browser/net/util...
File chrome/browser/net/utility_process_mojo_proxy_resolver_factory.cc (right):

https://codereview.chromium.org/1076083002/diff/60001/chrome/browser/net/util...
chrome/browser/net/utility_process_mojo_proxy_resolver_factory.cc:79:
base::Unretained(this))));
On 2015/04/15 00:46:44, eroman wrote:
> What makes the use of unretained safe here?

UtilityProcessMojoProxyResolverFactory is singleton.

https://codereview.chromium.org/1076083002/diff/60001/chrome/browser/net/util...
chrome/browser/net/utility_process_mojo_proxy_resolver_factory.cc:104: delete
utility_process_host_.get();
On 2015/04/15 00:46:44, eroman wrote:
> Why is it written like this, rather than just a call to reset?

utility_process_host_ is a WeakPtr.

https://codereview.chromium.org/1076083002/diff/60001/net/proxy/mojo_proxy_re...
File net/proxy/mojo_proxy_resolver_factory.h (right):

https://codereview.chromium.org/1076083002/diff/60001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_factory.h:20: // |host_resolver| as the DNS
resolver. The return value should be deleted
On 2015/04/15 00:46:44, eroman wrote:
> nit: deleted -> released (deleted sounds too much like calling "delete" in my
> mind).

Done.

https://codereview.chromium.org/1076083002/diff/60001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_mojo.h (right):

https://codereview.chromium.org/1076083002/diff/60001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_mojo.h:42: // using |mojo_proxy_resolver| that will
delete |runner| when it is deleted.
On 2015/04/15 00:46:44, eroman wrote:
> this comment is hard to parse

How about now?

https://codereview.chromium.org/1076083002/diff/60001/net/proxy/proxy_service...
File net/proxy/proxy_service_mojo.cc (right):

https://codereview.chromium.org/1076083002/diff/60001/net/proxy/proxy_service...
net/proxy/proxy_service_mojo.cc:27: static scoped_ptr<base::ScopedClosureRunner>
Create(
On 2015/04/15 00:46:44, eroman wrote:
> This code is hard to read and the ownerships hard to follow.

Moved ownership of the MojoHostResolverImpl back into ProxyResolverMojo.

[eroman, 2015-05-02 02:22:31]

lgtm

https://codereview.chromium.org/1076083002/diff/100001/chrome/browser/net/uti...
File chrome/browser/net/utility_process_mojo_proxy_resolver_factory.cc (right):

https://codereview.chromium.org/1076083002/diff/100001/chrome/browser/net/uti...
chrome/browser/net/utility_process_mojo_proxy_resolver_factory.cc:87: void
UtilityProcessMojoProxyResolverFactory::OnResolverDestroyed() {
The assumption is that this code always runs on a single thread. Please add an
assertion for the current thread in these functions

https://codereview.chromium.org/1076083002/diff/100001/net/proxy/proxy_resolv...
File net/proxy/proxy_resolver_mojo.h (right):

https://codereview.chromium.org/1076083002/diff/100001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_mojo.h:42: // When deleted, the closure contained
within |runner| will be run.
can you come up with a better name than |runner|?

[Sam McNally, 2015-05-04 06:10:45]

Patchset #4 (id:120001) has been deleted

[Sam McNally, 2015-05-04 06:17:23]

https://codereview.chromium.org/1076083002/diff/100001/chrome/browser/net/uti...
File chrome/browser/net/utility_process_mojo_proxy_resolver_factory.cc (right):

https://codereview.chromium.org/1076083002/diff/100001/chrome/browser/net/uti...
chrome/browser/net/utility_process_mojo_proxy_resolver_factory.cc:87: void
UtilityProcessMojoProxyResolverFactory::OnResolverDestroyed() {
On 2015/05/02 02:22:31, eroman wrote:
> The assumption is that this code always runs on a single thread. Please add an
> assertion for the current thread in these functions

Done.

https://codereview.chromium.org/1076083002/diff/100001/net/proxy/proxy_resolv...
File net/proxy/proxy_resolver_mojo.h (right):

https://codereview.chromium.org/1076083002/diff/100001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_mojo.h:42: // When deleted, the closure contained
within |runner| will be run.
On 2015/05/02 02:22:31, eroman wrote:
> can you come up with a better name than |runner|?

Done.

[Anand Mistry (off Chromium), 2015-05-05 03:47:41]

lgtm

https://codereview.chromium.org/1076083002/diff/200001/chrome/browser/net/uti...
File chrome/browser/net/utility_process_mojo_proxy_resolver_factory.cc (right):

https://codereview.chromium.org/1076083002/diff/200001/chrome/browser/net/uti...
chrome/browser/net/utility_process_mojo_proxy_resolver_factory.cc:108:
DCHECK(thread_checker_.CalledOnValidThread());
DCHECK_EQ(num_proxy_resolvers_, 0u);

[Sam McNally, 2015-05-05 03:54:04]

https://codereview.chromium.org/1076083002/diff/200001/chrome/browser/net/uti...
File chrome/browser/net/utility_process_mojo_proxy_resolver_factory.cc (right):

https://codereview.chromium.org/1076083002/diff/200001/chrome/browser/net/uti...
chrome/browser/net/utility_process_mojo_proxy_resolver_factory.cc:108:
DCHECK(thread_checker_.CalledOnValidThread());
On 2015/05/05 03:47:41, Anand Mistry wrote:
> DCHECK_EQ(num_proxy_resolvers_, 0u);

Done.

[Sam McNally, 2015-05-05 03:54:09]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2015-05-05 03:54:09]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
amistry@chromium.org
Link to the patchset: https://codereview.chromium.org/1076083002/#ps220001
(title: " ")

[commit-bot: I haz the power, 2015-05-05 03:54:51]

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

[commit-bot: I haz the power, 2015-05-05 05:06:32]

Committed patchset #8 (id:220001)

[commit-bot: I haz the power, 2015-05-05 05:07:45]

Patchset 8 (id:??) landed as
https://crrev.com/1d5df4d2c461802b42ce92fcab3ae5450890b1dc
Cr-Commit-Position: refs/heads/master@{#328279}