Make net::WebSocketChannel deletion safe and enable new IPCs

net/websockets/websocket_channel.cc

Index: net/websockets/websocket_channel.cc
diff --git a/net/websockets/websocket_channel.cc b/net/websockets/websocket_channel.cc
index c370bd7d40ef782bb66c30e2cd1aee62cdbae2e6..b7c0b41809d1e1a4d6af2836c33c684028514641 100644
--- a/net/websockets/websocket_channel.cc
+++ b/net/websockets/websocket_channel.cc
@@ -8,6 +8,7 @@
 
 #include "base/basictypes.h"  // for size_t
 #include "base/bind.h"
+#include "base/compiler_specific.h"
 #include "base/safe_numerics.h"
 #include "base/strings/string_util.h"
 #include "net/base/big_endian.h"
@@ -26,6 +27,9 @@ namespace {
 const int kDefaultSendQuotaLowWaterMark = 1 << 16;
 const int kDefaultSendQuotaHighWaterMark = 1 << 17;
 const size_t kWebSocketCloseCodeLength = 2;
+typedef WebSocketEventInterface::ChannelState ChannelState;
+const ChannelState CHANNEL_ALIVE = WebSocketEventInterface::CHANNEL_ALIVE;
+const ChannelState CHANNEL_DELETED = WebSocketEventInterface::CHANNEL_DELETED;
 
 }  // namespace
 
@@ -65,10 +69,12 @@ class WebSocketChannel::ConnectDelegate
 
   virtual void OnSuccess(scoped_ptr<WebSocketStream> stream) OVERRIDE {
     creator_->OnConnectSuccess(stream.Pass());
+    // |this| may have been deleted.
   }
 
   virtual void OnFailure(uint16 websocket_error) OVERRIDE {
     creator_->OnConnectFailure(websocket_error);
+    // |this| has been deleted.
   }
 
  private:
