Fix crashes stemming from ClientSocketPoolBase::ReleaseSocket().

BUG=http://crbug.com/13908
TEST=Go to a page that loads lots of content from the same host, and then triggers a redirect (which will cancel the requests).  A good example is http://photo.sora.net/album/theme/index.php.  Unittest coverage provided in TCPClientSocketPoolTest.CancelActiveRequestWithPendingRequests.

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

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

3 files changed, 61 insertions, 20 deletions

diff --git a/net/base/tcp_client_socket_pool.cc b/net/base/tcp_client_socket_pool.cc
index b8bb5c6..94b53ca 100644
--- a/net/base/tcp_client_socket_pool.cc
+++ b/net/base/tcp_client_socket_pool.cc
@@ -237,12 +237,17 @@ void ClientSocketPoolBase::CancelRequest(const std::string& group_name,
     group.connecting_requests.erase(map_it);
     group.active_socket_count--;
 
+    if (!group.pending_requests.empty()) {
+      ProcessPendingRequest(group_name, &group);
+      return;  // |group| may be invalid after this, so return to be safe.
+    }
+
     // Delete group if no longer needed.
     if (group.active_socket_count == 0 && group.idle_sockets.empty()) {
       CHECK(group.pending_requests.empty());
       CHECK(group.connecting_requests.empty());
       group_map_.erase(group_name);
-      return;
+      return;  // |group| is invalid after this, so return to be safe.
     }
   }
 
@@ -385,16 +390,7 @@ void ClientSocketPoolBase::DoReleaseSocket(const std::string& group_name,
 
   // Process one pending request.
   if (!group.pending_requests.empty()) {
-    Request r = group.pending_requests.front();
-    group.pending_requests.pop_front();
-
-    int rv = RequestSocket(
-        group_name, r.resolve_info, r.priority, r.handle, r.callback);
-
-    CheckSocketCounts(group);
-
-    if (rv != ERR_IO_PENDING)
-      r.callback->Run(rv);
+    ProcessPendingRequest(group_name, &group);
     return;
   }
 
@@ -486,6 +482,20 @@ void ClientSocketPoolBase::RemoveConnectingSocket(
   connecting_socket_map_.erase(it);
 }
 
+void ClientSocketPoolBase::ProcessPendingRequest(const std::string& group_name,
+                                                 Group* group) {
+  Request r = group->pending_requests.front();
+  group->pending_requests.pop_front();
+
+  int rv = RequestSocket(
+      group_name, r.resolve_info, r.priority, r.handle, r.callback);
+  
+  // |group| may be invalid after RequestSocket.
+
+  if (rv != ERR_IO_PENDING)
+    r.callback->Run(rv);
+}
+
 ConnectingSocket*
 TCPClientSocketPool::TCPConnectingSocketFactory::NewConnectingSocket(
     const std::string& group_name,
diff --git a/net/base/tcp_client_socket_pool.h b/net/base/tcp_client_socket_pool.h
index 40d8505..5e83e7e 100644
--- a/net/base/tcp_client_socket_pool.h
+++ b/net/base/tcp_client_socket_pool.h
@@ -240,6 +240,9 @@ class ClientSocketPoolBase : public base::RefCounted<ClientSocketPoolBase> {
 
   static void CheckSocketCounts(const Group& group);
 
+  // Process a request from a group's pending_requests queue.
+  void ProcessPendingRequest(const std::string& group_name, Group* group);
+
   GroupMap group_map_;
 
   ConnectingSocketMap connecting_socket_map_;
diff --git a/net/base/tcp_client_socket_pool_unittest.cc b/net/base/tcp_client_socket_pool_unittest.cc
index 5334190..7892cb0 100644
--- a/net/base/tcp_client_socket_pool_unittest.cc
+++ b/net/base/tcp_client_socket_pool_unittest.cc
@@ -222,6 +222,12 @@ class TCPClientSocketPoolTest : public testing::Test {
     TestSocketRequest::completion_count = 0;
   }
 
+  virtual void TearDown() {
+    // The tests often call Reset() on handles at the end which may post
+    // DoReleaseSocket() tasks.
+    MessageLoop::current()->RunAllPending();
+  }
+
   ScopedHostMapper scoped_host_mapper_;
   HostResolver host_resolver_;
   MockClientSocketFactory client_socket_factory_;
@@ -243,9 +249,6 @@ TEST_F(TCPClientSocketPoolTest, Basic) {
   EXPECT_TRUE(handle.socket());
 
   handle.Reset();
-
-  // The handle's Reset method may have posted a task.
-  MessageLoop::current()->RunAllPending();
 }
 
 TEST_F(TCPClientSocketPoolTest, InitHostResolutionFailure) {
@@ -408,8 +411,6 @@ TEST_F(TCPClientSocketPoolTest, TwoRequestsCancelOne) {
 
   EXPECT_EQ(OK, req2.WaitForResult());
   req2.handle.Reset();
-  // The handle's Reset method may have posted a task.
-  MessageLoop::current()->RunAllPending();
 }
 
 TEST_F(TCPClientSocketPoolTest, ConnectCancelConnect) {
@@ -439,8 +440,6 @@ TEST_F(TCPClientSocketPoolTest, ConnectCancelConnect) {
   EXPECT_FALSE(callback.have_result());
 
   handle.Reset();
-  // The handle's Reset method may have posted a task.
-  MessageLoop::current()->RunAllPending();
 }
 
 TEST_F(TCPClientSocketPoolTest, CancelRequest) {
@@ -551,8 +550,37 @@ TEST_F(TCPClientSocketPoolTest, RequestTwice) {
   EXPECT_EQ(OK, callback.WaitForResult());
 
   handle.Reset();
-  // The handle's Reset method may have posted a task.
-  MessageLoop::current()->RunAllPending();
+}
+
+// Make sure that pending requests get serviced after active requests get
+// cancelled.
+TEST_F(TCPClientSocketPoolTest, CancelActiveRequestWithPendingRequests) {
+  client_socket_factory_.set_client_socket_type(
+      MockClientSocketFactory::MOCK_PENDING_CLIENT_SOCKET);
+
+  scoped_ptr<TestSocketRequest> reqs[kNumRequests];
+
+  // Queue up all the requests
+
+  HostResolver::RequestInfo info("www.google.com", 80);
+  for (size_t i = 0; i < arraysize(reqs); ++i) {
+    reqs[i].reset(new TestSocketRequest(pool_.get(), &request_order_));
+    int rv = reqs[i]->handle.Init("a", info, 5, reqs[i].get());
+    EXPECT_EQ(ERR_IO_PENDING, rv);
+  }
+
+  // Now, kMaxSocketsPerGroup requests should be active.  Let's cancel them.
+  for (int i = 0; i < kMaxSocketsPerGroup; ++i)
+    reqs[i]->handle.Reset();
+
+  // Let's wait for the rest to complete now.
+
+  for (size_t i = kMaxSocketsPerGroup; i < arraysize(reqs); ++i) {
+    EXPECT_EQ(OK, reqs[i]->WaitForResult());
+    reqs[i]->handle.Reset();
+  }
+
+  EXPECT_EQ(kNumPendingRequests, TestSocketRequest::completion_count);
 }
 
 }  // namespace