Fix chrome.socket.tcp API to use all addresses received from DNS.

Previously chrome.socket.tcp API was using only one IP address it
receives from DNS. Fixed it to pass all addresses to TCPClientSocket so
it attempts to connect to all of them before failing.

BUG=508210

Review URL: https://codereview.chromium.org/1229763002

Cr-Commit-Position: refs/heads/master@{#337962}

13 files changed, 49 insertions, 103 deletions

diff --git a/chrome/browser/extensions/api/socket/udp_socket_unittest.cc b/chrome/browser/extensions/api/socket/udp_socket_unittest.cc
index deced9d..cc3ebad 100644
--- a/chrome/browser/extensions/api/socket/udp_socket_unittest.cc
+++ b/chrome/browser/extensions/api/socket/udp_socket_unittest.cc
@@ -36,6 +36,12 @@ static void OnCompleted(int bytes_read,
 static const char test_message[] = "$$TESTMESSAGETESTMESSAGETESTMESSAGETEST$$";
 static const int test_message_length = arraysize(test_message);
 
+net::AddressList CreateAddressList(const char* address_string, int port) {
+  net::IPAddressNumber ip;
+  EXPECT_TRUE(net::ParseIPLiteralToNumber(address_string, &ip));
+  return net::AddressList::CreateFromIPAddress(ip, port);
+}
+
 static void OnSendCompleted(int result) {
   EXPECT_EQ(test_message_length, result);
 }
@@ -46,7 +52,8 @@ TEST(UDPSocketUnitTest, TestUDPSocketRecvFrom) {
 
   // Confirm that we can call two RecvFroms in quick succession without
   // triggering crbug.com/146606.
-  socket.Connect("127.0.0.1", 40000, base::Bind(&OnConnected));
+  socket.Connect(CreateAddressList("127.0.0.1", 40000),
+                 base::Bind(&OnConnected));
   socket.RecvFrom(4096, base::Bind(&OnCompleted));
   socket.RecvFrom(4096, base::Bind(&OnCompleted));
 }
@@ -72,14 +79,14 @@ TEST(UDPSocketUnitTest, TestUDPMulticastTimeToLive) {
   UDPSocket socket("abcdefghijklmnopqrst");
   EXPECT_NE(0, socket.SetMulticastTimeToLive(-1));  // Negative TTL shall fail.
   EXPECT_EQ(0, socket.SetMulticastTimeToLive(3));
-  socket.Connect(kGroup, 13333, base::Bind(&OnConnected));
+  socket.Connect(CreateAddressList(kGroup, 13333), base::Bind(&OnConnected));
 }
 
 TEST(UDPSocketUnitTest, TestUDPMulticastLoopbackMode) {
   const char kGroup[] = "237.132.100.17";
   UDPSocket socket("abcdefghijklmnopqrst");
   EXPECT_EQ(0, socket.SetMulticastLoopbackMode(false));
-  socket.Connect(kGroup, 13333, base::Bind(&OnConnected));
+  socket.Connect(CreateAddressList(kGroup, 13333), base::Bind(&OnConnected));
 }
 
 static void QuitMessageLoop() {
@@ -125,7 +132,8 @@ TEST(UDPSocketUnitTest, TestUDPMulticastRecv) {
 
   // Sender
   EXPECT_EQ(0, src.SetMulticastTimeToLive(0));
-  src.Connect(kGroup, kPort, base::Bind(&SendMulticastPacket, &src));
+  src.Connect(CreateAddressList(kGroup, kPort),
+              base::Bind(&SendMulticastPacket, &src));
 
   // If not received within the test action timeout, quit the message loop.
   io_loop.task_runner()->PostDelayedTask(
diff --git a/extensions/browser/api/socket/socket.cc b/extensions/browser/api/socket/socket.cc
index bf701fc..ed41ce0 100644
--- a/extensions/browser/api/socket/socket.cc
+++ b/extensions/browser/api/socket/socket.cc
@@ -114,18 +114,6 @@ bool Socket::StringAndPortToIPEndPoint(const std::string& ip_address_str,
   return true;
 }
 
-bool Socket::StringAndPortToAddressList(const std::string& ip_address_str,
-                                        uint16 port,
-                                        net::AddressList* address_list) {
-  DCHECK(address_list);
-  net::IPAddressNumber ip_number;
-  if (!net::ParseIPLiteralToNumber(ip_address_str, &ip_number))
-    return false;
-
-  *address_list = net::AddressList::CreateFromIPAddress(ip_number, port);
-  return true;
-}
-
 void Socket::IPEndPointToStringAndPort(const net::IPEndPoint& address,
                                        std::string* ip_address_str,
                                        uint16* port) {
diff --git a/extensions/browser/api/socket/socket.h b/extensions/browser/api/socket/socket.h
index 7f33b83..b548d23 100644
--- a/extensions/browser/api/socket/socket.h
+++ b/extensions/browser/api/socket/socket.h
@@ -70,8 +70,7 @@ class Socket : public ApiResource {
   // Note: |address| contains the resolved IP address, not the hostname of
   // the remote endpoint. In order to upgrade this socket to TLS, callers
   // must also supply the hostname of the endpoint via set_hostname().
-  virtual void Connect(const std::string& address,
-                       uint16 port,
+  virtual void Connect(const net::AddressList& address,
                        const CompletionCallback& callback) = 0;
   virtual void Disconnect() = 0;
   virtual int Bind(const std::string& address, uint16 port) = 0;
@@ -90,8 +89,7 @@ class Socket : public ApiResource {
                         const RecvFromCompletionCallback& callback) = 0;
   virtual void SendTo(scoped_refptr<net::IOBuffer> io_buffer,
                       int byte_count,
-                      const std::string& address,
-                      uint16 port,
+                      const net::IPEndPoint& address,
                       const CompletionCallback& callback) = 0;
 
   virtual bool SetKeepAlive(bool enable, int delay);
@@ -109,9 +107,6 @@ class Socket : public ApiResource {
 
   virtual SocketType GetSocketType() const = 0;
 
-  static bool StringAndPortToAddressList(const std::string& ip_address_str,
-                                         uint16 port,
-                                         net::AddressList* address_list);
   static bool StringAndPortToIPEndPoint(const std::string& ip_address_str,
                                         uint16 port,
                                         net::IPEndPoint* ip_end_point);
diff --git a/extensions/browser/api/socket/socket_api.cc b/extensions/browser/api/socket/socket_api.cc
index 838b770..0a173fd 100644
--- a/extensions/browser/api/socket/socket_api.cc
+++ b/extensions/browser/api/socket/socket_api.cc
@@ -169,11 +169,11 @@ void SocketAsyncApiFunction::OnFirewallHoleOpened(
 #endif  // OS_CHROMEOS
 
 SocketExtensionWithDnsLookupFunction::SocketExtensionWithDnsLookupFunction()
-    : resource_context_(NULL),
-      request_handle_(new net::HostResolver::RequestHandle),
-      addresses_(new net::AddressList) {}
+    : resource_context_(NULL) {
+}
 
-SocketExtensionWithDnsLookupFunction::~SocketExtensionWithDnsLookupFunction() {}
+SocketExtensionWithDnsLookupFunction::~SocketExtensionWithDnsLookupFunction() {
+}
 
 bool SocketExtensionWithDnsLookupFunction::PrePrepare() {
   if (!SocketAsyncApiFunction::PrePrepare())
@@ -183,25 +183,19 @@ bool SocketExtensionWithDnsLookupFunction::PrePrepare() {
 }
 
 void SocketExtensionWithDnsLookupFunction::StartDnsLookup(
-    const std::string& hostname) {
+    const net::HostPortPair& host_port_pair) {
   net::HostResolver* host_resolver =
       HostResolverWrapper::GetInstance()->GetHostResolver(resource_context_);
   DCHECK(host_resolver);
 
-  // Yes, we are passing zero as the port. There are some interesting but not
-  // presently relevant reasons why HostResolver asks for the port of the
-  // hostname you'd like to resolve, even though it doesn't use that value in
-  // determining its answer.
-  net::HostPortPair host_port_pair(hostname, 0);
+  // RequestHandle is not needed because we never need to cancel requests.
+  net::HostResolver::RequestHandle request_handle;
 
   net::HostResolver::RequestInfo request_info(host_port_pair);
   int resolve_result = host_resolver->Resolve(
-      request_info,
-      net::DEFAULT_PRIORITY,
-      addresses_.get(),
+      request_info, net::DEFAULT_PRIORITY, &addresses_,
       base::Bind(&SocketExtensionWithDnsLookupFunction::OnDnsLookup, this),
-      request_handle_.get(),
-      net::BoundNetLog());
+      &request_handle, net::BoundNetLog());
 
   if (resolve_result != net::ERR_IO_PENDING)
     OnDnsLookup(resolve_result);
@@ -209,8 +203,7 @@ void SocketExtensionWithDnsLookupFunction::StartDnsLookup(
 
 void SocketExtensionWithDnsLookupFunction::OnDnsLookup(int resolve_result) {
   if (resolve_result == net::OK) {
-    DCHECK(!addresses_->empty());
-    resolved_address_ = addresses_->front().ToStringWithoutPort();
+    DCHECK(!addresses_.empty());
   } else {
     error_ = kDnsLookupFailedError;
   }
@@ -312,7 +305,7 @@ void SocketConnectFunction::AsyncWorkStart() {
     return;
   }
 
-  StartDnsLookup(hostname_);
+  StartDnsLookup(net::HostPortPair(hostname_, port_));
 }
 
 void SocketConnectFunction::AfterDnsLookup(int lookup_result) {
@@ -333,8 +326,7 @@ void SocketConnectFunction::StartConnect() {
     return;
   }
 
-  socket->Connect(resolved_address_,
-                  port_,
+  socket->Connect(addresses_,
                   base::Bind(&SocketConnectFunction::OnConnect, this));
 }
 
@@ -637,7 +629,7 @@ void SocketSendToFunction::AsyncWorkStart() {
     }
   }
 
-  StartDnsLookup(hostname_);
+  StartDnsLookup(net::HostPortPair(hostname_, port_));
 }
 
 void SocketSendToFunction::AfterDnsLookup(int lookup_result) {
@@ -658,10 +650,7 @@ void SocketSendToFunction::StartSendTo() {
     return;
   }
 
-  socket->SendTo(io_buffer_,
-                 io_buffer_size_,
-                 resolved_address_,
-                 port_,
+  socket->SendTo(io_buffer_, io_buffer_size_, addresses_.front(),
                  base::Bind(&SocketSendToFunction::OnCompleted, this));
 }
 
diff --git a/extensions/browser/api/socket/socket_api.h b/extensions/browser/api/socket/socket_api.h
index d6e4736..9f4fda7 100644
--- a/extensions/browser/api/socket/socket_api.h
+++ b/extensions/browser/api/socket/socket_api.h
@@ -147,19 +147,16 @@ class SocketExtensionWithDnsLookupFunction : public SocketAsyncApiFunction {
   // AsyncApiFunction:
   bool PrePrepare() override;
 
-  void StartDnsLookup(const std::string& hostname);
+  void StartDnsLookup(const net::HostPortPair& host_port_pair);
   virtual void AfterDnsLookup(int lookup_result) = 0;
 
-  std::string resolved_address_;
+  net::AddressList addresses_;
 
  private:
   void OnDnsLookup(int resolve_result);
 
   // Weak pointer to the resource context.
   content::ResourceContext* resource_context_;
-
-  scoped_ptr<net::HostResolver::RequestHandle> request_handle_;
-  scoped_ptr<net::AddressList> addresses_;
 };
 
 class SocketCreateFunction : public SocketAsyncApiFunction {
diff --git a/extensions/browser/api/socket/tcp_socket.cc b/extensions/browser/api/socket/tcp_socket.cc
index 4863602..8d5257b 100644
--- a/extensions/browser/api/socket/tcp_socket.cc
+++ b/extensions/browser/api/socket/tcp_socket.cc
@@ -77,8 +77,7 @@ TCPSocket* TCPSocket::CreateServerSocketForTesting(
 
 TCPSocket::~TCPSocket() { Disconnect(); }
 
-void TCPSocket::Connect(const std::string& address,
-                        uint16 port,
+void TCPSocket::Connect(const net::AddressList& address,
                         const CompletionCallback& callback) {
   DCHECK(!callback.is_null());
 
@@ -95,14 +94,8 @@ void TCPSocket::Connect(const std::string& address,
     if (is_connected_)
       break;
 
-    net::AddressList address_list;
-    if (!StringAndPortToAddressList(address, port, &address_list)) {
-      result = net::ERR_ADDRESS_INVALID;
-      break;
-    }
-
     socket_.reset(
-        new net::TCPClientSocket(address_list, NULL, net::NetLog::Source()));
+        new net::TCPClientSocket(address, NULL, net::NetLog::Source()));
 
     connect_callback_ = callback;
     result = socket_->Connect(
@@ -170,8 +163,7 @@ void TCPSocket::RecvFrom(int count,
 
 void TCPSocket::SendTo(scoped_refptr<net::IOBuffer> io_buffer,
                        int byte_count,
-                       const std::string& address,
-                       uint16 port,
+                       const net::IPEndPoint& address,
                        const CompletionCallback& callback) {
   callback.Run(net::ERR_FAILED);
 }
diff --git a/extensions/browser/api/socket/tcp_socket.h b/extensions/browser/api/socket/tcp_socket.h
index 396eff7..40dd5fd 100644
--- a/extensions/browser/api/socket/tcp_socket.h
+++ b/extensions/browser/api/socket/tcp_socket.h
@@ -29,8 +29,7 @@ class TCPSocket : public Socket {
 
   ~TCPSocket() override;
 
-  void Connect(const std::string& address,
-               uint16 port,
+  void Connect(const net::AddressList& address,
                const CompletionCallback& callback) override;
   void Disconnect() override;
   int Bind(const std::string& address, uint16 port) override;
@@ -38,8 +37,7 @@ class TCPSocket : public Socket {
   void RecvFrom(int count, const RecvFromCompletionCallback& callback) override;
   void SendTo(scoped_refptr<net::IOBuffer> io_buffer,
               int byte_count,
-              const std::string& address,
-              uint16 port,
+              const net::IPEndPoint& address,
               const CompletionCallback& callback) override;
   bool SetKeepAlive(bool enable, int delay) override;
   bool SetNoDelay(bool no_delay) override;
diff --git a/extensions/browser/api/socket/tls_socket.cc b/extensions/browser/api/socket/tls_socket.cc
index 4b34c4b..1853d31 100644
--- a/extensions/browser/api/socket/tls_socket.cc
+++ b/extensions/browser/api/socket/tls_socket.cc
@@ -75,8 +75,7 @@ TLSSocket::~TLSSocket() {
   Disconnect();
 }
 
-void TLSSocket::Connect(const std::string& address,
-                        uint16 port,
+void TLSSocket::Connect(const net::AddressList& address,
                         const CompletionCallback& callback) {
   callback.Run(net::ERR_CONNECTION_FAILED);
 }
diff --git a/extensions/browser/api/socket/tls_socket.h b/extensions/browser/api/socket/tls_socket.h
index f0f9029..32e2161 100644
--- a/extensions/browser/api/socket/tls_socket.h
+++ b/extensions/browser/api/socket/tls_socket.h
@@ -45,8 +45,7 @@ class TLSSocket : public ResumableTCPSocket {
   // inner net::StreamSocket. The remaining few do actual TLS work.
 
   // Fails.
-  void Connect(const std::string& address,
-               uint16 port,
+  void Connect(const net::AddressList& address,
                const CompletionCallback& callback) override;
   // Forwards.
   void Disconnect() override;
diff --git a/extensions/browser/api/socket/udp_socket.cc b/extensions/browser/api/socket/udp_socket.cc
index 2822046..47eae2d 100644
--- a/extensions/browser/api/socket/udp_socket.cc
+++ b/extensions/browser/api/socket/udp_socket.cc
@@ -35,20 +35,16 @@ UDPSocket::UDPSocket(const std::string& owner_extension_id)
 
 UDPSocket::~UDPSocket() { Disconnect(); }
 
-void UDPSocket::Connect(const std::string& address,
-                        uint16 port,
+void UDPSocket::Connect(const net::AddressList& address,
                         const CompletionCallback& callback) {
   int result = net::ERR_CONNECTION_FAILED;
   do {
     if (is_connected_)
       break;
 
-    net::IPEndPoint ip_end_point;
-    if (!StringAndPortToIPEndPoint(address, port, &ip_end_point)) {
-      result = net::ERR_ADDRESS_INVALID;
-      break;
-    }
-
+    // UDP API only connects to the first address received from DNS so
+    // connection may not work even if other addresses are reachable.
+    net::IPEndPoint ip_end_point = address.front();
     result = socket_.Open(ip_end_point.GetFamily());
     if (result != net::OK)
       break;
@@ -177,8 +173,7 @@ void UDPSocket::RecvFrom(int count,
 
 void UDPSocket::SendTo(scoped_refptr<net::IOBuffer> io_buffer,
                        int byte_count,
-                       const std::string& address,
-                       uint16 port,
+                       const net::IPEndPoint& address,
                        const CompletionCallback& callback) {
   DCHECK(!callback.is_null());
 
@@ -193,21 +188,13 @@ void UDPSocket::SendTo(scoped_refptr<net::IOBuffer> io_buffer,
 
   int result = net::ERR_FAILED;
   do {
-    net::IPEndPoint ip_end_point;
-    if (!StringAndPortToIPEndPoint(address, port, &ip_end_point)) {
-      result = net::ERR_ADDRESS_INVALID;
-      break;
-    }
-
     if (!socket_.is_connected()) {
       result = net::ERR_SOCKET_NOT_CONNECTED;
       break;
     }
 
     result = socket_.SendTo(
-        io_buffer.get(),
-        byte_count,
-        ip_end_point,
+        io_buffer.get(), byte_count, address,
         base::Bind(&UDPSocket::OnSendToComplete, base::Unretained(this)));
   } while (false);
 
diff --git a/extensions/browser/api/socket/udp_socket.h b/extensions/browser/api/socket/udp_socket.h
index 9b63b2c..d43bc27 100644
--- a/extensions/browser/api/socket/udp_socket.h
+++ b/extensions/browser/api/socket/udp_socket.h
@@ -18,8 +18,7 @@ class UDPSocket : public Socket {
   explicit UDPSocket(const std::string& owner_extension_id);
   ~UDPSocket() override;
 
-  void Connect(const std::string& address,
-               uint16 port,
+  void Connect(const net::AddressList& address,
                const CompletionCallback& callback) override;
   void Disconnect() override;
   int Bind(const std::string& address, uint16 port) override;
@@ -27,8 +26,7 @@ class UDPSocket : public Socket {
   void RecvFrom(int count, const RecvFromCompletionCallback& callback) override;
   void SendTo(scoped_refptr<net::IOBuffer> io_buffer,
               int byte_count,
-              const std::string& address,
-              uint16 port,
+              const net::IPEndPoint& address,
               const CompletionCallback& callback) override;
 
   bool IsConnected() override;
diff --git a/extensions/browser/api/sockets_tcp/sockets_tcp_api.cc b/extensions/browser/api/sockets_tcp/sockets_tcp_api.cc
index f9e87ec..3523f1b 100644
--- a/extensions/browser/api/sockets_tcp/sockets_tcp_api.cc
+++ b/extensions/browser/api/sockets_tcp/sockets_tcp_api.cc
@@ -279,7 +279,7 @@ void SocketsTcpConnectFunction::AsyncWorkStart() {
     return;
   }
 
-  StartDnsLookup(params_->peer_address);
+  StartDnsLookup(net::HostPortPair(params_->peer_address, params_->peer_port));
 }
 
 void SocketsTcpConnectFunction::AfterDnsLookup(int lookup_result) {
@@ -298,8 +298,7 @@ void SocketsTcpConnectFunction::StartConnect() {
     return;
   }
 
-  socket->Connect(resolved_address_,
-                  params_->peer_port,
+  socket->Connect(addresses_,
                   base::Bind(&SocketsTcpConnectFunction::OnCompleted, this));
 }
 
diff --git a/extensions/browser/api/sockets_udp/sockets_udp_api.cc b/extensions/browser/api/sockets_udp/sockets_udp_api.cc
index 5cdcee6..cf5d277 100644
--- a/extensions/browser/api/sockets_udp/sockets_udp_api.cc
+++ b/extensions/browser/api/sockets_udp/sockets_udp_api.cc
@@ -244,7 +244,7 @@ void SocketsUdpSendFunction::AsyncWorkStart() {
     return;
   }
 
-  StartDnsLookup(params_->address);
+  StartDnsLookup(net::HostPortPair(params_->address, params_->port));
 }
 
 void SocketsUdpSendFunction::AfterDnsLookup(int lookup_result) {
@@ -263,10 +263,7 @@ void SocketsUdpSendFunction::StartSendTo() {
     return;
   }
 
-  socket->SendTo(io_buffer_,
-                 io_buffer_size_,
-                 resolved_address_,
-                 params_->port,
+  socket->SendTo(io_buffer_, io_buffer_size_, addresses_.front(),
                  base::Bind(&SocketsUdpSendFunction::OnCompleted, this));
 }