Do not attempt to reuse active sockets after a socket pool flush (usually a network change).

Implements this functionality by adding an |id_| field to ClientSocketPoolBaseHelper that is incremented on each Flush().
BUG=45872

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

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

11 files changed, 103 insertions, 23 deletions

diff --git a/net/socket/client_socket_handle.cc b/net/socket/client_socket_handle.cc
index 4f61a68..4adae02 100644
--- a/net/socket/client_socket_handle.cc
+++ b/net/socket/client_socket_handle.cc
@@ -37,7 +37,7 @@ void ClientSocketHandle::ResetInternal(bool cancel) {
     if (pool_)
       // If we've still got a socket, release it back to the ClientSocketPool so
       // it can be deleted or reused.
-      pool_->ReleaseSocket(group_name_, release_socket());
+      pool_->ReleaseSocket(group_name_, release_socket(), pool_id_);
   } else if (cancel) {
     // If we did not get initialized yet, so we've got a socket request pending.
     // Cancel it.
@@ -50,6 +50,7 @@ void ClientSocketHandle::ResetInternal(bool cancel) {
   idle_time_ = base::TimeDelta();
   init_time_ = base::TimeTicks();
   setup_time_ = base::TimeDelta();
+  pool_id_ = -1;
 }
 
 LoadState ClientSocketHandle::GetLoadState() const {
@@ -75,6 +76,7 @@ void ClientSocketHandle::HandleInitCompletion(int result) {
     ResetInternal(false);  // The request failed, so there's nothing to cancel.
     return;
   }
+  CHECK_NE(-1, pool_id_) << "Pool should have set |pool_id_| to a valid value.";
   setup_time_ = base::TimeTicks::Now() - init_time_;
 
   scoped_refptr<ClientSocketPoolHistograms> histograms = pool_->histograms();
diff --git a/net/socket/client_socket_handle.h b/net/socket/client_socket_handle.h
index 4b36e58..13095ab 100644
--- a/net/socket/client_socket_handle.h
+++ b/net/socket/client_socket_handle.h
@@ -93,6 +93,7 @@ class ClientSocketHandle {
   void set_is_reused(bool is_reused) { is_reused_ = is_reused; }
   void set_socket(ClientSocket* s) { socket_.reset(s); }
   void set_idle_time(base::TimeDelta idle_time) { idle_time_ = idle_time; }
+  void set_pool_id(int id) { pool_id_ = id; }
 
   // These may only be used if is_initialized() is true.
   const std::string& group_name() const { return group_name_; }
@@ -142,6 +143,7 @@ class ClientSocketHandle {
   CompletionCallbackImpl<ClientSocketHandle> callback_;
   CompletionCallback* user_callback_;
   base::TimeDelta idle_time_;
+  int pool_id_;  // See ClientSocketPool::ReleaseSocket() for an explanation.
   base::TimeTicks init_time_;
   base::TimeDelta setup_time_;
 
diff --git a/net/socket/client_socket_pool.h b/net/socket/client_socket_pool.h
index c58e72d..e631980 100644
--- a/net/socket/client_socket_pool.h
+++ b/net/socket/client_socket_pool.h
@@ -67,9 +67,19 @@ class ClientSocketPool : public base::RefCounted<ClientSocketPool> {
   // Called to release a socket once the socket is no longer needed.  If the
   // socket still has an established connection, then it will be added to the
   // set of idle sockets to be used to satisfy future RequestSocket calls.
-  // Otherwise, the ClientSocket is destroyed.
+  // Otherwise, the ClientSocket is destroyed.  |id| is used to differentiate
+  // between updated versions of the same pool instance.  The pool's id will
+  // change when it flushes, so it can use this |id| to discard sockets with
+  // mismatched ids.
   virtual void ReleaseSocket(const std::string& group_name,
-                             ClientSocket* socket) = 0;
+                             ClientSocket* socket,
+                             int id) = 0;
+
+  // This flushes all state from the ClientSocketPool.  This means that all
+  // idle and connecting sockets are discarded.  Active sockets being
+  // held by ClientSocketPool clients will be discarded when released back to
+  // the pool.
+  virtual void Flush() = 0;
 
   // Called to close any idle connections held by the connection manager.
   virtual void CloseIdleSockets() = 0;
diff --git a/net/socket/client_socket_pool_base.cc b/net/socket/client_socket_pool_base.cc
index f584996c..80857e5 100644
--- a/net/socket/client_socket_pool_base.cc
+++ b/net/socket/client_socket_pool_base.cc
@@ -142,7 +142,8 @@ ClientSocketPoolBaseHelper::ClientSocketPoolBaseHelper(
       connect_job_factory_(connect_job_factory),
       network_change_notifier_(network_change_notifier),
       backup_jobs_enabled_(false),
-      ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)) {
+      ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)),
+      pool_generation_number_(0) {
   DCHECK_LE(0, max_sockets_per_group);
   DCHECK_LE(max_sockets_per_group, max_sockets);
 
@@ -370,7 +371,8 @@ void ClientSocketPoolBaseHelper::CancelRequest(
 }
 
 void ClientSocketPoolBaseHelper::ReleaseSocket(const std::string& group_name,
-                                               ClientSocket* socket) {
+                                               ClientSocket* socket,
+                                               int id) {
   Group& group = group_map_[group_name];
   group.num_releasing_sockets++;
   num_releasing_sockets_++;
@@ -379,7 +381,7 @@ void ClientSocketPoolBaseHelper::ReleaseSocket(const std::string& group_name,
   // another to begin doing work.  This also avoids nasty recursion issues.
   // NOTE: We cannot refer to the handle argument after this method returns.
   MessageLoop::current()->PostTask(FROM_HERE, NewRunnableMethod(
-      this, &ClientSocketPoolBaseHelper::DoReleaseSocket, group_name, socket));
+      this, &ClientSocketPoolBaseHelper::DoReleaseSocket, group_name, socket, id));
 }
 
 void ClientSocketPoolBaseHelper::CloseIdleSockets() {
@@ -483,7 +485,8 @@ void ClientSocketPoolBaseHelper::DecrementIdleCount() {
 }
 
 void ClientSocketPoolBaseHelper::DoReleaseSocket(const std::string& group_name,
-                                                 ClientSocket* socket) {
+                                                 ClientSocket* socket,
+                                                 int id) {
   // Running callbacks can cause the last outside reference to be released.
   // Hold onto a reference.
   scoped_refptr<ClientSocketPoolBaseHelper> ref_holder(this);
@@ -505,7 +508,8 @@ void ClientSocketPoolBaseHelper::DoReleaseSocket(const std::string& group_name,
   CHECK_GT(num_releasing_sockets_, 0);
   num_releasing_sockets_--;
 
-  const bool can_reuse = socket->IsConnectedAndIdle();
+  const bool can_reuse = socket->IsConnectedAndIdle() &&
+      id == pool_generation_number_;
   if (can_reuse) {
     AddIdleSocket(socket, true /* used socket */, &group);
   } else {
@@ -629,7 +633,8 @@ void ClientSocketPoolBaseHelper::OnConnectJobComplete(
   }
 }
 
-void ClientSocketPoolBaseHelper::OnIPAddressChanged() {
+void ClientSocketPoolBaseHelper::Flush() {
+  pool_generation_number_++;
   CancelAllConnectJobs();
   CloseIdleSockets();
 }
@@ -712,6 +717,7 @@ void ClientSocketPoolBaseHelper::HandOutSocket(
   handle->set_socket(socket);
   handle->set_is_reused(reused);
   handle->set_idle_time(idle_time);
+  handle->set_pool_id(pool_generation_number_);
 
   if (reused) {
     net_log.AddEvent(
diff --git a/net/socket/client_socket_pool_base.h b/net/socket/client_socket_pool_base.h
index 470c120..a037b0e 100644
--- a/net/socket/client_socket_pool_base.h
+++ b/net/socket/client_socket_pool_base.h
@@ -188,8 +188,12 @@ class ClientSocketPoolBaseHelper
 
   // See ClientSocketPool::ReleaseSocket for documentation on this function.
   void ReleaseSocket(const std::string& group_name,
-                     ClientSocket* socket);
+                     ClientSocket* socket,
+                     int id);
 
+  // See ClientSocketPool::Flush for documentation on this function.
+  void Flush();
+  
   // See ClientSocketPool::CloseIdleSockets for documentation on this function.
   void CloseIdleSockets();
 
@@ -216,7 +220,7 @@ class ClientSocketPoolBaseHelper
   virtual void OnConnectJobComplete(int result, ConnectJob* job);
 
   // NetworkChangeNotifier::Observer methods:
-  virtual void OnIPAddressChanged();
+  virtual void OnIPAddressChanged() { Flush(); }
 
   // For testing.
   bool may_have_stalled_group() const { return may_have_stalled_group_; }
@@ -335,7 +339,8 @@ class ClientSocketPoolBaseHelper
   void DecrementIdleCount();
 
   // Called via PostTask by ReleaseSocket.
-  void DoReleaseSocket(const std::string& group_name, ClientSocket* socket);
+  void DoReleaseSocket(
+      const std::string& group_name, ClientSocket* socket, int id);
 
   // Scans the group map for groups which have an available socket slot and
   // at least one pending request. Returns number of groups found, and if found
@@ -451,6 +456,11 @@ class ClientSocketPoolBaseHelper
 
   // A factory to pin the backup_job tasks.
   ScopedRunnableMethodFactory<ClientSocketPoolBaseHelper> method_factory_;
+
+  // A unique id for the pool.  It gets incremented every time we Flush() the
+  // pool.  This is so that when sockets get released back to the pool, we can
+  // make sure that they are discarded rather than reused.
+  int pool_generation_number_;
 };
 
 }  // namespace internal
@@ -542,8 +552,8 @@ class ClientSocketPoolBase {
     return helper_->CancelRequest(group_name, handle);
   }
 
-  void ReleaseSocket(const std::string& group_name, ClientSocket* socket) {
-    return helper_->ReleaseSocket(group_name, socket);
+  void ReleaseSocket(const std::string& group_name, ClientSocket* socket, int id) {
+    return helper_->ReleaseSocket(group_name, socket, id);
   }
 
   void CloseIdleSockets() { return helper_->CloseIdleSockets(); }
@@ -586,6 +596,8 @@ class ClientSocketPoolBase {
 
   void enable_backup_jobs() { helper_->enable_backup_jobs(); }
 
+  void Flush() { helper_->Flush(); }
+
  private:
   // This adaptor class exists to bridge the
   // internal::ClientSocketPoolBaseHelper::ConnectJobFactory and
diff --git a/net/socket/client_socket_pool_base_unittest.cc b/net/socket/client_socket_pool_base_unittest.cc
index a694a6f..4b10cdb 100644
--- a/net/socket/client_socket_pool_base_unittest.cc
+++ b/net/socket/client_socket_pool_base_unittest.cc
@@ -310,8 +310,13 @@ class TestClientSocketPool : public ClientSocketPool {
 
   virtual void ReleaseSocket(
       const std::string& group_name,
-      ClientSocket* socket) {
-    base_.ReleaseSocket(group_name, socket);
+      ClientSocket* socket,
+      int id) {
+    base_.ReleaseSocket(group_name, socket, id);
+  }
+
+  virtual void Flush() {
+    base_.Flush();
   }
 
   virtual void CloseIdleSockets() {
@@ -1660,6 +1665,30 @@ TEST_F(ClientSocketPoolBaseTest, CallbackThatReleasesPool) {
   callback.WaitForResult();
 }
 
+TEST_F(ClientSocketPoolBaseTest, DoNotReuseSocketAfterFlush) {
+  CreatePool(kDefaultMaxSockets, kDefaultMaxSocketsPerGroup);
+  connect_job_factory_->set_job_type(TestConnectJob::kMockPendingJob);
+
+  ClientSocketHandle handle;
+  TestCompletionCallback callback;
+  EXPECT_EQ(ERR_IO_PENDING,
+            InitHandle(&handle, "a", kDefaultPriority,
+                       &callback, pool_, BoundNetLog()));
+  EXPECT_EQ(OK, callback.WaitForResult());
+  EXPECT_EQ(ClientSocketHandle::UNUSED, handle.reuse_type());
+
+  pool_->Flush();
+
+  handle.Reset();
+  MessageLoop::current()->RunAllPending();
+
+  EXPECT_EQ(ERR_IO_PENDING,
+            InitHandle(&handle, "a", kDefaultPriority,
+                       &callback, pool_, BoundNetLog()));
+  EXPECT_EQ(OK, callback.WaitForResult());
+  EXPECT_EQ(ClientSocketHandle::UNUSED, handle.reuse_type());
+}
+
 }  // namespace
 
 }  // namespace net
diff --git a/net/socket/socks_client_socket_pool.cc b/net/socket/socks_client_socket_pool.cc
index 4c446ab..ad924f8 100644
--- a/net/socket/socks_client_socket_pool.cc
+++ b/net/socket/socks_client_socket_pool.cc
@@ -195,8 +195,13 @@ void SOCKSClientSocketPool::CancelRequest(
 
 void SOCKSClientSocketPool::ReleaseSocket(
     const std::string& group_name,
-    ClientSocket* socket) {
-  base_.ReleaseSocket(group_name, socket);
+    ClientSocket* socket,
+    int id) {
+  base_.ReleaseSocket(group_name, socket, id);
+}
+
+void SOCKSClientSocketPool::Flush() {
+  base_.Flush();
 }
 
 void SOCKSClientSocketPool::CloseIdleSockets() {
diff --git a/net/socket/socks_client_socket_pool.h b/net/socket/socks_client_socket_pool.h
index bd975ba..c202ce1 100644
--- a/net/socket/socks_client_socket_pool.h
+++ b/net/socket/socks_client_socket_pool.h
@@ -126,7 +126,10 @@ class SOCKSClientSocketPool : public ClientSocketPool {
                              const ClientSocketHandle* handle);
 
   virtual void ReleaseSocket(const std::string& group_name,
-                             ClientSocket* socket);
+                             ClientSocket* socket,
+                             int id);
+
+  virtual void Flush();
 
   virtual void CloseIdleSockets();
 
diff --git a/net/socket/socks_client_socket_pool_unittest.cc b/net/socket/socks_client_socket_pool_unittest.cc
index 33bc146..452fbcc 100644
--- a/net/socket/socks_client_socket_pool_unittest.cc
+++ b/net/socket/socks_client_socket_pool_unittest.cc
@@ -106,6 +106,7 @@ class MockTCPClientSocketPool : public TCPClientSocketPool {
         AddressList(), net_log.net_log());
     MockConnectJob* job = new MockConnectJob(socket, handle, callback);
     job_list_.push_back(job);
+    handle->set_pool_id(1);
     return job->Connect();
   }
 
@@ -121,7 +122,9 @@ class MockTCPClientSocketPool : public TCPClientSocketPool {
   }
 
   virtual void ReleaseSocket(const std::string& group_name,
-                             ClientSocket* socket) {
+                             ClientSocket* socket,
+                             int id) {
+    EXPECT_EQ(1, id);
     release_count_++;
     delete socket;
   }
diff --git a/net/socket/tcp_client_socket_pool.cc b/net/socket/tcp_client_socket_pool.cc
index 0f2c079..c7eac14 100644
--- a/net/socket/tcp_client_socket_pool.cc
+++ b/net/socket/tcp_client_socket_pool.cc
@@ -224,8 +224,13 @@ void TCPClientSocketPool::CancelRequest(
 
 void TCPClientSocketPool::ReleaseSocket(
     const std::string& group_name,
-    ClientSocket* socket) {
-  base_.ReleaseSocket(group_name, socket);
+    ClientSocket* socket,
+    int id) {
+  base_.ReleaseSocket(group_name, socket, id);
+}
+
+void TCPClientSocketPool::Flush() {
+  base_.Flush();
 }
 
 void TCPClientSocketPool::CloseIdleSockets() {
diff --git a/net/socket/tcp_client_socket_pool.h b/net/socket/tcp_client_socket_pool.h
index ea2ef70..8eee3c7 100644
--- a/net/socket/tcp_client_socket_pool.h
+++ b/net/socket/tcp_client_socket_pool.h
@@ -134,7 +134,10 @@ class TCPClientSocketPool : public ClientSocketPool {
                              const ClientSocketHandle* handle);
 
   virtual void ReleaseSocket(const std::string& group_name,
-                             ClientSocket* socket);
+                             ClientSocket* socket,
+                             int id);
+
+  virtual void Flush();
 
   virtual void CloseIdleSockets();