Clean up weird ClientSocketPoolBase unit tests.

The tests were re-entrantly reusing a TestCompletionCallback subclass in
a weird manner, which has now been fixed.  Also fix a WebSocket test
modelled after them.

BUG=114130

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

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

net/socket/client_socket_pool_base_unittest.cc

@@ -9,6 +9,7 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
+#include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_vector.h"
 #include "base/memory/weak_ptr.h"
@@ -1432,125 +1433,77 @@ TEST_F(ClientSocketPoolBaseTest, CancelRequest) {
   EXPECT_EQ(ClientSocketPoolTest::kIndexOutOfBounds, GetOrderOfRequest(8));
 }
 
-class RequestSocketCallback : public TestCompletionCallbackBase {
- public:
-  RequestSocketCallback(ClientSocketHandle* handle,
-                        TestClientSocketPool* pool,
-                        TestConnectJobFactory* test_connect_job_factory,
-                        TestConnectJob::JobType next_job_type)
-      : handle_(handle),
-        pool_(pool),
-        within_callback_(false),
-        test_connect_job_factory_(test_connect_job_factory),
-        next_job_type_(next_job_type),
-        callback_(base::Bind(&RequestSocketCallback::OnComplete,
-                             base::Unretained(this))) {
-  }
-
-  ~RequestSocketCallback() override {}
-
-  const CompletionCallback& callback() const { return callback_; }
-
- private:
-  void OnComplete(int result) {
-    SetResult(result);
-    ASSERT_EQ(OK, result);
-
-    if (!within_callback_) {
-      test_connect_job_factory_->set_job_type(next_job_type_);
-
-      // Don't allow reuse of the socket.  Disconnect it and then release it and
-      // run through the MessageLoop once to get it completely released.
-      handle_->socket()->Disconnect();
-      handle_->Reset();
-      {
-        // TODO: Resolve conflicting intentions of stopping recursion with the
-        // |!within_callback_| test (above) and the call to |RunUntilIdle()|
-        // below.  http://crbug.com/114130.
-        base::MessageLoop::ScopedNestableTaskAllower allow(
-            base::MessageLoop::current());
-        base::MessageLoop::current()->RunUntilIdle();
-      }
-      within_callback_ = true;
-      TestCompletionCallback next_job_callback;
-      scoped_refptr<TestSocketParams> params(
-          new TestSocketParams(false /* ignore_limits */));
-      int rv = handle_->Init("a",
-                             params,
-                             DEFAULT_PRIORITY,
-                             next_job_callback.callback(),
-                             pool_,
-                             BoundNetLog());
-      switch (next_job_type_) {
-        case TestConnectJob::kMockJob:
-          EXPECT_EQ(OK, rv);
-          break;
-        case TestConnectJob::kMockPendingJob:
-          EXPECT_EQ(ERR_IO_PENDING, rv);
-
-          // For pending jobs, wait for new socket to be created. This makes
-          // sure there are no more pending operations nor any unclosed sockets
-          // when the test finishes.
-          // We need to give it a little bit of time to run, so that all the
-          // operations that happen on timers (e.g. cleanup of idle
-          // connections) can execute.
-          {
-            base::MessageLoop::ScopedNestableTaskAllower allow(
-                base::MessageLoop::current());
-            base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(10));
-            EXPECT_EQ(OK, next_job_callback.WaitForResult());
-          }
-          break;
-        default:
-          FAIL() << "Unexpected job type: " << next_job_type_;
-          break;
-      }
-    }
+// Function to be used as a callback on socket request completion.  It first
+// disconnects the successfully connected socket from the first request, and
+// then reuses the ClientSocketHandle to request another socket.
+//
+// |nested_callback| is called with the result of the second socket request.
+void RequestSocketOnComplete(ClientSocketHandle* handle,
+                             TestClientSocketPool* pool,
+                             TestConnectJobFactory* test_connect_job_factory,
+                             TestConnectJob::JobType next_job_type,
+                             const CompletionCallback& nested_callback,
+                             int first_request_result) {
+  EXPECT_EQ(OK, first_request_result);
+
+  test_connect_job_factory->set_job_type(next_job_type);
+
+  // Don't allow reuse of the socket.  Disconnect it and then release it.
+  if (handle->socket())
+    handle->socket()->Disconnect();
+  handle->Reset();
+
+  scoped_refptr<TestSocketParams> params(
+      new TestSocketParams(false /* ignore_limits */));
+  TestCompletionCallback callback;
+  int rv =
+      handle->Init("a", params, LOWEST, nested_callback, pool, BoundNetLog());
+  if (rv != ERR_IO_PENDING) {
+    DCHECK_EQ(TestConnectJob::kMockJob, next_job_type);
+    nested_callback.Run(rv);
+  } else {
+    DCHECK_EQ(TestConnectJob::kMockPendingJob, next_job_type);
   }
+}
 
-  ClientSocketHandle* const handle_;
-  TestClientSocketPool* const pool_;
-  bool within_callback_;
-  TestConnectJobFactory* const test_connect_job_factory_;
-  TestConnectJob::JobType next_job_type_;
-  CompletionCallback callback_;
-};
-
+// Tests the case where a second socket is requested in a completion callback,
+// and the second socket connects asynchronously.  Reuses the same
+// ClientSocketHandle for the second socket, after disconnecting the first.
 TEST_F(ClientSocketPoolBaseTest, RequestPendingJobTwice) {
   CreatePool(kDefaultMaxSockets, kDefaultMaxSocketsPerGroup);
 
   connect_job_factory_->set_job_type(TestConnectJob::kMockPendingJob);
   ClientSocketHandle handle;
-  RequestSocketCallback callback(
-      &handle, pool_.get(), connect_job_factory_,
-      TestConnectJob::kMockPendingJob);
-  int rv = handle.Init("a",
-                       params_,
-                       DEFAULT_PRIORITY,
-                       callback.callback(),
-                       pool_.get(),
-                       BoundNetLog());
+  TestCompletionCallback second_result_callback;
+  int rv = handle.Init(
+      "a", params_, DEFAULT_PRIORITY,
+      base::Bind(&RequestSocketOnComplete, &handle, pool_.get(),
+                 connect_job_factory_, TestConnectJob::kMockPendingJob,
+                 second_result_callback.callback()),
+      pool_.get(), BoundNetLog());
   ASSERT_EQ(ERR_IO_PENDING, rv);
 
-  EXPECT_EQ(OK, callback.WaitForResult());
+  EXPECT_EQ(OK, second_result_callback.WaitForResult());
 }
 
