Remove weak pointers from CastSocket.

extensions/browser/api/cast_channel/cast_socket.cc

Index: extensions/browser/api/cast_channel/cast_socket.cc
diff --git a/extensions/browser/api/cast_channel/cast_socket.cc b/extensions/browser/api/cast_channel/cast_socket.cc
index f446bff88d53957c8a4bad5af185a03b251dea62..38a6477b5d10507b67da56b0297a8a5b7cacf093 100644
--- a/extensions/browser/api/cast_channel/cast_socket.cc
+++ b/extensions/browser/api/cast_channel/cast_socket.cc
@@ -99,7 +99,9 @@ CastSocket::CastSocket(const std::string& owner_extension_id,
   current_read_buffer_ = header_read_buffer_;
 }
 
-CastSocket::~CastSocket() { }
+CastSocket::~CastSocket() {
+  CloseInternal();
+}
 
 ReadyState CastSocket::ready_state() const {
   return ready_state_;
@@ -176,19 +178,21 @@ void CastSocket::Connect(const net::CompletionCallback& callback) {
   connect_callback_ = callback;
   connect_state_ = CONN_STATE_TCP_CONNECT;
   if (connect_timeout_.InMicroseconds() > 0) {
-    GetTimer()->Start(
-        FROM_HERE,
-        connect_timeout_,
-        base::Bind(&CastSocket::CancelConnect, AsWeakPtr()));
+    DCHECK(cancel_connect_callback_.is_null());
+    cancel_connect_callback_ = base::Bind(&CastSocket::CancelConnect,
+                                          base::Unretained(this));
+    GetTimer()->Start(FROM_HERE, connect_timeout_, cancel_connect_callback_);
   }
   DoConnectLoop(net::OK);
 }
 
 void CastSocket::PostTaskToStartConnectLoop(int result) {
   DCHECK(CalledOnValidThread());
-  base::MessageLoop::current()->PostTask(
-      FROM_HERE,
-      base::Bind(&CastSocket::DoConnectLoop, AsWeakPtr(), result));
+  DCHECK(connect_loop_callback_.is_null());
+  connect_loop_callback_ = base::Bind(&CastSocket::DoConnectLoop,
+                                      base::Unretained(this),
+                                      result);
+  base::MessageLoop::current()->PostTask(FROM_HERE, connect_loop_callback_);

[Kevin M, 2014/07/28 22:08:16]

Should this be turned into PostNonNestableTask now, or should that be in a
different CL?

[Wez, 2014/07/28 23:14:56]

PostNonNestableTask() isn't required here, since we don't nest
MessageLoop::Run() in production code - it should always be safe to call
PostTask(), w/out risking re-entrancy within the PostTask itself.

[Wez, 2014/07/28 23:14:56]

This will blow up if PostTaskToStartConnectLoop() is called and something
deletes the CastSocket before the task is processed.

[mark a. foltz, 2014/07/29 00:16:24]

So Reset()-ing the callback in the dtor is a no-op?

Is there any way to cancel a pending task once submitted to the MessageLoop or
must we rely on indirect techniques like WeakPtr?

[mark a. foltz, 2014/07/29 00:16:25]

Acknowledged.

[Wez, 2014/07/29 00:41:25]

base::Closure is ref-counted, so when you message_loop->PostTask(..., a) and
then a.Reset(), the message_loop's queue still holds a reference to the task,
even though |a| itself is cleared.
There is base::CancelableCallback. Internally, though, that just uses
WeakPtrFactory. MessageQueues don't support cancellation, AFAIK, presumably
because that would complicate them and impact performance of the common case of
non-cancelled tasks.

 }
 
 void CastSocket::CancelConnect() {
@@ -261,8 +265,9 @@ int CastSocket::DoTcpConnect() {
   VLOG_WITH_CONNECTION(1) << "DoTcpConnect";
   connect_state_ = CONN_STATE_TCP_CONNECT_COMPLETE;
   tcp_socket_ = CreateTcpSocket();
+  DCHECK(connect_loop_callback_.is_null());
   return tcp_socket_->Connect(
-      base::Bind(&CastSocket::DoConnectLoop, AsWeakPtr()));
+      base::Bind(&CastSocket::DoConnectLoop, base::Unretained(this)));
 }
 
 int CastSocket::DoTcpConnectComplete(int result) {
@@ -280,8 +285,9 @@ int CastSocket::DoSslConnect() {
   VLOG_WITH_CONNECTION(1) << "DoSslConnect";
   connect_state_ = CONN_STATE_SSL_CONNECT_COMPLETE;
   socket_ = CreateSslSocket(tcp_socket_.PassAs<net::StreamSocket>());
+  DCHECK(connect_loop_callback_.is_null());
   return socket_->Connect(
-      base::Bind(&CastSocket::DoConnectLoop, AsWeakPtr()));
+      base::Bind(&CastSocket::DoConnectLoop, base::Unretained(this)));
 }
 
 int CastSocket::DoSslConnectComplete(int result) {
@@ -305,13 +311,17 @@ int CastSocket::DoAuthChallengeSend() {
                           << CastMessageToString(challenge_message);
   // Post a task to send auth challenge so that DoWriteLoop is not nested inside
   // DoConnectLoop. This is not strictly necessary but keeps the write loop
-  // code decoupled from connect loop code.
-  base::MessageLoop::current()->PostTask(
-      FROM_HERE,
-      base::Bind(&CastSocket::SendCastMessageInternal,
-                 AsWeakPtr(),
-                 challenge_message,
-                 base::Bind(&CastSocket::DoConnectLoop, AsWeakPtr())));
+  // code decoupleld from connect loop code.

[Kevin M, 2014/07/28 22:08:16]

typo

[mark a. foltz, 2014/07/29 00:16:24]

Done.

+  DCHECK(connect_loop_callback_.is_null());
+  DCHECK(send_auth_challenge_callback_.is_null());
+  send_auth_challenge_callback_ =
+    base::Bind(&CastSocket::SendCastMessageInternal,

[Kevin M, 2014/07/28 22:08:16]

4 space indentation

[mark a. foltz, 2014/07/29 00:16:25]

Done.

+               base::Unretained(this),
+               challenge_message,
+               base::Bind(&CastSocket::DoConnectLoop,
+                          base::Unretained(this)));
+  base::MessageLoop::current()->PostTask(FROM_HERE,
+                                         send_auth_challenge_callback_);

[Wez, 2014/07/28 23:14:56]

This will blow up, as above.

[mark a. foltz, 2014/07/29 00:16:25]

Acknowledged.

   // Always return IO_PENDING since the result is always asynchronous.
   return net::ERR_IO_PENDING;
 }
@@ -354,15 +364,32 @@ void CastSocket::DoConnectCallback(int result) {
 }
 
 void CastSocket::Close(const net::CompletionCallback& callback) {
+  CloseInternal();
+  callback.Run(net::OK);
+  // |callback| can delete |this|
+}
+
+void CastSocket::CloseInternal() {
   DCHECK(CalledOnValidThread());
+  if (ready_state_ == READY_STATE_CLOSED) {
+    return;
+  }
   VLOG_WITH_CONNECTION(1) << "Close ReadyState = " << ready_state_;
   tcp_socket_.reset();
   socket_.reset();
   cert_verifier_.reset();
   transport_security_state_.reset();
+  // Reset callbacks passed into us.  Or should we invoke them with an error?

[Kevin M, 2014/07/28 22:08:16]

Stop the timer?

[mark a. foltz, 2014/07/29 00:16:24]

Done.

+  connect_callback_.Reset();

[Kevin M, 2014/07/28 22:08:16]

Question: does resetting the callbacks also dequeue them from the message loop?

[mark a. foltz, 2014/07/29 00:16:24]

Good question.

+  for (; !write_queue_.empty(); write_queue_.pop()) {
+    write_queue_.front().callback.Reset();
+  }
+  // Reset callbacks that we queued ourselves.
+  connect_loop_callback_.Reset();
+  read_loop_callback_.Reset();
+  cancel_connect_callback_.Reset();
+  send_auth_challenge_callback_.Reset();
   ready_state_ = READY_STATE_CLOSED;
-  callback.Run(net::OK);
-  // |callback| can delete |this|
 }
 
 void CastSocket::SendMessage(const MessageInfo& message,
@@ -454,11 +481,10 @@ int CastSocket::DoWrite() {
                           << request.io_buffer->BytesConsumed();
 
   write_state_ = WRITE_STATE_WRITE_COMPLETE;
-
   return socket_->Write(
       request.io_buffer.get(),
       request.io_buffer->BytesRemaining(),
-      base::Bind(&CastSocket::DoWriteLoop, AsWeakPtr()));
+      base::Bind(&CastSocket::DoWriteLoop, base::Unretained(this)));
 }
 
 int CastSocket::DoWriteComplete(int result) {
@@ -526,9 +552,10 @@ int CastSocket::DoWriteError(int result) {
 
 void CastSocket::PostTaskToStartReadLoop() {
   DCHECK(CalledOnValidThread());
-  base::MessageLoop::current()->PostTask(
-      FROM_HERE,
-      base::Bind(&CastSocket::StartReadLoop, AsWeakPtr()));
+  DCHECK(read_loop_callback_.is_null());
+  read_loop_callback_ = base::Bind(&CastSocket::StartReadLoop,
+                                   base::Unretained(this));
+  base::MessageLoop::current()->PostTask(FROM_HERE, read_loop_callback_);

[Wez, 2014/07/28 23:14:56]

Boom!

[mark a. foltz, 2014/07/29 00:16:25]

Acknowledged.

 }
 
 void CastSocket::StartReadLoop() {
@@ -603,7 +630,7 @@ int CastSocket::DoRead() {
   return socket_->Read(
       current_read_buffer_.get(),
       num_bytes_to_read,
-      base::Bind(&CastSocket::DoReadLoop, AsWeakPtr()));
+      base::Bind(&CastSocket::DoReadLoop, base::Unretained(this)));
 }
 
 int CastSocket::DoReadComplete(int result) {
@@ -723,8 +750,7 @@ bool CastSocket::Serialize(const CastMessage& message_proto,
 
 void CastSocket::CloseWithError(ChannelError error) {
   DCHECK(CalledOnValidThread());
-  socket_.reset(NULL);
-  ready_state_ = READY_STATE_CLOSED;
+  CloseInternal();
   error_state_ = error;
   if (delegate_)
     delegate_->OnError(this, error);
@@ -756,7 +782,7 @@ void CastSocket::MessageHeader::SetMessageSize(size_t size) {
 void CastSocket::MessageHeader::PrependToString(std::string* str) {
   MessageHeader output = *this;
   output.message_size = base::HostToNet32(message_size);
-  size_t header_size = base::checked_cast<size_t,uint32>(
+  size_t header_size = base::checked_cast<size_t, uint32>(
       MessageHeader::header_size());
   scoped_ptr<char, base::FreeDeleter> char_array(
       static_cast<char*>(malloc(header_size)));
@@ -769,7 +795,7 @@ void CastSocket::MessageHeader::PrependToString(std::string* str) {
 void CastSocket::MessageHeader::ReadFromIOBuffer(
     net::GrowableIOBuffer* buffer, MessageHeader* header) {
   uint32 message_size;
-  size_t header_size = base::checked_cast<size_t,uint32>(
+  size_t header_size = base::checked_cast<size_t, uint32>(
       MessageHeader::header_size());
   memcpy(&message_size, buffer->StartOfBuffer(), header_size);
   header->message_size = base::NetToHost32(message_size);