Treat 0 returned from HttpStream::ReadResponseBody correctly.

net/http/http_response_body_drainer_unittest.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_response_body_drainer.h"
 
 #include <cstring>
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
@@ skipping 51 matching lines @@
   DISALLOW_COPY_AND_ASSIGN(CloseResultWaiter);
 };
 
 class MockHttpStream : public HttpStream {
  public:
   MockHttpStream(CloseResultWaiter* result_waiter)
       : result_waiter_(result_waiter),
         closed_(false),
         stall_reads_forever_(false),
         num_chunks_(0),
+        delay_eof_chunk_(false),
         is_complete_(false),
         ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {}
   virtual ~MockHttpStream() {}
 
   // HttpStream implementation.
   virtual int InitializeStream(const HttpRequestInfo* request_info,
                                const BoundNetLog& net_log,
                                const CompletionCallback& callback) OVERRIDE {
     return ERR_UNEXPECTED;
   }
@@ skipping 41 matching lines @@
 
   virtual void LogNumRttVsBytesMetrics() const OVERRIDE {}
 
   virtual void Drain(HttpNetworkSession*) OVERRIDE {}
 
   // Methods to tweak/observer mock behavior:
   void StallReadsForever() { stall_reads_forever_ = true; }
 
   void set_num_chunks(int num_chunks) { num_chunks_ = num_chunks; }
 
+  void DelayEofChunk() { delay_eof_chunk_ = true; }

[mmenke, 2012/10/12 18:55:05]

Think the logic would be a little simpler if you made this
"is_final_chunk_zero_bytes_" and changed behavior accordingly.  Then the program
flow would be a bit more consistent in both cases.

Note that for unknown reasons, the final chunk is read synchronously, so you'd
currently need to put the code in the fall through case (where "return buf_len;"
is).

+
  private:
   void CompleteRead();
 
   bool closed() const { return closed_; }
 
   CloseResultWaiter* const result_waiter_;
   scoped_refptr<IOBuffer> user_buf_;
   CompletionCallback callback_;
   bool closed_;
   bool stall_reads_forever_;
   int num_chunks_;
+  bool delay_eof_chunk_;
   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);

[mmenke, 2012/10/12 18:55:05]

While you're here, could you turn these into CHECKs?  DCHECKs don't make any
sense in test code.  Thanks.

 
   if (stall_reads_forever_)
     return ERR_IO_PENDING;
 
-  if (num_chunks_ == 0)
+  if (num_chunks_ == 0 && !delay_eof_chunk_)
     return ERR_UNEXPECTED;
 
-  if (buf_len > kMagicChunkSize && num_chunks_ > 1) {
+  if (buf_len > kMagicChunkSize && (num_chunks_ > 1 || delay_eof_chunk_)) {
     user_buf_ = buf;
     callback_ = callback;
     MessageLoop::current()->PostTask(
         FROM_HERE,
         base::Bind(&MockHttpStream::CompleteRead, weak_factory_.GetWeakPtr()));
     return ERR_IO_PENDING;
   }
 
   num_chunks_--;
   if (!num_chunks_)
     is_complete_ = true;
 
   return buf_len;
 }
 
 void MockHttpStream::CompleteRead() {
   CompletionCallback callback = callback_;
   std::memset(user_buf_->data(), 1, kMagicChunkSize);
   user_buf_ = NULL;
   callback_.Reset();
-  num_chunks_--;
-  if (!num_chunks_)
+  if (num_chunks_) {
+    num_chunks_--;
+    if (!num_chunks_ && !delay_eof_chunk_)
+      is_complete_ = true;
+    callback.Run(kMagicChunkSize);
+  } else {
     is_complete_ = true;
-  callback.Run(kMagicChunkSize);
+    callback.Run(0);
+  }
 }
 
 class HttpResponseBodyDrainerTest : public testing::Test {
  protected:
   HttpResponseBodyDrainerTest()
       : proxy_service_(ProxyService::CreateDirect()),
         ssl_config_service_(new SSLConfigServiceDefaults),
         http_server_properties_(new HttpServerPropertiesImpl),
         session_(CreateNetworkSession()),
         mock_stream_(new MockHttpStream(&result_waiter_)),
@@ skipping 23 matching lines @@
   drainer_->Start(session_);
   EXPECT_FALSE(result_waiter_.WaitForResult());
 }
 
 TEST_F(HttpResponseBodyDrainerTest, DrainBodyAsyncOK) {
   mock_stream_->set_num_chunks(3);
   drainer_->Start(session_);
   EXPECT_FALSE(result_waiter_.WaitForResult());
 }
 
+TEST_F(HttpResponseBodyDrainerTest, DrainBodyAsyncDelayedOK) {
+  mock_stream_->set_num_chunks(3);
+  mock_stream_->DelayEofChunk();
+  drainer_->Start(session_);
+  EXPECT_FALSE(result_waiter_.WaitForResult());
+}
+
 TEST_F(HttpResponseBodyDrainerTest, DrainBodySizeEqualsDrainBuffer) {
   mock_stream_->set_num_chunks(
       HttpResponseBodyDrainer::kDrainBodyBufferSize / kMagicChunkSize);
   drainer_->Start(session_);
   EXPECT_FALSE(result_waiter_.WaitForResult());
 }
 
 TEST_F(HttpResponseBodyDrainerTest, DrainBodyTimeOut) {
   mock_stream_->set_num_chunks(2);
   mock_stream_->StallReadsForever();
@@ skipping 30 matching lines @@
 
 TEST_F(HttpResponseBodyDrainerTest, StartWithNothingToDo) {
   mock_stream_->set_num_chunks(0);
   drainer_->StartWithSize(session_, 0);
   EXPECT_FALSE(result_waiter_.WaitForResult());
 }
 
 }  // namespace
 
 }  // namespace net