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/string_util.h"
+#include "net/base/io_buffer.h"
+#include "net/http/http_request_info.h"
+#include "net/http/http_stream_factory.h"
+#include "net/ssl/ssl_config_service.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 kWebSocketErrorNetworkByteLength = 2;

[tyoshino (SeeGerritForStatus), 2013/06/26 09:27:06]

It's not always error. And the length of the field is independent from what the
byte ordering of the field is.

Though it's also used at L31 to define NetworkErrorCode, this constant is mainly
used in parsing code to identify close code field. I'd recommend that you name
this as kWebSocketCloseCodeLength.

[Adam Rice, 2013/06/27 17:29:49]

Done.

+
+union NetworkErrorCode {
+  char code_as_char[kWebSocketErrorNetworkByteLength];
+  unsigned short code_as_network_short;
+};
+
+// A subclass of IOBufferWithSize formed by concatenating two other subclasses
+// of IOBufferWithSize.
+class ConcatenatedIOBufferWithSize : public IOBufferWithSize {
+ public:
+  ConcatenatedIOBufferWithSize(const IOBufferWithSize* part1,
+                               const IOBufferWithSize* part2)

[tyoshino (SeeGerritForStatus), 2013/06/26 09:40:17]

you don't need a new class. you can just have a utility method that creates a
new IOBufferWithSize instance with concatenated contents, I think.

[Adam Rice, 2013/06/27 17:29:49]

I must have been confused when I wrote this. Changed to a utility function.

+      : IOBufferWithSize(part1->size() + part2->size()) {
+    data_ = new char[size_];
+    std::copy(GetConstData(part1), GetConstData(part1) + part1->size(), data_);
+    std::copy(GetConstData(part2),
+              GetConstData(part2) + part2->size(),
+              data_ + part1->size());
+  }
+
+ private:
+  // We take const pointers as arguments, but IOBuffer doesn't actually have a
+  // const accessor for its data member. So we implement our own.
+  static const char* GetConstData(const IOBufferWithSize* iobuffer) {
+    return const_cast<IOBufferWithSize*>(iobuffer)->data();
+  }
+};
+
+}  // namespace
+
+struct WebSocketChannel::SendBuffer {
+  SendBuffer() : frames(), total_bytes(0) {}
+  ScopedVector<WebSocketFrameChunk> frames;
+  size_t total_bytes;
+};
+
+WebSocketChannel::WebSocketChannel(
+    const GURL& socket_url,
+    scoped_ptr<WebSocketEventInterface> event_interface)
+    : socket_url_(socket_url),
+      event_interface_(event_interface.Pass()),
+      currently_sending_(),
+      send_next_(),
+      read_frame_chunks_(),
+      current_frame_header_(),
+      incomplete_control_frame_body_(),
+      send_quota_low_water_mark_(kDefaultSendQuotaLowWaterMark),
+      send_quota_high_water_mark_(kDefaultSendQuotaHighWaterMark),
+      current_send_quota_(0),
+      state_(FRESHLY_CONSTRUCTED),
+      weak_factory_(this) {}
+
+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_protocols,
+    const GURL& origin,
+    URLRequestContext* url_request_context) {
+  // Delegate to the tested version.
+  SendAddChannelRequestWithFactory(
+      requested_protocols,
+      origin,
+      url_request_context,
+      base::Bind(&WebSocketStream::CreateAndConnectStream));
+}
+
+void WebSocketChannel::SendAddChannelRequestWithFactory(
+    const std::vector<std::string>& requested_protocols,
+    const GURL& origin,
+    URLRequestContext* url_request_context,
+    base::Callback<void(const GURL&,
+                        const std::vector<std::string>&,
+                        const GURL&,
+                        URLRequestContext*,
+                        const WebSocketStream::SuccessCallback&,
+                        const WebSocketStream::FailureCallback&)> factory) {
+  DCHECK_EQ(FRESHLY_CONSTRUCTED, state_);
+  factory.Run(
+      socket_url_,
+      requested_protocols,
+      origin,
+      url_request_context,
+      base::Bind(&WebSocketChannel::OnConnectSuccess,
+                 weak_factory_.GetWeakPtr()),
+      base::Bind(&WebSocketChannel::OnConnectFailure,
+                 weak_factory_.GetWeakPtr()));
+  state_ = CONNECTING;
+}
+
+void WebSocketChannel::OnConnectSuccess(scoped_ptr<WebSocketStream> stream) {
+  DCHECK(stream);
+  DCHECK_EQ(CONNECTING, state_);
+  stream_ = stream.Pass();
+  event_interface_->OnAddChannelResponse(false, stream_->Protocol());
+  // TODO(ricea): Get flow control information from the WebSocketStream once we
+  // have a multiplexing WebSocketStream.
+  event_interface_->OnFlowControl(send_quota_high_water_mark_);
+  current_send_quota_ = send_quota_high_water_mark_;
+  state_ = CONNECTED;
+  ReadFrames();
+}
+
+void WebSocketChannel::OnConnectFailure(unsigned short web_socket_error) {
+  DCHECK_EQ(CONNECTING, state_);
+  event_interface_->OnAddChannelResponse(true, "");
+  state_ = CLOSED;
+}
+
+void WebSocketChannel::SendFrame(bool fin,
+                                 WebSocketFrameHeader::OpCode op_code,
+                                 const std::vector<char>& data) {
+  if (data.size() > INT_MAX) {
+    DCHECK_LE(data.size(), static_cast<size_t>(INT_MAX));

[yhirano, 2013/06/26 11:33:29]

What is this DCHECK_LE?

[Adam Rice, 2013/06/27 17:29:49]

It will cause a debug build to abort if the frame is larger than INT_MAX, which
would definitely mean there was a bug somewhere.

Actually, the largest single frame we can send without overflowing
IOBufferWithSize is probably INT-MAX-14, but it depends on the protocol in use,
so the final enforcement will have to happen in WebSocket*Stream::WriteFrames().

[yhirano, 2013/06/28 06:49:17]

How about NOTREACHED?

[Adam Rice, 2013/06/28 07:59:29]

That is much better, thank you.

+    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;
+  }
+  DCHECK_EQ(CONNECTED, state_);
+  CHECK_GE(current_send_quota_, 0);  // Security-critical invariant
+  typedef std::vector<char>::size_type size_type;
+  if (data.size() > static_cast<size_type>(current_send_quota_)) {
+    FailChannel(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?
+  Send(fin, op_code, data);
+}
+
+void WebSocketChannel::Send(bool fin,
+                            WebSocketFrameHeader::OpCode op_code,
+                            const std::vector<char>& data) {
+  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->payload_length = buffer->size();
+  scoped_ptr<WebSocketFrameChunk> chunk(new WebSocketFrameChunk());
+  chunk->header = header.Pass();
+  chunk->final_chunk = true;
+  chunk->data = buffer;
+  if (currently_sending_) {
+    // 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 (!send_next_) {
+      send_next_.reset(new SendBuffer);
+    }
+    send_next_->frames.push_back(chunk.release());
+    send_next_->total_bytes += buffer->size();
+  } else {
+    currently_sending_.reset(new SendBuffer);
+    currently_sending_->frames.push_back(chunk.release());
+    currently_sending_->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(
+      &(currently_sending_->frames),
+      base::Bind(&WebSocketChannel::OnWriteDone, base::Unretained(this)));
+  if (result != ERR_IO_PENDING) {
+    OnWriteDone(result);
+  }
+}
+
+void WebSocketChannel::OnWriteDone(int result) {
+  DCHECK(state_ != FRESHLY_CONSTRUCTED && state_ != CONNECTING);
+  DCHECK_NE(ERR_IO_PENDING, result);
+  DCHECK(currently_sending_);
+  switch (result) {
+    case OK:
+      if (current_send_quota_ < send_quota_low_water_mark_) {

[yhirano, 2013/06/26 11:33:29]

I think this clause should be executed only if send_next_ is null.

[Adam Rice, 2013/06/27 17:29:49]

You're probably right. I have moved it into the if statement below.

I am concerned that this may lead to lockstep behaviour, ie. if the renderer is
able to send out frames faster than the browser can issue writes, then the
renderer will never get a quota refresh until its quota reaches zero and the
browser catches up. This might cause poor parallelism causing poor throughput.
But it is probably better to wait until we can do real experiments instead of
speculating about it now.

+        // 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_;
+        event_interface_->OnFlowControl(fresh_quota);
+        current_send_quota_ += fresh_quota;
+      }
+      if (send_next_) {
+        currently_sending_ = send_next_.Pass();
+        WriteFrames();
+      } else {
+        currently_sending_.reset();
+      }
+      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 if 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);
+  }
+}
+
+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();
+      break;

[tyoshino (SeeGerritForStatus), 2013/06/26 09:27:06]

let's call ReadFrames() and return.

[Adam Rice, 2013/06/27 17:29:49]

Done.

+
+    case ERR_CONNECTION_CLOSED: {
+      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");
+      }
+      break;

[tyoshino (SeeGerritForStatus), 2013/06/26 09:27:06]

let's return

[Adam Rice, 2013/06/27 17:29:49]

Done.

+    }
+
+    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");
+      }
+      break;

