HttpStreamParser: Don't reuse sockets which receive unparsed data.

net/http/http_stream_parser.cc

 // Copyright (c) 2012 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/http/http_stream_parser.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
@@ skipping 196 matching lines @@
       request_(request),
       request_headers_(nullptr),
       request_headers_length_(0),
       read_buf_(read_buffer),
       read_buf_unused_offset_(0),
       response_header_start_offset_(-1),
       received_bytes_(0),
       sent_bytes_(0),
       response_(nullptr),
       response_body_length_(-1),
+      response_is_keep_alive_(false),
       response_body_read_(0),
       user_read_buf_(nullptr),
       user_read_buf_len_(0),
       connection_(connection),
       net_log_(net_log),
       sent_last_chunk_(false),
       upload_error_(OK),
       weak_ptr_factory_(this) {
   io_callback_ = base::Bind(&HttpStreamParser::OnIOComplete,
                             weak_ptr_factory_.GetWeakPtr());
@@ skipping 360 matching lines @@
     read_buf_->SetCapacity(read_buf_->capacity() + kHeaderBufInitialSize);
 
   // http://crbug.com/16371: We're seeing |user_buf_->data()| return NULL.
   // See if the user is passing in an IOBuffer with a NULL |data_|.
   CHECK(read_buf_->data());
 
   return connection_->socket()
       ->Read(read_buf_.get(), read_buf_->RemainingCapacity(), io_callback_);
 }
 
 int HttpStreamParser::DoReadHeadersComplete(int result) {

[mmenke, 2016/05/20 21:50:02]

We could probably null out response_ now, whenever this method returns with an
io_state of STATE_DONE, but seems beyond the scope of this CL.

   // DoReadHeadersComplete is called with the result of Socket::Read, which is a
   // (byte_count | error), and returns (error | OK).
 
   result = HandleReadHeaderResult(result);
 
   // TODO(mmenke):  The code below is ugly and hacky.  A much better and more
   // flexible long term solution would be to separate out the read and write
   // loops, though this would involve significant changes, both here and
   // elsewhere (WebSockets, for instance).
 
@@ skipping 254 matching lines @@
 
   if (end_of_header_offset == -1) {
     io_state_ = STATE_READ_HEADERS;
     // Prevent growing the headers buffer indefinitely.
     if (read_buf_->offset() >= kMaxHeaderBufSize) {
       io_state_ = STATE_DONE;
       return ERR_RESPONSE_HEADERS_TOO_BIG;
     }
   } else {
     CalculateResponseBodySize();
+
     // If the body is zero length, the caller may not call ReadResponseBody,
     // which is where any extra data is copied to read_buf_, so we move the
     // data here.
     if (response_body_length_ == 0) {
       int extra_bytes = read_buf_->offset() - end_of_header_offset;
       if (extra_bytes) {
         CHECK_GT(extra_bytes, 0);
         memmove(read_buf_->StartOfBuffer(),
                 read_buf_->StartOfBuffer() + end_of_header_offset,
                 extra_bytes);
       }
       read_buf_->SetCapacity(extra_bytes);
       if (response_->headers->response_code() / 100 == 1) {
         // After processing a 1xx response, the caller will ask for the next
         // header, so reset state to support that. We don't completely ignore a
         // 1xx response because it cannot be returned in reply to a CONNECT
         // request so we return OK here, which lets the caller inspect the
         // response and reject it in the event that we're setting up a CONNECT
         // tunnel.
         response_header_start_offset_ = -1;
         response_body_length_ = -1;
         // Now waiting for the second set of headers to be read.
       } else {
+        // Only set keep-alive based on final set of headers.
+        response_is_keep_alive_ = response_->headers->IsKeepAlive();
+
         io_state_ = STATE_DONE;
       }
       return OK;
     }
 
+    // Only set keep-alive based on final set of headers.
+    response_is_keep_alive_ = response_->headers->IsKeepAlive();
+
     // Note where the headers stop.
     read_buf_unused_offset_ = end_of_header_offset;
     // Now waiting for the body to be read.
   }
   return OK;
 }
 
 int HttpStreamParser::FindAndParseResponseHeaders() {
   int end_offset = -1;
   DCHECK_EQ(0, read_buf_unused_offset_);
@@ skipping 170 matching lines @@
          reuse_type == ClientSocketHandle::UNUSED_IDLE;
 }
 
 void HttpStreamParser::SetConnectionReused() {
   connection_->set_reuse_type(ClientSocketHandle::REUSED_IDLE);
 }
 
 bool HttpStreamParser::CanReuseConnection() const {
   if (!CanFindEndOfResponse())
     return false;
-  if (!response_->headers || !response_->headers->IsKeepAlive())
+
+  if (!response_is_keep_alive_)
     return false;
+
+  // Check if extra data was received after reading the entire response body. If
+  // extra data was received reusing the socket is not a great idea. This does
+  // have the down side of papering over certain server bugs, but seems to be
+  // the best option here.
+  //
+  // TODO(mmenke): Consider logging this - hard to decipher socket reuse
+  //     behavior makes NetLogs harder to read.
+  if (IsResponseBodyComplete() && read_buf_->offset() > 0)

[asanka, 2016/06/01 19:43:35]

Is it possible for (read_buf_->offset() > 0) to be true but IsMoreDataBuffered()
to be false? If not, shall the use the latter? Otherwise it's not obvious that
that's what is being tested here.

[mmenke, 2016/06/02 15:56:23]

Done.  Good suggestion - it's much clearer.

+    return false;
+
   return connection_->socket() && connection_->socket()->IsConnected();
 }
 
 void HttpStreamParser::GetSSLInfo(SSLInfo* ssl_info) {
   if (request_->url.SchemeIsCryptographic() && connection_->socket()) {
     SSLClientSocket* ssl_socket =
         static_cast<SSLClientSocket*>(connection_->socket());
     ssl_socket->GetSSLInfo(ssl_info);
   }
 }
@@ skipping 58 matching lines @@
 }
 
 void HttpStreamParser::ValidateStatusLine(const std::string& status_line) {
   HttpStatusLineValidator::StatusLineStatus status =
       HttpStatusLineValidator::ValidateStatusLine(status_line);
   UMA_HISTOGRAM_ENUMERATION("Net.HttpStatusLineStatus", status,
                             HttpStatusLineValidator::STATUS_LINE_MAX);
 }
 
 }  // namespace net