Do not enqueue multiple socket writes performing an HTTP chunked request

When performing an HTTP request that uses a chunked transfer encoding, and
both the underlying socket and the UploadDataStream provide data
asynchronously, ensure that writes to the underlying socket maintain a
strict ordering and that only one callback (socket write, chunked data
available) is pending at a time.

BUG=132243
TEST=HttpStreamParser.AsyncChunkAndAsyncSocket


Review URL: https://chromiumcodereview.appspot.com/10536138

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@142470 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 176 insertions, 4 deletions

diff --git a/net/http/http_stream_parser.cc b/net/http/http_stream_parser.cc
index d368f90..cb5cb11 100644
--- a/net/http/http_stream_parser.cc
+++ b/net/http/http_stream_parser.cc
@@ -347,9 +347,10 @@ void HttpStreamParser::OnChunkAvailable() {
   // headers, we will automatically start reading the chunks once we get into
   // STATE_SENDING_CHUNKED_BODY so nothing to do here.
   DCHECK(io_state_ == STATE_SENDING_HEADERS ||
-         io_state_ == STATE_SENDING_CHUNKED_BODY);
-  if (io_state_ == STATE_SENDING_CHUNKED_BODY)
-    OnIOComplete(0);
+         io_state_ == STATE_SENDING_CHUNKED_BODY ||
+         io_state_ == STATE_SEND_REQUEST_WAIT_FOR_BODY_CHUNK_COMPLETE);
+  if (io_state_ == STATE_SEND_REQUEST_WAIT_FOR_BODY_CHUNK_COMPLETE)
+    OnIOComplete(OK);
 }
 
 int HttpStreamParser::DoLoop(int result) {
@@ -374,6 +375,9 @@ int HttpStreamParser::DoLoop(int result) {
         else
           result = DoSendNonChunkedBody(result);
         break;
+      case STATE_SEND_REQUEST_WAIT_FOR_BODY_CHUNK_COMPLETE:
+        result = DoSendRequestWaitForBodyChunkComplete(result);
+        break;
       case STATE_REQUEST_SENT:
         DCHECK(result != ERR_IO_PENDING);
         can_do_more = false;
@@ -473,7 +477,8 @@ int HttpStreamParser::DoSendChunkedBody(int result) {
                                          request_body_buf_->capacity());
     request_body_buf_->DidAppend(chunk_length);
   } else if (consumed == ERR_IO_PENDING) {
-    // Nothing to send. More POST data is yet to come.
+    // Nothing to send. More request data is yet to come.
+    io_state_ = STATE_SEND_REQUEST_WAIT_FOR_BODY_CHUNK_COMPLETE;
     return ERR_IO_PENDING;
   } else {
     // There won't be other errors.
@@ -514,6 +519,18 @@ int HttpStreamParser::DoSendNonChunkedBody(int result) {
   return result;
 }
 
+int HttpStreamParser::DoSendRequestWaitForBodyChunkComplete(int result) {
+  if (result != OK) {
+    io_state_ = STATE_DONE;
+    return result;
+  }
+  // Sending the chunked body was paused while waiting for more chunks to
+  // be available. Resume sending chunks now that one or more chunks have
+  // arrived.
+  io_state_ = STATE_SENDING_CHUNKED_BODY;
+  return OK;
+}
+
 int HttpStreamParser::DoReadHeaders() {
   io_state_ = STATE_READ_HEADERS_COMPLETE;
 
diff --git a/net/http/http_stream_parser.h b/net/http/http_stream_parser.h
index aee5236..44de8d7 100644
--- a/net/http/http_stream_parser.h
+++ b/net/http/http_stream_parser.h
@@ -115,6 +115,7 @@ class NET_EXPORT_PRIVATE HttpStreamParser  : public ChunkCallback {
     // or not.
     STATE_SENDING_CHUNKED_BODY,
     STATE_SENDING_NON_CHUNKED_BODY,
+    STATE_SEND_REQUEST_WAIT_FOR_BODY_CHUNK_COMPLETE,
     STATE_REQUEST_SENT,
     STATE_READ_HEADERS,
     STATE_READ_HEADERS_COMPLETE,
@@ -147,6 +148,7 @@ class NET_EXPORT_PRIVATE HttpStreamParser  : public ChunkCallback {
   int DoSendHeaders(int result);
   int DoSendChunkedBody(int result);
   int DoSendNonChunkedBody(int result);
+  int DoSendRequestWaitForBodyChunkComplete(int result);
   int DoReadHeaders();
   int DoReadHeadersComplete(int result);
   int DoReadBody();
diff --git a/net/http/http_stream_parser_unittest.cc b/net/http/http_stream_parser_unittest.cc
index 7f9df56..843c9a6 100644
--- a/net/http/http_stream_parser_unittest.cc
+++ b/net/http/http_stream_parser_unittest.cc
@@ -6,12 +6,21 @@
 
 #include "base/file_path.h"
 #include "base/file_util.h"
+#include "base/memory/ref_counted.h"
 #include "base/scoped_temp_dir.h"
 #include "base/string_piece.h"
 #include "base/stringprintf.h"
+#include "googleurl/src/gurl.h"
+#include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
+#include "net/base/test_completion_callback.h"
 #include "net/base/upload_data.h"
 #include "net/base/upload_data_stream.h"
+#include "net/http/http_request_headers.h"
+#include "net/http/http_request_info.h"
+#include "net/http/http_response_info.h"
+#include "net/socket/client_socket_handle.h"
+#include "net/socket/socket_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
@@ -150,4 +159,148 @@ TEST(HttpStreamParser, ShouldMergeRequestHeadersAndBody_LargeBodyInMemory) {
       "some header", body.get()));
 }
 
+// Test to ensure the HttpStreamParser state machine does not get confused
+// when sending a request with a chunked body, where chunks become available
+// asynchronously, over a socket where writes may also complete
+// asynchronously.
+// This is a regression test for http://crbug.com/132243
+TEST(HttpStreamParser, AsyncChunkAndAsyncSocket) {
+  // The chunks that will be written in the request, as reflected in the
+  // MockWrites below.
+  static const char kChunk1[] = "Chunk 1";
+  static const char kChunk2[] = "Chunky 2";
+  static const char kChunk3[] = "Test 3";
+
+  MockWrite writes[] = {
+    MockWrite(ASYNC, 0,
+              "GET /one.html HTTP/1.1\r\n"
+              "Host: localhost\r\n"
+              "Transfer-Encoding: chunked\r\n"
+              "Connection: keep-alive\r\n\r\n"),
+    MockWrite(ASYNC, 1, "7\r\nChunk 1\r\n"),
+    MockWrite(ASYNC, 2, "8\r\nChunky 2\r\n"),
+    MockWrite(ASYNC, 3, "6\r\nTest 3\r\n"),
+    MockWrite(ASYNC, 4, "0\r\n\r\n"),
+  };
+
+  // The size of the response body, as reflected in the Content-Length of the
+  // MockRead below.
+  static const int kBodySize = 8;
+
+  MockRead reads[] = {
+    MockRead(ASYNC, 5, "HTTP/1.1 200 OK\r\n"),
+    MockRead(ASYNC, 6, "Content-Length: 8\r\n\r\n"),
+    MockRead(ASYNC, 7, "one.html"),
+    MockRead(SYNCHRONOUS, 8, 0),  // EOF
+  };
+
+  scoped_refptr<DeterministicSocketData> data(
+      new DeterministicSocketData(reads, arraysize(reads),
+                                  writes, arraysize(writes)));
+  data->set_connect_data(MockConnect(SYNCHRONOUS, OK));
+
+  scoped_ptr<DeterministicMockTCPClientSocket> transport(
+      new DeterministicMockTCPClientSocket(NULL, data));
+  data->set_socket(transport->AsWeakPtr());
+
+  TestCompletionCallback callback;
+  int rv = transport->Connect(callback.callback());
+  rv = callback.GetResult(rv);
+  ASSERT_EQ(OK, rv);
+
+  scoped_ptr<ClientSocketHandle> socket_handle(new ClientSocketHandle);
+  socket_handle->set_socket(transport.release());
+
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("http://localhost");
+  request_info.load_flags = LOAD_NORMAL;
+
+  scoped_refptr<GrowableIOBuffer> read_buffer(new GrowableIOBuffer);
+  HttpStreamParser parser(socket_handle.get(), &request_info, read_buffer,
+                          BoundNetLog());
+
+  scoped_refptr<UploadData> upload_data(new UploadData);
+  upload_data->set_is_chunked(true);
+
+  upload_data->AppendChunk(kChunk1, arraysize(kChunk1) - 1, false);
+
+  scoped_ptr<UploadDataStream> upload_stream(
+      new UploadDataStream(upload_data));
+  ASSERT_EQ(OK, upload_stream->Init());
+
+  HttpRequestHeaders request_headers;
+  request_headers.SetHeader("Host", "localhost");
+  request_headers.SetHeader("Transfer-Encoding", "chunked");
+  request_headers.SetHeader("Connection", "keep-alive");
+
+  HttpResponseInfo response_info;
+  // This will attempt to Write() the initial request and headers, which will
+  // complete asynchronously.
+  rv = parser.SendRequest("GET /one.html HTTP/1.1\r\n", request_headers,
+                          upload_stream.Pass(), &response_info,
+                          callback.callback());
+  ASSERT_EQ(ERR_IO_PENDING, rv);
+
+  // Complete the initial request write. Additionally, this should enqueue the
+  // first chunk.
+  data->RunFor(1);
+  ASSERT_FALSE(callback.have_result());
+
+  // Now append another chunk (while the first write is still pending), which
+  // should not confuse the state machine.
+  upload_data->AppendChunk(kChunk2, arraysize(kChunk2) - 1, false);
+  ASSERT_FALSE(callback.have_result());
+
+  // Complete writing the first chunk, which should then enqueue the second
+  // chunk for writing and return, because it is set to complete
+  // asynchronously.
+  data->RunFor(1);
+  ASSERT_FALSE(callback.have_result());
+
+  // Complete writing the second chunk. However, because no chunks are
+  // available yet, no further writes should be called until a new chunk is
+  // added.
+  data->RunFor(1);
+  ASSERT_FALSE(callback.have_result());
+
+  // Add the final chunk. This will enqueue another write, but it will not
+  // complete due to the async nature.
+  upload_data->AppendChunk(kChunk3, arraysize(kChunk3) - 1, true);
+  ASSERT_FALSE(callback.have_result());
+
+  // Finalize writing the last chunk, which will enqueue the trailer.
+  data->RunFor(1);
+  ASSERT_FALSE(callback.have_result());
+
+  // Finalize writing the trailer.
+  data->RunFor(1);
+  ASSERT_TRUE(callback.have_result());
+
+  // Warning: This will hang if the callback doesn't already have a result,
+  // due to the deterministic socket provider. Do not remove the above
+  // ASSERT_TRUE, which will avoid this hang.
+  rv = callback.WaitForResult();
+  ASSERT_EQ(OK, rv);
+
+  // Attempt to read the response status and the response headers.
+  rv = parser.ReadResponseHeaders(callback.callback());
+  ASSERT_EQ(ERR_IO_PENDING, rv);
+  data->RunFor(2);
+
+  ASSERT_TRUE(callback.have_result());
+  rv = callback.WaitForResult();
+  ASSERT_GT(rv, 0);
+
+  // Finally, attempt to read the response body.
+  scoped_refptr<IOBuffer> body_buffer(new IOBuffer(kBodySize));
+  rv = parser.ReadResponseBody(body_buffer, kBodySize, callback.callback());
+  ASSERT_EQ(ERR_IO_PENDING, rv);
+  data->RunFor(1);
+
+  ASSERT_TRUE(callback.have_result());
+  rv = callback.WaitForResult();
+  ASSERT_EQ(kBodySize, rv);
+}
+
 }  // namespace net