+// Tests the case where a second socket is requested in a completion callback,
+// and the second socket connects synchronously.  Reuses the same
+// ClientSocketHandle for the second socket, after disconnecting the first.
 TEST_F(ClientSocketPoolBaseTest, RequestPendingJobThenSynchronous) {
   CreatePool(kDefaultMaxSockets, kDefaultMaxSocketsPerGroup);
 
   connect_job_factory_->set_job_type(TestConnectJob::kMockPendingJob);
   ClientSocketHandle handle;
-  RequestSocketCallback callback(
-      &handle, pool_.get(), connect_job_factory_, TestConnectJob::kMockJob);
-  int rv = handle.Init("a",
-                       params_,
-                       DEFAULT_PRIORITY,
-                       callback.callback(),
-                       pool_.get(),
-                       BoundNetLog());
+  TestCompletionCallback second_result_callback;
+  int rv = handle.Init(
+      "a", params_, DEFAULT_PRIORITY,
+      base::Bind(&RequestSocketOnComplete, &handle, pool_.get(),
+                 connect_job_factory_, TestConnectJob::kMockPendingJob,
+                 second_result_callback.callback()),
+      pool_.get(), BoundNetLog());
   ASSERT_EQ(ERR_IO_PENDING, rv);
 
-  EXPECT_EQ(OK, callback.WaitForResult());
+  EXPECT_EQ(OK, second_result_callback.WaitForResult());
 }
 
 // Make sure that pending requests get serviced after active requests get

net/socket/websocket_transport_client_socket_pool_unittest.cc

@@ -374,60 +374,38 @@ TEST_F(WebSocketTransportClientSocketPoolTest, CancelRequest) {
   EXPECT_EQ(ClientSocketPoolTest::kIndexOutOfBounds, GetOrderOfRequest(7));
 }
 
