Prefer IPv4 in the ConnectBackupJob to work around broken IPv6 networks

where connects take a long time to fail.

R=eroman@chromium.org,mbelshe@chromium.org
BUG=none
TEST=none

Review URL: http://codereview.chromium.org/6905080

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@84251 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 210 insertions, 54 deletions

diff --git a/net/base/address_list.cc b/net/base/address_list.cc
index 83df606..1804736 100644
--- a/net/base/address_list.cc
+++ b/net/base/address_list.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -22,56 +22,6 @@ char* do_strdup(const char* src) {
 #endif
 }
 
-// Make a copy of |info| (the dynamically-allocated parts are copied as well).
-// If |recursive| is true, chained entries via ai_next are copied too.
-// Copy returned by this function should be deleted using
-// DeleteCopyOfAddrinfo(), and NOT freeaddrinfo().
-struct addrinfo* CreateCopyOfAddrinfo(const struct addrinfo* info,
-                                      bool recursive) {
-  DCHECK(info);
-  struct addrinfo* copy = new addrinfo;
-
-  // Copy all the fields (some of these are pointers, we will fix that next).
-  memcpy(copy, info, sizeof(addrinfo));
-
-  // ai_canonname is a NULL-terminated string.
-  if (info->ai_canonname) {
-    copy->ai_canonname = do_strdup(info->ai_canonname);
-  }
-
-  // ai_addr is a buffer of length ai_addrlen.
-  if (info->ai_addr) {
-    copy->ai_addr = reinterpret_cast<sockaddr *>(new char[info->ai_addrlen]);
-    memcpy(copy->ai_addr, info->ai_addr, info->ai_addrlen);
-  }
-
-  // Recursive copy.
-  if (recursive && info->ai_next)
-    copy->ai_next = CreateCopyOfAddrinfo(info->ai_next, recursive);
-  else
-    copy->ai_next = NULL;
-
-  return copy;
-}
-
-// Free an addrinfo that was created by CreateCopyOfAddrinfo().
-void FreeMyAddrinfo(struct addrinfo* info) {
-  DCHECK(info);
-  if (info->ai_canonname)
-    free(info->ai_canonname);  // Allocated by strdup.
-
-  if (info->ai_addr)
-    delete [] reinterpret_cast<char*>(info->ai_addr);
-
-  struct addrinfo* next = info->ai_next;
-
-  delete info;
-
-  // Recursive free.
-  if (next)
-    FreeMyAddrinfo(next);
-}
-
 // Assign the port for all addresses in the list.
 void SetPortRecursive(struct addrinfo* info, int port) {
   uint16* port_field = GetPortFieldFromAddrinfo(info);
@@ -283,12 +233,12 @@ AddressList::Data::Data(struct addrinfo* ai, bool is_system_created)
 }
 
 AddressList::Data::~Data() {
-  // Call either freeaddrinfo(head), or FreeMyAddrinfo(head), depending who
-  // created the data.
+  // Call either freeaddrinfo(head), or FreeCopyOfAddrinfo(head), depending on
+  // who created the data.
   if (is_system_created)
     freeaddrinfo(head);
   else
-    FreeMyAddrinfo(head);
+    FreeCopyOfAddrinfo(head);
 }
 
 }  // namespace net
diff --git a/net/base/net_util.cc b/net/base/net_util.cc
index 5c7aab4..e97cb7f 100644
--- a/net/base/net_util.cc
+++ b/net/base/net_util.cc
@@ -911,6 +911,14 @@ void AppendFormattedComponent(const std::string& spec,
   }
 }
 
+char* do_strdup(const char* src) {
+#if defined(OS_WIN)
+  return _strdup(src);
+#else
+  return strdup(src);
+#endif
+}
+
 }  // namespace
 
 const FormatUrlType kFormatUrlOmitNothing                     = 0;
@@ -2075,6 +2083,51 @@ bool IPNumberMatchesPrefix(const IPAddressNumber& ip_number,
   return true;
 }
 
