Fix potential teardown race conditions with TCPEngineTransport's PostTasks.

blimp/net/tcp_engine_transport.cc

Index: blimp/net/tcp_engine_transport.cc
diff --git a/blimp/net/tcp_engine_transport.cc b/blimp/net/tcp_engine_transport.cc
index 9d18f1770b6e716af391af87d2d07564b9b11f89..a4dfdfbe01c23344c0bbb12ecdc0ac3b65d88216 100644
--- a/blimp/net/tcp_engine_transport.cc
+++ b/blimp/net/tcp_engine_transport.cc
@@ -22,7 +22,7 @@ namespace blimp {
 
 TCPEngineTransport::TCPEngineTransport(const net::IPEndPoint& address,
                                        net::NetLog* net_log)
-    : address_(address), net_log_(net_log) {}
+    : address_(address), net_log_(net_log), weak_factory_(this) {}
 
 TCPEngineTransport::~TCPEngineTransport() {}
 
@@ -42,25 +42,20 @@ void TCPEngineTransport::Connect(const net::CompletionCallback& callback) {
     }
   }
 
-  net::CompletionCallback accept_callback = base::Bind(
-      &TCPEngineTransport::OnTCPConnectAccepted, base::Unretained(this));
+  net::CompletionCallback accept_callback =
+      base::Bind(&TCPEngineTransport::OnTCPConnectAccepted,
+                 weak_factory_.GetWeakPtr(), callback);

[Wez, 2016/11/18 02:35:47]

nit: Up to you, but I quite liked that the |callback| got saved into the object,
and later ResetAndReturn().Run()'d, since there are other stateful aspects to
this, e.g. the |accepted_socket_| anyway. It also has the nice property that you
can DCHECK(!connect_callback_) at the top of Connect() to trap duplicate calls,
if you want.

[Kevin M, 2016/11/18 18:17:38]

Yeah, that's true. I'll revert that change.

 
   int result = server_socket_->Accept(&accepted_socket_, accept_callback);
   if (result == net::ERR_IO_PENDING) {
-    connect_callback_ = callback;
     return;
   }
 
-  if (result != net::OK) {
-    server_socket_.reset();
-  }
-
-  base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
-                                                base::Bind(callback, result));
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(accept_callback, result));
 }
 
 std::unique_ptr<MessagePort> TCPEngineTransport::TakeMessagePort() {
-  DCHECK(connect_callback_.is_null());
   DCHECK(accepted_socket_);
   return MessagePort::CreateForStreamSocketWithCompression(
       std::move(accepted_socket_));
@@ -79,13 +74,15 @@ void TCPEngineTransport::GetLocalAddress(net::IPEndPoint* address) const {
   server_socket_->GetLocalAddress(address);
 }
 
-void TCPEngineTransport::OnTCPConnectAccepted(int result) {
+void TCPEngineTransport::OnTCPConnectAccepted(net::CompletionCallback callback,
+                                              int result) {
   DCHECK_NE(net::ERR_IO_PENDING, result);
   DCHECK(accepted_socket_);
   if (result != net::OK) {
+    server_socket_.reset();

[Wez, 2016/11/18 02:35:47]

nit: Is this strictly correct? I have vague recollections of rare situations in
which you can be woken to accept an incoming TCP connection, only to find that
Accept returns nothing, at least under Linux...

[Kevin M, 2016/11/18 18:17:38]

Done.

     accepted_socket_.reset();
   }
-  base::ResetAndReturn(&connect_callback_).Run(result);
+  base::ResetAndReturn(&callback).Run(result);

[Wez, 2016/11/18 02:35:47]

nit: If you leave this being passed the callback as a parameter then you don't
really need ResetAndReturn :)

[Kevin M, 2016/11/18 18:17:38]

Acknowledged.

 }
 
 }  // namespace blimp