Do not check if callback is null in test Socket classes.

Do not check if callback is null in test-only AsyncSocket subclasses in
RunCallback().  If the method was supposed to handle the case of null
callback, it should be called MaybeRunCallback() instead.

Add DCHECKs on callback at call sites to document expectations.

Also, avoid double move when unnecessary.

Also, fix GCMConnectionHandlerImplTests.  Calling
MockTCPClientSocket::Connect() with null callback on ASYNC mock data
is not consistent with the async idiom used in //net.  Sure it didn't
use to blow up, because MockClientSocket::RunCallback() special-cased
null callbacks.  :(

Also, use make_unique instead of new throughout
google_apis/gcm/engine/connection_handler_impl_unittest.cc.

Bug: 807724

Change-Id: I3adb8d2a09c87fa004b1b4e7a8cf6f0edff064a6
Reviewed-on: https://chromium-review.googlesource.com/1064193
Commit-Queue: Bence Béky <bnc@chromium.org>
Reviewed-by: Eric Roman <eroman@chromium.org>
Reviewed-by: Nicolas Zea <zea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561528}

google_apis/gcm/engine/connection_handler_impl_unittest.cc

@@ -22,9 +22,11 @@
 #include "google_apis/gcm/base/socket_stream.h"
 #include "google_apis/gcm/protocol/mcs.pb.h"
 #include "net/base/ip_address.h"
+#include "net/base/test_completion_callback.h"
 #include "net/log/net_log_source.h"
 #include "net/socket/socket_test_util.h"
 #include "net/socket/stream_socket.h"
