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 20 matching lines @@
     chunk_size_needs_to_divide_evenly_into_buffer_size);
 
 class CloseResultWaiter {
  public:
   CloseResultWaiter()
       : result_(false),
         have_result_(false),
         waiting_for_result_(false) {}
 
   int WaitForResult() {
-    DCHECK(!waiting_for_result_);
+    CHECK(!waiting_for_result_);
     while (!have_result_) {
       waiting_for_result_ = true;
       MessageLoop::current()->Run();
       waiting_for_result_ = false;
     }
     return result_;
   }
 
   void set_result(bool result) {
     result_ = result;
     have_result_ = true;
     if (waiting_for_result_)
       MessageLoop::current()->Quit();
   }
 
  private:
   int result_;
   bool have_result_;
   bool waiting_for_result_;
 
   DISALLOW_COPY_AND_ASSIGN(CloseResultWaiter);
 };
 
 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_sync_(false),
+        is_last_chunk_zero_size_(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 19 matching lines @@
   virtual void SetConnectionReused() OVERRIDE {}
   virtual bool IsConnectionReusable() const OVERRIDE { return false; }
   virtual void GetSSLInfo(SSLInfo* ssl_info) OVERRIDE {}
   virtual void GetSSLCertRequestInfo(
       SSLCertRequestInfo* cert_request_info) OVERRIDE {}
 
   // Mocked API
   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);
   }
 
   virtual HttpStream* RenewStreamForAuth() OVERRIDE {
     return NULL;
   }
 
   virtual bool IsResponseBodyComplete() const OVERRIDE { return is_complete_; }
 
   virtual bool IsSpdyHttpStream() const OVERRIDE { return false; }
 
   virtual void LogNumRttVsBytesMetrics() const OVERRIDE {}
 
   virtual void Drain(HttpNetworkSession*) OVERRIDE {}
 
   // Methods to tweak/observer mock behavior:
-  void StallReadsForever() { stall_reads_forever_ = true; }
+  void set_stall_reads_forever() { stall_reads_forever_ = true; }
 
   void set_num_chunks(int num_chunks) { num_chunks_ = num_chunks; }
 
+  void set_sync() { is_sync_ = true; }
+
+  void set_is_last_chunk_zero_size() { is_last_chunk_zero_size_ = true; }
+
  private:
+  int ReadInternal(IOBuffer* buf, int buf_len);
   void CompleteRead();
 
   bool closed() const { return closed_; }
 
   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_sync_;
+  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);
+int MockHttpStream::ReadResponseBody(IOBuffer* buf,
+                                     int buf_len,
+                                     const CompletionCallback& callback) {
+  CHECK(!callback.is_null());
+  CHECK(callback_.is_null());
+  CHECK(buf);
 
   if (stall_reads_forever_)
     return ERR_IO_PENDING;
 
-  if (num_chunks_ == 0)
+  if (is_complete_)
     return ERR_UNEXPECTED;
 
-  if (buf_len > kMagicChunkSize && num_chunks_ > 1) {
+  if (!is_sync_) {
     user_buf_ = buf;
+    buf_len_ = buf_len;
     callback_ = callback;
     MessageLoop::current()->PostTask(
         FROM_HERE,
         base::Bind(&MockHttpStream::CompleteRead, weak_factory_.GetWeakPtr()));
     return ERR_IO_PENDING;
+  } else {
+    return ReadInternal(buf, buf_len);
   }
+}
 
-  num_chunks_--;
-  if (!num_chunks_)
+int MockHttpStream::ReadInternal(IOBuffer* buf, int buf_len) {
+  if (num_chunks_ != 0) {

[mmenke, 2012/10/16 16:32:24]

I suggest you put the following here:

if (is_last_chunk_zero_size_ && num_chunks_ == 1)
  buf_len = 0;

Then you can remove the "if (num_chunks_ != 0) {", the else branch, and the " &&
!is_last_chunk_zero_size_"

+    if (buf_len > kMagicChunkSize)
+      buf_len = kMagicChunkSize;
+    std::memset(buf->data(), 1, buf_len);
+    num_chunks_--;
+    if (!num_chunks_ && !is_last_chunk_zero_size_)
+      is_complete_ = true;
+    return buf_len;
+  } else {
     is_complete_ = true;
-
-  return buf_len;
+    return 0;
+  }
 }
 
 void MockHttpStream::CompleteRead() {
+  int result = ReadInternal(user_buf_, buf_len_);
+  user_buf_ = NULL;
   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);
+  callback.Run(result);
 }
 
 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 11 matching lines @@
 
   scoped_ptr<ProxyService> proxy_service_;
   scoped_refptr<SSLConfigService> ssl_config_service_;
   scoped_ptr<HttpServerPropertiesImpl> http_server_properties_;
   const scoped_refptr<HttpNetworkSession> session_;
   CloseResultWaiter result_waiter_;
   MockHttpStream* const mock_stream_;  // Owned by |drainer_|.
   HttpResponseBodyDrainer* const drainer_;  // Deletes itself.
 };
 
-TEST_F(HttpResponseBodyDrainerTest, DrainBodySyncOK) {
+TEST_F(HttpResponseBodyDrainerTest, DrainBodySyncSingleOK) {
   mock_stream_->set_num_chunks(1);
+  mock_stream_->set_sync();
   drainer_->Start(session_);
   EXPECT_FALSE(result_waiter_.WaitForResult());
 }
+
+TEST_F(HttpResponseBodyDrainerTest, DrainBodySyncOK) {
+  mock_stream_->set_num_chunks(3);
+  mock_stream_->set_sync();
+  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 the case when the final chunk is 0 bytes. This can happen when
+// the final 0-byte chunk of a chunk-encoded http response is read in a last
+// call to ReadResponseBody, after all data were returned from HttpStream.
+TEST_F(HttpResponseBodyDrainerTest, DrainBodyAsyncEmptyChunk) {
+  mock_stream_->set_num_chunks(3);
+  mock_stream_->set_is_last_chunk_zero_size();
+  drainer_->Start(session_);
+  EXPECT_FALSE(result_waiter_.WaitForResult());
+}
+
+TEST_F(HttpResponseBodyDrainerTest, DrainBodySyncEmptyChunk) {
+  mock_stream_->set_num_chunks(4);
+  mock_stream_->set_sync();
+  mock_stream_->set_is_last_chunk_zero_size();
+  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();
+  mock_stream_->set_stall_reads_forever();
   drainer_->Start(session_);
   EXPECT_TRUE(result_waiter_.WaitForResult());
 }
 
 TEST_F(HttpResponseBodyDrainerTest, CancelledBySession) {
   mock_stream_->set_num_chunks(2);
-  mock_stream_->StallReadsForever();
+  mock_stream_->set_stall_reads_forever();
   drainer_->Start(session_);
   // HttpNetworkSession should delete |drainer_|.
 }
 
 TEST_F(HttpResponseBodyDrainerTest, DrainBodyTooLarge) {
   int too_many_chunks =
       HttpResponseBodyDrainer::kDrainBodyBufferSize / kMagicChunkSize;
   too_many_chunks += 1;  // Now it's too large.
 
   mock_stream_->set_num_chunks(too_many_chunks);
@@ skipping 13 matching lines @@
 
 TEST_F(HttpResponseBodyDrainerTest, StartWithNothingToDo) {
   mock_stream_->set_num_chunks(0);
   drainer_->StartWithSize(session_, 0);
   EXPECT_FALSE(result_waiter_.WaitForResult());
 }
 
 }  // namespace
 
 }  // namespace net