Implement utility-side proxy resolver factory Mojo service.

Implement utility-side proxy resolver factory Mojo service.

BUG=11746
Committed: https://crrev.com/352f74947a93a5633ea7614d8af5df1afe13ca77
Cr-Commit-Position: refs/heads/master@{#318010}

[Sam McNally, 2015-02-12 03:27:22]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2015-02-12 03:31:09]

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

[eroman, 2015-02-12 23:19:50]

not lgtm.

ProxyResolverFactory should not need to be mojo-ized.

ProxyResolverFactory is an abstraction for passing synchronous ProxyResolver
implementations to MultiThreadedProxyResolver, which will then instantiate a
ProxyResolver for each of its worker threads.

For our purposes I envision having the mojo-ized ProxyResolver connect to
utility process, and on that end it instantiates a:
    ProxyResolverV8Tracing


MultiThreadedProxyResolver is really just used for running the system proxy
resolver (which wouldn't be able to run in the the sandboxed utility process
anyway).

Does that make sense, or am I misunderstanding the change?

Cheers.

[Sam McNally, 2015-02-12 23:33:44]

On 2015/02/12 23:19:50, eroman wrote:
> not lgtm.
> 
> ProxyResolverFactory should not need to be mojo-ized.
> 
> ProxyResolverFactory is an abstraction for passing synchronous ProxyResolver
> implementations to MultiThreadedProxyResolver, which will then instantiate a
> ProxyResolver for each of its worker threads.
> 
> For our purposes I envision having the mojo-ized ProxyResolver connect to
> utility process, and on that end it instantiates a:
>     ProxyResolverV8Tracing
> 
> 
> MultiThreadedProxyResolver is really just used for running the system proxy
> resolver (which wouldn't be able to run in the the sandboxed utility process
> anyway).
> 
> Does that make sense, or am I misunderstanding the change?
> 
> Cheers.

This is a factory for MojoProxyResolver instances, not a Mojo version of the
existing ProxyResolverFactory. Naming is hard.

MojoProxyResolverFactory does more or less what you envision: when a request is
received, create a ProxyResolverV8Tracing (or call a callback for testing) and
the mojo shims (MojoProxyResolver and HostResolverMojo), hook them up and when
either connection is dropped, tear it down.

[eroman, 2015-02-12 23:41:06]

Ah my mistake, maybe I should actually read the files then :)

[eroman, 2015-02-12 23:55:31]

Could you give me a bit more of a high level overview? I am having a hard time
following how these classes fit together.

Where/how is the factory instantiated?

Also what is mojo::GetProxy() and how does the memory ownership work in
expressions like:

new MojoProxyResolverHolder(proxy_resolver_impl_factory_, request.Pass(),
host_resolver.Pass());

[Sam McNally, 2015-02-13 00:31:55]

On 2015/02/12 23:55:31, eroman wrote:
> Could you give me a bit more of a high level overview? I am having a hard time
> following how these classes fit together.

We have mojo interfaces HostResolver and ProxyResolver. Around those interfaces
we have shims to convert between the mojo interface and the existing C++
interface. ProxyResolverMojo implements net::ProxyResolver and forwards to the
mojo ProxyResolver interface. MojoProxyResolver implements the mojo
ProxyResolver interface and forwards to the C++ ProxyResolver interface.
HostResolver follows the same pattern.

So the classes chain together:
ProxyResolverMojo (implementing ProxyResolver) => interfaces::ProxyResolver =>
MojoProxyResolver => ProxyResolver (usually a ProxyResolverV8Tracing)
with interfaces::ProxyResolver bridging the browser and utility processes.
Again, HostResolver works the similarly, but with processes swapped. The
ProxyResolverV8Tracing in the utility process is given the HostResolverMojo as
its HostResolver.

The ProxyResolverFactory mojo interface CreateResolver represents ProxyResolver
creation at the mojo level: a ProxyResolver requires a HostResolver. The
MojoProxyResolverFactory implementation of that interface places a
HostResolverMojo around the mojo HostResolver interface and a MojoProxyResolver
around the mojo ProxyResolver interface. This transforms the mojo interface into
an object that requires a C++ ProxyResolver and C++ HostResolver to be passed to
the ProxyResolver impl.

> 
> Where/how is the factory instantiated?

The factory will be instantiated when the browser process requests a
ProxyResolverFactory service from the utility process. The callback to create
the factory will be registered during utility process startup. It would look
something like
https://codereview.chromium.org/862813002/diff/10001/chrome/utility/chrome_co....

> 
> Also what is mojo::GetProxy()

mojo::GetProxy gets the mojo::InterfaceRequest for a mojo::InterfacePtr (FooPtr
is a typedef for mojo::InterfacePtr<Foo>). An InterfaceRequest is essentially an
out parameter for an InterfacePtr. You could write a mojo interface:
interface FooFactory {
  CreateFoo(...) => (Foo foo);
};
The response to CreateFoo would be an InterfacePtr<Foo>. The alternative is
interface FooFactory {
  CreateFoo(Foo& foo, ...);
};
The & indicates that CreateFoo "returns" an InterfacePtr<Foo>. The way this is
expressed in C++ is passing an InterfaceRequest<Foo> derived from your
InterfacePtr<Foo> using mojo::GetProxy().