+#include "net/test/gtest_util.h"
 #include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -206,37 +208,39 @@ net::StreamSocket* GCMConnectionHandlerImplTest::BuildSocket(
     const WriteList& write_list) {
   mock_reads_ = read_list;
   mock_writes_ = write_list;
-  data_provider_.reset(
-      new net::StaticSocketDataProvider(mock_reads_, mock_writes_));
+  data_provider_ = std::make_unique<net::StaticSocketDataProvider>(
+      mock_reads_, mock_writes_);
   socket_factory_.AddSocketDataProvider(data_provider_.get());
+  run_loop_ = std::make_unique<base::RunLoop>();
 
   socket_ = socket_factory_.CreateTransportClientSocket(
       address_list_, NULL, NULL, net::NetLogSource());
-  socket_->Connect(net::CompletionCallback());
+  net::TestCompletionCallback callback;
+  int rv = socket_->Connect(callback.callback());
+  EXPECT_THAT(rv, net::test::IsError(net::ERR_IO_PENDING));
 
-  run_loop_.reset(new base::RunLoop());
-  PumpLoop();
+  rv = callback.WaitForResult();
+  EXPECT_THAT(rv, net::test::IsOk());
 
-  DCHECK(socket_->IsConnected());
+  EXPECT_TRUE(socket_->IsConnected());
   return socket_.get();
 }
 
 void GCMConnectionHandlerImplTest::PumpLoop() {
   run_loop_->RunUntilIdle();
-  run_loop_.reset(new base::RunLoop());
+  run_loop_ = std::make_unique<base::RunLoop>();
 }
 
 void GCMConnectionHandlerImplTest::Connect(
     ScopedMessage* dst_proto) {
-  connection_handler_.reset(new ConnectionHandlerImpl(
+  connection_handler_ = std::make_unique<ConnectionHandlerImpl>(
       TestTimeouts::tiny_timeout(),
-          base::Bind(&GCMConnectionHandlerImplTest::ReadContinuation,
-                     base::Unretained(this),
-                     dst_proto),
-          base::Bind(&GCMConnectionHandlerImplTest::WriteContinuation,
-                     base::Unretained(this)),
-          base::Bind(&GCMConnectionHandlerImplTest::ConnectionContinuation,
-                     base::Unretained(this))));
+      base::Bind(&GCMConnectionHandlerImplTest::ReadContinuation,
+                 base::Unretained(this), dst_proto),
+      base::Bind(&GCMConnectionHandlerImplTest::WriteContinuation,
+                 base::Unretained(this)),
+      base::Bind(&GCMConnectionHandlerImplTest::ConnectionContinuation,
+                 base::Unretained(this)));
   EXPECT_FALSE(connection_handler()->CanSendMessage());
   connection_handler_->Init(*BuildLoginRequest(kAuthId, kAuthToken, ""),
                             TRAFFIC_ANNOTATION_FOR_TESTS, socket_.get());
@@ -251,7 +255,7 @@ void GCMConnectionHandlerImplTest::ReadContinuation(
 
 void GCMConnectionHandlerImplTest::WaitForMessage() {
   run_loop_->Run();
-  run_loop_.reset(new base::RunLoop());
+  run_loop_ = std::make_unique<base::RunLoop>();
 }
 
 void GCMConnectionHandlerImplTest::WriteContinuation() {

net/socket/socket_test_util.cc

@@ -879,8 +879,7 @@ void MockClientSocket::RunCallbackAsync(CompletionOnceCallback callback,
 
 void MockClientSocket::RunCallback(CompletionOnceCallback callback,
                                    int result) {
-  if (!callback.is_null())
-    std::move(callback).Run(result);
+  std::move(callback).Run(result);
 }
 
 MockTCPClientSocket::MockTCPClientSocket(const AddressList& addresses,
@@ -918,6 +917,8 @@ int MockTCPClientSocket::Read(IOBuffer* buf,
       buf, buf_len,
       base::Bind(&MockTCPClientSocket::RetryRead, base::Unretained(this)));
   if (rv == ERR_IO_PENDING) {
+    DCHECK(callback);
+
     pending_read_buf_ = buf;
     pending_read_buf_len_ = buf_len;
     pending_read_callback_ = std::move(callback);
@@ -1014,6 +1015,8 @@ int MockTCPClientSocket::Connect(CompletionOnceCallback callback) {
   if (mode == SYNCHRONOUS)
     return result;
 
+  DCHECK(callback);
+
   if (result == ERR_IO_PENDING)
     pending_connect_callback_ = std::move(callback);
   else
@@ -1090,8 +1093,7 @@ void MockTCPClientSocket::OnWriteComplete(int rv) {
 
   // There must be a read pending.
   DCHECK(!pending_write_callback_.is_null());
-  CompletionOnceCallback callback = std::move(pending_write_callback_);
-  RunCallback(std::move(callback), rv);
+  RunCallback(std::move(pending_write_callback_), rv);
 }
 
 void MockTCPClientSocket::OnConnectComplete(const MockConnect& data) {
@@ -1099,8 +1101,7 @@ void MockTCPClientSocket::OnConnectComplete(const MockConnect& data) {
   if (!data_)
     return;
 
-  CompletionOnceCallback callback = std::move(pending_connect_callback_);
-  RunCallback(std::move(callback), data.result);
+  RunCallback(std::move(pending_connect_callback_), data.result);
 }
 
 void MockTCPClientSocket::OnDataProviderDestroyed() {
@@ -1342,8 +1343,7 @@ void MockProxyClientSocket::RunCallbackAsync(CompletionOnceCallback callback,
 
 void MockProxyClientSocket::RunCallback(CompletionOnceCallback callback,
                                         int result) {
-  if (!callback.is_null())
-    std::move(callback).Run(result);
+  std::move(callback).Run(result);
 }
 
 // static
@@ -1527,8 +1527,7 @@ void MockSSLClientSocket::RunCallbackAsync(CompletionOnceCallback callback,
 
 void MockSSLClientSocket::RunCallback(CompletionOnceCallback callback,
                                       int result) {
-  if (!callback.is_null())
-    std::move(callback).Run(result);
+  std::move(callback).Run(result);
 }
 
 void MockSSLClientSocket::OnReadComplete(const MockRead& data) {
@@ -1569,6 +1568,8 @@ MockUDPClientSocket::~MockUDPClientSocket() {
 int MockUDPClientSocket::Read(IOBuffer* buf,
                               int buf_len,
                               CompletionOnceCallback callback) {
+  DCHECK(callback);
+
   if (!connected_ || !data_)
     return ERR_UNEXPECTED;
   data_transferred_ = true;
@@ -1603,6 +1604,7 @@ int MockUDPClientSocket::Write(
     const NetworkTrafficAnnotationTag& /* traffic_annotation */) {
   DCHECK(buf);
   DCHECK_GT(buf_len, 0);
+  DCHECK(callback);
 
   if (!connected_ || !data_)
     return ERR_UNEXPECTED;
@@ -1631,6 +1633,7 @@ int MockUDPClientSocket::WriteAsync(
     const NetworkTrafficAnnotationTag& /* traffic_annotation */) {
   DCHECK(buffer);
   DCHECK_GT(buf_len, 0u);
+  DCHECK(callback);
 
   if (!connected_ || !data_)
     return ERR_UNEXPECTED;
@@ -1657,6 +1660,7 @@ int MockUDPClientSocket::WriteAsync(
     CompletionOnceCallback callback,
     const NetworkTrafficAnnotationTag& /* traffic_annotation */) {
   DCHECK(!buffers.empty());
+  DCHECK(callback);
 
   if (!connected_ || !data_)
     return ERR_UNEXPECTED;
@@ -1785,6 +1789,7 @@ void MockUDPClientSocket::OnReadComplete(const MockRead& data) {
 
   // There must be a read pending.
   DCHECK(pending_read_buf_.get());
+  DCHECK(pending_read_callback_);
   // You can't complete a read with another ERR_IO_PENDING status code.
   DCHECK_NE(ERR_IO_PENDING, data.result);
   // Since we've been waiting for data, need_read_data_ should be true.
@@ -1808,8 +1813,7 @@ void MockUDPClientSocket::OnWriteComplete(int rv) {
 
   // There must be a read pending.
   DCHECK(!pending_write_callback_.is_null());
-  CompletionOnceCallback callback = std::move(pending_write_callback_);
-  RunCallback(std::move(callback), rv);
+  RunCallback(std::move(pending_write_callback_), rv);
 }
 
 void MockUDPClientSocket::OnConnectComplete(const MockConnect& data) {
@@ -1866,8 +1870,7 @@ void MockUDPClientSocket::RunCallbackAsync(CompletionOnceCallback callback,
 
 void MockUDPClientSocket::RunCallback(CompletionOnceCallback callback,
                                       int result) {
-  if (!callback.is_null())
-    std::move(callback).Run(result);
+  std::move(callback).Run(result);
 }
 
 TestSocketRequest::TestSocketRequest(