@@ -144,6 +150,7 @@ void WebSocketChannel::SendFrame(bool fin,
     FailChannel(SEND_GOING_AWAY,
                 kWebSocketMuxErrorSendQuotaViolation,
                 "Send quota exceeded");
+    // |this| is deleted here.
     return;
   }
   if (!WebSocketFrameHeader::IsKnownDataOpCode(op_code)) {
@@ -160,7 +167,9 @@ void WebSocketChannel::SendFrame(bool fin,
   // TODO(ricea): For kOpCodeText, do UTF-8 validation?
   scoped_refptr<IOBuffer> buffer(new IOBuffer(data.size()));
   std::copy(data.begin(), data.end(), buffer->data());
-  SendIOBuffer(fin, op_code, buffer, data.size());
+  ChannelState unused ALLOW_UNUSED =
+      SendIOBuffer(fin, op_code, buffer, data.size());
+  // |this| may have been deleted.
 }
 
 void WebSocketChannel::SendFlowControl(int64 quota) {
@@ -180,9 +189,12 @@ void WebSocketChannel::StartClosingHandshake(uint16 code,
     NOTREACHED() << "StartClosingHandshake() called in state " << state_;
     return;
   }
-  // TODO(ricea): Validate |code|? Check that |reason| is valid UTF-8?
+  // TODO(ricea): Validate |code|
   // TODO(ricea): There should be a timeout for the closing handshake.
-  SendClose(code, reason);  // Sets state_ to SEND_CLOSED
+  ChannelState unused ALLOW_UNUSED = SendClose(
+      code, IsStringUTF8(reason) ? reason : std::string());  // Sets state_ to
+                                                             // SEND_CLOSED
+  // If |unused| is CHANNEL_DELETED, then |this| has been deleted.
 }
 
 void WebSocketChannel::SendAddChannelRequestForTesting(
@@ -190,10 +202,8 @@ void WebSocketChannel::SendAddChannelRequestForTesting(
     const std::vector<std::string>& requested_subprotocols,
     const GURL& origin,
     const WebSocketStreamFactory& factory) {
-  SendAddChannelRequestWithFactory(socket_url,
-                                   requested_subprotocols,
-                                   origin,
-                                   factory);
+  SendAddChannelRequestWithFactory(
+      socket_url, requested_subprotocols, origin, factory);
 }
 
 void WebSocketChannel::SendAddChannelRequestWithFactory(
@@ -219,41 +229,51 @@ void WebSocketChannel::OnConnectSuccess(scoped_ptr<WebSocketStream> stream) {
   DCHECK_EQ(CONNECTING, state_);
   stream_ = stream.Pass();
   state_ = CONNECTED;
-  event_interface_->OnAddChannelResponse(false, stream_->GetSubProtocol());
+  if (event_interface_->OnAddChannelResponse(
+          false, stream_->GetSubProtocol()) == CHANNEL_DELETED)
+    return;
 
   // TODO(ricea): Get flow control information from the WebSocketStream once we
   // have a multiplexing WebSocketStream.
   current_send_quota_ = send_quota_high_water_mark_;
-  event_interface_->OnFlowControl(send_quota_high_water_mark_);
+  if (event_interface_->OnFlowControl(send_quota_high_water_mark_) ==
+      CHANNEL_DELETED)
+    return;
 
   // |stream_request_| is not used once the connection has succeeded.
   stream_request_.reset();
-  ReadFrames();
+  ChannelState unused ALLOW_UNUSED = ReadFrames();
+  // |this| may have been deleted.
 }
 
 void WebSocketChannel::OnConnectFailure(uint16 websocket_error) {
   DCHECK_EQ(CONNECTING, state_);
   state_ = CLOSED;
   stream_request_.reset();
-  event_interface_->OnAddChannelResponse(true, "");
+  ChannelState unused ALLOW_UNUSED =
+      event_interface_->OnAddChannelResponse(true, "");
+  // |this| has been deleted.
 }
 
-void WebSocketChannel::WriteFrames() {
+ChannelState WebSocketChannel::WriteFrames() {
   int result = OK;
   do {
     // This use of base::Unretained is safe because this object owns the
     // WebSocketStream and destroying it cancels all callbacks.
     result = stream_->WriteFrames(
         data_being_sent_->frames(),
-        base::Bind(
-            &WebSocketChannel::OnWriteDone, base::Unretained(this), false));
+        base::Bind(base::IgnoreResult(&WebSocketChannel::OnWriteDone),
+                   base::Unretained(this),
+                   false));
     if (result != ERR_IO_PENDING) {
-      OnWriteDone(true, result);
+      if (OnWriteDone(true, result) == CHANNEL_DELETED)
+        return CHANNEL_DELETED;
     }
   } while (result == OK && data_being_sent_);
+  return CHANNEL_ALIVE;
 }
 
-void WebSocketChannel::OnWriteDone(bool synchronous, int result) {
+ChannelState WebSocketChannel::OnWriteDone(bool synchronous, int result) {
   DCHECK_NE(FRESHLY_CONSTRUCTED, state_);
   DCHECK_NE(CONNECTING, state_);
   DCHECK_NE(ERR_IO_PENDING, result);
@@ -263,7 +283,8 @@ void WebSocketChannel::OnWriteDone(bool synchronous, int result) {
       if (data_to_send_next_) {
         data_being_sent_ = data_to_send_next_.Pass();
         if (!synchronous) {
-          WriteFrames();
+          if (WriteFrames() == CHANNEL_DELETED)
+            return CHANNEL_DELETED;
         }
       } else {
         data_being_sent_.reset();
@@ -279,10 +300,11 @@ void WebSocketChannel::OnWriteDone(bool synchronous, int result) {
           // server, if the protocol in use supports quota.
           int fresh_quota = send_quota_high_water_mark_ - current_send_quota_;
           current_send_quota_ += fresh_quota;
-          event_interface_->OnFlowControl(fresh_quota);
+          if (event_interface_->OnFlowControl(fresh_quota) == CHANNEL_DELETED)
+            return CHANNEL_DELETED;
         }
       }
-      return;
+      return CHANNEL_ALIVE;
 
     // If a recoverable error condition existed, it would go here.
 
@@ -292,14 +314,14 @@ void WebSocketChannel::OnWriteDone(bool synchronous, int result) {
       stream_->Close();
       if (state_ != CLOSED) {

[yhirano, 2013/10/10 09:22:27]

Given that event_interface_->OnDropChannel() and
event_interface_->OnAddChannelResponse(true, ...) delete the
WebSocketDispatcherHost (hence WebSocketChannel) immediately, there can't be a
WebSocketChannel with state_ == CLOSED.
Can we have DCHECK_NE(state_, CLOSED)? Or maybe we don't have to define CLOSED
at all.

[Adam Rice, 2013/10/11 05:42:54]

Yes. I changed all the conditions on CLOSED to DCHECKs. I want to keep the
CLOSED state, because it found a bug in my tests, and could find bugs in real
code in future.

An implementation of WebSocketStream is not supposed to call any callbacks after
Close() has been called, but one of my test implementations was breaking that
contract. Some future implementation of WebSocketStream might also break the
contract (actually, WebSocketBasicStream can break the contract if it is using
an implementation of StreamSocket that breaks its own contract).

         state_ = CLOSED;
-        event_interface_->OnDropChannel(kWebSocketErrorAbnormalClosure,
-                                        "Abnormal Closure");
+        return event_interface_->OnDropChannel(kWebSocketErrorAbnormalClosure,
+                                               "Abnormal Closure");
       }
-      return;
+      return CHANNEL_ALIVE;
   }
 }
 
-void WebSocketChannel::ReadFrames() {
+ChannelState WebSocketChannel::ReadFrames() {
   int result = OK;
   do {
     // This use of base::Unretained is safe because this object owns the
@@ -307,15 +329,18 @@ void WebSocketChannel::ReadFrames() {
     // destroyed.
     result = stream_->ReadFrames(
         &read_frames_,
-        base::Bind(
-            &WebSocketChannel::OnReadDone, base::Unretained(this), false));
+        base::Bind(base::IgnoreResult(&WebSocketChannel::OnReadDone),
+                   base::Unretained(this),
+                   false));
     if (result != ERR_IO_PENDING) {
-      OnReadDone(true, result);
+      if (OnReadDone(true, result) == CHANNEL_DELETED)
+        return CHANNEL_DELETED;
     }
   } while (result == OK && state_ != CLOSED);
+  return CHANNEL_ALIVE;
 }
 
-void WebSocketChannel::OnReadDone(bool synchronous, int result) {
+ChannelState WebSocketChannel::OnReadDone(bool synchronous, int result) {
   DCHECK_NE(FRESHLY_CONSTRUCTED, state_);
   DCHECK_NE(CONNECTING, state_);
   DCHECK_NE(ERR_IO_PENDING, result);
@@ -328,21 +353,22 @@ void WebSocketChannel::OnReadDone(bool synchronous, int result) {
       for (size_t i = 0; i < read_frames_.size(); ++i) {
         scoped_ptr<WebSocketFrame> frame(read_frames_[i]);
         read_frames_[i] = NULL;
-        ProcessFrame(frame.Pass());
+        if (ProcessFrame(frame.Pass()) == CHANNEL_DELETED)
+          return CHANNEL_DELETED;
       }
       read_frames_.clear();
       // There should always be a call to ReadFrames pending.
       // TODO(ricea): Unless we are out of quota.
       if (!synchronous && state_ != CLOSED) {
-        ReadFrames();
+        if (ReadFrames() == CHANNEL_DELETED)
+          return CHANNEL_DELETED;
       }
-      return;
+      return CHANNEL_ALIVE;
 
     case ERR_WS_PROTOCOL_ERROR:
-      FailChannel(SEND_REAL_ERROR,
-                  kWebSocketErrorProtocolError,
-                  "WebSocket Protocol Error");
-      return;
+      return FailChannel(SEND_REAL_ERROR,
+                         kWebSocketErrorProtocolError,
+                         "WebSocket Protocol Error");
 
     default:
       DCHECK_LT(result, 0)
@@ -356,46 +382,44 @@ void WebSocketChannel::OnReadDone(bool synchronous, int result) {
           code = closing_code_;
           reason = closing_reason_;
         }
-        event_interface_->OnDropChannel(code, reason);
+        return event_interface_->OnDropChannel(code, reason);
       }
-      return;
+      NOTREACHED();
+      return CHANNEL_ALIVE;
   }
 }
 
-void WebSocketChannel::ProcessFrame(scoped_ptr<WebSocketFrame> frame) {
+ChannelState WebSocketChannel::ProcessFrame(scoped_ptr<WebSocketFrame> frame) {
   if (frame->header.masked) {
     // RFC6455 Section 5.1 "A client MUST close a connection if it detects a
     // masked frame."
-    FailChannel(SEND_REAL_ERROR,
-                kWebSocketErrorProtocolError,
-                "Masked frame from server");
-    return;
+    return FailChannel(SEND_REAL_ERROR,
+                       kWebSocketErrorProtocolError,
+                       "Masked frame from server");
   }
   const WebSocketFrameHeader::OpCode opcode = frame->header.opcode;
   if (WebSocketFrameHeader::IsKnownControlOpCode(opcode) &&
       !frame->header.final) {
-    FailChannel(SEND_REAL_ERROR,
-                kWebSocketErrorProtocolError,
-                "Control message with FIN bit unset received");
-    return;
+    return FailChannel(SEND_REAL_ERROR,
+                       kWebSocketErrorProtocolError,
+                       "Control message with FIN bit unset received");
   }
 
   // Respond to the frame appropriately to its type.
-  HandleFrame(
+  return HandleFrame(
       opcode, frame->header.final, frame->data, frame->header.payload_length);
 }
 
-void WebSocketChannel::HandleFrame(const WebSocketFrameHeader::OpCode opcode,
-                                   bool final,
-                                   const scoped_refptr<IOBuffer>& data_buffer,
-                                   size_t size) {
+ChannelState WebSocketChannel::HandleFrame(
+    const WebSocketFrameHeader::OpCode opcode,
+    bool final,
+    const scoped_refptr<IOBuffer>& data_buffer,
+    size_t size) {
   DCHECK_NE(RECV_CLOSED, state_)
       << "HandleFrame() does not support being called re-entrantly from within "
          "SendClose()";
   if (state_ == CLOSED || state_ == CLOSE_WAIT) {

[yhirano, 2013/10/10 09:22:27]

Same as L315

[Adam Rice, 2013/10/11 05:42:54]

Done.

-    DVLOG_IF(1, state_ == CLOSED) << "A frame was received while in the CLOSED "
-                                     "state. This is possible after a channel "
-                                     "failed, but should be very rare.";
+    DCHECK(state_ != CLOSED);
     std::string frame_name;
     switch (opcode) {
       case WebSocketFrameHeader::kOpCodeText:    // fall-thru
@@ -422,10 +446,9 @@ void WebSocketChannel::HandleFrame(const WebSocketFrameHeader::OpCode opcode,
     }
     // SEND_REAL_ERROR makes no difference here, as FailChannel() won't send
     // another Close frame.
-    FailChannel(SEND_REAL_ERROR,
-                kWebSocketErrorProtocolError,
-                frame_name + " received after close");
-    return;
+    return FailChannel(SEND_REAL_ERROR,
+                       kWebSocketErrorProtocolError,
+                       frame_name + " received after close");
   }
   switch (opcode) {
     case WebSocketFrameHeader::kOpCodeText:    // fall-thru
@@ -444,26 +467,30 @@ void WebSocketChannel::HandleFrame(const WebSocketFrameHeader::OpCode opcode,
         // cause of receiving very large chunks.
 
         // Sends the received frame to the renderer process.
-        event_interface_->OnDataFrame(final, opcode, data);
+        if (event_interface_->OnDataFrame(final, opcode, data) ==
+            CHANNEL_DELETED)
+          return CHANNEL_DELETED;
       } else {
         VLOG(3) << "Ignored data packet received in state " << state_;
       }
-      return;
+      return CHANNEL_ALIVE;
 
     case WebSocketFrameHeader::kOpCodePing:
       VLOG(1) << "Got Ping of size " << size;
       if (state_ == CONNECTED) {
-        SendIOBuffer(
-            true, WebSocketFrameHeader::kOpCodePong, data_buffer, size);
+        if (SendIOBuffer(
+                true, WebSocketFrameHeader::kOpCodePong, data_buffer, size) ==
+            CHANNEL_DELETED)
+          return CHANNEL_DELETED;
       } else {
         VLOG(3) << "Ignored ping in state " << state_;
       }
-      return;
+      return CHANNEL_ALIVE;
 
     case WebSocketFrameHeader::kOpCodePong:
       VLOG(1) << "Got Pong of size " << size;
       // There is no need to do anything with pong messages.
-      return;
+      return CHANNEL_ALIVE;
 
     case WebSocketFrameHeader::kOpCodeClose: {
       uint16 code = kWebSocketNormalClosure;
@@ -475,8 +502,11 @@ void WebSocketChannel::HandleFrame(const WebSocketFrameHeader::OpCode opcode,
       switch (state_) {
         case CONNECTED:
           state_ = RECV_CLOSED;
-          SendClose(code, reason);  // Sets state_ to CLOSE_WAIT
-          event_interface_->OnClosingHandshake();
+          if (SendClose(code, reason)  // Sets state_ to CLOSE_WAIT
+              == CHANNEL_DELETED)
+            return CHANNEL_DELETED;
+          if (event_interface_->OnClosingHandshake() == CHANNEL_DELETED)
+            return CHANNEL_DELETED;
           closing_code_ = code;
           closing_reason_ = reason;
           break;
@@ -494,20 +524,20 @@ void WebSocketChannel::HandleFrame(const WebSocketFrameHeader::OpCode opcode,
           LOG(DFATAL) << "Got Close in unexpected state " << state_;
           break;
       }
-      return;
+      return CHANNEL_ALIVE;
     }
 
     default:
-      FailChannel(
+      return FailChannel(
           SEND_REAL_ERROR, kWebSocketErrorProtocolError, "Unknown opcode");
-      return;
   }
 }
 
-void WebSocketChannel::SendIOBuffer(bool fin,
-                                    WebSocketFrameHeader::OpCode op_code,
-                                    const scoped_refptr<IOBuffer>& buffer,
-                                    size_t size) {
+ChannelState WebSocketChannel::SendIOBuffer(
+    bool fin,
+    WebSocketFrameHeader::OpCode op_code,
+    const scoped_refptr<IOBuffer>& buffer,
+    size_t size) {
   DCHECK(state_ == CONNECTED || state_ == RECV_CLOSED);
   DCHECK(stream_);
   scoped_ptr<WebSocketFrame> frame(new WebSocketFrame(op_code));
@@ -527,13 +557,15 @@ void WebSocketChannel::SendIOBuffer(bool fin,
   } else {
     data_being_sent_.reset(new SendBuffer);
     data_being_sent_->AddFrame(frame.Pass());
-    WriteFrames();
+    if (WriteFrames() == CHANNEL_DELETED)
+      return CHANNEL_DELETED;
   }
+  return CHANNEL_ALIVE;
 }
 
-void WebSocketChannel::FailChannel(ExposeError expose,
-                                   uint16 code,
-                                   const std::string& reason) {
+ChannelState WebSocketChannel::FailChannel(ExposeError expose,
+                                           uint16 code,
+                                           const std::string& reason) {
   DCHECK_NE(FRESHLY_CONSTRUCTED, state_);
   DCHECK_NE(CONNECTING, state_);
   // TODO(ricea): Logging.
@@ -545,7 +577,9 @@ void WebSocketChannel::FailChannel(ExposeError expose,
       send_code = code;
       send_reason = reason;
     }
-    SendClose(send_code, send_reason);  // Sets state_ to SEND_CLOSED
+    if (SendClose(send_code, send_reason)  // Sets state_ to SEND_CLOSED
+        == CHANNEL_DELETED)
+      return CHANNEL_DELETED;
   }
   // Careful study of RFC6455 section 7.1.7 and 7.1.1 indicates the browser
   // should close the connection itself without waiting for the closing
@@ -554,11 +588,14 @@ void WebSocketChannel::FailChannel(ExposeError expose,
   state_ = CLOSED;
 
   if (old_state != CLOSED) {

[yhirano, 2013/10/10 09:22:27]

Same as L315

[Adam Rice, 2013/10/11 05:42:54]

Done.

-    event_interface_->OnDropChannel(code, reason);
+    return event_interface_->OnDropChannel(code, reason);
   }
+  NOTREACHED();
+  return CHANNEL_ALIVE;
 }
 
-void WebSocketChannel::SendClose(uint16 code, const std::string& reason) {
+ChannelState WebSocketChannel::SendClose(uint16 code,
+                                         const std::string& reason) {
   DCHECK(state_ == CONNECTED || state_ == RECV_CLOSED);
   // TODO(ricea): Ensure reason.length() <= 123
   scoped_refptr<IOBuffer> body;
@@ -577,8 +614,11 @@ void WebSocketChannel::SendClose(uint16 code, const std::string& reason) {
     std::copy(
         reason.begin(), reason.end(), body->data() + kWebSocketCloseCodeLength);
   }
-  SendIOBuffer(true, WebSocketFrameHeader::kOpCodeClose, body, size);
+  if (SendIOBuffer(true, WebSocketFrameHeader::kOpCodeClose, body, size) ==
+      CHANNEL_DELETED)
+    return CHANNEL_DELETED;
   state_ = (state_ == CONNECTED) ? SEND_CLOSED : CLOSE_WAIT;
+  return CHANNEL_ALIVE;
 }
 
 void WebSocketChannel::ParseClose(const scoped_refptr<IOBuffer>& buffer,
@@ -611,8 +651,8 @@ void WebSocketChannel::ParseClose(const scoped_refptr<IOBuffer>& buffer,
     *code = kWebSocketErrorAbnormalClosure;
   }
   std::string text(data + kWebSocketCloseCodeLength, data + size);
-  // TODO(ricea): Is this check strict enough? In particular, check the
-  // "Security Considerations" from RFC3629.
+  // IsStringUTF8() blocks surrogate pairs and non-characters, so it is strictly
+  // stronger than required by RFC3629.
   if (IsStringUTF8(text)) {
     reason->swap(text);
   }