> and how does the memory ownership work in
> expressions like:
> 
> new MojoProxyResolverHolder(proxy_resolver_impl_factory_, request.Pass(),
> host_resolver.Pass());

MojoProxyResolverHolder sets itself as error handler for both request
(indirectly - it passes |this| to MojoProxyResolverHolder as the error handler
to set on its Binding, which is where the InterfaceRequest ends up) and
host_resolver. Then if either connection disconnects, OnConnectionError() is
called and the MojoProxyResolverHolder deletes itself.

Hope this helps!

[Anand Mistry (off Chromium), 2015-02-16 06:46:15]

https://codereview.chromium.org/918933002/diff/40001/net/proxy/mojo_proxy_res...
File net/proxy/mojo_proxy_resolver_factory.cc (right):

https://codereview.chromium.org/918933002/diff/40001/net/proxy/mojo_proxy_res...
net/proxy/mojo_proxy_resolver_factory.cc:21: class MojoProxyResolverHolder :
public mojo::ErrorHandler {
Please comment on the life span of this. It's not immediately clear that this is
bound to the life of the ProxyResolver's message pipe.

https://codereview.chromium.org/918933002/diff/40001/net/proxy/mojo_proxy_res...
File net/proxy/mojo_proxy_resolver_factory.h (right):

https://codereview.chromium.org/918933002/diff/40001/net/proxy/mojo_proxy_res...
net/proxy/mojo_proxy_resolver_factory.h:16: class MojoProxyResolverFactory :
public interfaces::ProxyResolverFactory {
Shouldn't this be MojoProxyResolverFactoryImpl to be consistent with the other
mojo service implementations?

https://codereview.chromium.org/918933002/diff/40001/net/proxy/mojo_proxy_res...
File net/proxy/mojo_proxy_resolver_factory_unittest.cc (right):

https://codereview.chromium.org/918933002/diff/40001/net/proxy/mojo_proxy_res...
net/proxy/mojo_proxy_resolver_factory_unittest.cc:53: void WaitForEvent(Event
event) {
Ok, I've seen this pattern a few times. I think it's time to factor this out
into a helper class. Maybe something like:

class EventWaiter {
 void WaitForEvent(int event);
 void NotifyEvent(int event);
};

[Sam McNally, 2015-02-16 07:38:19]

Patchset #4 (id:80001) has been deleted

[Sam McNally, 2015-02-16 23:59:40]

Patchset #5 (id:120001) has been deleted

[Sam McNally, 2015-02-17 00:00:57]

https://codereview.chromium.org/918933002/diff/40001/net/proxy/mojo_proxy_res...
File net/proxy/mojo_proxy_resolver_factory.cc (right):

https://codereview.chromium.org/918933002/diff/40001/net/proxy/mojo_proxy_res...
net/proxy/mojo_proxy_resolver_factory.cc:21: class MojoProxyResolverHolder :
public mojo::ErrorHandler {
On 2015/02/16 06:46:15, Anand Mistry wrote:
> Please comment on the life span of this. It's not immediately clear that this
is
> bound to the life of the ProxyResolver's message pipe.

Done.

https://codereview.chromium.org/918933002/diff/40001/net/proxy/mojo_proxy_res...
File net/proxy/mojo_proxy_resolver_factory.h (right):

https://codereview.chromium.org/918933002/diff/40001/net/proxy/mojo_proxy_res...
net/proxy/mojo_proxy_resolver_factory.h:16: class MojoProxyResolverFactory :
public interfaces::ProxyResolverFactory {
On 2015/02/16 06:46:15, Anand Mistry wrote:
> Shouldn't this be MojoProxyResolverFactoryImpl to be consistent with the other
> mojo service implementations?

Done.

https://codereview.chromium.org/918933002/diff/40001/net/proxy/mojo_proxy_res...
File net/proxy/mojo_proxy_resolver_factory_unittest.cc (right):

https://codereview.chromium.org/918933002/diff/40001/net/proxy/mojo_proxy_res...
net/proxy/mojo_proxy_resolver_factory_unittest.cc:53: void WaitForEvent(Event
event) {
On 2015/02/16 06:46:15, Anand Mistry wrote:
> Ok, I've seen this pattern a few times. I think it's time to factor this out
> into a helper class. Maybe something like:
> 
> class EventWaiter {
>  void WaitForEvent(int event);
>  void NotifyEvent(int event);
> };

Done in https://codereview.chromium.org/904313003/.

[Anand Mistry (off Chromium), 2015-02-19 05:23:08]

https://codereview.chromium.org/918933002/diff/100001/net/proxy/mojo_proxy_re...
File net/proxy/mojo_proxy_resolver_factory_impl.cc (right):

https://codereview.chromium.org/918933002/diff/100001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_factory_impl.cc:17: return make_scoped_ptr(
ProxyResolverV8::EnsureIsolateCreated();

[eroman, 2015-02-20 01:43:37]

https://codereview.chromium.org/918933002/diff/100001/net/proxy/mojo_proxy_re...
File net/proxy/mojo_proxy_resolver_factory_impl.cc (right):

https://codereview.chromium.org/918933002/diff/100001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_factory_impl.cc:21: // A class to manage the
lifetime of a MojoProxyResolverHolder and a
Is this comment correct? MojoProxyResolverHolder --> MojoProxyResolver.

https://codereview.chromium.org/918933002/diff/100001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_factory_impl.cc:51:
binding_.set_error_handler(this);
what is the difference between the error handler on the binding and that placed
on the ProxyResolverPtr (by the unittest).

What would be a good place to read up on mojo concepts (header files, design
documents, or whatever).
I confess I am still quite confused between the various layerings here.

https://codereview.chromium.org/918933002/diff/100001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_factory_impl.cc:78: new
MojoProxyResolverHolder(proxy_resolver_impl_factory_, request.Pass(),
This still doesn't smell right to me.

At the very least there should be some comments explaining the expected
lifetime.

So OnConnectionError() can be called either by the host resolver, or by the
proxyresolver, and this results in the deletion of the "holder":
  - is it guaranteed that at least one of these will be called? Is it possible
to tear things down in a manner where neither connectionerror gets called and
things leak.
  - is it guaranteed that OnConnectionError() can be called at most once,
between both the callers

[Sam McNally, 2015-02-20 03:25:04]

https://codereview.chromium.org/918933002/diff/100001/net/proxy/mojo_proxy_re...
File net/proxy/mojo_proxy_resolver_factory_impl.cc (right):

https://codereview.chromium.org/918933002/diff/100001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_factory_impl.cc:17: return make_scoped_ptr(
On 2015/02/19 05:23:08, Anand Mistry wrote:
> ProxyResolverV8::EnsureIsolateCreated();

Done.

https://codereview.chromium.org/918933002/diff/100001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_factory_impl.cc:21: // A class to manage the
lifetime of a MojoProxyResolverHolder and a
On 2015/02/20 01:43:37, eroman wrote:
> Is this comment correct? MojoProxyResolverHolder --> MojoProxyResolver.

Done.

https://codereview.chromium.org/918933002/diff/100001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_factory_impl.cc:51:
binding_.set_error_handler(this);
On 2015/02/20 01:43:37, eroman wrote:
> what is the difference between the error handler on the binding and that
placed
> on the ProxyResolverPtr (by the unittest).

They serve the same function, but operate on opposite ends of the message pipe.
This error handler will detect when the client has closed its ProxyResolverPtr;
the error handler on the ProxyResolverPtr will detect when the ProxyResolver
implementation has disconnected - outside of tests this would usually be due to
the utility process crashing.
> 
> What would be a good place to read up on mojo concepts (header files, design
> documents, or whatever).
> I confess I am still quite confused between the various layerings here.

The most up to date place is probably the comments in the bindings headers in
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/mojo/...
or
https://docs.google.com/document/d/1FqXnpW1oOENS5mgvCjupyiWs4wdtxPJ9IyCYK2WMl....

https://codereview.chromium.org/918933002/diff/100001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_factory_impl.cc:78: new
MojoProxyResolverHolder(proxy_resolver_impl_factory_, request.Pass(),
On 2015/02/20 01:43:37, eroman wrote:
> This still doesn't smell right to me.
> 
> At the very least there should be some comments explaining the expected
> lifetime.
Done.
> 
> So OnConnectionError() can be called either by the host resolver, or by the
> proxyresolver, and this results in the deletion of the "holder":
>   - is it guaranteed that at least one of these will be called? Is it possible
> to tear things down in a manner where neither connectionerror gets called and
> things leak.
It may be possible to leak during process shutdown. Otherwise, as long as the
client closes its connection, OnConnectionError() will be called.

>   - is it guaranteed that OnConnectionError() can be called at most once,
> between both the callers
> 
Yes. When OnConnectionError() is called, the Binding and InterfacePtr are
deleted so nothing would be around to call it a second time.

[eroman, 2015-02-25 00:31:18]

lgtm

https://codereview.chromium.org/918933002/diff/140001/net/proxy/mojo_proxy_re...
File net/proxy/mojo_proxy_resolver_factory_impl.cc (right):

https://codereview.chromium.org/918933002/diff/140001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_factory_impl.cc:18:
ProxyResolverV8::EnsureIsolateCreated();
This should actually be moved elsewhere so it is just a party of
V8ProxyResolver's lazy initialization... I will work on it in a separate CL

[Anand Mistry (off Chromium), 2015-02-25 02:54:14]

lgtm

[Sam McNally, 2015-02-25 06:34:34]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2015-02-25 06:35:00]

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

[commit-bot: I haz the power, 2015-02-25 09:45:32]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2015-02-25 09:46:59]

Patchset 5 (id:??) landed as
https://crrev.com/352f74947a93a5633ea7614d8af5df1afe13ca77
Cr-Commit-Position: refs/heads/master@{#318010}