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..48c2b75852ea307eb29546d16056082cb9f54cb2 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() {}
 
@@ -43,20 +43,17 @@ void TCPEngineTransport::Connect(const net::CompletionCallback& callback) {
   }
 
   net::CompletionCallback accept_callback = base::Bind(
-      &TCPEngineTransport::OnTCPConnectAccepted, base::Unretained(this));
+      &TCPEngineTransport::OnTCPConnectAccepted, weak_factory_.GetWeakPtr());
 
+  connect_callback_ = callback;

[perumaal, 2016/11/17 22:44:22]

Could you just pass the connect_callback_ as a parameter to OnTCPConnectAccepted
using base::Bind? This gets rid of this temporary yet permanent member variable.
You could remove the weak pointer in this case.

(Case in point:
https://codereview.chromium.org/2509163002/diff/20001/blimp/net/tcp_engine_tr...)

[Kevin M, 2016/11/17 23:02:49]

From the bug:
"TCPEngineTransport::Connect() uses PostTask to complete asynchronously in the
case of the underlying TCP Accept() call completely synchronously, but posts the
completion-callback directly as the task.  This means that tearing down the
Transport may still leave the completion callback in-flight, which callers would
not typically expect to be the case."

[Kevin M, 2016/11/17 23:05:41]

Sorry, I misread. Yes, I can move the callback as a bound param. Can't remove
the weak pointer, though - it guarantees that the callback is only invoked if
|this| is alive.

   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(&TCPEngineTransport::OnTCPConnectAccepted,
+                            weak_factory_.GetWeakPtr(), result));
 }
 
 std::unique_ptr<MessagePort> TCPEngineTransport::TakeMessagePort() {
@@ -83,6 +80,7 @@ void TCPEngineTransport::OnTCPConnectAccepted(int result) {
   DCHECK_NE(net::ERR_IO_PENDING, result);
   DCHECK(accepted_socket_);
   if (result != net::OK) {
+    server_socket_.reset();
     accepted_socket_.reset();
   }
   base::ResetAndReturn(&connect_callback_).Run(result);