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..b1f4984e7b0a8e648044a44da85168d6eec192ed 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,24 @@ 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_.IsCancelled());
+    cancel_connect_callback_.Reset(base::Bind(&CastSocket::CancelConnect,
+                                              base::Unretained(this)));

[Wez, 2014/07/30 19:14:53]

nit: Style guide would prefer wrapping this after "Reset(".

[mark a. foltz, 2014/07/31 18:33:17]

It prefers wrapping at parenthesis

+    GetTimer()->Start(FROM_HERE,
+                      connect_timeout_,
+                      cancel_connect_callback_.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_.IsCancelled());
+  connect_loop_callback_.Reset(base::Bind(&CastSocket::DoConnectLoop,
+                                          base::Unretained(this),
+                                          result));
+  base::MessageLoop::current()->PostTask(FROM_HERE,
+                                         connect_loop_callback_.callback());
 }
 
 void CastSocket::CancelConnect() {
@@ -204,6 +211,7 @@ void CastSocket::CancelConnect() {
 // 1. Connect method: this starts the flow
 // 2. Callback from network operations that finish asynchronously
 void CastSocket::DoConnectLoop(int result) {
+  connect_loop_callback_.Cancel();
   if (is_canceled_) {
     LOG(ERROR) << "CANCELLED - Aborting DoConnectLoop.";
     return;
@@ -261,8 +269,9 @@ int CastSocket::DoTcpConnect() {
   VLOG_WITH_CONNECTION(1) << "DoTcpConnect";
   connect_state_ = CONN_STATE_TCP_CONNECT_COMPLETE;
   tcp_socket_ = CreateTcpSocket();
+  DCHECK(connect_loop_callback_.IsCancelled());

[Wez, 2014/07/30 19:14:53]

nit: Check this on entry to DoTcpConnect()?

[Wez, 2014/07/30 19:14:53]

IIUC this is basically checking that after a PostTaskToRestartConnectLoop we
haven't somehow come around DoConnectLoop again, without having first returned
to the main loop?

[mark a. foltz, 2014/07/31 18:33:17]

Done.

[mark a. foltz, 2014/07/31 18:33:17]

There should be at most one pending callback to reenter the connect loop. 
Having multiple pending callbacks (from both net:: and the MessageLoop) would
mean the behavior of the state machine becomes indeterminate.

   return tcp_socket_->Connect(
-      base::Bind(&CastSocket::DoConnectLoop, AsWeakPtr()));
+      base::Bind(&CastSocket::DoConnectLoop, base::Unretained(this)));
 }
 
 int CastSocket::DoTcpConnectComplete(int result) {
@@ -280,8 +289,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_.IsCancelled());

[Wez, 2014/07/30 19:14:53]

nit: Check this on entry to the method?

[mark a. foltz, 2014/07/31 18:33:16]

Done.

   return socket_->Connect(
-      base::Bind(&CastSocket::DoConnectLoop, AsWeakPtr()));
+      base::Bind(&CastSocket::DoConnectLoop, base::Unretained(this)));
 }
 
 int CastSocket::DoSslConnectComplete(int result) {
@@ -306,16 +316,28 @@ int CastSocket::DoAuthChallengeSend() {
   // 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,
+  DCHECK(send_auth_challenge_callback_.IsCancelled());
+  send_auth_challenge_callback_.Reset(
       base::Bind(&CastSocket::SendCastMessageInternal,
-                 AsWeakPtr(),
+                 base::Unretained(this),
                  challenge_message,
-                 base::Bind(&CastSocket::DoConnectLoop, AsWeakPtr())));
+                 base::Bind(&CastSocket::DoAuthChallengeSendWriteComplete,
+                            base::Unretained(this))));

[Wez, 2014/07/30 19:14:53]

Ick... do we really need a clean stack for SendCastMessageInternal as well as
for the completion callback...?

IIUC the problem here is basically with actions that the Delegate might perform
in error callbacks, in which case can we simplify all this code to avoid the
need for stack-popping via PostTask on operations like this by just using
PostTask to deliver the callbacks to the Delegate...?

[mark a. foltz, 2014/07/31 18:33:17]

It appears to be "not strictly necessary" but otherwise the control flow for the
two loops gets entangled.
No I think even in the normal flow we wanted to avoid mutual recursion between
DoConnectLoop and DoWriteLoop.

+  base::MessageLoop::current()->PostTask(
+      FROM_HERE,
+      send_auth_challenge_callback_.callback());
   // Always return IO_PENDING since the result is always asynchronous.
   return net::ERR_IO_PENDING;
 }
 
