WebSocketChannel implementation

net/websockets/websocket_channel.cc

+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/websockets/websocket_channel.h"
+
+#include <algorithm>
+
+#include "base/basictypes.h"  // for size_t
+#include "base/bind.h"
+#include "base/safe_numerics.h"
+#include "base/strings/string_util.h"
+#include "net/base/big_endian.h"
+#include "net/base/io_buffer.h"
+#include "net/base/net_log.h"
+#include "net/websockets/websocket_errors.h"
+#include "net/websockets/websocket_event_interface.h"
+#include "net/websockets/websocket_frame.h"
+#include "net/websockets/websocket_mux.h"
+#include "net/websockets/websocket_stream.h"
+
+namespace net {
+
+namespace {
+
+const int kDefaultSendQuotaLowWaterMark = 1 << 16;
+const int kDefaultSendQuotaHighWaterMark = 1 << 17;
+const size_t kWebSocketCloseCodeLength = 2;
+
+// Concatenate the data from two IOBufferWithSize objects into a single one.
+IOBufferWithSize* ConcatenateIOBuffers(
+    const scoped_refptr<IOBufferWithSize>& part1,
+    const scoped_refptr<IOBufferWithSize>& part2) {
+  int newsize = part1->size() + part2->size();
+  IOBufferWithSize* newbuffer = new IOBufferWithSize(newsize);
+  std::copy(part1->data(), part1->data() + part1->size(), newbuffer->data());
+  std::copy(part2->data(),
+            part2->data() + part2->size(),
+            newbuffer->data() + part1->size());
+  return newbuffer;
+}
+
+}  // namespace
+
+struct WebSocketChannel::SendBuffer {
+  SendBuffer() : total_bytes(0) {}
+  ScopedVector<WebSocketFrameChunk> frames;
+  size_t total_bytes;
+};
+
+// Implementation of WebSocketStream::ConnectDelegate that simply forwards the
+// calls on to the WebSocketChannel that created it.
+class WebSocketChannel::ConnectDelegate
+    : public WebSocketStream::ConnectDelegate {
+ public:
+  explicit ConnectDelegate(WebSocketChannel* creator) : creator_(creator) {}
+
+  virtual void OnSuccess(scoped_ptr<WebSocketStream> stream) OVERRIDE {
+    creator_->OnConnectSuccess(stream.Pass());
+  }
+
+  virtual void OnFailure(uint16 websocket_error) OVERRIDE {
+    creator_->OnConnectFailure(websocket_error);
+  }
+
+ private:
+  // A pointer to the WebSocketChannel that created us. We do not need to worry
+  // about this pointer being stale, because deleting WebSocketChannel cancels
+  // the connect process, deleting this object and preventing its callbacks from
+  // being called.
+  WebSocketChannel* const creator_;
+
+  DISALLOW_COPY_AND_ASSIGN(ConnectDelegate);
+};
+
+WebSocketChannel::WebSocketChannel(
+    const GURL& socket_url,
+    scoped_ptr<WebSocketEventInterface> event_interface)
+    : socket_url_(socket_url),
+      event_interface_(event_interface.Pass()),
+      send_quota_low_water_mark_(kDefaultSendQuotaLowWaterMark),
+      send_quota_high_water_mark_(kDefaultSendQuotaHighWaterMark),
+      current_send_quota_(0),
+      state_(FRESHLY_CONSTRUCTED) {}
+
+WebSocketChannel::~WebSocketChannel() {
+  // The stream may hold a pointer to read_frame_chunks_, and so it needs to be
+  // destroyed first.
+  stream_.reset();
+}
+
+void WebSocketChannel::SendAddChannelRequest(
+    const std::vector<std::string>& requested_subprotocols,
+    const GURL& origin,
+    URLRequestContext* url_request_context) {
+  // Delegate to the tested version.
+  SendAddChannelRequestWithFactory(
+      requested_subprotocols,
+      origin,
+      url_request_context,
+      base::Bind(&WebSocketStream::CreateAndConnectStream));
+}
+
+void WebSocketChannel::SendAddChannelRequestWithFactory(
+    const std::vector<std::string>& requested_subprotocols,
+    const GURL& origin,
+    URLRequestContext* url_request_context,
+    base::Callback<scoped_ptr<WebSocketStreamRequest>(
+        const GURL&,
+        const std::vector<std::string>&,
+        const GURL&,
+        URLRequestContext*,
+        const BoundNetLog&,
+        scoped_ptr<WebSocketStream::ConnectDelegate>)> factory) {
+  DCHECK_EQ(FRESHLY_CONSTRUCTED, state_);
+  scoped_ptr<WebSocketStream::ConnectDelegate> connect_delegate(
+      new WebSocketChannel::ConnectDelegate(this));
+  stream_request_ = factory.Run(socket_url_,
+                                requested_subprotocols,
+                                origin,
+                                url_request_context,
+                                BoundNetLog(),
+                                connect_delegate.Pass());
+  state_ = CONNECTING;
+}
+
+void WebSocketChannel::OnConnectSuccess(scoped_ptr<WebSocketStream> stream) {
+  DCHECK(stream);
+  DCHECK_EQ(CONNECTING, state_);
+  stream_ = stream.Pass();
+  state_ = CONNECTED;
+  event_interface_->OnAddChannelResponse(false, stream_->GetSubProtocol());
+
+  // 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_);
+
+  // We don't need this any more.
+  stream_request_.reset();
+  ReadFrames();
+}
+
+void WebSocketChannel::OnConnectFailure(uint16 web_socket_error) {

[tyoshino (SeeGerritForStatus), 2013/07/02 08:23:01]

websocket_error

[Adam Rice, 2013/07/02 13:49:29]

Done.

+  DCHECK_EQ(CONNECTING, state_);
+  state_ = CLOSED;
+  stream_request_.reset();
+  event_interface_->OnAddChannelResponse(true, "");
+}
+
+void WebSocketChannel::SendFrame(bool fin,
+                                 WebSocketFrameHeader::OpCode op_code,
+                                 const std::vector<char>& data) {
+  if (data.size() > INT_MAX) {
+    NOTREACHED() << "Frame size sanity check failed";
+    return;
+  }
+  if (stream_ == NULL) {
+    LOG(DFATAL) << "Got SendFrame without a connection established; "
+                << "misbehaving renderer? fin=" << fin << " op_code=" << op_code
+                << " data.size()=" << data.size();
+    return;
+  }
+  if (state_ == SEND_CLOSED || state_ == RECV_CLOSED || state_ == CLOSED) {
+    VLOG(1) << "SendFrame called in state " << state_
+            << ". This may be a bug, or a harmless race.";
+    return;
+  }
+  if (state_ != CONNECTED) {
+    NOTREACHED() << "SendFrame() called in state " << state_;
+    return;
+  }
+  if (data.size() > base::checked_numeric_cast<size_t>(current_send_quota_)) {
+    FailChannel(SEND_INTERNAL_ERROR,
+                kWebSocketMuxErrorSendQuotaViolation,
+                "Send quota exceeded");
+    return;
+  }
+  if (!WebSocketFrameHeader::IsKnownDataOpCode(op_code)) {
+    LOG(DFATAL) << "Got SendFrame with bogus op_code " << op_code
+                << "; misbehaving renderer? fin=" << fin
+                << " data.size()=" << data.size();
+    return;
+  }
+  current_send_quota_ -= data.size();
+  // TODO(ricea): If current_send_quota_ has dropped below
+  // send_quota_low_water_mark_, we may want to consider increasing the "low
+  // water mark" and "high water mark", but only if we think we are not
+  // saturating the link to the WebSocket server.
+  // TODO(ricea): For kOpCodeText, do UTF-8 validation?
+  scoped_refptr<IOBufferWithSize> buffer(new IOBufferWithSize(data.size()));
+  std::copy(data.begin(), data.end(), buffer->data());
+  SendIOBufferWithSize(fin, op_code, buffer);
+}
+
+void WebSocketChannel::SendIOBufferWithSize(
+    bool fin,
+    WebSocketFrameHeader::OpCode op_code,
+    const scoped_refptr<IOBufferWithSize>& buffer) {
+  DCHECK(state_ == CONNECTED || state_ == RECV_CLOSED);
+  DCHECK(stream_);
+  scoped_ptr<WebSocketFrameHeader> header(new WebSocketFrameHeader(op_code));
+  header->final = fin;
+  header->masked = true;
+  header->payload_length = buffer->size();
+  scoped_ptr<WebSocketFrameChunk> chunk(new WebSocketFrameChunk());
+  chunk->header = header.Pass();
+  chunk->final_chunk = true;
+  chunk->data = buffer;
+  if (data_being_sent_) {
+    // Either the link to the WebSocket server is saturated, or we are simply
+    // processing a batch of messages.
+    // TODO(ricea): We need to keep some statistics to work out which situation
+    // we are in and adjust quota appropriately.
+    if (!data_to_send_next_) {
+      data_to_send_next_.reset(new SendBuffer);
+    }
+    data_to_send_next_->frames.push_back(chunk.release());
+    data_to_send_next_->total_bytes += buffer->size();
+  } else {
+    data_being_sent_.reset(new SendBuffer);
+    data_being_sent_->frames.push_back(chunk.release());
+    data_being_sent_->total_bytes += buffer->size();
+    WriteFrames();
+  }
+}
+
+void WebSocketChannel::WriteFrames() {
+  // This is safe because we own the WebSocketStream and destroying it cancels
+  // all callbacks.
+  int result = stream_->WriteFrames(
+      &(data_being_sent_->frames),
+      base::Bind(&WebSocketChannel::OnWriteDone, base::Unretained(this)));
+  if (result != ERR_IO_PENDING) {
+    OnWriteDone(result);

[tyoshino (SeeGerritForStatus), 2013/07/02 08:23:01]

let's use loop. recursive call to WriteFrames from OnWriteDone L247 can be
source of stack overflow attack.

[Adam Rice, 2013/07/02 13:49:29]

Done.

+  }
+}
+
+void WebSocketChannel::OnWriteDone(int result) {
+  DCHECK(state_ != FRESHLY_CONSTRUCTED && state_ != CONNECTING);
+  DCHECK_NE(ERR_IO_PENDING, result);
+  DCHECK(data_being_sent_);
+  switch (result) {
+    case OK:
+      if (data_to_send_next_) {
+        data_being_sent_ = data_to_send_next_.Pass();
+        WriteFrames();
+      } else {
+        data_being_sent_.reset();
+        if (current_send_quota_ < send_quota_low_water_mark_) {
+          // TODO(ricea): Increase low_water_mark and high_water_mark if
+          // throughput is high, reduce them if throughput is low.  Low water
+          // mark needs to be >= the bandwidth delay product *of the IPC
+          // channel*. Because factors like context-switch time, thread wake-up
+          // time, and bus speed come into play it is complex and probably needs
+          // to be determined empirically.
+          DCHECK_LE(send_quota_low_water_mark_, send_quota_high_water_mark_);
+          // TODO(ricea): Truncate quota by the quota specified by the remote
+          // 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);
+        }
+      }
+      break;
+
+    // If a recoverable error condition existed, it would go here.
+
+    default:
+      DCHECK_LT(result, 0)
+          << "WriteFrames() should only return OK or ERR_ codes";
+      stream_->Close();
+      state_ = CLOSED;
+      event_interface_->OnDropChannel(kWebSocketErrorAbnormalClosure,
+                                      "Abnormal Closure");
+      break;
+  }
+}
+
+void WebSocketChannel::ReadFrames() {
+  // This use of base::Unretained is safe because we own the WebSocketStream,
+  // and any pending reads will be cancelled when it is destroyed.
+  int result = stream_->ReadFrames(
+      &read_frame_chunks_,
+      base::Bind(&WebSocketChannel::OnReadDone, base::Unretained(this)));
+  if (result != ERR_IO_PENDING) {
+    OnReadDone(result);
+  }

[tyoshino (SeeGerritForStatus), 2013/07/02 08:23:01]

ditto (loop)

[Adam Rice, 2013/07/02 13:49:29]

Done.

+}
+
+void WebSocketChannel::OnReadDone(int result) {
+  DCHECK(state_ != FRESHLY_CONSTRUCTED && state_ != CONNECTING);
+  DCHECK_NE(ERR_IO_PENDING, result);
+  switch (result) {
+    case OK:
+      // ReadFrames() must use ERR_CONNECTION_CLOSED for a closed connection
+      // with no data read, not an empty response.
+      DCHECK(!read_frame_chunks_.empty())
+          << "ReadFrames() returned OK, but nothing was read.";
+      for (size_t i = 0; i < read_frame_chunks_.size(); ++i) {
+        scoped_ptr<WebSocketFrameChunk> chunk(read_frame_chunks_[i]);
+        read_frame_chunks_[i] = NULL;
+        ProcessFrameChunk(chunk.Pass());
+      }
+      read_frame_chunks_.clear();
+      // We need to always keep a call to ReadFrames pending.
+      ReadFrames();
+      return;
+
+    case ERR_CONNECTION_CLOSED: {

[tyoshino (SeeGerritForStatus), 2013/07/02 08:23:01]

stream_->Close() is unnecessary for this case?

[Adam Rice, 2013/07/02 13:49:29]

It is probably correct to call Close() here. Which makes this case the same as
the default case, so I have combined them.

+      State old_state = state_;
+      state_ = CLOSED;
+      if (old_state != RECV_CLOSED && old_state != CLOSED) {
+        // We need to inform the render process of the unexpected closure.
+        event_interface_->OnDropChannel(kWebSocketErrorAbnormalClosure,
+                                        "Abnormal Closure");
+      }
+      return;
+    }
+
+    default: {
+      DCHECK_LT(result, 0)
+          << "ReadFrames() should only return OK or ERR_ codes";
+      stream_->Close();
+      State old_state = state_;
+      state_ = CLOSED;
+      if (old_state != RECV_CLOSED && old_state != CLOSED) {
+        event_interface_->OnDropChannel(kWebSocketErrorAbnormalClosure,
+                                        "Abnormal Closure");
+      }
+      return;
+    }
+  }
+}
+
+// TODO(ricea): This method is too long. Break it up.
+void WebSocketChannel::ProcessFrameChunk(
+    scoped_ptr<WebSocketFrameChunk> chunk) {
+  bool first_chunk = false;
+  if (chunk->header) {

[tyoshino (SeeGerritForStatus), 2013/07/02 08:23:01]

Check if current_frame_header_ is NULL.

[Adam Rice, 2013/07/02 13:49:29]

Done.

+    first_chunk = true;
+    current_frame_header_.swap(chunk->header);
+    if (current_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;
+    }
+  }
+  if (!current_frame_header_) {
+    DCHECK(state_ != CONNECTED) << "Unexpected header-less frame received "

[tyoshino (SeeGerritForStatus), 2013/07/02 08:23:01]

why checking state_?

[Adam Rice, 2013/07/02 13:49:29]

If the header was invalid, then we will have called FailChannel() which sets
current_frame_header_ to NULL and state_ to SEND_CLOSED or CLOSED.

On the other hand, if the state_ is CONNECTED, then we know that FailChannel()
was not called and so it must be a bug on our side.

[tyoshino (SeeGerritForStatus), 2013/07/02 16:34:19]

I see. Thanks for adding the comment.

+                                << "(final_chunk = " << chunk->final_chunk
+                                << ", data size = " << chunk->data->size()
+                                << ")";
+    return;
+  }
+  scoped_refptr<IOBufferWithSize> data_buffer;
+  data_buffer.swap(chunk->data);

[tyoshino (SeeGerritForStatus), 2013/07/02 08:23:01]

let's save chunk->final_chunk and call chunk.reset() here.

[Adam Rice, 2013/07/02 13:49:29]

Done.

I thought about splitting the method here, but I don't think it will actually
help readability.

[tyoshino (SeeGerritForStatus), 2013/07/02 16:34:19]

ok

+  WebSocketFrameHeader::OpCode opcode = current_frame_header_->opcode;
+  if (WebSocketFrameHeader::IsKnownControlOpCode(opcode)) {
+    if (chunk->final_chunk) {
+      if (incomplete_control_frame_body_) {
+        VLOG(2) << "Rejoining a split control frame, opcode " << opcode;
+        data_buffer =
+            ConcatenateIOBuffers(incomplete_control_frame_body_, data_buffer);
+        incomplete_control_frame_body_ = NULL;
+      }
+    } else {
+      // TODO(ricea): Enforce a maximum size of 125 bytes on the control frames
+      // we accept.
+      VLOG(2) << "Encountered a split control frame, opcode " << opcode;
+      if (incomplete_control_frame_body_) {
+        // The really horrid case. We need to create a new IOBufferWithSize
+        // combining the new one and the old one. This should virtually never
+        // happen.
+        // TODO(ricea): This algorithm is O(N^2). Use a fixed 127-byte byffer

[tyoshino (SeeGerritForStatus), 2013/07/02 08:23:01]

buffer

you could do similar to SocketStream::pending_write_bufs_ in the future.

[Adam Rice, 2013/07/02 13:49:29]

Done.

+        // instead.
+        VLOG(3) << "Hit the really horrid case";
+        incomplete_control_frame_body_ =
+            ConcatenateIOBuffers(incomplete_control_frame_body_, data_buffer);
+      } else {
+        // The merely horrid case. Store the IOBufferWithSize to use when the
+        // rest of the control frame arrives.
+        incomplete_control_frame_body_.swap(data_buffer);
+      }
+      return;
+    }
+  }
+

[tyoshino (SeeGerritForStatus), 2013/07/02 08:23:01]

L392 to L485 can be factored out into a separate method.

[Adam Rice, 2013/07/02 13:49:29]

Done.

+  switch (opcode) {
+    case WebSocketFrameHeader::kOpCodeText:  // fall-thru
+    case WebSocketFrameHeader::kOpCodeBinary:
+      if (!first_chunk) {
+        opcode = WebSocketFrameHeader::kOpCodeContinuation;
+      }
+    // fall-thru
+    case WebSocketFrameHeader::kOpCodeContinuation:
+      if (state_ == RECV_CLOSED) {

[tyoshino (SeeGerritForStatus), 2013/07/02 08:23:01]

l400, l425, l440 can be merged and moved out to L391

[Adam Rice, 2013/07/02 13:49:29]

Well-spotted. I did not notice that. Done.

+        FailChannel(SEND_REAL_ERROR,
+                    kWebSocketErrorProtocolError,
+                    "Data packet received after close");
+        return;
+      } else if (state_ == CONNECTED) {
+        const bool final = chunk->final_chunk && current_frame_header_->final;
+        // TODO(ricea): Can this copy be eliminated?
+        const char* const data_begin = data_buffer->data();
+        const char* const data_end = data_begin + data_buffer->size();
+        const std::vector<char> data(data_begin, data_end);
+        // TODO(ricea): Handle the (improbable) case when ReadFrames returns far
+        // more data at once than we want to send in a single IPC (in which case
+        // we need to buffer the data and return to the event loop with a
+        // callback to send the rest in 32K chunks).
+
+        // Send the received frame to the renderer process.
+        event_interface_->OnDataFrame(final, opcode, data);
+      } else {
+        VLOG(3) << "Ignored data packet received in state " << state_;
+      }
+      break;
+
+    case WebSocketFrameHeader::kOpCodePing:
+      VLOG(1) << "Got Ping of size " << data_buffer->size();
+      if (state_ == RECV_CLOSED) {
+        FailChannel(SEND_REAL_ERROR,
+                    kWebSocketErrorProtocolError,
+                    "Ping received after Close");
+        return;
+      } else if (state_ == CONNECTED) {
+        SendIOBufferWithSize(
+            true, WebSocketFrameHeader::kOpCodePong, data_buffer);
+      } else {
+        VLOG(3) << "Ignored ping in state " << state_;
+      }
+      break;
+
+    case WebSocketFrameHeader::kOpCodePong:
+      VLOG(1) << "Got Pong of size " << data_buffer->size();
+      if (state_ == RECV_CLOSED) {
+        FailChannel(SEND_REAL_ERROR,
+                    kWebSocketErrorProtocolError,
+                    "Pong received after Close");
+        return;
+      }
+      // We do not need to do anything with pong messages.
+      break;
+
+    case WebSocketFrameHeader::kOpCodeClose: {
+      uint16 code = kWebSocketNormalClosure;
+      std::string reason;
+      ParseClose(data_buffer, &code, &reason);
+      // TODO(ricea): Find a way to safely log the message from the close
+      // message (escape control codes and so on).
+      VLOG(1) << "Got Close with code " << code;
+      switch (state_) {
+        case CONNECTED:
+          state_ = RECV_CLOSED;
+          SendClose(code, reason);
+          event_interface_->OnDropChannel(code, reason);
+          break;
+
+        case RECV_CLOSED:
+          FailChannel(SEND_REAL_ERROR,
+                      kWebSocketErrorProtocolError,
+                      "Close received after Close");
+          break;
+
+        case SEND_CLOSED:
+          state_ = CLOSED;
+          event_interface_->OnDropChannel(code, reason);
+          break;
+
+        default:
+          LOG(DFATAL) << "Got Close in unexpected state " << state_;
+          break;
+      }
+      break;
+    }
+
+    default:
+      FailChannel(
+          SEND_REAL_ERROR, kWebSocketErrorProtocolError, "Unknown opcode");
+      break;
+  }
+  if (chunk->final_chunk) {
+    // Make sure we do not apply this frame header to any future chunks.
+    current_frame_header_.reset();
+  }
+}
+
+void WebSocketChannel::SendFlowControl(int64 quota) {
+  DCHECK_EQ(CONNECTED, state_);
+  // TODO(ricea): Add interface to WebSocketStream and implement.
+  // stream_->SendFlowControl(quota);
+}
+
+void WebSocketChannel::SendDropChannel(uint16 code,
+                                       const std::string& reason) {
+  if (state_ == SEND_CLOSED || state_ == CLOSED) {
+    VLOG(1) << "SendDropChannel called in state " << state_
+            << ". This may be a bug, or a harmless race.";
+    return;
+  }
+  if (state_ != CONNECTED) {
+    NOTREACHED() << "SendDropChannel() called in state " << state_;
+    return;
+  }
+  // TODO(ricea): Validate |code|? Check that |reason| is valid UTF-8?
+  // TODO(ricea): There should be a timeout for the closing handshake.
+  SendClose(code, reason);

[tyoshino (SeeGerritForStatus), 2013/07/02 08:23:01]

shouldn't SendDropChannel just close the stream? SendClose is for core WebSocket
protocol's closure

[Adam Rice, 2013/07/02 13:49:29]

I'm not sure I get your point. SendDropChannel() is the public interface. It is
what will be called if someone does ws.close() from Javascript. So
http://dev.w3.org/html5/websockets/#dom-websocket-close applies, ie. we should
start the closing handshake by sending a Close frame. SendClose() is the
internal method that actually sends a Close frame.

[tyoshino (SeeGerritForStatus), 2013/07/02 16:34:19]

I see. So, for ws.close(), this code looks good. But the comment in .h and the
method name say this sends DropChannel. As we don't have mux yet, it should
temporarily mean "just close" for now.

Can we give it different name? E.g. StartClosingHandshake.

[Adam Rice, 2013/07/03 07:24:21]

Done.

+}
+
+void WebSocketChannel::FailChannel(ExposeError expose,
+                                   uint16 code,
+                                   const std::string& reason) {
+  // TODO(ricea): Logging.
+  State old_state = state_;
+  if (state_ == CONNECTED) {
+    uint16 send_code = kWebSocketErrorGoingAway;
+    std::string send_reason = "Internal Error";
+    if (expose == SEND_REAL_ERROR) {
+      send_code = code;
+      send_reason = reason;
+    }
+    SendClose(send_code, send_reason);
+  }
+  // This method is mostly called in response to an invalid frame, in
+  // which case we should not re-use the header.
+  current_frame_header_.reset();
+  if (old_state != RECV_CLOSED && old_state != CLOSED) {
+    event_interface_->OnDropChannel(code, reason);
+  }

[tyoshino (SeeGerritForStatus), 2013/07/02 08:23:01]

set state_ to CLOSED and maybe close stream?

[Adam Rice, 2013/07/02 13:49:29]

I don't think I should be calling OnDropChannel here.

Looking at the DOM WebSocket spec, it says "When the WebSocket closing handshake
is started, the user agent must queue a task to change the readyState
attribute's value to CLOSING (2).". Which means I need to add an extra message,
distinct from OnDropChannel, to trigger that.

I think I need to split the Fail cases into those that should close the
connection immediately from the client side (for example, errors during the
closing handshake, or timeouts), and those that attempt to perform The WebSocket
Closing Handshake.

[tyoshino (SeeGerritForStatus), 2013/07/02 16:34:19]

See also http://tools.ietf.org/html/rfc6455#section-7.1.7
Because of the reason similar to what you say by L528, we don't have to wait for
response like normal closing 
Right.
Oh. Which use case?

[Adam Rice, 2013/07/03 07:24:21]

Oh. I think I misinterpreted the RFC. I was thinking that when the client has to
_Fail the WebSocket Connection_, then it would normally _Start the WebSocket
Closing Handshake_ and wait for the server to send a Close frame back and close
the connection.

But now I've read it twice more, I think the client should send a Close frame
(if it thinks the socket is still alive), and then immediately close the TCP/IP
connection. I'm not sure how that interacts with multiplexing but I will worry
about that later.

So the two Fail cases are:
1) Send Close frame and disconnect, and
2) Just disconnect.

