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..bc01bb6578327768c0db2b6fc2e7e0d60fc2bdf9 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,12 @@ 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;
+
+// This function avoids a bunch of boilerplate code.
+void AllowUnused(ChannelState ALLOW_UNUSED unused) {}
 
 }  // namespace
 
@@ -65,10 +72,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:
@@ -141,9 +150,10 @@ void WebSocketChannel::SendFrame(bool fin,
     return;
   }
   if (data.size() > base::checked_numeric_cast<size_t>(current_send_quota_)) {
-    FailChannel(SEND_GOING_AWAY,
-                kWebSocketMuxErrorSendQuotaViolation,
-                "Send quota exceeded");
+    AllowUnused(FailChannel(SEND_GOING_AWAY,
+                            kWebSocketMuxErrorSendQuotaViolation,
+                            "Send quota exceeded"));
+    // |this| is deleted here.
     return;
   }
   if (!WebSocketFrameHeader::IsKnownDataOpCode(op_code)) {
@@ -160,7 +170,8 @@ 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());
+  AllowUnused(SendIOBuffer(fin, op_code, buffer, data.size()));
+  // |this| may have been deleted.
 }
 
 void WebSocketChannel::SendFlowControl(int64 quota) {
@@ -180,9 +191,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
+  AllowUnused(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 +204,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 +231,50 @@ 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();
+  AllowUnused(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, "");
+  AllowUnused(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);
@@ -262,9 +283,8 @@ void WebSocketChannel::OnWriteDone(bool synchronous, int result) {
     case OK:
       if (data_to_send_next_) {
         data_being_sent_ = data_to_send_next_.Pass();
-        if (!synchronous) {
-          WriteFrames();
-        }
+        if (!synchronous)
+          return WriteFrames();
       } else {
         data_being_sent_.reset();
         if (current_send_quota_ < send_quota_low_water_mark_) {
@@ -279,10 +299,10 @@ 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);
+          return event_interface_->OnFlowControl(fresh_quota);
         }
       }
-      return;
+      return CHANNEL_ALIVE;

[tyoshino (SeeGerritForStatus), 2013/10/15 10:16:10]

between L303 and 304?

[Adam Rice, 2013/10/15 10:50:11]

I think I cannot move it up unless I add another return CHANNEL_ALIVE; statement
after line 287.

[tyoshino (SeeGerritForStatus), 2013/10/15 11:47:22]

Oh! sorry.

 
     // If a recoverable error condition existed, it would go here.
 
@@ -290,16 +310,14 @@ void WebSocketChannel::OnWriteDone(bool synchronous, int result) {
       DCHECK_LT(result, 0)
           << "WriteFrames() should only return OK or ERR_ codes";
       stream_->Close();
-      if (state_ != CLOSED) {
-        state_ = CLOSED;
-        event_interface_->OnDropChannel(kWebSocketErrorAbnormalClosure,
-                                        "Abnormal Closure");
-      }
-      return;
+      DCHECK_NE(CLOSED, state_);
+      state_ = CLOSED;
+      return event_interface_->OnDropChannel(kWebSocketErrorAbnormalClosure,
+                                             "Abnormal Closure");
   }
 }
 
-void WebSocketChannel::ReadFrames() {
+ChannelState WebSocketChannel::ReadFrames() {
   int result = OK;
   do {
     // This use of base::Unretained is safe because this object owns the
@@ -307,15 +325,19 @@ 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);
+    DCHECK_NE(CLOSED, state_);
+  } while (result == OK);
+  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,74 +350,69 @@ 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();
-      }
-      return;
+      DCHECK_NE(CLOSED, state_);
+      if (!synchronous)
+        return ReadFrames();
+      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)
           << "ReadFrames() should only return OK or ERR_ codes";
       stream_->Close();
