Treat 0 returned from HttpStream::ReadResponseBody correctly.

net/http/http_response_body_drainer_unittest.cc

Index: net/http/http_response_body_drainer_unittest.cc
diff --git a/net/http/http_response_body_drainer_unittest.cc b/net/http/http_response_body_drainer_unittest.cc
index d9b6492a5378ad55986d89375c2a53894e441bc9..09b4be4bd03cb27f5c642b24440486ae18b351a2 100644
--- a/net/http/http_response_body_drainer_unittest.cc
+++ b/net/http/http_response_body_drainer_unittest.cc
@@ -38,7 +38,7 @@ class CloseResultWaiter {
         waiting_for_result_(false) {}
 
   int WaitForResult() {
-    DCHECK(!waiting_for_result_);
+    CHECK(!waiting_for_result_);
     while (!have_result_) {
       waiting_for_result_ = true;
       MessageLoop::current()->Run();
@@ -66,9 +66,12 @@ class MockHttpStream : public HttpStream {
  public:
   MockHttpStream(CloseResultWaiter* result_waiter)
       : result_waiter_(result_waiter),
+        buf_len_(0),
         closed_(false),
         stall_reads_forever_(false),
         num_chunks_(0),
+        is_async_(false),
+        is_last_chunk_zero_size_(false),
         is_complete_(false),
         ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {}
   virtual ~MockHttpStream() {}
@@ -108,7 +111,7 @@ class MockHttpStream : public HttpStream {
   virtual int ReadResponseBody(IOBuffer* buf, int buf_len,
                                const CompletionCallback& callback) OVERRIDE;
   virtual void Close(bool not_reusable) OVERRIDE {
-    DCHECK(!closed_);
+    CHECK(!closed_);
     closed_ = true;
     result_waiter_->set_result(not_reusable);
   }
@@ -130,7 +133,12 @@ class MockHttpStream : public HttpStream {
 
   void set_num_chunks(int num_chunks) { num_chunks_ = num_chunks; }
 
+  void SetAsync() { is_async_ = true; }

[mmenke, 2012/10/16 14:41:28]

I suggest making async the default case, and replacing this with set_sync().  In
general, reads will be async, so most tests will then check the more common
path.

+
+  void LastChunkZeroSize() { is_last_chunk_zero_size_ = true; }

[mmenke, 2012/10/16 14:41:28]

nit:  These should actually be set_async() and set_is_last_chunk_zero_size().

(StallReadsForever should also be set_stall_reads_forever())

+
  private:
+  void ReadImpl();
   void CompleteRead();
 
   bool closed() const { return closed_; }
@@ -138,50 +146,64 @@ class MockHttpStream : public HttpStream {
   CloseResultWaiter* const result_waiter_;
   scoped_refptr<IOBuffer> user_buf_;
   CompletionCallback callback_;
+  int buf_len_;
   bool closed_;
   bool stall_reads_forever_;
   int num_chunks_;
+  bool is_async_;
+  bool is_last_chunk_zero_size_;
   bool is_complete_;
   base::WeakPtrFactory<MockHttpStream> weak_factory_;
 };
 
 int MockHttpStream::ReadResponseBody(
     IOBuffer* buf, int buf_len, const CompletionCallback& callback) {
-  DCHECK(!callback.is_null());
-  DCHECK(callback_.is_null());
-  DCHECK(buf);
+  CHECK(!callback.is_null());
+  CHECK(callback_.is_null());
+  CHECK(buf);
 
   if (stall_reads_forever_)
     return ERR_IO_PENDING;
 
-  if (num_chunks_ == 0)
+  if (num_chunks_ == 0 && !is_last_chunk_zero_size_)

[mmenke, 2012/10/16 14:41:28]

I think that rather than handling this here, it should be handled in ReadImpl. 
If |is_last_chunk_zero_size_| and num_chunks_ is 1, then make the last chunk 0
in the same place you set is_complete_ to true.  Makes ReadImpl a little
simpler, and reduces redundant code in the is_last_chunk_zero_size_ and
non-is_last_chunk_zero_size_ cases.

     return ERR_UNEXPECTED;
 
-  if (buf_len > kMagicChunkSize && num_chunks_ > 1) {
-    user_buf_ = buf;
-    callback_ = callback;
+  user_buf_ = buf;
+  buf_len_ = buf_len;
+  callback_ = callback;
+
+  if (is_async_) {
     MessageLoop::current()->PostTask(
         FROM_HERE,
         base::Bind(&MockHttpStream::CompleteRead, weak_factory_.GetWeakPtr()));
     return ERR_IO_PENDING;
   }
 
-  num_chunks_--;
-  if (!num_chunks_)
-    is_complete_ = true;
+  ReadImpl();
+  return buf_len_;
+}
 
-  return buf_len;
+void MockHttpStream::ReadImpl() {

[mmenke, 2012/10/16 14:41:28]

Think ReadInternal would be a little more consistent.

+  if (num_chunks_ != 0) {
+    if (buf_len_ > kMagicChunkSize)
+      buf_len_ = kMagicChunkSize;
+    std::memset(user_buf_->data(), 1, buf_len_);
+    num_chunks_--;
+    if (!num_chunks_ && !is_last_chunk_zero_size_)
+      is_complete_ = true;
+  }
+  else {

[mmenke, 2012/10/16 14:41:28]

nit:  This should be "} else {"

+    is_complete_ = true;
+    buf_len_ = 0;
+  }
+  user_buf_ = NULL;
+  callback_.Reset();

[mmenke, 2012/10/16 14:41:28]

Think it's a little weird to put these here.  Suggest moving it back to
CompleteRead, and only setting it in the async case.

 }
 
 void MockHttpStream::CompleteRead() {
   CompletionCallback callback = callback_;
-  std::memset(user_buf_->data(), 1, kMagicChunkSize);
-  user_buf_ = NULL;
-  callback_.Reset();
-  num_chunks_--;
-  if (!num_chunks_)
-    is_complete_ = true;
-  callback.Run(kMagicChunkSize);
+  ReadImpl();
+  callback.Run(buf_len_);
 }
 
 class HttpResponseBodyDrainerTest : public testing::Test {
@@ -213,14 +235,36 @@ class HttpResponseBodyDrainerTest : public testing::Test {
   HttpResponseBodyDrainer* const drainer_;  // Deletes itself.
 };
 
-TEST_F(HttpResponseBodyDrainerTest, DrainBodySyncOK) {
+TEST_F(HttpResponseBodyDrainerTest, DrainBodySyncSingleOK) {
   mock_stream_->set_num_chunks(1);
   drainer_->Start(session_);
   EXPECT_FALSE(result_waiter_.WaitForResult());
 }
 
+TEST_F(HttpResponseBodyDrainerTest, DrainBodySyncOK) {
+  mock_stream_->set_num_chunks(3);
+  drainer_->Start(session_);
+  EXPECT_FALSE(result_waiter_.WaitForResult());
+}
+
 TEST_F(HttpResponseBodyDrainerTest, DrainBodyAsyncOK) {
   mock_stream_->set_num_chunks(3);
+  mock_stream_->SetAsync();
+  drainer_->Start(session_);
+  EXPECT_FALSE(result_waiter_.WaitForResult());
+}
+
+TEST_F(HttpResponseBodyDrainerTest, DrainBodyAsyncDelayedOK) {

[mmenke, 2012/10/16 14:41:28]

How about a comment just before the test along the lines of:

"Test the case that the final read chunk is 0 bytes.  This can happen when
reading the final 0-byte chunk of a chunk encoded file."

May also want to rename this test "DrainBodyAsyncEmptyChunk" or some such (And
the same for the next one).

+  mock_stream_->set_num_chunks(3);
+  mock_stream_->SetAsync();
+  mock_stream_->LastChunkZeroSize();
+  drainer_->Start(session_);
+  EXPECT_FALSE(result_waiter_.WaitForResult());
+}
+
+TEST_F(HttpResponseBodyDrainerTest, DrainBodySyncDelayedOK) {
+  mock_stream_->set_num_chunks(4);
+  mock_stream_->LastChunkZeroSize();
   drainer_->Start(session_);
   EXPECT_FALSE(result_waiter_.WaitForResult());
 }