[tyoshino (SeeGerritForStatus), 2013/06/26 09:27:06]

let's return

[Adam Rice, 2013/06/27 17:29:49]

Done.

+    }
+  }
+  // We need to always keep a call to ReadFrames pending.
+  if (state_ != CLOSED) {
+    ReadFrames();
+  }
+}
+
+// TODO(ricea): This method is too long. Break it up.
+void WebSocketChannel::ProcessFrameChunk(
+    scoped_ptr<WebSocketFrameChunk> chunk) {
+  scoped_ptr<WebSocketFrameHeader> frame_header;
+  frame_header.swap(current_frame_header_);

[tyoshino (SeeGerritForStatus), 2013/06/26 09:27:06]

Write the reason of swap.

[Adam Rice, 2013/06/27 17:29:49]

I added a comment. I hope it helps.

+  bool first_chunk = false;
+  if (chunk->header) {
+    first_chunk = true;
+    frame_header.swap(chunk->header);
+  }
+  scoped_refptr<IOBufferWithSize> data_buffer;
+  data_buffer.swap(chunk->data);
+  if (WebSocketFrameHeader::IsKnownControlOpCode(frame_header->opcode)) {
+    if (chunk->final_chunk) {
+      if (incomplete_control_frame_body_) {
+        VLOG(2) << "Rejoining a split control frame, opcode "
+                << frame_header->opcode;
+        scoped_refptr<IOBufferWithSize> old_data_buffer;
+        old_data_buffer.swap(incomplete_control_frame_body_);
+        scoped_refptr<IOBufferWithSize> new_data_buffer;
+        new_data_buffer.swap(data_buffer);
+        data_buffer = new ConcatenatedIOBufferWithSize(old_data_buffer.get(),
+                                                       new_data_buffer.get());
+      }
+    } else {
+      // TODO(ricea): Enforce a maximum size of 127 bytes on the control frames
+      // we accept.
+      VLOG(2) << "Encountered a split control frame, opcode "
+              << frame_header->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
+        // instead.
+        VLOG(3) << "Hit the really horrid case";
+        scoped_refptr<IOBufferWithSize> old_body_;

[tyoshino (SeeGerritForStatus), 2013/06/26 09:27:06]

not member variable. remove last _

[Adam Rice, 2013/06/27 17:29:49]

I have no idea why I put that _ there. Removed.

+        old_body_.swap(incomplete_control_frame_body_);
+        incomplete_control_frame_body_ = new ConcatenatedIOBufferWithSize(
+            old_body_.get(), data_buffer.get());
+      } 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);
+      }
+      current_frame_header_.swap(frame_header);
+      return;
+    }
+  }
+
+  WebSocketFrameHeader::OpCode opcode = frame_header->opcode;
+  switch (frame_header->opcode) {
+    case WebSocketFrameHeader::kOpCodeText:  // fall-thru
+    case WebSocketFrameHeader::kOpCodeBinary:
+      if (!first_chunk) {
+        opcode = WebSocketFrameHeader::kOpCodeContinuation;
+      }
+    // fall-thru
+    case WebSocketFrameHeader::kOpCodeContinuation: {
+      if (state_ == RECV_CLOSED) {
+        FailChannel(1002, "Data packet received after close");
+        return;
+      }
+      bool final = chunk->final_chunk && frame_header->final;
+      // TODO(ricea): Can this copy be eliminated?
+      char* data_begin = data_buffer->data();
+      char* data_end = data_begin + data_buffer->size();
+      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).
+      event_interface_->OnSendFrame(final, opcode, data);

[tyoshino (SeeGerritForStatus), 2013/06/26 09:27:06]

why this is On"Send"?

[Adam Rice, 2013/06/27 17:29:49]

Having received the message, we will send the IPC WebSocketMsg_SendFrame to pass
it to the renderer. Maybe OnDataFrame would be a better name?

[tyoshino (SeeGerritForStatus), 2013/06/28 07:55:27]

Yes. It's so common convention that OnSomething() naming means a handler for
Something event. OnDataFrame sounds good.

[Adam Rice, 2013/06/28 08:28:06]

Done.

+      break;
+    }
+
+    case WebSocketFrameHeader::kOpCodePing:
+      VLOG(1) << "Got Ping of size " << data_buffer->size();
+      if (state_ == RECV_CLOSED) {
+        FailChannel(1002, "Ping received after Close");
+        return;
+      }
+      SendIOBufferWithSize(
+          true, WebSocketFrameHeader::kOpCodePong, data_buffer);
+      break;
+
+    case WebSocketFrameHeader::kOpCodePong:
+      VLOG(1) << "Got Pong of size " << data_buffer->size();