+struct addrinfo* CreateCopyOfAddrinfo(const struct addrinfo* info,
+                                      bool recursive) {
+  DCHECK(info);
+  struct addrinfo* copy = new addrinfo;
+
+  // Copy all the fields (some of these are pointers, we will fix that next).
+  memcpy(copy, info, sizeof(addrinfo));
+
+  // ai_canonname is a NULL-terminated string.
+  if (info->ai_canonname) {
+    copy->ai_canonname = do_strdup(info->ai_canonname);
+  }
+
+  // ai_addr is a buffer of length ai_addrlen.
+  if (info->ai_addr) {
+    copy->ai_addr = reinterpret_cast<sockaddr *>(new char[info->ai_addrlen]);
+    memcpy(copy->ai_addr, info->ai_addr, info->ai_addrlen);
+  }
+
+  // Recursive copy.
+  if (recursive && info->ai_next)
+    copy->ai_next = CreateCopyOfAddrinfo(info->ai_next, recursive);
+  else
+    copy->ai_next = NULL;
+
+  return copy;
+}
+
+void FreeCopyOfAddrinfo(struct addrinfo* info) {
+  DCHECK(info);
+  if (info->ai_canonname)
+    free(info->ai_canonname);  // Allocated by strdup.
+
+  if (info->ai_addr)
+    delete [] reinterpret_cast<char*>(info->ai_addr);
+
+  struct addrinfo* next = info->ai_next;
+
+  delete info;
+
+  // Recursive free.
+  if (next)
+    FreeCopyOfAddrinfo(next);
+}
+
 // Returns the port field of the sockaddr in |info|.
 uint16* GetPortFieldFromAddrinfo(struct addrinfo* info) {
   const struct addrinfo* const_info = info;
diff --git a/net/base/net_util.h b/net/base/net_util.h
index 3f36182..fa7733e 100644
--- a/net/base/net_util.h
+++ b/net/base/net_util.h
@@ -402,6 +402,16 @@ bool IPNumberMatchesPrefix(const IPAddressNumber& ip_number,
                            const IPAddressNumber& ip_prefix,
                            size_t prefix_length_in_bits);
 
+// Makes a copy of |info|. The dynamically-allocated parts are copied as well.
+// If |recursive| is true, chained entries via ai_next are copied too.
+// The copy returned by this function should be freed using
+// FreeCopyOfAddrinfo(), and NOT freeaddrinfo().
+struct addrinfo* CreateCopyOfAddrinfo(const struct addrinfo* info,
+                                      bool recursive);
+
+// Frees an addrinfo that was created by CreateCopyOfAddrinfo().
+void FreeCopyOfAddrinfo(struct addrinfo* info);
+
 // Returns the port field of the sockaddr in |info|.
 const uint16* GetPortFieldFromAddrinfo(const struct addrinfo* info);
 uint16* GetPortFieldFromAddrinfo(struct addrinfo* info);
diff --git a/net/socket/client_socket_pool_base.cc b/net/socket/client_socket_pool_base.cc
index cc4ae66..2a5f216 100644
--- a/net/socket/client_socket_pool_base.cc
+++ b/net/socket/client_socket_pool_base.cc
@@ -45,6 +45,7 @@ ConnectJob::ConnectJob(const std::string& group_name,
       delegate_(delegate),
       net_log_(net_log),
       idle_(true),
+      prefer_ipv4_(false),
       preconnect_state_(NOT_PRECONNECT) {
   DCHECK(!group_name.empty());
   DCHECK(delegate);
@@ -1073,6 +1074,14 @@ void ClientSocketPoolBaseHelper::Group::OnBackupSocketTimerFired(
       group_name, **pending_requests_.begin(), pool);
   backup_job->net_log().AddEvent(NetLog::TYPE_SOCKET_BACKUP_CREATED, NULL);
   SIMPLE_STATS_COUNTER("socket.backup_created");
+  // The purpose of the backup job is to initiate a new connect job when the
+  // first connect job takes too long.  We call set_prefer_ipv4 here to use
+  // the backup job for a second purpose: if the first connect job is IPv6,
+  // try an IPv4 connect job in parallel.  This hides the long timeout error
+  // on some networks with broken IPv6 support.
+  // TODO(wtc): remove the set_prefer_ipv4 call when broken IPv6 networks are
+  // gone.
+  backup_job->set_prefer_ipv4();
   int rv = backup_job->Connect();
   pool->connecting_socket_count_++;
   AddJob(backup_job);
diff --git a/net/socket/client_socket_pool_base.h b/net/socket/client_socket_pool_base.h
index 318cab8..25bc7c7 100644
--- a/net/socket/client_socket_pool_base.h
+++ b/net/socket/client_socket_pool_base.h
@@ -80,6 +80,7 @@ class ConnectJob {
   bool is_unused_preconnect() const {
     return preconnect_state_ == UNUSED_PRECONNECT;
   }
+  bool prefer_ipv4() const { return prefer_ipv4_; }
 
   // Initialized by the ClientSocketPoolBaseHelper.
   // TODO(willchan): Move most of the constructor arguments over here.  We
@@ -87,6 +88,11 @@ class ConnectJob {
   // the initialization.
   void Initialize(bool is_preconnect);
 
+  // Instructs the ConnectJob to try IPv4 addresses first.  This can be useful
+  // when IPv6 is likely to be broken.
+  // TODO(wtc): this should be folded into the Initialize() method.
+  void set_prefer_ipv4() { prefer_ipv4_ = true; }
+
   // Releases |socket_| to the client.  On connection error, this should return
   // NULL.
   StreamSocket* ReleaseSocket() { return socket_.release(); }
@@ -142,6 +148,8 @@ class ConnectJob {
   BoundNetLog net_log_;
   // A ConnectJob is idle until Connect() has been called.
   bool idle_;
+  // True if we should try IPv4 addresses first.
+  bool prefer_ipv4_;
   PreconnectState preconnect_state_;
 
   DISALLOW_COPY_AND_ASSIGN(ConnectJob);
diff --git a/net/socket/transport_client_socket_pool.cc b/net/socket/transport_client_socket_pool.cc
index 28fdb6a..66b4ec9 100644
--- a/net/socket/transport_client_socket_pool.cc
+++ b/net/socket/transport_client_socket_pool.cc
@@ -12,6 +12,7 @@
 #include "base/time.h"
 #include "net/base/net_log.h"
 #include "net/base/net_errors.h"
+#include "net/base/sys_addrinfo.h"
 #include "net/socket/client_socket_factory.h"
 #include "net/socket/client_socket_handle.h"
 #include "net/socket/client_socket_pool_base.h"
@@ -90,6 +91,38 @@ LoadState TransportConnectJob::GetLoadState() const {
   }
 }
 
+// static
+void TransportConnectJob::MakeAddrListStartWithIPv4(AddressList* addrlist) {
+  if (addrlist->head()->ai_family != AF_INET6)
+    return;
+  bool has_ipv4 = false;
+  for (const struct addrinfo* ai = addrlist->head(); ai; ai = ai->ai_next) {
+    if (ai->ai_family != AF_INET6) {
+      has_ipv4 = true;
+      break;
+    }
+  }
+  if (!has_ipv4)
+    return;
+
+  struct addrinfo* head = CreateCopyOfAddrinfo(addrlist->head(), true);
+  struct addrinfo* tail = head;
+  while (tail->ai_next)
+    tail = tail->ai_next;
+  char* canonname = head->ai_canonname;
+  head->ai_canonname = NULL;
+  while (head->ai_family == AF_INET6) {
+    tail->ai_next = head;
+    tail = head;
+    head = head->ai_next;
+    tail->ai_next = NULL;
+  }
+  head->ai_canonname = canonname;
+
+  addrlist->Copy(head, true);
+  FreeCopyOfAddrinfo(head);
+}
+
 void TransportConnectJob::OnIOComplete(int result) {
   int rv = DoLoop(result);
   if (rv != ERR_IO_PENDING)
@@ -142,6 +175,8 @@ int TransportConnectJob::DoResolveHostComplete(int result) {
 
 int TransportConnectJob::DoTransportConnect() {
   next_state_ = STATE_TRANSPORT_CONNECT_COMPLETE;
+  if (prefer_ipv4())
+    MakeAddrListStartWithIPv4(&addresses_);
   set_socket(client_socket_factory_->CreateTransportClientSocket(
         addresses_, net_log().net_log(), net_log().source()));
   connect_start_time_ = base::TimeTicks::Now();
diff --git a/net/socket/transport_client_socket_pool.h b/net/socket/transport_client_socket_pool.h
index bf7243c..4626e05 100644
--- a/net/socket/transport_client_socket_pool.h
+++ b/net/socket/transport_client_socket_pool.h
@@ -63,6 +63,13 @@ class TransportConnectJob : public ConnectJob {
   // ConnectJob methods.
   virtual LoadState GetLoadState() const;
 
+  // Makes |addrlist| start with an IPv4 address if |addrlist| contains any
+  // IPv4 address.
+  //
+  // WARNING: this method should only be used to implement the prefer-IPv4
+  // hack.  It is a public method for the unit tests.
+  static void MakeAddrListStartWithIPv4(AddressList* addrlist);
+
  private:
   enum State {
     STATE_RESOLVE_HOST,
diff --git a/net/socket/transport_client_socket_pool_unittest.cc b/net/socket/transport_client_socket_pool_unittest.cc
index 4ab5994..2fee5ea 100644
--- a/net/socket/transport_client_socket_pool_unittest.cc
+++ b/net/socket/transport_client_socket_pool_unittest.cc
@@ -10,6 +10,7 @@
 #include "base/threading/platform_thread.h"
 #include "net/base/mock_host_resolver.h"
 #include "net/base/net_errors.h"
+#include "net/base/sys_addrinfo.h"
 #include "net/base/test_completion_callback.h"
 #include "net/socket/client_socket_factory.h"
 #include "net/socket/client_socket_handle.h"
@@ -355,6 +356,89 @@ class TransportClientSocketPoolTest : public testing::Test {
   ClientSocketPoolTest test_base_;
 };
 
+TEST(TransportConnectJobTest, MakeAddrListStartWithIPv4) {
+  IPAddressNumber ip_number;
+  ASSERT_TRUE(ParseIPLiteralToNumber("192.168.1.1", &ip_number));
+  AddressList addrlist_v4_1(ip_number, 80, false);
+  ASSERT_TRUE(ParseIPLiteralToNumber("192.168.1.2", &ip_number));
+  AddressList addrlist_v4_2(ip_number, 80, false);
+  ASSERT_TRUE(ParseIPLiteralToNumber("2001:4860:b006::64", &ip_number));
+  AddressList addrlist_v6_1(ip_number, 80, false);
+  ASSERT_TRUE(ParseIPLiteralToNumber("2001:4860:b006::66", &ip_number));
+  AddressList addrlist_v6_2(ip_number, 80, false);
+
+  AddressList addrlist;
+  const struct addrinfo* ai;
+
+  // Test 1: IPv4 only.  Expect no change.
+  addrlist.Copy(addrlist_v4_1.head(), true);
+  addrlist.Append(addrlist_v4_2.head());
+  TransportConnectJob::MakeAddrListStartWithIPv4(&addrlist);
+  ai = addrlist.head();
+  EXPECT_EQ(AF_INET, ai->ai_family);
+  ai = ai->ai_next;
+  EXPECT_EQ(AF_INET, ai->ai_family);
+  EXPECT_TRUE(ai->ai_next == NULL);
+
+  // Test 2: IPv6 only.  Expect no change.
+  addrlist.Copy(addrlist_v6_1.head(), true);
+  addrlist.Append(addrlist_v6_2.head());
+  TransportConnectJob::MakeAddrListStartWithIPv4(&addrlist);
+  ai = addrlist.head();
+  EXPECT_EQ(AF_INET6, ai->ai_family);
+  ai = ai->ai_next;
+  EXPECT_EQ(AF_INET6, ai->ai_family);
+  EXPECT_TRUE(ai->ai_next == NULL);
+
+  // Test 3: IPv4 then IPv6.  Expect no change.
+  addrlist.Copy(addrlist_v4_1.head(), true);
+  addrlist.Append(addrlist_v4_2.head());
+  addrlist.Append(addrlist_v6_1.head());
+  addrlist.Append(addrlist_v6_2.head());
+  TransportConnectJob::MakeAddrListStartWithIPv4(&addrlist);
+  ai = addrlist.head();
+  EXPECT_EQ(AF_INET, ai->ai_family);
+  ai = ai->ai_next;
+  EXPECT_EQ(AF_INET, ai->ai_family);
+  ai = ai->ai_next;
+  EXPECT_EQ(AF_INET6, ai->ai_family);
+  ai = ai->ai_next;
+  EXPECT_EQ(AF_INET6, ai->ai_family);
+  EXPECT_TRUE(ai->ai_next == NULL);
+
+  // Test 4: IPv6, IPv4, IPv6, IPv4.  Expect first IPv6 moved to the end.
+  addrlist.Copy(addrlist_v6_1.head(), true);
+  addrlist.Append(addrlist_v4_1.head());
+  addrlist.Append(addrlist_v6_2.head());
+  addrlist.Append(addrlist_v4_2.head());
+  TransportConnectJob::MakeAddrListStartWithIPv4(&addrlist);
+  ai = addrlist.head();
+  EXPECT_EQ(AF_INET, ai->ai_family);
+  ai = ai->ai_next;
+  EXPECT_EQ(AF_INET6, ai->ai_family);
+  ai = ai->ai_next;
+  EXPECT_EQ(AF_INET, ai->ai_family);
+  ai = ai->ai_next;
+  EXPECT_EQ(AF_INET6, ai->ai_family);
+  EXPECT_TRUE(ai->ai_next == NULL);
+
+  // Test 5: IPv6, IPv6, IPv4, IPv4.  Expect first two IPv6's moved to the end.
+  addrlist.Copy(addrlist_v6_1.head(), true);
+  addrlist.Append(addrlist_v6_2.head());
+  addrlist.Append(addrlist_v4_1.head());
+  addrlist.Append(addrlist_v4_2.head());
+  TransportConnectJob::MakeAddrListStartWithIPv4(&addrlist);
+  ai = addrlist.head();
+  EXPECT_EQ(AF_INET, ai->ai_family);
+  ai = ai->ai_next;
+  EXPECT_EQ(AF_INET, ai->ai_family);
+  ai = ai->ai_next;
+  EXPECT_EQ(AF_INET6, ai->ai_family);
+  ai = ai->ai_next;
+  EXPECT_EQ(AF_INET6, ai->ai_family);
+  EXPECT_TRUE(ai->ai_next == NULL);
+}
+
 TEST_F(TransportClientSocketPoolTest, Basic) {
   TestCompletionCallback callback;
   ClientSocketHandle handle;