-class RequestSocketCallback : public TestCompletionCallbackBase {
- public:
-  RequestSocketCallback(ClientSocketHandle* handle,
-                        WebSocketTransportClientSocketPool* pool)
-      : handle_(handle),
-        pool_(pool),
-        within_callback_(false),
-        callback_(base::Bind(&RequestSocketCallback::OnComplete,
-                             base::Unretained(this))) {}
+// Function to be used as a callback on socket request completion.  It first
+// disconnects the successfully connected socket from the first request, and
+// then reuses the ClientSocketHandle to request another socket.  The second
+// request is expected to succeed asynchronously.
+//
+// |nested_callback| is called with the result of the second socket request.
+void RequestSocketOnComplete(ClientSocketHandle* handle,
+                             WebSocketTransportClientSocketPool* pool,
+                             const CompletionCallback& nested_callback,
+                             int first_request_result) {
+  EXPECT_EQ(OK, first_request_result);
+
+  // Don't allow reuse of the socket.  Disconnect it and then release it.
+  handle->socket()->Disconnect();
+  handle->Reset();
 
-  ~RequestSocketCallback() override {}
-
-  const CompletionCallback& callback() const { return callback_; }
-
- private:
-  void OnComplete(int result) {
-    SetResult(result);
-    ASSERT_EQ(OK, result);
-
-    if (!within_callback_) {
-      // Don't allow reuse of the socket.  Disconnect it and then release it and
-      // run through the MessageLoop once to get it completely released.
-      handle_->socket()->Disconnect();
-      handle_->Reset();
-      {
-        base::MessageLoop::ScopedNestableTaskAllower allow(
-            base::MessageLoop::current());
-        base::MessageLoop::current()->RunUntilIdle();
-      }
-      within_callback_ = true;
-      scoped_refptr<TransportSocketParams> dest(
-          new TransportSocketParams(
-              HostPortPair("www.google.com", 80),
-              false,
-              false,
-              OnHostResolutionCallback(),
-              TransportSocketParams::COMBINE_CONNECT_AND_WRITE_DEFAULT));
-      int rv =
-          handle_->Init("a", dest, LOWEST, callback(), pool_, BoundNetLog());
-      EXPECT_EQ(OK, rv);
-    }
-  }
-
-  ClientSocketHandle* const handle_;
-  WebSocketTransportClientSocketPool* const pool_;
-  bool within_callback_;
-  CompletionCallback callback_;
-
-  DISALLOW_COPY_AND_ASSIGN(RequestSocketCallback);
-};
+  scoped_refptr<TransportSocketParams> dest(new TransportSocketParams(
+      HostPortPair("www.google.com", 80), false, false,
+      OnHostResolutionCallback(),
+      TransportSocketParams::COMBINE_CONNECT_AND_WRITE_DEFAULT));
+  int rv =
+      handle->Init("a", dest, LOWEST, nested_callback, pool, BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  if (ERR_IO_PENDING != rv)
+    nested_callback.Run(rv);
+}
 
+// Tests the case where a second socket is requested in a completion callback,
+// and the second socket connects asynchronously.  Reuses the same
+// ClientSocketHandle for the second socket, after disconnecting the first.
 TEST_F(WebSocketTransportClientSocketPoolTest, RequestTwice) {
   ClientSocketHandle handle;
-  RequestSocketCallback callback(&handle, &pool_);
   scoped_refptr<TransportSocketParams> dest(
       new TransportSocketParams(
           HostPortPair("www.google.com", 80),
@@ -435,15 +413,13 @@ TEST_F(WebSocketTransportClientSocketPoolTest, RequestTwice) {
           false,
           OnHostResolutionCallback(),
           TransportSocketParams::COMBINE_CONNECT_AND_WRITE_DEFAULT));
-  int rv = handle.Init(
-      "a", dest, LOWEST, callback.callback(), &pool_, BoundNetLog());
+  TestCompletionCallback second_result_callback;
+  int rv = handle.Init("a", dest, LOWEST,
+                       base::Bind(&RequestSocketOnComplete, &handle, &pool_,
+                                  second_result_callback.callback()),
+                       &pool_, BoundNetLog());
   ASSERT_EQ(ERR_IO_PENDING, rv);
-
-  // The callback is going to request "www.google.com". We want it to complete
-  // synchronously this time.
-  host_resolver_->set_synchronous_mode(true);
-
-  EXPECT_EQ(OK, callback.WaitForResult());
+  EXPECT_EQ(OK, second_result_callback.WaitForResult());
 
   handle.Reset();
 }