[yhirano, 2013/06/26 11:33:29]

Can you write a TODO comment that describes how to handle a PONG message in the
future?

[Adam Rice, 2013/06/27 17:29:49]

We don't need to anything with pong messages.

+      if (state_ == RECV_CLOSED) {
+        FailChannel(1002, "Pong received after Close");
+        return;
+      }
+      break;
+
+    case WebSocketFrameHeader::kOpCodeClose: {
+      unsigned short reason;

[tyoshino (SeeGerritForStatus), 2013/06/26 09:27:06]

initialize to some default value.

[Adam Rice, 2013/06/27 17:29:49]

Done.

+      std::string reason_text;
+      ParseClose(*data_buffer, &reason, &reason_text);
+      // 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 reason " << reason;
+      switch (state_) {
+        case CONNECTED:
+          state_ = RECV_CLOSED;
+          SendClose(reason, reason_text);
+          event_interface_->OnDropChannel(reason, reason_text);
+          break;
+
+        case RECV_CLOSED:
+          FailChannel(1002, "Close received after Close");
+          break;
+
+        case SEND_CLOSED:
+          state_ = CLOSED;
+          event_interface_->OnDropChannel(reason, reason_text);
+          break;
+
+        default:
+          LOG(DFATAL) << "Got Close in unexpected state " << state_;
+          break;
+      }
+      break;
+    }
+
+    default:
+      NOTREACHED();
+      break;
+  }
+  if (!chunk->final_chunk) {
+    // Preserve the frame header for the next call.
+    current_frame_header_.swap(frame_header);
+  }
+}
+
+void WebSocketChannel::SendFlowControl(int64 quota) {
+  DCHECK_EQ(CONNECTED, state_);
+  // TODO(ricea): Add interface to WebSocketStream and implement.
+  // stream_->SendFlowControl(quota);
+}
+
+void WebSocketChannel::SendDropChannel(unsigned short reason,
+                                       const std::string& reason_text) {
+  if (state_ == SEND_CLOSED || state_ == CLOSED) {
+    VLOG(1) << "SendDropChannel called in state " << state_
+            << ". This may be a bug, or a harmless race.";
+    return;
+  }
+  DCHECK_EQ(CONNECTED, state_);
+  // TODO(ricea): Validate reason? Check that reason_text is valid UTF-8?
+  // TODO(ricea): There should probably be a timeout for the closing handshake.
+  SendClose(reason, reason_text);
+}
+
+void WebSocketChannel::FailChannel(unsigned short code,
+                                   const std::string& reason) {
+  // TODO(ricea): Logging.
+  State old_state = state_;
+  if (state_ == CONNECTED) {
+    SendClose(kWebSocketErrorGoingAway, "Internal Error");
+  }
+  if (old_state != RECV_CLOSED && old_state != CLOSED) {
+    event_interface_->OnDropChannel(code, reason);
+  }
+}
+
+void WebSocketChannel::SendClose(unsigned short code,
+                                 const std::string& reason) {
+  DCHECK(state_ == CONNECTED || state_ == RECV_CLOSED);
+  uint64 payload_length = kWebSocketErrorNetworkByteLength + reason.length();
+  std::vector<char> data(payload_length);
+  NetworkErrorCode network_code;
+  network_code.code_as_network_short = base::HostToNet16(code);
+  std::copy(network_code.code_as_char,
+            network_code.code_as_char + kWebSocketErrorNetworkByteLength,
+            data.begin());

[tyoshino (SeeGerritForStatus), 2013/06/26 09:27:06]

how about using net::WriteBigEndian?

[Adam Rice, 2013/06/27 17:29:49]

Done.

+  std::copy(reason.begin(),
+            reason.end(),
+            data.begin() + kWebSocketErrorNetworkByteLength);
+  Send(true, WebSocketFrameHeader::kOpCodeClose, data);
+  // TODO(ricea): This is wrong.
+  state_ = state_ == CONNECTED ? SEND_CLOSED : CLOSED;
+}
+
+void WebSocketChannel::ParseClose(const IOBufferWithSize& buffer,
+                                  unsigned short* reason,
+                                  std::string* reason_text) {
+  const char* data = const_cast<IOBufferWithSize&>(buffer).data();
+  CHECK_GE(buffer.size(), 0);  // Possibly security-critical invariant.
+  size_t size = static_cast<size_t>(buffer.size());
+  reason_text->clear();
+  if (size < kWebSocketErrorNetworkByteLength) {

[tyoshino (SeeGerritForStatus), 2013/06/26 09:27:06]

it works but i'd code this like

if (size < kWebSocket...) {
  if (size != 0) {
    // error
    return;
  }
  // no code
  return;
}

or

if (size < kWebSocket...) {
  if (size == 0) {
    // no code
    return;
  }
  // error
  return;
}

or

if (size == 0) {
  // no code
  return;
} else if (size < kWebSocket...) {
  // error
  return;
}

[Adam Rice, 2013/06/27 17:29:49]

Done.

+    if (size == 1) {
+      VLOG(1) << "Close frame with 1-byte payload received "
+              << "(the byte is " << std::hex << static_cast<int>(data[0])
+              << ")";
+    }
+    *reason = 1005;

[yhirano, 2013/06/26 11:33:29]

Explaining what 1005 error is in a comment would be helpful.

[Adam Rice, 2013/06/27 17:29:49]

I'm not sure why I wrote 1005 instead of just using the value from the
WebSocketError enum. Perhaps I wrote this code first?

I changed it to use the enum value, which should be clear enough.

+    return;
+  }
+  NetworkErrorCode network_code;
+  std::copy(
+      data, data + kWebSocketErrorNetworkByteLength, network_code.code_as_char);
+  unsigned short unchecked_reason =
+      base::NetToHost16(network_code.code_as_network_short);

[tyoshino (SeeGerritForStatus), 2013/06/26 09:27:06]

how about using net::ReadBigEndian?

[Adam Rice, 2013/06/27 17:29:49]

Done.

+  if (unchecked_reason >=
+          static_cast<unsigned short>(kWebSocketNormalClosure) &&
+      unchecked_reason <=
+          static_cast<unsigned short>(kWebSocketErrorPrivateReservedMax)) {
+    *reason = unchecked_reason;
+  } else {
+    VLOG(1) << "Close frame contained reason code outside of the valid range: "
+            << unchecked_reason;
+    *reason = 1008;
+  }
+  std::string text(data + kWebSocketErrorNetworkByteLength, 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, text);
+  }
+}
+
+}  // namespace net