There is no case where we wait for a server response after Failing.

+}
+
+void WebSocketChannel::SendClose(uint16 code,
+                                 const std::string& reason) {
+  DCHECK(state_ == CONNECTED || state_ == RECV_CLOSED);
+  uint64 payload_length = kWebSocketCloseCodeLength + reason.length();
+  scoped_refptr<IOBufferWithSize> body = new IOBufferWithSize(payload_length);
+  WriteBigEndian(body->data(), code);
+  COMPILE_ASSERT(sizeof(code) == kWebSocketCloseCodeLength,
+                 they_should_both_be_two);
+  std::copy(
+      reason.begin(), reason.end(), body->data() + kWebSocketCloseCodeLength);
+  SendIOBufferWithSize(true, WebSocketFrameHeader::kOpCodeClose, body);
+  state_ = state_ == CONNECTED ? SEND_CLOSED : CLOSED;
+}
+
+void WebSocketChannel::ParseClose(const scoped_refptr<IOBufferWithSize>& buffer,
+                                  uint16* code,
+                                  std::string* reason) {
+  const char* data = buffer->data();
+  size_t size = base::checked_numeric_cast<size_t>(buffer->size());
+  reason->clear();
+  if (size < kWebSocketCloseCodeLength) {
+    *code = kWebSocketErrorNoStatusReceived;
+    if (size != 0) {
+      VLOG(1) << "Close frame with payload size " << size << " received "
+              << "(the first byte is " << std::hex << static_cast<int>(data[0])
+              << ")";
+      return;
+    }
+    return;
+  }
+  uint16 unchecked_code = 0;
+  ReadBigEndian(data, &unchecked_code);
+  COMPILE_ASSERT(sizeof(unchecked_code) == kWebSocketCloseCodeLength,
+                 they_should_both_be_two_bytes);

[tyoshino (SeeGerritForStatus), 2013/07/02 08:23:01]

sorry, is this normalization specified in the spec?

[Adam Rice, 2013/07/02 13:49:29]

It's a bit ambiguous, to be honest. The clearest language is in section 11.7,
where it says "The status code is an integer number between 1000 and 4999
(inclusive)."

[tyoshino (SeeGerritForStatus), 2013/07/02 16:34:19]

That makes some sense, but not to confuse users, we need some variable by which
we can tell the user of this class that some normalization happened. Currently,
Chrome and Firefox Aurora both pass received code as-is to the app.

+  if (unchecked_code >= static_cast<uint16>(kWebSocketNormalClosure) &&
+      unchecked_code <=
+          static_cast<uint16>(kWebSocketErrorPrivateReservedMax)) {
+    *code = unchecked_code;
+  } else {
+    VLOG(1) << "Close frame contained code outside of the valid range: "
+            << unchecked_code;
+    *code = kWebSocketErrorProtocolError;

[tyoshino (SeeGerritForStatus), 2013/07/02 08:23:01]

this is confusing. please pass the received code as is or it looks that the peer
told us that we made mistake.

[Adam Rice, 2013/07/02 13:49:29]

I agree that it is confusing, but I think it is the least worst option.

Previously I was using 1008 ("an endpoint is terminating the connection because
it has received a message that violates its policy"), but I think that was
worse.

I don't want to pass through garbage status codes like 65535, because then every
other bit of code that handles status codes would have to be able to be able to
correctly handle them. It is safer to have the validation code in one place.

[tyoshino (SeeGerritForStatus), 2013/07/02 16:34:19]

I understand, but we must use some code that can tell us that the peer endpoint
is to be blamed, but we don't have such a code. If you really want to do some
check, that shouldn't normalization but validation and I'd like you to report
1006 and wasClean=false to the application layer.

[Adam Rice, 2013/07/03 07:24:21]

Okay, that makes sense. However, if all other browsers pass through the code
without validation then I would need a concrete security issue to justify being
different.

+  }
+  std::string text(data + kWebSocketCloseCodeLength, data + size);
+  // TODO(ricea): Is this check strict enough? In particular, check the
+  // "Security Considerations" from RFC3629.
+  if (IsStringUTF8(text)) {
+    using std::swap;
+    swap(*reason, text);

[tyoshino (SeeGerritForStatus), 2013/07/02 08:23:01]

is this swap more efficient than substitution?

[Adam Rice, 2013/07/02 13:49:29]

I'm not sure what you mean by a substitution. It is more efficient than a copy
when the string is heap allocated, and equivalent otherwise.

[tyoshino (SeeGerritForStatus), 2013/07/02 16:34:19]

Thanks. Never mind.

+  }
+}
+
+}  // namespace net