Implement browser-side host resolver Mojo service.

net/dns/host_resolver_service_impl_unittest.cc

Index: net/dns/host_resolver_service_impl_unittest.cc
diff --git a/net/dns/host_resolver_service_impl_unittest.cc b/net/dns/host_resolver_service_impl_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..612d8b8d17db12ceb82eacfd9fe350bb4178d48b
--- /dev/null
+++ b/net/dns/host_resolver_service_impl_unittest.cc
@@ -0,0 +1,280 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/dns/host_resolver_service_impl.h"
+
+#include <string>
+
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "base/time/time.h"
+#include "net/base/address_list.h"
+#include "net/base/net_errors.h"
+#include "net/base/net_util.h"
+#include "net/dns/mock_host_resolver.h"
+#include "net/dns/type_converters.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/mojo/src/mojo/edk/embedder/test_embedder.h"
+#include "third_party/mojo/src/mojo/public/cpp/bindings/binding.h"
+
+namespace net {
+
+namespace {
+
+class TestRequestClient : public interfaces::HostResolverRequestClient {
+ public:
+  TestRequestClient() : done_(false), run_loop_(nullptr), binding_(this) {}
+
+  void Bind(interfaces::HostResolverRequestClientPtr* ptr);

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

Is there a reason for not doing this in the constructor?

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

Nope.

+  void WaitForResult(base::RunLoop* run_loop);
+
+  // Overridden from interfaces::HostResolverRequestClient.
+  void ReportResult(int32_t error, interfaces::AddressListPtr results) override;
+
+  int32_t error_;
+  interfaces::AddressListPtr results_;
+
+ private:
+  bool done_;
+  base::RunLoop* run_loop_;

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

Store the base::Closure returned by RunLoop::QuitClosure() and create the run
loop on the stack in WaitForResult().

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

Done.

+
+  mojo::Binding<interfaces::HostResolverRequestClient> binding_;
+};
+
+void TestRequestClient::Bind(interfaces::HostResolverRequestClientPtr* ptr) {
+  binding_.Bind(ptr);
+}
+
+void TestRequestClient::WaitForResult(base::RunLoop* run_loop) {
+  if (done_)
+    return;
+
+  run_loop_ = run_loop;
+  run_loop->Run();
+  ASSERT_TRUE(done_);
+}
+
+void TestRequestClient::ReportResult(int32_t error,
+                                     interfaces::AddressListPtr results) {
+  ASSERT_FALSE(done_);

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

Quitting the RunLoop first might avoid test failures getting stuck running a
RunLoop.

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

Done.

+  error_ = error;
+  results_ = results.Pass();
+  done_ = true;
+  if (run_loop_) {
+    run_loop_->Quit();
+  }
+}
+
+}  // namespace
+
+class HostResolverServiceImplTest : public testing::Test {
+ protected:
+  static void SetUpTestCase() {
+    mojo::embedder::test::InitWithSimplePlatformSupport();

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

This should be in net/test/run_all_unittests.cc.

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

Done.

+  }
+
+  void SetUp() override {
+    mock_host_resolver_.rules()->AddRule("example.com", "1.2.3.4");
+    mock_host_resolver_.rules()->AddRule("chromium.org", "8.8.8.8");
+    mock_host_resolver_.rules()->AddSimulatedFailure("failure.fail");
+
+    resolver_service_.reset(new HostResolverServiceImpl(&mock_host_resolver_));
+    resolver_service_binding_.reset(

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

It seems a bit weird for the Binding to not be a member of the impl class.

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

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.

+        new mojo::Binding<interfaces::HostResolverService>(
+            resolver_service_.get()));
+    resolver_service_binding_->Bind(&resolver_service_ptr_);
+  }
+
+  interfaces::HostResolverRequestInfoPtr CreateRequest(const std::string& host,
+                                                       uint8_t port,

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

Please make this 16bit for consistency

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

Oops.

+                                                       bool is_my_ip_address) {
+    interfaces::HostResolverRequestInfoPtr request =
+        interfaces::HostResolverRequestInfo::New();
+    request->host = host;
+    request->port = port;
+    request->address_family = interfaces::ADDRESS_FAMILY_IPV4;
+    request->is_my_ip_address = is_my_ip_address;
+    return request.Pass();
+  }
+
+  void WaitForOutstandingRequests(size_t num) {

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

Please explain this method, this doesn't look like a good approach.

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

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

+    // Can't just use RunLoop::RunUntilIdle() since Mojo internally may be using
+    // another message loop.
+    while (resolver_service_->OutstandingRequestsForTesting() < num) {
+      base::RunLoop run_loop;
+      base::MessageLoop::current()->PostDelayedTask(
+          FROM_HERE, run_loop.QuitClosure(),
+          base::TimeDelta::FromMilliseconds(10));
+      run_loop.Run();
+    }
+  }
+
+  MockHostResolver mock_host_resolver_;
+  scoped_ptr<HostResolverServiceImpl> resolver_service_;
+
+  scoped_ptr<mojo::Binding<interfaces::HostResolverService>>
+      resolver_service_binding_;
+  interfaces::HostResolverServicePtr resolver_service_ptr_;

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

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.

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

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.

+};
+
+TEST_F(HostResolverServiceImplTest, Resolve) {
+  TestRequestClient client;
+  interfaces::HostResolverRequestClientPtr client_ptr;
+  client.Bind(&client_ptr);
+
+  interfaces::HostResolverRequestInfoPtr request =
+      CreateRequest("example.com", 80, false);
+  resolver_service_ptr_->Resolve(request.Pass(), client_ptr.Pass());
+  base::RunLoop run_loop;
+  client.WaitForResult(&run_loop);
+
+  EXPECT_EQ(net::OK, client.error_);
+  AddressList address_list = (*client.results_).To<AddressList>();
+  EXPECT_EQ(1U, address_list.size());
+  EXPECT_EQ("1.2.3.4:80", address_list[0].ToString());
+}
+
+TEST_F(HostResolverServiceImplTest, ResolveSynchronous) {
+  TestRequestClient client;
+  interfaces::HostResolverRequestClientPtr client_ptr;
+  client.Bind(&client_ptr);
+
+  mock_host_resolver_.set_synchronous_mode(true);
+
+  interfaces::HostResolverRequestInfoPtr request =
+      CreateRequest("example.com", 80, false);
+  resolver_service_ptr_->Resolve(request.Pass(), client_ptr.Pass());
+  base::RunLoop run_loop;
+  client.WaitForResult(&run_loop);
+
+  EXPECT_EQ(net::OK, client.error_);
+  AddressList address_list = (*client.results_).To<AddressList>();
+  EXPECT_EQ(1U, address_list.size());
+  EXPECT_EQ("1.2.3.4:80", address_list[0].ToString());
+}
+
+TEST_F(HostResolverServiceImplTest, ResolveMultiple) {
+  TestRequestClient client1;
+  interfaces::HostResolverRequestClientPtr client1_ptr;
+  client1.Bind(&client1_ptr);
+  TestRequestClient client2;
+  interfaces::HostResolverRequestClientPtr client2_ptr;
+  client2.Bind(&client2_ptr);
+
+  mock_host_resolver_.set_ondemand_mode(true);
+
+  interfaces::HostResolverRequestInfoPtr request1 =
+      CreateRequest("example.com", 80, false);
+  resolver_service_ptr_->Resolve(request1.Pass(), client1_ptr.Pass());
+  interfaces::HostResolverRequestInfoPtr request2 =
+      CreateRequest("chromium.org", 80, false);
+  resolver_service_ptr_->Resolve(request2.Pass(), client2_ptr.Pass());
+  WaitForOutstandingRequests(2);
+  mock_host_resolver_.ResolveAllPending();
+
+  base::RunLoop run_loop1;
+  client1.WaitForResult(&run_loop1);
+  base::RunLoop run_loop2;
+  client2.WaitForResult(&run_loop2);
+
+  EXPECT_EQ(net::OK, client1.error_);
+  AddressList address_list = (*client1.results_).To<AddressList>();
+  EXPECT_EQ(1U, address_list.size());
+  EXPECT_EQ("1.2.3.4:80", address_list[0].ToString());
+  EXPECT_EQ(net::OK, client2.error_);
+  address_list = (*client2.results_).To<AddressList>();
+  EXPECT_EQ(1U, address_list.size());
+  EXPECT_EQ("8.8.8.8:80", address_list[0].ToString());
+}
+
+TEST_F(HostResolverServiceImplTest, ResolveDuplicate) {
+  TestRequestClient client1;
+  interfaces::HostResolverRequestClientPtr client1_ptr;
+  client1.Bind(&client1_ptr);
+  TestRequestClient client2;
+  interfaces::HostResolverRequestClientPtr client2_ptr;
+  client2.Bind(&client2_ptr);
+
+  mock_host_resolver_.set_ondemand_mode(true);
+
+  interfaces::HostResolverRequestInfoPtr request1 =
+      CreateRequest("example.com", 80, false);
+  resolver_service_ptr_->Resolve(request1.Pass(), client1_ptr.Pass());
+  interfaces::HostResolverRequestInfoPtr request2 =
+      CreateRequest("example.com", 80, false);
+  resolver_service_ptr_->Resolve(request2.Pass(), client2_ptr.Pass());
+  WaitForOutstandingRequests(2);
+  mock_host_resolver_.ResolveAllPending();
+
+  base::RunLoop run_loop1;
+  client1.WaitForResult(&run_loop1);
+  base::RunLoop run_loop2;
+  client2.WaitForResult(&run_loop2);
+
+  EXPECT_EQ(net::OK, client1.error_);
+  AddressList address_list = (*client1.results_).To<AddressList>();
+  EXPECT_EQ(1U, address_list.size());
+  EXPECT_EQ("1.2.3.4:80", address_list[0].ToString());
+  EXPECT_EQ(net::OK, client2.error_);
+  address_list = (*client2.results_).To<AddressList>();
+  EXPECT_EQ(1U, address_list.size());
+  EXPECT_EQ("1.2.3.4:80", address_list[0].ToString());
+}
+
+TEST_F(HostResolverServiceImplTest, ResolveFailure) {
+  TestRequestClient client;
+  interfaces::HostResolverRequestClientPtr client_ptr;
+  client.Bind(&client_ptr);
+
+  interfaces::HostResolverRequestInfoPtr request =
+      CreateRequest("failure.fail", 80, false);
+  resolver_service_ptr_->Resolve(request.Pass(), client_ptr.Pass());
+  base::RunLoop run_loop;
+  client.WaitForResult(&run_loop);
+
+  EXPECT_EQ(net::ERR_NAME_NOT_RESOLVED, client.error_);
+  EXPECT_TRUE(client.results_.is_null());
+}
+
+TEST_F(HostResolverServiceImplTest, DestroyClient) {

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

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.

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

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

+  scoped_ptr<TestRequestClient> client(new TestRequestClient);
+  interfaces::HostResolverRequestClientPtr client_ptr;
+  client->Bind(&client_ptr);
+
+  mock_host_resolver_.set_ondemand_mode(true);
+
+  interfaces::HostResolverRequestInfoPtr request =
+      CreateRequest("example.com", 80, false);
+  resolver_service_ptr_->Resolve(request.Pass(), client_ptr.Pass());
+  WaitForOutstandingRequests(1);
+
+  client.reset();
+  base::RunLoop().RunUntilIdle();
+
+  mock_host_resolver_.ResolveAllPending();
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(HostResolverServiceImplTest, DestroyService) {
+  TestRequestClient client;
+  interfaces::HostResolverRequestClientPtr client_ptr;
+  client.Bind(&client_ptr);
+
+  mock_host_resolver_.set_ondemand_mode(true);
+
+  interfaces::HostResolverRequestInfoPtr request =
+      CreateRequest("example.com", 80, false);
+  resolver_service_ptr_->Resolve(request.Pass(), client_ptr.Pass());
+  WaitForOutstandingRequests(1);
+
+  resolver_service_binding_.reset();
+  resolver_service_.reset();
+  base::RunLoop().RunUntilIdle();
+
+  mock_host_resolver_.ResolveAllPending();
+  base::RunLoop().RunUntilIdle();

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

Check that the client detected that its pipe closed?

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

Done.

+}
+
+}  // namespace net