-      if (state_ != CLOSED) {
-        state_ = CLOSED;
-        uint16 code = kWebSocketErrorAbnormalClosure;
-        std::string reason = "Abnormal Closure";
-        if (closing_code_ != 0) {
-          code = closing_code_;
-          reason = closing_reason_;
-        }
-        event_interface_->OnDropChannel(code, reason);
+      DCHECK_NE(CLOSED, state_);
+      state_ = CLOSED;
+      uint16 code = kWebSocketErrorAbnormalClosure;
+      std::string reason = "Abnormal Closure";
+      if (closing_code_ != 0) {
+        code = closing_code_;
+        reason = closing_reason_;
       }
-      return;
+      return event_interface_->OnDropChannel(code, reason);
   }
 }
 
-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) {
-    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_NE(CLOSED, state_);
+  if (state_ == CLOSE_WAIT) {
     std::string frame_name;
     switch (opcode) {
       case WebSocketFrameHeader::kOpCodeText:    // fall-thru
@@ -422,10 +439,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 +460,23 @@ 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);
-      } else {
-        VLOG(3) << "Ignored data packet received in state " << state_;
+        return event_interface_->OnDataFrame(final, opcode, data);
       }
-      return;
+      VLOG(3) << "Ignored data packet received in state " << state_;
+      return CHANNEL_ALIVE;
 
     case WebSocketFrameHeader::kOpCodePing:
       VLOG(1) << "Got Ping of size " << size;
-      if (state_ == CONNECTED) {
-        SendIOBuffer(
+      if (state_ == CONNECTED)
+        return SendIOBuffer(
             true, WebSocketFrameHeader::kOpCodePong, data_buffer, size);
-      } else {
-        VLOG(3) << "Ignored ping in state " << state_;
-      }
-      return;
+      VLOG(3) << "Ignored ping in state " << state_;
+      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 +488,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)

[tyoshino (SeeGerritForStatus), 2013/10/15 10:16:10]

break after ==?

[Adam Rice, 2013/10/15 10:50:11]

Done.

+            return CHANNEL_DELETED;
+          if (event_interface_->OnClosingHandshake() == CHANNEL_DELETED)
+            return CHANNEL_DELETED;
           closing_code_ = code;
           closing_reason_ = reason;
           break;
@@ -494,20 +510,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));
@@ -524,20 +540,20 @@ void WebSocketChannel::SendIOBuffer(bool fin,
     if (!data_to_send_next_)
       data_to_send_next_.reset(new SendBuffer);
     data_to_send_next_->AddFrame(frame.Pass());
-  } else {
-    data_being_sent_.reset(new SendBuffer);
-    data_being_sent_->AddFrame(frame.Pass());
-    WriteFrames();
+    return CHANNEL_ALIVE;
   }
+  data_being_sent_.reset(new SendBuffer);
+  data_being_sent_->AddFrame(frame.Pass());
+  return WriteFrames();
 }
 
-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_);
+  DCHECK_NE(CLOSED, state_);
   // TODO(ricea): Logging.
-  State old_state = state_;
   if (state_ == CONNECTED) {
     uint16 send_code = kWebSocketErrorGoingAway;
     std::string send_reason = "Internal Error";
@@ -545,7 +561,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

[tyoshino (SeeGerritForStatus), 2013/10/15 10:16:10]

ditto

[Adam Rice, 2013/10/15 10:50:11]

Done.

+        == 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
@@ -553,12 +571,11 @@ void WebSocketChannel::FailChannel(ExposeError expose,
   stream_->Close();
   state_ = CLOSED;
 
-  if (old_state != CLOSED) {
-    event_interface_->OnDropChannel(code, reason);
-  }
+  return event_interface_->OnDropChannel(code, reason);
 }
 
-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 +594,13 @@ 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;
+  // SendIOBuffer() checks |state_|, so it is best not to change it until after
+  // SendIOBuffer() returns.
   state_ = (state_ == CONNECTED) ? SEND_CLOSED : CLOSE_WAIT;
+  return CHANNEL_ALIVE;
 }
 
 void WebSocketChannel::ParseClose(const scoped_refptr<IOBuffer>& buffer,
@@ -611,8 +633,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);
   }