Clean up weird ClientSocketPoolBase and WebSocket unit tests.

net/socket/websocket_transport_client_socket_pool_unittest.cc

 // Copyright 2014 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.
 
 #include "net/socket/websocket_transport_client_socket_pool.h"
 
 #include <queue>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 356 matching lines @@
   EXPECT_EQ(ClientSocketPoolTest::kRequestNotFound,
             GetOrderOfRequest(3));  // Canceled request.
   EXPECT_EQ(3, GetOrderOfRequest(4));
   EXPECT_EQ(4, GetOrderOfRequest(5));
   EXPECT_EQ(5, GetOrderOfRequest(6));
 
   // Make sure we test order of all requests made.
   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);
 
-  ~RequestSocketCallback() override {}
+  // Don't allow reuse of the socket.  Disconnect it and then release it.
+  handle->socket()->Disconnect();
+  handle->Reset();
 
-  const CompletionCallback& callback() const { return callback_; }
+  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);

[mmenke, 2015/05/18 21:07:55]

The old test was synchronous, but it was relying on spinning the message loop
re-entrantly, which was needed for the WebSocketEndpointLockManager to release
the the lock while spinning the message loop, something that normally can't
happen in the middle of this callback.

So I switched this test to be asynchronous.

+  if (ERR_IO_PENDING != rv)
+    nested_callback.Run(rv);
+}
 
- 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);
-};
-
+// 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(WebSocketTransportClientSocketPoolTest, RequestTwice) {
   ClientSocketHandle handle;
-  RequestSocketCallback callback(&handle, &pool_);
   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.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();
 }
 
 // Make sure that pending requests get serviced after active requests get
 // cancelled.
 TEST_F(WebSocketTransportClientSocketPoolTest,
        CancelActiveRequestWithPendingRequests) {
   client_socket_factory_.set_default_client_socket_type(
       MockTransportClientSocketFactory::MOCK_PENDING_CLIENT_SOCKET);
@@ skipping 675 matching lines @@
   EXPECT_EQ(OK, request(1)->WaitForResult());
   // Third socket should still be waiting for endpoint.
   ASSERT_FALSE(request(2)->handle()->is_initialized());
   EXPECT_EQ(LOAD_STATE_WAITING_FOR_AVAILABLE_SOCKET,
             request(2)->handle()->GetLoadState());
 }
 
 }  // namespace
 
 }  // namespace net