net: Don't merge HTTP headers and body if the body is not in memory.

UploadDataStream::MarkConsumedAndFillBuffer() may touch the file
system if the body data comes from a file.

BUG=72001,107966
TEST=added tests.


Review URL: http://codereview.chromium.org/9270030

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

3 files changed, 112 insertions, 27 deletions

diff --git a/net/http/http_stream_parser.cc b/net/http/http_stream_parser.cc
index a6e3caf..7966889 100644
--- a/net/http/http_stream_parser.cc
+++ b/net/http/http_stream_parser.cc
@@ -139,34 +139,30 @@ int HttpStreamParser::SendRequest(const std::string& request_line,
   // If we have a small request body, then we'll merge with the headers into a
   // single write.
   bool did_merge = false;
-  if (request_body_ != NULL &&
-      !request_body_->is_chunked() &&
-      request_body_->size() > 0) {
-    size_t merged_size = request.size() + request_body_->size();
-    if (merged_size <= kMaxMergedHeaderAndBodySize) {
-      scoped_refptr<IOBuffer> merged_request_headers_and_body(
-          new IOBuffer(merged_size));
-      // We'll repurpose |request_headers_| to store the merged headers and
-      // body.
-      request_headers_ = new DrainableIOBuffer(
-          merged_request_headers_and_body, merged_size);
-
-      char *buf = request_headers_->data();
-      memcpy(buf, request.data(), request.size());
-      buf += request.size();
-
-      size_t todo = request_body_->size();
-      while (todo) {
-        size_t buf_len = request_body_->buf_len();
-        memcpy(buf, request_body_->buf()->data(), buf_len);
-        todo -= buf_len;
-        buf += buf_len;
-        request_body_->MarkConsumedAndFillBuffer(buf_len);
-      }
-      DCHECK(request_body_->eof());
-
-      did_merge = true;
+  if (ShouldMerge(request, request_body_.get())) {
+    size_t merged_size = request.size() + request_body->size();
+    scoped_refptr<IOBuffer> merged_request_headers_and_body(
+        new IOBuffer(merged_size));
+    // We'll repurpose |request_headers_| to store the merged headers and
+    // body.
+    request_headers_ = new DrainableIOBuffer(
+        merged_request_headers_and_body, merged_size);
+
+    char *buf = request_headers_->data();
+    memcpy(buf, request.data(), request.size());
+    buf += request.size();
+
+    size_t todo = request_body_->size();
+    while (todo) {
+      size_t buf_len = request_body_->buf_len();
+      memcpy(buf, request_body_->buf()->data(), buf_len);
+      todo -= buf_len;
+      buf += buf_len;
+      request_body_->MarkConsumedAndFillBuffer(buf_len);
     }
+    DCHECK(request_body_->eof());
+
+    did_merge = true;
   }
 
   if (!did_merge) {
@@ -814,4 +810,18 @@ int HttpStreamParser::EncodeChunk(const base::StringPiece& payload,
   return cursor - output;
 }
 
+// static
+bool HttpStreamParser::ShouldMerge(const std::string& request,
+                                   const UploadDataStream* request_body) {
+  if (request_body != NULL &&
+      // IsInMemory() ensures that the request body is not chunked.
+      request_body->IsInMemory() &&
+      request_body->size() > 0) {
+    size_t merged_size = request.size() + request_body->size();
+    if (merged_size <= kMaxMergedHeaderAndBodySize)
+      return true;
+  }
+  return false;
+}
+
 }  // namespace net
diff --git a/net/http/http_stream_parser.h b/net/http/http_stream_parser.h
index 34ec522..b87beab 100644
--- a/net/http/http_stream_parser.h
+++ b/net/http/http_stream_parser.h
@@ -91,6 +91,11 @@ class NET_EXPORT_PRIVATE HttpStreamParser  : public ChunkCallback {
                          char* output,
                          size_t output_size);
 
+  // Returns true if request and body should be merged (i.e. the sum is
+  // small enough and the body is in memory, and not chunked).
+  static bool ShouldMerge(const std::string& request,
+                          const UploadDataStream* request_body);
+
   // The number of extra bytes required to encode a chunk.
   static const size_t kChunkHeaderFooterSize;
 
diff --git a/net/http/http_stream_parser_unittest.cc b/net/http/http_stream_parser_unittest.cc
index d40eac6..1e96038 100644
--- a/net/http/http_stream_parser_unittest.cc
+++ b/net/http/http_stream_parser_unittest.cc
@@ -4,9 +4,14 @@
 
 #include "net/http/http_stream_parser.h"
 
+#include "base/file_path.h"
+#include "base/file_util.h"
+#include "base/scoped_temp_dir.h"
 #include "base/string_piece.h"
 #include "base/stringprintf.h"
 #include "net/base/net_errors.h"
+#include "net/base/upload_data.h"
+#include "net/base/upload_data_stream.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
@@ -74,4 +79,69 @@ TEST(HttpStreamParser, EncodeChunk_TooLargePayload) {
   ASSERT_EQ(ERR_INVALID_ARGUMENT, num_bytes_written);
 }
 
+TEST(HttpStreamParser, ShouldMerge_NoBody) {
+  // Shouldn't be merged if upload data is non-existent.
+  ASSERT_FALSE(HttpStreamParser::ShouldMerge("some header", NULL));
+}
+
+TEST(HttpStreamParser, ShouldMerge_EmptyBody) {
+  scoped_refptr<UploadData> upload_data = new UploadData;
+  scoped_ptr<UploadDataStream> body(
+      UploadDataStream::Create(upload_data.get(), NULL));
+  // Shouldn't be merged if upload data is empty.
+  ASSERT_FALSE(HttpStreamParser::ShouldMerge("some header", body.get()));
+}
+
+TEST(HttpStreamParser, ShouldMerge_ChunkedBody) {
+  scoped_refptr<UploadData> upload_data = new UploadData;
+  upload_data->set_is_chunked(true);
+  const std::string payload = "123";
+  upload_data->AppendChunk(payload.data(), payload.size(), true);
+
+  scoped_ptr<UploadDataStream> body(
+      UploadDataStream::Create(upload_data.get(), NULL));
+  // Shouldn't be merged if upload data carries chunked data.
+  ASSERT_FALSE(HttpStreamParser::ShouldMerge("some header", body.get()));
+}
+
+TEST(HttpStreamParser, ShouldMerge_FileBody) {
+  scoped_refptr<UploadData> upload_data = new UploadData;
+
+  // Create an empty temporary file.
+  ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  FilePath temp_file_path;
+  ASSERT_TRUE(file_util::CreateTemporaryFileInDir(temp_dir.path(),
+                                                  &temp_file_path));
+
+  upload_data->AppendFileRange(temp_file_path, 0, 0, base::Time());
+
+  scoped_ptr<UploadDataStream> body(
+      UploadDataStream::Create(upload_data.get(), NULL));
+  // Shouldn't be merged if upload data carries a file, as it's not in-memory.
+  ASSERT_FALSE(HttpStreamParser::ShouldMerge("some header", body.get()));
+}
+
+TEST(HttpStreamParser, ShouldMerge_SmallBodyInMemory) {
+  scoped_refptr<UploadData> upload_data = new UploadData;
+  const std::string payload = "123";
+  upload_data->AppendBytes(payload.data(), payload.size());
+
+  scoped_ptr<UploadDataStream> body(
+      UploadDataStream::Create(upload_data.get(), NULL));
+  // Yes, should be merged if the in-memory body is small here.
+  ASSERT_TRUE(HttpStreamParser::ShouldMerge("some header", body.get()));
+}
+
+TEST(HttpStreamParser, ShouldMerge_LargeBodyInMemory) {
+  scoped_refptr<UploadData> upload_data = new UploadData;
+  const std::string payload(10000, 'a');  // 'a' x 10000.
+  upload_data->AppendBytes(payload.data(), payload.size());
+
+  scoped_ptr<UploadDataStream> body(
+      UploadDataStream::Create(upload_data.get(), NULL));
+  // Shouldn't be merged if the in-memory body is large here.
+  ASSERT_FALSE(HttpStreamParser::ShouldMerge("some header", body.get()));
+}
+
 }  // namespace net