Implement browser-side host resolver Mojo service.

Implement browser-side host resolver Mojo service.

BUG=11746
Committed: https://crrev.com/7e6ebfdc8f3536dc0246fcec458b27cdd836c3b5
Cr-Commit-Position: refs/heads/master@{#316160}

[Anand Mistry (off Chromium), 2015-02-04 08:33:00]

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

[Anand Mistry (off Chromium), 2015-02-04 08:33:00]

This is the DNS resolver service used by the proxy resolver. The intention is to
instantiate this in the browser process and send a handle to it to the proxy
resolver.

[eroman, 2015-02-05 19:07:47]

Diving into this review today!

But first as a related side-comment, is there going to be a circular dependency
issue with //net using mojo, and mojo using //net?

Not sure what the long term vision is, but mojo shell uses net for loading
services as i understand. What if that network configuration needs proxy
support, would it be a problem that proxy support it itself expressed using mojo
services?

[eroman, 2015-02-05 19:51:11]

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
File net/dns/host_resolver_service_impl.cc (right):

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl.cc:41: BoundNetLog net_log_;
This is not actually initialized. Rather may want to leave this out and pass the
dependency in where used.

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl.cc:51: for (Job* job : pending_jobs_) {
How about having cancellation be done as part of deletion?

Then STLDeleteElements(&pending_jobs_) would be sufficient, and explicit job
cancel method not needed

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl.cc:96: int32_t result = resolver_->Resolve(
"int" is what the net stack uses, I suggest you stick to the same type here.
Even though in practice we compile with 32-bit ints on all platforms at the
moment.

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl.cc:111: void
HostResolverServiceImpl::Job::OnResolveDone(int32_t result) {
why int32_t? The //net code defines error as "int"

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
File net/dns/host_resolver_service_impl.h (right):

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl.h:23: class HostResolverServiceImpl
I am not excited about the naming. "Service" is already a term used throughout
network stack, including for instance "ProxyService".

I would be happier if "Mojo" appeared somewhere in the name, as in MojoService
to make it clearer what these are.

(there may be an underlying net service, and then they may also be a MojoService
that wraps it).

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl.h:26: explicit
HostResolverServiceImpl(HostResolver* resolver);
Please describe the lifetime expectations for |resovler|

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
File net/dns/host_resolver_service_impl_unittest.cc (right):

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl_unittest.cc:91: uint8_t port,
Please make this 16bit for consistency

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl_unittest.cc:102: void
WaitForOutstandingRequests(size_t num) {
Please explain this method, this doesn't look like a good approach.

https://codereview.chromium.org/892393005/diff/70001/net/dns/type_converters.h
File net/dns/type_converters.h (right):

https://codereview.chromium.org/892393005/diff/70001/net/dns/type_converters....
net/dns/type_converters.h:14: struct
TypeConverter<net::HostResolver::RequestInfo,
What are these about?

[Anand Mistry (off Chromium), 2015-02-05 21:09:28]

On 2015/02/05 19:07:47, eroman wrote:
> Diving into this review today!
> 
> But first as a related side-comment, is there going to be a circular
dependency
> issue with //net using mojo, and mojo using //net?
> 
> Not sure what the long term vision is, but mojo shell uses net for loading
> services as i understand. What if that network configuration needs proxy
> support, would it be a problem that proxy support it itself expressed using
mojo
> services?

Well, part of the goal is get a better idea what the long term vision should be
:P
There's also the question of the long-term plan for //net and if that's moved
out-of-process, what would that look like.

Short term, my plan is to not change any existing part of //net and instead, add
libraries that depend on //net and are linked into either the browser or child.
This would keep the proxy service separate from //net and help avoid affecting
any existing targets (i.e. iOS, Android, other). If, in a few months time,
everything is stable and everyone is happy, we can consider folding the mojo
services into //net:net and //net:net_with_v8. But for now, I think it's better
if they remain split.

[Sam McNally, 2015-02-05 23:58:36]

https://codereview.chromium.org/892393005/diff/70001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/892393005/diff/70001/net/BUILD.gn#newcode800
net/BUILD.gn:800: defines = [ "NET_IMPLEMENTATION" ]
Remove.

https://codereview.chromium.org/892393005/diff/70001/net/BUILD.gn#newcode808
net/BUILD.gn:808: "//mojo/environment:chromium",
This should be in test deps instead.

https://codereview.chromium.org/892393005/diff/70001/net/DEPS
File net/DEPS (right):

https://codereview.chromium.org/892393005/diff/70001/net/DEPS#newcode7
net/DEPS:7: "+third_party/mojo/src/mojo",
+third_party/mojo/src/mojo/public

Most of //net should have no reason to use anything from mojo/edk.

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
File net/dns/host_resolver_service_impl.cc (right):

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl.cc:25: void Start();
I'd prefer to not have a way to create a Job without starting it.

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
File net/dns/host_resolver_service_impl.h (right):

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl.h:48: std::set<Job*> pending_jobs_;
std::set<linked_ptr<Job>>

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
File net/dns/host_resolver_service_impl_unittest.cc (right):

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl_unittest.cc:30: void
Bind(interfaces::HostResolverRequestClientPtr* ptr);
Is there a reason for not doing this in the constructor?

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl_unittest.cc:41: base::RunLoop* run_loop_;
Store the base::Closure returned by RunLoop::QuitClosure() and create the run
loop on the stack in WaitForResult().

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl_unittest.cc:61: ASSERT_FALSE(done_);
Quitting the RunLoop first might avoid test failures getting stuck running a
RunLoop.

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl_unittest.cc:75:
mojo::embedder::test::InitWithSimplePlatformSupport();
This should be in net/test/run_all_unittests.cc.

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl_unittest.cc:84:
resolver_service_binding_.reset(
It seems a bit weird for the Binding to not be a member of the impl class.

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl_unittest.cc:119:
interfaces::HostResolverServicePtr resolver_service_ptr_;
For the most part I don't think it's valuable to use a real mojo pipe to make
requests. Avoiding WaitForOutstandingRequests would be nicer.

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl_unittest.cc:241:
TEST_F(HostResolverServiceImplTest, DestroyClient) {
I'm not sure what this is testing. As far as I can tell, destroying the client
shouldn't affect any in-flight requests, especially the since the Binding is
outside the class.

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl_unittest.cc:277:
base::RunLoop().RunUntilIdle();
Check that the client detected that its pipe closed?

[Anand Mistry (off Chromium), 2015-02-06 06:32:41]

https://codereview.chromium.org/892393005/diff/70001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/892393005/diff/70001/net/BUILD.gn#newcode800
net/BUILD.gn:800: defines = [ "NET_IMPLEMENTATION" ]
On 2015/02/05 23:58:36, Sam McNally wrote:
> Remove.

Done.

https://codereview.chromium.org/892393005/diff/70001/net/BUILD.gn#newcode808
net/BUILD.gn:808: "//mojo/environment:chromium",
On 2015/02/05 23:58:36, Sam McNally wrote:
> This should be in test deps instead.

Done.

https://codereview.chromium.org/892393005/diff/70001/net/DEPS
File net/DEPS (right):

https://codereview.chromium.org/892393005/diff/70001/net/DEPS#newcode7
net/DEPS:7: "+third_party/mojo/src/mojo",
On 2015/02/05 23:58:36, Sam McNally wrote:
> +third_party/mojo/src/mojo/public
> 
> Most of //net should have no reason to use anything from mojo/edk.

Done.

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
File net/dns/host_resolver_service_impl.cc (right):

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl.cc:25: void Start();
On 2015/02/05 23:58:36, Sam McNally wrote:
> I'd prefer to not have a way to create a Job without starting it.

Done.

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl.cc:41: BoundNetLog net_log_;
On 2015/02/05 19:51:11, eroman wrote:
> This is not actually initialized. Rather may want to leave this out and pass
the
> dependency in where used.

Done.

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl.cc:51: for (Job* job : pending_jobs_) {
On 2015/02/05 19:51:11, eroman wrote:
> How about having cancellation be done as part of deletion?
> 
> Then STLDeleteElements(&pending_jobs_) would be sufficient, and explicit job
> cancel method not needed

Done.

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl.cc:96: int32_t result = resolver_->Resolve(
On 2015/02/05 19:51:11, eroman wrote:
> "int" is what the net stack uses, I suggest you stick to the same type here.
> Even though in practice we compile with 32-bit ints on all platforms at the
> moment.

Oops. Was stuck in the Mojo mentality of fixed-size types.

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl.cc:111: void
HostResolverServiceImpl::Job::OnResolveDone(int32_t result) {
On 2015/02/05 19:51:11, eroman wrote:
> why int32_t? The //net code defines error as "int"

Fixed.

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
File net/dns/host_resolver_service_impl.h (right):

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl.h:23: class HostResolverServiceImpl
On 2015/02/05 19:51:11, eroman wrote:
> I am not excited about the naming. "Service" is already a term used throughout
> network stack, including for instance "ProxyService".
> 
> I would be happier if "Mojo" appeared somewhere in the name, as in MojoService
> to make it clearer what these are.
> 
> (there may be an underlying net service, and then they may also be a
MojoService
> that wraps it).

https://memegen.googleplex.com/14366727
https://memegen.googleplex.com/5826145141391360
https://memegen.googleplex.com/6156198185271296

On a more serious note, Sam and I have come up with the following naming scheme:
- Mojo interfaces are just named HostResolver and ProxyResolver (no Service).
The rationale being that these are in separate namespaces and it's ok to clash.
On the flip side, there's no Mojo in the name, but I think that's implicit from
being in the net::interfaces namespace.
- The clients wrappers would be called HostResolverMojo and ProxyResolverMojo
(implementing the HostResolver and ProxyResolver interfaces respectively). This
fits with the existing convention for ProxyResolver, who's implementations
include ProxyResolverMac, ProxyReoslverV8, ProxyResolverV8Tracing, etc.
- The implementations of these interfaces should be called MojoHostResolverImpl
and MojoProxyResolverImpl. Mojo being somewhere in the name, and this scheme has
no potential clashes. We've ruled out HostResolverMojoImpl and
HostResolverImplMojo since these can easily be confused with the client
wrappers.

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl.h:26: explicit
HostResolverServiceImpl(HostResolver* resolver);
On 2015/02/05 19:51:11, eroman wrote:
> Please describe the lifetime expectations for |resovler|

Done.

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl.h:48: std::set<Job*> pending_jobs_;
On 2015/02/05 23:58:36, Sam McNally wrote:
> std::set<linked_ptr<Job>>

I tried that, but it ended up being fairly convoluted. Specifically, the
linked_ptr wasn't used anywhere, and you need to go from Job* to linked_ptr<Job>
and it just didn't look right.

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
File net/dns/host_resolver_service_impl_unittest.cc (right):

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl_unittest.cc:30: void
Bind(interfaces::HostResolverRequestClientPtr* ptr);
On 2015/02/05 23:58:36, Sam McNally wrote:
> Is there a reason for not doing this in the constructor?

Nope.

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl_unittest.cc:41: base::RunLoop* run_loop_;
On 2015/02/05 23:58:36, Sam McNally wrote:
> Store the base::Closure returned by RunLoop::QuitClosure() and create the run
> loop on the stack in WaitForResult().

Done.

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl_unittest.cc:61: ASSERT_FALSE(done_);
On 2015/02/05 23:58:36, Sam McNally wrote:
> Quitting the RunLoop first might avoid test failures getting stuck running a
> RunLoop.

Done.

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl_unittest.cc:75:
mojo::embedder::test::InitWithSimplePlatformSupport();
On 2015/02/05 23:58:36, Sam McNally wrote:
> This should be in net/test/run_all_unittests.cc.

Done.

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl_unittest.cc:84:
resolver_service_binding_.reset(
On 2015/02/05 23:58:36, Sam McNally wrote:
> It seems a bit weird for the Binding to not be a member of the impl class.

I thought about it, but right now, it's not clear what the lifecycle of the impl
is and whether we should use a strong or weak binding, so I left it out. We can
easily add it later.

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl_unittest.cc:91: uint8_t port,
On 2015/02/05 19:51:11, eroman wrote:
> Please make this 16bit for consistency

Oops.

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl_unittest.cc:102: void
WaitForOutstandingRequests(size_t num) {
On 2015/02/05 19:51:11, eroman wrote:
> Please explain this method, this doesn't look like a good approach.

Added a better explanation. I don't like it either, but I don't think there's a
better way without being more intrusive.

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl_unittest.cc:119:
interfaces::HostResolverServicePtr resolver_service_ptr_;
On 2015/02/05 23:58:36, Sam McNally wrote:
> For the most part I don't think it's valuable to use a real mojo pipe to make
> requests. Avoiding WaitForOutstandingRequests would be nicer.

I disagree. I think it's good to test this with Mojo given the interactions with
things like connection errors. It's also been extremely valuable in making the
implementation robust and has actually surfaced some errors.

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl_unittest.cc:241:
TEST_F(HostResolverServiceImplTest, DestroyClient) {
On 2015/02/05 23:58:36, Sam McNally wrote:
> I'm not sure what this is testing. As far as I can tell, destroying the client
> shouldn't affect any in-flight requests, especially the since the Binding is
> outside the class.

Destroying the client should (and does) cause a connection error, cancelling the
request. Also, make sure nothing crashes.

https://codereview.chromium.org/892393005/diff/70001/net/dns/host_resolver_se...
net/dns/host_resolver_service_impl_unittest.cc:277:
base::RunLoop().RunUntilIdle();
On 2015/02/05 23:58:36, Sam McNally wrote:
> Check that the client detected that its pipe closed?

Done.

https://codereview.chromium.org/892393005/diff/70001/net/dns/type_converters.h
File net/dns/type_converters.h (right):

https://codereview.chromium.org/892393005/diff/70001/net/dns/type_converters....
net/dns/type_converters.h:14: struct
TypeConverter<net::HostResolver::RequestInfo,
On 2015/02/05 19:51:11, eroman wrote:
> What are these about?

Type converters are the canonical Mojo way to convert between different types,
specifically to/from mojo structs. See
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/common/common...
for a few common examples.

[Sam McNally, 2015-02-06 07:12:32]

https://codereview.chromium.org/892393005/diff/90001/net/dns/mojo_host_resolv...
File net/dns/mojo_host_resolver_impl.cc (right):

https://codereview.chromium.org/892393005/diff/90001/net/dns/mojo_host_resolv...
net/dns/mojo_host_resolver_impl.cc:67: size_t num_erased =
pending_jobs_.erase(job);
Please use the iterator erase overload. Doing two lookups is wasteful.

https://codereview.chromium.org/892393005/diff/90001/net/dns/mojo_host_resolv...
File net/dns/mojo_host_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/892393005/diff/90001/net/dns/mojo_host_resolv...
net/dns/mojo_host_resolver_impl_unittest.cc:28: : done_(false), binding_(this) {
binding_(this, ptr)

https://codereview.chromium.org/892393005/diff/90001/net/dns/mojo_host_resolv...
net/dns/mojo_host_resolver_impl_unittest.cc:61: run_loop_quit_closure_.Run();
Before the ASSERT please.

https://codereview.chromium.org/892393005/diff/90001/net/dns/mojo_host_resolv...
net/dns/mojo_host_resolver_impl_unittest.cc:260: EXPECT_FALSE(client_ptr);
I don't think this is testing what you think it is. |client_ptr| is false
because you've Passed it. You need to set an error handler on
TestRequestClient's Binding to wait for the pipe to close.

[Anand Mistry (off Chromium), 2015-02-08 23:09:21]

https://codereview.chromium.org/892393005/diff/90001/net/dns/mojo_host_resolv...
File net/dns/mojo_host_resolver_impl.cc (right):

https://codereview.chromium.org/892393005/diff/90001/net/dns/mojo_host_resolv...
net/dns/mojo_host_resolver_impl.cc:67: size_t num_erased =
pending_jobs_.erase(job);
On 2015/02/06 07:12:32, Sam McNally wrote:
> Please use the iterator erase overload. Doing two lookups is wasteful.

I've gone back to having a Start() method. I realised that the current approach
creates an ABA problem.

https://codereview.chromium.org/892393005/diff/90001/net/dns/mojo_host_resolv...
File net/dns/mojo_host_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/892393005/diff/90001/net/dns/mojo_host_resolv...
net/dns/mojo_host_resolver_impl_unittest.cc:28: : done_(false), binding_(this) {
On 2015/02/06 07:12:32, Sam McNally wrote:
> binding_(this, ptr)

Done.

https://codereview.chromium.org/892393005/diff/90001/net/dns/mojo_host_resolv...
net/dns/mojo_host_resolver_impl_unittest.cc:61: run_loop_quit_closure_.Run();
On 2015/02/06 07:12:32, Sam McNally wrote:
> Before the ASSERT please.

Done.

https://codereview.chromium.org/892393005/diff/90001/net/dns/mojo_host_resolv...
net/dns/mojo_host_resolver_impl_unittest.cc:260: EXPECT_FALSE(client_ptr);
On 2015/02/06 07:12:32, Sam McNally wrote:
> I don't think this is testing what you think it is. |client_ptr| is false
> because you've Passed it. You need to set an error handler on
> TestRequestClient's Binding to wait for the pipe to close.

Done.

[Sam McNally, 2015-02-09 00:06:22]

lgtm

https://codereview.chromium.org/892393005/diff/130001/net/dns/mojo_host_resol...
File net/dns/mojo_host_resolver_impl.cc (right):

https://codereview.chromium.org/892393005/diff/130001/net/dns/mojo_host_resol...
net/dns/mojo_host_resolver_impl.cc:111: VLOG(1) << address.ToString();
Is this meant to be DVLOG like the others?

[Anand Mistry (off Chromium), 2015-02-09 04:22:50]

New patchsets have been uploaded after l-g-t-m from sammc@chromium.org

[Anand Mistry (off Chromium), 2015-02-09 04:23:40]

Also removed an unnecessary use of WeakPtr.

https://codereview.chromium.org/892393005/diff/130001/net/dns/mojo_host_resol...
File net/dns/mojo_host_resolver_impl.cc (right):

https://codereview.chromium.org/892393005/diff/130001/net/dns/mojo_host_resol...
net/dns/mojo_host_resolver_impl.cc:111: VLOG(1) << address.ToString();
On 2015/02/09 00:06:22, Sam McNally wrote:
> Is this meant to be DVLOG like the others?

Done.

[Anand Mistry (off Chromium), 2015-02-12 00:02:54]

eroman: Ping!

[eroman, 2015-02-12 00:09:03]

Apologies for the delay, looking now!

[eroman, 2015-02-12 02:30:19]

lgtm

https://codereview.chromium.org/892393005/diff/150001/net/dns/mojo_host_resol...
File net/dns/mojo_host_resolver_impl.cc (right):

https://codereview.chromium.org/892393005/diff/150001/net/dns/mojo_host_resol...
net/dns/mojo_host_resolver_impl.cc:113: if (result)
Compare result == OK instead of relying on success having value 0.

https://codereview.chromium.org/892393005/diff/150001/net/dns/mojo_host_resol...
net/dns/mojo_host_resolver_impl.cc:116: client_->ReportResult(0,
interfaces::AddressList::From(result_));
Pass |result| in place of 0 (don't want to hardcode particular error values...
although practically speaking OK is never going to change).

https://codereview.chromium.org/892393005/diff/150001/net/dns/type_converters.h
File net/dns/type_converters.h (right):

https://codereview.chromium.org/892393005/diff/150001/net/dns/type_converters...
net/dns/type_converters.h:5: #ifndef NET_DNS_TYPE_CONVERTERS_H_
Please call this file mojo_type_converters. Otherwise i don't think the filename
is particularly descriptive.

https://codereview.chromium.org/892393005/diff/150001/net/interfaces/host_res...
File net/interfaces/host_resolver_service.mojom (right):

https://codereview.chromium.org/892393005/diff/150001/net/interfaces/host_res...
net/interfaces/host_resolver_service.mojom:54: ReportResult(int32 error,
AddressList? result);
Thinking out loud here... 

There is an expectation that the network error codes will always fit within an
int32.

In practice this is likely to be safe since "int" is used and I doubt chrome
will be moving away from 32-bit ints anytime soon.

However what techniques can we use to better future proof this code (and other
code with similar expectations)? Since if these sorts of changes were to happen
we would silently truncate errors and unfortunately our compiler settings
wouldn't even warn about it.

[Anand Mistry (off Chromium), 2015-02-12 04:44:36]

New patchsets have been uploaded after l-g-t-m from eroman@chromium.org

[Anand Mistry (off Chromium), 2015-02-12 04:47:15]

https://codereview.chromium.org/892393005/diff/150001/net/dns/mojo_host_resol...
File net/dns/mojo_host_resolver_impl.cc (right):

https://codereview.chromium.org/892393005/diff/150001/net/dns/mojo_host_resol...
net/dns/mojo_host_resolver_impl.cc:113: if (result)
On 2015/02/12 02:30:18, eroman wrote:
> Compare result == OK instead of relying on success having value 0.

Done.

https://codereview.chromium.org/892393005/diff/150001/net/dns/mojo_host_resol...
net/dns/mojo_host_resolver_impl.cc:116: client_->ReportResult(0,
interfaces::AddressList::From(result_));
On 2015/02/12 02:30:18, eroman wrote:
> Pass |result| in place of 0 (don't want to hardcode particular error values...
> although practically speaking OK is never going to change).

Done.

https://codereview.chromium.org/892393005/diff/150001/net/dns/type_converters.h
File net/dns/type_converters.h (right):

https://codereview.chromium.org/892393005/diff/150001/net/dns/type_converters...
net/dns/type_converters.h:5: #ifndef NET_DNS_TYPE_CONVERTERS_H_
On 2015/02/12 02:30:18, eroman wrote:
> Please call this file mojo_type_converters. Otherwise i don't think the
filename
> is particularly descriptive.

Done.

https://codereview.chromium.org/892393005/diff/150001/net/interfaces/host_res...
File net/interfaces/host_resolver_service.mojom (right):

https://codereview.chromium.org/892393005/diff/150001/net/interfaces/host_res...
net/interfaces/host_resolver_service.mojom:54: ReportResult(int32 error,
AddressList? result);
On 2015/02/12 02:30:18, eroman wrote:
> Thinking out loud here... 
> 
> There is an expectation that the network error codes will always fit within an
> int32.
> 
> In practice this is likely to be safe since "int" is used and I doubt chrome
> will be moving away from 32-bit ints anytime soon.
> 
> However what techniques can we use to better future proof this code (and other
> code with similar expectations)? Since if these sorts of changes were to
happen
> we would silently truncate errors and unfortunately our compiler settings
> wouldn't even warn about it.

I think we have a couple of different options here. If we rely on ints being
32-bit, we could static_assert(sizeof(int) == sizeof(int32_t)). Of course, that
will won't help if we ever need to move to a 64-bit int platform. Another option
would be to use base::checked_cast(), which will check at run-time. That could
be appropriate for our uses. For error codes specifically, I wonder if we can
ensure that the enum values always fit in an int32_t, but maybe that's more
effort than it's worth.

[Anand Mistry (off Chromium), 2015-02-12 04:56:18]

amistry@chromium.org changed reviewers:
+ jamesr@chromium.org

[Anand Mistry (off Chromium), 2015-02-12 04:56:19]

jamesr: Need an LGTM from you according to presubmit check:
** Presubmit Messages **
Missing LGTM from OWNERS of dependencies added to DEPS:
    '+third_party/mojo/src/mojo/edk',
    '+third_party/mojo/src/mojo/public',

[jamesr, 2015-02-12 18:23:11]

New DEPS lgtm but some of the code is using clearly deprecated APIs.  Please
don't do that.

https://codereview.chromium.org/892393005/diff/190001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/892393005/diff/190001/net/BUILD.gn#newcode793
net/BUILD.gn:793: if (use_v8_in_net && !is_android) {
why !android?

https://codereview.chromium.org/892393005/diff/190001/net/dns/mojo_host_resol...
File net/dns/mojo_host_resolver_impl.h (right):

https://codereview.chromium.org/892393005/diff/190001/net/dns/mojo_host_resol...
net/dns/mojo_host_resolver_impl.h:24: : public
mojo::InterfaceImpl<interfaces::HostResolver> {
InterfaceImpl is deprecated (hence the DEPRECATED warning in the header) so you
shouldn't use it in new code.  Inherit directly from interfaces::HostResolver
and use a mojo::Binding<> or mojo::StrongBinding<>

[eroman, 2015-02-12 18:43:34]

https://codereview.chromium.org/892393005/diff/190001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/892393005/diff/190001/net/BUILD.gn#newcode808
net/BUILD.gn:808: static_library("type_converters") {
how about mojo_type_converts, to match the filenames.

[Anand Mistry (off Chromium), 2015-02-12 22:53:10]

New patchsets have been uploaded after l-g-t-m from jamesr@chromium.org

[Anand Mistry (off Chromium), 2015-02-12 22:54:09]

https://codereview.chromium.org/892393005/diff/190001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/892393005/diff/190001/net/BUILD.gn#newcode793
net/BUILD.gn:793: if (use_v8_in_net && !is_android) {
On 2015/02/12 18:23:11, jamesr wrote:
> why !android?

We're only targeting desktop chrome + chromeos. I don't know if proxy scripts
are even supported on Android.

https://codereview.chromium.org/892393005/diff/190001/net/BUILD.gn#newcode808
net/BUILD.gn:808: static_library("type_converters") {
On 2015/02/12 18:43:34, eroman wrote:
> how about mojo_type_converts, to match the filenames.

Done.

https://codereview.chromium.org/892393005/diff/190001/net/dns/mojo_host_resol...
File net/dns/mojo_host_resolver_impl.h (right):

https://codereview.chromium.org/892393005/diff/190001/net/dns/mojo_host_resol...
net/dns/mojo_host_resolver_impl.h:24: : public
mojo::InterfaceImpl<interfaces::HostResolver> {
On 2015/02/12 18:23:11, jamesr wrote:
> InterfaceImpl is deprecated (hence the DEPRECATED warning in the header) so
you
> shouldn't use it in new code.  Inherit directly from interfaces::HostResolver
> and use a mojo::Binding<> or mojo::StrongBinding<>

Oops. Actually, we do use Binding<>. Not sure why I put this here.

[eroman, 2015-02-12 22:57:04]

https://codereview.chromium.org/892393005/diff/190001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/892393005/diff/190001/net/BUILD.gn#newcode793
net/BUILD.gn:793: if (use_v8_in_net && !is_android) {
On 2015/02/12 22:54:08, Anand Mistry wrote:
> On 2015/02/12 18:23:11, jamesr wrote:
> > why !android?
> 
> We're only targeting desktop chrome + chromeos. I don't know if proxy scripts
> are even supported on Android.

The V8 PAC resolver is in fact not currently supported on Android.

It does kinda-sorta support PAC through use of the system proxy resolver (which
in turn is using Chromium's net implementation...)

[jamesr, 2015-02-12 22:58:25]

https://codereview.chromium.org/892393005/diff/210001/net/dns/mojo_host_resol...
File net/dns/mojo_host_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/892393005/diff/210001/net/dns/mojo_host_resol...
net/dns/mojo_host_resolver_impl_unittest.cc:28: explicit
TestRequestClient(interfaces::HostResolverRequestClientPtr* ptr)
it's better for this sort of class to take an InterfaceRequest<T> than a
InterfacePtr<T>*. The binding constructor from an InterfacePtr<T>* will probably
go away in the future

https://codereview.chromium.org/892393005/diff/210001/net/dns/mojo_host_resol...
net/dns/mojo_host_resolver_impl_unittest.cc:168: TestRequestClient
client(&client_ptr);
and here you want to say

TestRequestClient(GetProxy(&client_ptr));

that way the pipe allocation is here and a bit more explicit than having it
inside the mojo::Binding(..., InterfacePtr<>*) constructor call

[Anand Mistry (off Chromium), 2015-02-12 23:42:28]

https://codereview.chromium.org/892393005/diff/210001/net/dns/mojo_host_resol...
File net/dns/mojo_host_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/892393005/diff/210001/net/dns/mojo_host_resol...
net/dns/mojo_host_resolver_impl_unittest.cc:28: explicit
TestRequestClient(interfaces::HostResolverRequestClientPtr* ptr)
On 2015/02/12 22:58:25, jamesr wrote:
> it's better for this sort of class to take an InterfaceRequest<T> than a
> InterfacePtr<T>*. The binding constructor from an InterfacePtr<T>* will
probably
> go away in the future

Done.

https://codereview.chromium.org/892393005/diff/210001/net/dns/mojo_host_resol...
net/dns/mojo_host_resolver_impl_unittest.cc:168: TestRequestClient
client(&client_ptr);
On 2015/02/12 22:58:25, jamesr wrote:
> and here you want to say
> 
> TestRequestClient(GetProxy(&client_ptr));
> 
> that way the pipe allocation is here and a bit more explicit than having it
> inside the mojo::Binding(..., InterfacePtr<>*) constructor call

Done.

[jamesr, 2015-02-13 00:59:54]

lgtm

https://codereview.chromium.org/892393005/diff/230001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/892393005/diff/230001/net/BUILD.gn#newcode794
net/BUILD.gn:794: static_library("net_browser_services") {
these should be source_set, not static_library. static_library is pretty much
never what you want in GN

https://codereview.chromium.org/892393005/diff/230001/net/BUILD.gn#newcode808
net/BUILD.gn:808: static_library("mojo_type_converters") {
ditto

https://codereview.chromium.org/892393005/diff/230001/net/dns/mojo_host_resol...
File net/dns/mojo_host_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/892393005/diff/230001/net/dns/mojo_host_resol...
net/dns/mojo_host_resolver_impl_unittest.cc:164:
scoped_ptr<mojo::Binding<interfaces::HostResolver>> resolver_service_binding_;
this class would be much simpler if you had MojoHostResolverImpl be a member
variable instead of heap-allocated and initialized this in the c'tor. Remember
that gtest creates a new instance of the fixture class for each test case

https://codereview.chromium.org/892393005/diff/230001/net/test/run_all_unitte...
File net/test/run_all_unittests.cc (right):

https://codereview.chromium.org/892393005/diff/230001/net/test/run_all_unitte...
net/test/run_all_unittests.cc:77:
mojo::embedder::test::InitWithSimplePlatformSupport();
you need a corresponding Shutdown() call after running the test suite

[Anand Mistry (off Chromium), 2015-02-13 02:08:14]

New patchsets have been uploaded after l-g-t-m from jamesr@chromium.org

[Anand Mistry (off Chromium), 2015-02-13 02:29:09]

https://codereview.chromium.org/892393005/diff/230001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/892393005/diff/230001/net/BUILD.gn#newcode794
net/BUILD.gn:794: static_library("net_browser_services") {
On 2015/02/13 00:59:54, jamesr wrote:
> these should be source_set, not static_library. static_library is pretty much
> never what you want in GN

Done.

https://codereview.chromium.org/892393005/diff/230001/net/BUILD.gn#newcode808
net/BUILD.gn:808: static_library("mojo_type_converters") {
On 2015/02/13 00:59:54, jamesr wrote:
> ditto

Done.

https://codereview.chromium.org/892393005/diff/230001/net/dns/mojo_host_resol...
File net/dns/mojo_host_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/892393005/diff/230001/net/dns/mojo_host_resol...
net/dns/mojo_host_resolver_impl_unittest.cc:164:
scoped_ptr<mojo::Binding<interfaces::HostResolver>> resolver_service_binding_;
On 2015/02/13 00:59:54, jamesr wrote:
> this class would be much simpler if you had MojoHostResolverImpl be a member
> variable instead of heap-allocated and initialized this in the c'tor. Remember
> that gtest creates a new instance of the fixture class for each test case

Using a scoped_ptr<> here is intentional. In one test, I wanted to test deleting
the binding and making sure the interaction with Mojo is correctly handled.

https://codereview.chromium.org/892393005/diff/230001/net/test/run_all_unitte...
File net/test/run_all_unittests.cc (right):

https://codereview.chromium.org/892393005/diff/230001/net/test/run_all_unitte...
net/test/run_all_unittests.cc:77:
mojo::embedder::test::InitWithSimplePlatformSupport();
On 2015/02/13 00:59:54, jamesr wrote:
> you need a corresponding Shutdown() call after running the test suite

Unfortunately, my test starts CHECK failing after I add this and I have no idea
why:
[32965:32965:0213/130724:4171662561691:ERROR:test_embedder.cc(33)] Mojo embedder
shutdown: Leaking handle 3
[32965:32965:0213/130724:4171662561730:ERROR:test_embedder.cc(33)] Mojo embedder
shutdown: Leaking handle 4
[32965:32965:0213/130724:4171662561755:FATAL:message_pipe_dispatcher.cc(106)]
Check failed: !message_pipe_.
#0 0x0000007489ae base::debug::StackTrace::StackTrace()
#1 0x000000762182 logging::LogMessage::~LogMessage()
#2 0x0000005b6600 mojo::system::MessagePipeDispatcher::~MessagePipeDispatcher()
#3 0x0000005b6649 mojo::system::MessagePipeDispatcher::~MessagePipeDispatcher()
#4 0x00000059d73e base::RefCountedThreadSafe<>::DeleteInternal()
#5 0x00000059d705 base::DefaultRefCountedThreadSafeTraits<>::Destruct()
#6 0x00000059d6de base::RefCountedThreadSafe<>::Release()
#7 0x00000059d68c scoped_refptr<>::Release()
#8 0x00000059bded scoped_refptr<>::~scoped_refptr()
#9 0x0000005a6a2b mojo::system::HandleTable::Entry::~Entry()
#10 0x0000005a8d2c std::pair<>::~pair()
#11 0x0000005aa569 __gnu_cxx::new_allocator<>::destroy<>()
#12 0x0000005aa52b __gnu_cxx::hashtable<>::_M_delete_node()
#13 0x0000005ac11a __gnu_cxx::hashtable<>::clear()
#14 0x0000005ac059 __gnu_cxx::hashtable<>::~hashtable()
#15 0x0000005a8615 __gnu_cxx::hash_map<>::~hash_map()
#16 0x0000005a6a85 mojo::system::HandleTable::~HandleTable()
#17 0x0000005991a9 mojo::system::Core::~Core()
#18 0x0000005991e9 mojo::system::Core::~Core()
#19 0x0000005bb1e1 mojo::embedder::test::Shutdown()

None of the other Mojo uses in Chrome call Shutdown(). As a comparison, I added
this to device_unittests, extension_unittests and mojo_public_bindings_unittests
and they all check fail in exactly the same way. Looks like there's systematic
problem that everyone is getting wrong.

[Anand Mistry (off Chromium), 2015-02-13 03:05:38]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2015-02-13 03:06:53]

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

[commit-bot: I haz the power, 2015-02-13 04:19:23]

Committed patchset #14 (id:250001)

[commit-bot: I haz the power, 2015-02-13 04:20:00]

Patchset 14 (id:??) landed as
https://crrev.com/7e6ebfdc8f3536dc0246fcec458b27cdd836c3b5
Cr-Commit-Position: refs/heads/master@{#316160}