Fix error handling for message parse errors that occur during connection setup.

chrome/browser/extensions/api/cast_channel/cast_socket.cc

Index: chrome/browser/extensions/api/cast_channel/cast_socket.cc
diff --git a/chrome/browser/extensions/api/cast_channel/cast_socket.cc b/chrome/browser/extensions/api/cast_channel/cast_socket.cc
index 6130d92e18755dc825880177b785f7536baf5afc..15ed3ce0ce9b12ad07ec3f3094e52e317f4ec643 100644
--- a/chrome/browser/extensions/api/cast_channel/cast_socket.cc
+++ b/chrome/browser/extensions/api/cast_channel/cast_socket.cc
@@ -158,7 +158,7 @@ bool CastSocket::ExtractPeerCert(std::string* cert) {
 }
 
 bool CastSocket::VerifyChallengeReply() {
-  return AuthenticateChallengeReply(*challenge_reply_.get(), peer_cert_);
+  return AuthenticateChallengeReply(*challenge_reply_, peer_cert_);

[mark a. foltz, 2014/07/21 17:38:52]

Thank you for fixing this, in my email reply I forgot scoped_ptr overloads * and
does the right thing: CHECK-fails on NULL dereference.

[Kevin M, 2014/07/21 20:48:58]

Acknowledged.

 }
 
 void CastSocket::Connect(const net::CompletionCallback& callback) {
@@ -591,6 +591,7 @@ int CastSocket::DoReadComplete(int result) {
     if (!ProcessHeader()) {
       error_state_ = cast_channel::CHANNEL_ERROR_INVALID_MESSAGE;
       read_state_ = READ_STATE_ERROR;
+      return net::ERR_FAILED;

[Wez, 2014/07/22 22:33:09]

IIUC this change will mean we get read error notifications for CastChannels that
have neither succeeded nor failed connecting yet, which probably isn't what we
want?

[Kevin M, 2014/07/23 18:57:33]

This was removed.

     }
   } else if (current_read_buffer_.get() == body_read_buffer_.get() &&
              static_cast<uint32>(current_read_buffer_->offset()) ==
@@ -601,6 +602,7 @@ int CastSocket::DoReadComplete(int result) {
     } else {
       error_state_ = cast_channel::CHANNEL_ERROR_INVALID_MESSAGE;
       read_state_ = READ_STATE_ERROR;
+      return net::ERR_FAILED;
     }
   }
 
@@ -609,7 +611,7 @@ int CastSocket::DoReadComplete(int result) {
 
 int CastSocket::DoReadCallback() {
   read_state_ = READ_STATE_READ;
-  const CastMessage& message = *(current_message_.get());
+  const CastMessage& message = *current_message_;
   if (IsAuthMessage(message)) {
     // An auth message is received, check that connect flow is running.
     if (ready_state_ == READY_STATE_CONNECTING) {
@@ -630,13 +632,14 @@ int CastSocket::DoReadCallback() {
 }
 
 int CastSocket::DoReadError(int result) {
+  VLOG_WITH_CONNECTION(1) << "DoReadError: " << result;
   DCHECK_LE(result, 0);
+  read_state_ = READ_STATE_NONE;
   // If inside connection flow, then get back to connect loop.

[mark a. foltz, 2014/07/21 17:38:52]

I don't fully understand this piece of logic. A read error should immediately
abort the connection attempt and not restart the connect loop.  I think this is
trying to guarantee that we don't invoke the connect callback (to signal the
error) from a different callback passed to net::socket.  However this is
indirect and I'm not really convinced it's correct (what's the invariant on 
connect_state_ here)?

At the least, make sure that there is a test case that a read error during
connect does the right thing, and possibly add a CHECK-guard to guarantee that
the connect loop exits on a read error.  When Munjal is back, we can maybe think
about if there is a cleaner way to handle the control flow - I believe he has
more context for why we are posting tasks to hop between loops.

[Kevin M, 2014/07/21 20:48:58]

Changed the flow to call the completion callback directly instead of via the
messageloop. PTAL. Given the simplistic nature of the revised function (<any
error>=><ERR_FAILED>) we could eliminate this node from the state machine
entirely...

[mark a. foltz, 2014/07/21 21:23:00]

I'm not 100% comfortable changing the control flow in this way without review by
Munjal.  The issue is that the completion callback may delete the CastSocket on
the API side, and we don't want to do that inside a callback on the net:: side.

Let's do that cleanup in a separate patch.

[Kevin M, 2014/07/21 23:50:31]

Done. OK, back to the previous method.

   if (ready_state_ == READY_STATE_CONNECTING) {
     PostTaskToStartConnectLoop(result);
-    // does not try to report error also.

[Wez, 2014/07/22 22:33:09]

This comment implies that if we return something other than net::OK then we're
going to get a read-failure notification sent to the caller, even though we
haven't yet informed them of the connection succeeding, i.e. when they won't be
expecting one?

-    return net::OK;
   }
+  // Allow the read loop error handler to close the connection.
   return net::ERR_FAILED;
 }