Factor out chunk encoding logic into HttpStreamParser::EncodeChunk().

The logic is meaty enough to be factored out.
Add unit tests along the way.

The original patch (crrev.com/118265) was reverted as it introduced
a new static initializer. This version fixed that problem by defining
the constant in the .h file. To be extra careful, replaced
kChunkBufferSize with a member variable chunk_buffer_size_.

BUG=72001
TEST=add unit tests. run tools/linux/dump-static-initializers.py locally to confirm that new static initializers are not introduced.


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

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

6 files changed, 156 insertions, 29 deletions

diff --git a/net/base/upload_data_stream.cc b/net/base/upload_data_stream.cc
index 3300cd7..7f36b7f 100644
--- a/net/base/upload_data_stream.cc
+++ b/net/base/upload_data_stream.cc
@@ -13,6 +13,7 @@
 
 namespace net {
 
+const size_t UploadDataStream::kBufferSize = 16384;
 bool UploadDataStream::merge_chunks_ = true;
 
 UploadDataStream::~UploadDataStream() {
@@ -29,6 +30,11 @@ UploadDataStream* UploadDataStream::Create(UploadData* data, int* error_code) {
   return stream.release();
 }
 
+// static
+size_t UploadDataStream::GetBufferSize() {
+  return kBufferSize;
+}
+
 void UploadDataStream::MarkConsumedAndFillBuffer(size_t num_bytes) {
   DCHECK_LE(num_bytes, buf_len_);
   DCHECK(!eof_);
@@ -46,7 +52,7 @@ void UploadDataStream::MarkConsumedAndFillBuffer(size_t num_bytes) {
 
 UploadDataStream::UploadDataStream(UploadData* data)
     : data_(data),
-      buf_(new IOBuffer(kBufSize)),
+      buf_(new IOBuffer(kBufferSize)),
       buf_len_(0),
       next_element_(0),
       next_element_offset_(0),
@@ -59,12 +65,12 @@ UploadDataStream::UploadDataStream(UploadData* data)
 int UploadDataStream::FillBuf() {
   std::vector<UploadData::Element>& elements = *data_->elements();
 
-  while (buf_len_ < kBufSize && next_element_ < elements.size()) {
+  while (buf_len_ < kBufferSize && next_element_ < elements.size()) {
     bool advance_to_next_element = false;
 
     UploadData::Element& element = elements[next_element_];
 
-    size_t size_remaining = kBufSize - buf_len_;
+    size_t size_remaining = kBufferSize - buf_len_;
     if (element.type() == UploadData::TYPE_BYTES ||
         element.type() == UploadData::TYPE_CHUNK) {
       const std::vector<char>& d = element.bytes();
diff --git a/net/base/upload_data_stream.h b/net/base/upload_data_stream.h
index df21e63..a30850a 100644
--- a/net/base/upload_data_stream.h
+++ b/net/base/upload_data_stream.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -24,15 +24,18 @@ class NET_EXPORT UploadDataStream {
   // code will be set if the output parameter error_code is not empty.
   static UploadDataStream* Create(UploadData* data, int* error_code);
 
-  // Returns the stream's buffer and buffer length.
+  // Returns the stream's buffer.
   IOBuffer* buf() const { return buf_; }
+  // Returns the length of the data in the stream's buffer.
   size_t buf_len() const { return buf_len_; }
 
   // TODO(satish): We should ideally have UploadDataStream expose a Read()
   // method which returns data in a caller provided IOBuffer. That would do away
-  // with this method and make the interface cleaner as well with less memmove
+  // with this function and make the interface cleaner as well with less memmove
   // calls.
-  size_t GetMaxBufferSize() const { return kBufSize; }
+  //
+  // Returns the size of the stream's buffer pointed by buf().
+  static size_t GetBufferSize();
 
   // Call to indicate that a portion of the stream's buffer was consumed.  This
   // call modifies the stream's buffer so that it contains the next segment of
@@ -67,8 +70,6 @@ class NET_EXPORT UploadDataStream {
   static void set_merge_chunks(bool merge) { merge_chunks_ = merge; }
 
  private:
-  enum { kBufSize = 16384 };
-
   // Protects from public access since now we have a static creator function
   // which will do both creation and initialization and might return an error.
   explicit UploadDataStream(UploadData* data);
@@ -112,6 +113,8 @@ class NET_EXPORT UploadDataStream {
   // TODO(satish): Remove this once we have a better way to unit test POST
   // requests with chunked uploads.
   static bool merge_chunks_;
+  // The size of the stream's buffer pointed by buf_.
+  static const size_t kBufferSize;
 
   DISALLOW_COPY_AND_ASSIGN(UploadDataStream);
 };
diff --git a/net/http/http_stream_parser.cc b/net/http/http_stream_parser.cc
index dcde04f..aa8eaad 100644
--- a/net/http/http_stream_parser.cc
+++ b/net/http/http_stream_parser.cc
@@ -6,7 +6,6 @@
 
 #include "base/compiler_specific.h"
 #include "base/metrics/histogram.h"
-#include "base/stringprintf.h"
 #include "base/string_util.h"
 #include "net/base/address_list.h"
 #include "net/base/auth.h"
@@ -63,6 +62,9 @@ bool HeadersContainMultipleCopiesOfField(
 
 namespace net {
 
+// 2 CRLFs + max of 8 hex chars.
+const size_t HttpStreamParser::kChunkHeaderFooterSize = 12;
+
 HttpStreamParser::HttpStreamParser(ClientSocketHandle* connection,
                                    const HttpRequestInfo* request,
                                    GrowableIOBuffer* read_buffer,
@@ -85,6 +87,8 @@ HttpStreamParser::HttpStreamParser(ClientSocketHandle* connection,
           io_callback_(
               base::Bind(&HttpStreamParser::OnIOComplete,
                          base::Unretained(this)))),
+      chunk_buffer_size_(UploadDataStream::GetBufferSize() +
+                         kChunkHeaderFooterSize),
       chunk_length_(0),
       chunk_length_without_encoding_(0),
       sent_last_chunk_(false) {
@@ -127,9 +131,7 @@ int HttpStreamParser::SendRequest(const std::string& request_line,
   request_body_.reset(request_body);
   if (request_body_ != NULL && request_body_->is_chunked()) {
     request_body_->set_chunk_callback(this);
-    const int kChunkHeaderFooterSize = 12;  // 2 CRLFs + max of 8 hex chars.
-    chunk_buf_ = new IOBuffer(request_body_->GetMaxBufferSize() +
-                              kChunkHeaderFooterSize);
+    chunk_buf_ = new IOBuffer(chunk_buffer_size_);
   }
 
   io_state_ = STATE_SENDING_HEADERS;
@@ -359,23 +361,17 @@ int HttpStreamParser::DoSendBody(int result) {
     request_body_->MarkConsumedAndFillBuffer(chunk_length_without_encoding_);
     chunk_length_without_encoding_ = 0;
 
-    int buf_len = static_cast<int>(request_body_->buf_len());
     if (request_body_->eof()) {
-      static const char kLastChunk[] = "0\r\n\r\n";
-      chunk_length_ = strlen(kLastChunk);
-      memcpy(chunk_buf_->data(), kLastChunk, chunk_length_);
+      chunk_length_ = EncodeChunk(
+          base::StringPiece(), chunk_buf_->data(), chunk_buffer_size_);
       sent_last_chunk_ = true;
-    } else if (buf_len) {
+    } else if (request_body_->buf_len() > 0) {
       // Encode and send the buffer as 1 chunk.
-      std::string chunk_header = StringPrintf("%X\r\n", buf_len);
-      char* chunk_ptr = chunk_buf_->data();
-      memcpy(chunk_ptr, chunk_header.data(), chunk_header.length());
-      chunk_ptr += chunk_header.length();
-      memcpy(chunk_ptr, request_body_->buf()->data(), buf_len);
-      chunk_ptr += buf_len;
-      memcpy(chunk_ptr, "\r\n", 2);
-      chunk_length_without_encoding_ = buf_len;
-      chunk_length_ = chunk_header.length() + buf_len + 2;
+      const base::StringPiece payload(request_body_->buf()->data(),
+                                      request_body_->buf_len());
+      chunk_length_ = EncodeChunk(payload, chunk_buf_->data(),
+                                  chunk_buffer_size_);
+      chunk_length_without_encoding_ = payload.size();
     } else {
       // Nothing to send. More POST data is yet to come?
       return ERR_IO_PENDING;
@@ -783,4 +779,28 @@ void HttpStreamParser::GetSSLCertRequestInfo(
   }
 }
 
+int HttpStreamParser::EncodeChunk(const base::StringPiece& payload,
+                                  char* output,
+                                  size_t output_size) {
+  if (output_size < payload.size() + kChunkHeaderFooterSize)
+    return ERR_INVALID_ARGUMENT;
+
+  char* cursor = output;
+  // Add the header.
+  const int num_chars = base::snprintf(output, output_size,
+                                       "%X\r\n",
+                                       static_cast<int>(payload.size()));
+  cursor += num_chars;
+  // Add the payload if any.
+  if (payload.size() > 0) {
+    memcpy(cursor, payload.data(), payload.size());
+    cursor += payload.size();
+  }
+  // Add the trailing CRLF.
+  memcpy(cursor, "\r\n", 2);
+  cursor += 2;
+
+  return cursor - output;
+}
+
 }  // namespace net
diff --git a/net/http/http_stream_parser.h b/net/http/http_stream_parser.h
index ae9fc3e..3e649f1 100644
--- a/net/http/http_stream_parser.h
+++ b/net/http/http_stream_parser.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -9,7 +9,9 @@
 #include <string>
 
 #include "base/basictypes.h"
+#include "base/string_piece.h"
 #include "net/base/completion_callback.h"
+#include "net/base/net_export.h"
 #include "net/base/net_log.h"
 #include "net/base/upload_data_stream.h"
 #include "net/http/http_chunked_decoder.h"
@@ -26,7 +28,7 @@ class IOBuffer;
 class SSLCertRequestInfo;
 class SSLInfo;
 
-class HttpStreamParser  : public ChunkCallback {
+class NET_EXPORT_PRIVATE HttpStreamParser  : public ChunkCallback {
  public:
   // Any data in |read_buffer| will be used before reading from the socket
   // and any data left over after parsing the stream will be put into
@@ -77,6 +79,21 @@ class HttpStreamParser  : public ChunkCallback {
   // ChunkCallback methods.
   virtual void OnChunkAvailable() OVERRIDE;
 
+  // Encodes the given |payload| in the chunked format to |output|.
+  // Returns the number of bytes written to |output|. |output_size| should
+  // be large enough to store the encoded chunk, which is payload.size() +
+  // kChunkHeaderFooterSize. Returns ERR_INVALID_ARGUMENT if |output_size|
+  // is not large enough.
+  //
+  // The output will look like: "HEX\r\n[payload]\r\n"
+  // where HEX is a length in hexdecimal (without the "0x" prefix).
+  static int EncodeChunk(const base::StringPiece& payload,
+                         char* output,
+                         size_t output_size);
+
+  // The number of extra bytes required to encode a chunk.
+  static const size_t kChunkHeaderFooterSize;
+
  private:
   // FOO_COMPLETE states implement the second half of potentially asynchronous
   // operations and don't necessarily mean that FOO is complete.
@@ -195,6 +212,9 @@ class HttpStreamParser  : public ChunkCallback {
   // Stores an encoded chunk for chunked uploads.
   // Note: This should perhaps be improved to not create copies of the data.
   scoped_refptr<IOBuffer> chunk_buf_;
+  // The size of the chunk buffer (chunk_buf_). The chunk buffer is
+  // guaranteed to be large enough to hold the encoded chunk.
+  const size_t chunk_buffer_size_;
   size_t chunk_length_;
   size_t chunk_length_without_encoding_;
   bool sent_last_chunk_;
diff --git a/net/http/http_stream_parser_unittest.cc b/net/http/http_stream_parser_unittest.cc
new file mode 100644
index 0000000..d40eac6
--- /dev/null
+++ b/net/http/http_stream_parser_unittest.cc
@@ -0,0 +1,77 @@
+// 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 "base/string_piece.h"
+#include "base/stringprintf.h"
+#include "net/base/net_errors.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace net {
+
+const size_t kOutputSize = 1024;  // Just large enough for this test.
+// The number of bytes that can fit in a buffer of kOutputSize.
+const size_t kMaxPayloadSize =
+    kOutputSize - HttpStreamParser::kChunkHeaderFooterSize;
+
+// The empty payload is how the last chunk is encoded.
+TEST(HttpStreamParser, EncodeChunk_EmptyPayload) {
+  char output[kOutputSize];
+
+  const base::StringPiece kPayload = "";
+  const base::StringPiece kExpected = "0\r\n\r\n";
+  const int num_bytes_written =
+      HttpStreamParser::EncodeChunk(kPayload, output, sizeof(output));
+  ASSERT_EQ(kExpected.size(), static_cast<size_t>(num_bytes_written));
+  EXPECT_EQ(kExpected, base::StringPiece(output, num_bytes_written));
+}
+
+TEST(HttpStreamParser, EncodeChunk_ShortPayload) {
+  char output[kOutputSize];
+
+  const std::string kPayload("foo\x00\x11\x22", 6);
+  // 11 = payload size + sizeof("6") + CRLF x 2.
+  const std::string kExpected("6\r\nfoo\x00\x11\x22\r\n", 11);
+  const int num_bytes_written =
+      HttpStreamParser::EncodeChunk(kPayload, output, sizeof(output));
+  ASSERT_EQ(kExpected.size(), static_cast<size_t>(num_bytes_written));
+  EXPECT_EQ(kExpected, base::StringPiece(output, num_bytes_written));
+}
+
+TEST(HttpStreamParser, EncodeChunk_LargePayload) {
+  char output[kOutputSize];
+
+  const std::string kPayload(1000, '\xff');  // '\xff' x 1000.
+  // 3E8 = 1000 in hex.
+  const std::string kExpected = "3E8\r\n" + kPayload + "\r\n";
+  const int num_bytes_written =
+      HttpStreamParser::EncodeChunk(kPayload, output, sizeof(output));
+  ASSERT_EQ(kExpected.size(), static_cast<size_t>(num_bytes_written));
+  EXPECT_EQ(kExpected, base::StringPiece(output, num_bytes_written));
+}
+
+TEST(HttpStreamParser, EncodeChunk_FullPayload) {
+  char output[kOutputSize];
+
+  const std::string kPayload(kMaxPayloadSize, '\xff');
+  // 3F4 = 1012 in hex.
+  const std::string kExpected = "3F4\r\n" + kPayload + "\r\n";
+  const int num_bytes_written =
+      HttpStreamParser::EncodeChunk(kPayload, output, sizeof(output));
+  ASSERT_EQ(kExpected.size(), static_cast<size_t>(num_bytes_written));
+  EXPECT_EQ(kExpected, base::StringPiece(output, num_bytes_written));
+}
+
+TEST(HttpStreamParser, EncodeChunk_TooLargePayload) {
+  char output[kOutputSize];
+
+  // The payload is one byte larger the output buffer size.
+  const std::string kPayload(kMaxPayloadSize + 1, '\xff');
+  const int num_bytes_written =
+      HttpStreamParser::EncodeChunk(kPayload, output, sizeof(output));
+  ASSERT_EQ(ERR_INVALID_ARGUMENT, num_bytes_written);
+}
+
+}  // namespace net
diff --git a/net/net.gyp b/net/net.gyp
index 4a249cf..cef6185 100644
--- a/net/net.gyp
+++ b/net/net.gyp
@@ -1116,6 +1116,7 @@
         'http/http_response_headers_unittest.cc',
         'http/http_server_properties_impl_unittest.cc',
         'http/http_stream_factory_impl_unittest.cc',
+        'http/http_stream_parser_unittest.cc',
         'http/http_transaction_unittest.cc',
         'http/http_transaction_unittest.h',
         'http/http_util_unittest.cc',