+void CastSocket::DoAuthChallengeSendWriteComplete(int result) {
+  send_auth_challenge_callback_.Cancel();
+  VLOG_WITH_CONNECTION(2) << "DoAuthChallengeSendWriteComplete: " << result;
+  DCHECK_GT(result, 0);
+  DCHECK_EQ(write_queue_.size(), 1UL);
+  PostTaskToStartConnectLoop(result);
+}
+
 int CastSocket::DoAuthChallengeSendComplete(int result) {
   VLOG_WITH_CONNECTION(1) << "DoAuthChallengeSendComplete: " << result;
   if (result < 0)
@@ -354,15 +376,36 @@ void CastSocket::DoConnectCallback(int result) {
 }
 
 void CastSocket::Close(const net::CompletionCallback& callback) {
-  DCHECK(CalledOnValidThread());
+  CloseInternal();
+  callback.Run(net::OK);
+  // |callback| can delete |this|

[Wez, 2014/07/30 19:14:53]

nit: Callers must inherently cope with asynchronous completion of close, so you
could just PostTask() this, and deletion wouldn't be an issue?

[mark a. foltz, 2014/07/31 18:33:16]

This comment is out of date and removed.  Deletion can happen at any time via
the ApiResourceManager if the extension is disabled or the browser shuts down.

+}
+
+void CastSocket::CloseInternal() {
+  // TODO(mfoltz): Enforce this when CastChannelAPITest is rewritten to create
+  // and free sockets on the same thread.  crbug.com/398242
+  // 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();
+  GetTimer()->Stop();
+  // Reset callbacks passed into us.  Or should we invoke them with an error?

[Wez, 2014/07/30 19:14:53]

nit: Suggest blank line before this comment, and before the comment below, to
visually separate this function into logical blocks.

[Wez, 2014/07/30 19:14:53]

If the socket is being closed then there shouldn't be any problem with just
dropping write callbacks, I don't think?

[mark a. foltz, 2014/07/31 18:33:16]

Done.

[mark a. foltz, 2014/07/31 18:33:17]

Actually, there may be a Promise pending on the caller side waiting for the
callback to complete, so now I fire them.  This required refactoring
cast_channel_api.cc since we cannot guarantee the API callbacks are run while
the socket still exists in the ApiResourceManager.

+  connect_callback_.Reset();
+  for (; !write_queue_.empty(); write_queue_.pop()) {
+    write_queue_.front().callback.Reset();
+  }
+  // Cancel callbacks that we queued ourselves to re-enter the connect or read
+  // loops.
+  connect_loop_callback_.Cancel();
+  send_auth_challenge_callback_.Cancel();
+  read_loop_callback_.Cancel();
+  cancel_connect_callback_.Cancel();
   ready_state_ = READY_STATE_CLOSED;
-  callback.Run(net::OK);
-  // |callback| can delete |this|
 }
 
 void CastSocket::SendMessage(const MessageInfo& message,
@@ -377,7 +420,6 @@ void CastSocket::SendMessage(const MessageInfo& message,
     callback.Run(net::ERR_FAILED);
     return;
   }
-
   SendCastMessageInternal(message_proto, callback);
 }
 
@@ -454,11 +496,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) {
@@ -483,21 +524,11 @@ int CastSocket::DoWriteComplete(int result) {
 
 int CastSocket::DoWriteCallback() {
   DCHECK(!write_queue_.empty());
+  write_state_ = WRITE_STATE_WRITE;
   WriteRequest& request = write_queue_.front();
   int bytes_consumed = request.io_buffer->BytesConsumed();
-
-  // If inside connection flow, then there should be exaclty one item in
-  // the write queue.
-  if (ready_state_ == READY_STATE_CONNECTING) {
-    write_queue_.pop();
-    DCHECK(write_queue_.empty());
-    PostTaskToStartConnectLoop(bytes_consumed);
-  } else {
-    WriteRequest& request = write_queue_.front();
-    request.callback.Run(bytes_consumed);
-    write_queue_.pop();
-  }
-  write_state_ = WRITE_STATE_WRITE;
+  request.callback.Run(bytes_consumed);
+  write_queue_.pop();
   return net::OK;
 }
 
@@ -526,12 +557,15 @@ 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_.IsCancelled());
+  read_loop_callback_.Reset(base::Bind(&CastSocket::StartReadLoop,
+                                       base::Unretained(this)));
+  base::MessageLoop::current()->PostTask(FROM_HERE,
+                                         read_loop_callback_.callback());
 }
 
 void CastSocket::StartReadLoop() {
+  read_loop_callback_.Cancel();
   // Read loop would have already been started if read state is not NONE
   if (read_state_ == READ_STATE_NONE) {
     read_state_ = READ_STATE_READ;
@@ -603,7 +637,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 +757,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 +789,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 +802,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);