Return ChannelState from WebSocketChannel::SendFrame

The new WebSocketBlobSender class needs to call SendFrame() from within
its state-machine loop. For efficiency, this is best done
synchronously. In order that it can detect when it should abort because
the channel has been deleted, return ChannelState from SendFrame().

See design document at
https://docs.google.com/document/d/1CDiXB9pBumhFVVfmIn1CRI6v6byxyqWu2urEE9xp714/edit

Also remove some obsolete comments from the SendFrame() method.

BUG=571656
TEST=net_unittests

Review URL: https://codereview.chromium.org/1581613003

Cr-Commit-Position: refs/heads/master@{#369530}

net/websockets/websocket_channel.cc

@@ -363,40 +363,39 @@ bool WebSocketChannel::InClosingState() const {
   return state_ == SEND_CLOSED || state_ == CLOSE_WAIT || state_ == CLOSED;
 }
 
-void WebSocketChannel::SendFrame(bool fin,
-                                 WebSocketFrameHeader::OpCode op_code,
-                                 const std::vector<char>& data) {
+WebSocketChannel::ChannelState 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;
+    return CHANNEL_ALIVE;
   }
   if (stream_ == NULL) {
     LOG(DFATAL) << "Got SendFrame without a connection established; "
                 << "misbehaving renderer? fin=" << fin << " op_code=" << op_code
                 << " data.size()=" << data.size();
-    return;
+    return CHANNEL_ALIVE;
   }
   if (InClosingState()) {
     DVLOG(1) << "SendFrame called in state " << state_
              << ". This may be a bug, or a harmless race.";
-    return;
+    return CHANNEL_ALIVE;
   }
   if (state_ != CONNECTED) {
     NOTREACHED() << "SendFrame() called in state " << state_;
-    return;
+    return CHANNEL_ALIVE;
   }
   if (data.size() > base::checked_cast<size_t>(current_send_quota_)) {
     // TODO(ricea): Kill renderer.
-    ignore_result(
-        FailChannel("Send quota exceeded", kWebSocketErrorGoingAway, ""));
+    return FailChannel("Send quota exceeded", kWebSocketErrorGoingAway, "");
     // |this| has been deleted.
-    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;
+    return CHANNEL_ALIVE;
   }
   if (op_code == WebSocketFrameHeader::kOpCodeText ||
       (op_code == WebSocketFrameHeader::kOpCodeContinuation &&
@@ -406,12 +405,9 @@ void WebSocketChannel::SendFrame(bool fin,
     if (state == StreamingUtf8Validator::INVALID ||
         (state == StreamingUtf8Validator::VALID_MIDPOINT && fin)) {
       // TODO(ricea): Kill renderer.
-      ignore_result(
-          FailChannel("Browser sent a text frame containing invalid UTF-8",
-                      kWebSocketErrorGoingAway,
-                      ""));
+      return FailChannel("Browser sent a text frame containing invalid UTF-8",
+                         kWebSocketErrorGoingAway, "");
       // |this| has been deleted.
-      return;
     }
     sending_text_message_ = !fin;
     DCHECK(!fin || state == StreamingUtf8Validator::VALID_ENDPOINT);
@@ -423,7 +419,7 @@ void WebSocketChannel::SendFrame(bool fin,
   // server is not saturated.
   scoped_refptr<IOBuffer> buffer(new IOBuffer(data.size()));
   std::copy(data.begin(), data.end(), buffer->data());
-  ignore_result(SendFrameFromIOBuffer(fin, op_code, buffer, data.size()));
+  return SendFrameFromIOBuffer(fin, op_code, buffer, data.size());
   // |this| may have been deleted.
 }
 

net/websockets/websocket_channel.h

@@ -54,6 +54,11 @@ class NET_EXPORT WebSocketChannel {
       const BoundNetLog&,
       scoped_ptr<WebSocketStream::ConnectDelegate>)> WebSocketStreamCreator;
 
+  // Methods which return a value of type ChannelState may delete |this|. If the
+  // return value is CHANNEL_DELETED, then the caller must return without making
+  // any further access to member variables or methods.
+  using ChannelState = WebSocketEventInterface::ChannelState;
+
   // Creates a new WebSocketChannel in an idle state.
   // SendAddChannelRequest() must be called immediately afterwards to start the
   // connection process.
@@ -67,20 +72,20 @@ class NET_EXPORT WebSocketChannel {
       const std::vector<std::string>& requested_protocols,
       const url::Origin& origin);
 
-  // Sends a data frame to the remote side. The frame should usually be no
-  // larger than 32KB to prevent the time required to copy the buffers from from
-  // unduly delaying other tasks that need to run on the IO thread. This method
-  // has a hard limit of 2GB. It is the responsibility of the caller to ensure
-  // that they have sufficient send quota to send this data, otherwise the
-  // connection will be closed without sending. |fin| indicates the last frame
-  // in a message, equivalent to "FIN" as specified in section 5.2 of
-  // RFC6455. |data| is the "Payload Data". If |op_code| is kOpCodeText, or it
-  // is kOpCodeContinuation and the type the message is Text, then |data| must
-  // be a chunk of a valid UTF-8 message, however there is no requirement for
-  // |data| to be split on character boundaries.
-  void SendFrame(bool fin,
-                 WebSocketFrameHeader::OpCode op_code,
-                 const std::vector<char>& data);
+  // Sends a data frame to the remote side. It is the responsibility of the
+  // caller to ensure that they have sufficient send quota to send this data,
+  // otherwise the connection will be closed without sending. |fin| indicates
+  // the last frame in a message, equivalent to "FIN" as specified in section
+  // 5.2 of RFC6455. |data| is the "Payload Data". If |op_code| is kOpCodeText,
+  // or it is kOpCodeContinuation and the type the message is Text, then |data|
+  // must be a chunk of a valid UTF-8 message, however there is no requirement
+  // for |data| to be split on character boundaries. Calling SendFrame may
+  // result in synchronous calls to |event_interface_| which may result in this
+  // object being deleted. In that case, the return value will be
+  // CHANNEL_DELETED.
+  ChannelState SendFrame(bool fin,
+                         WebSocketFrameHeader::OpCode op_code,
+                         const std::vector<char>& data);
 
   // Sends |quota| units of flow control to the remote side. If the underlying
   // transport has a concept of |quota|, then it permits the remote server to
@@ -171,11 +176,6 @@ class NET_EXPORT WebSocketChannel {
     uint64_t size_;
   };
 
-  // Methods which return a value of type ChannelState may delete |this|. If the
-  // return value is CHANNEL_DELETED, then the caller must return without making
-  // any further access to member variables or methods.
-  typedef WebSocketEventInterface::ChannelState ChannelState;
-
   // The object passes through a linear progression of states from
   // FRESHLY_CONSTRUCTED to CLOSED, except that the SEND_CLOSED and RECV_CLOSED
   // states may be skipped in case of error.