net: Notify the http job and cache transaction about a filter

completing the request (returning 0 bytes from a read).

BUG=91898
TEST=net_unittests
Review URL: http://codereview.chromium.org/7569027

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

net/http/http_cache_transaction.cc

@@ -365,6 +365,15 @@ int HttpCache::Transaction::Read(IOBuffer* buf, int buf_len,
 void HttpCache::Transaction::StopCaching() {
 }
 
+void HttpCache::Transaction::DoneReading() {
+  if (cache_ && entry_) {
+    DCHECK(reading_);
+    DCHECK_NE(mode_, UPDATE);
+    if (mode_ & WRITE)
+      DoneWritingToEntry(true);
+  }
+}
+
 const HttpResponseInfo* HttpCache::Transaction::GetResponseInfo() const {
   // Null headers means we encountered an error or haven't a response yet
   if (auth_response_.headers)

net/http/http_cache_transaction.h

@@ -109,6 +109,7 @@ class HttpCache::Transaction : public HttpTransaction {
   virtual bool IsReadyToRestartForAuth();
   virtual int Read(IOBuffer* buf, int buf_len, CompletionCallback* callback);
   virtual void StopCaching();
+  virtual void DoneReading();
   virtual const HttpResponseInfo* GetResponseInfo() const;
   virtual LoadState GetLoadState() const;
   virtual uint64 GetUploadProgress(void) const;

net/http/http_cache_unittest.cc

@@ -5101,3 +5101,37 @@ TEST(HttpCache, ReadMetadata) {
   EXPECT_EQ(4, cache.disk_cache()->open_count());
   EXPECT_EQ(1, cache.disk_cache()->create_count());
 }
+
+// Tests that we don't mark entries as truncated when a filter detects the end
+// of the stream.
+TEST(HttpCache, FilterCompletion) {
+  MockHttpCache cache;
+  TestCompletionCallback callback;
+
+  {
+    scoped_ptr<net::HttpTransaction> trans;
+    int rv = cache.http_cache()->CreateTransaction(&trans);
+    EXPECT_EQ(net::OK, rv);
+
+    MockHttpRequest request(kSimpleGET_Transaction);
+    rv = trans->Start(&request, &callback, net::BoundNetLog());
+    EXPECT_EQ(net::OK, callback.GetResult(rv));
+
+    scoped_refptr<net::IOBuffer> buf(new net::IOBuffer(256));
+    rv = trans->Read(buf, 256, &callback);
+    EXPECT_GT(callback.GetResult(rv), 0);
+
+    // Now make sure that the entry is preserved.
+    trans->DoneReading();
+  }
+
+  // Make sure that teh ActiveEntry is gone.
+  MessageLoop::current()->RunAllPending();
+
+  // Read from the cache.
+  RunTransactionTest(cache.http_cache(), kSimpleGET_Transaction);
+
+  EXPECT_EQ(1, cache.network_layer()->transaction_count());
+  EXPECT_EQ(1, cache.disk_cache()->open_count());
+  EXPECT_EQ(1, cache.disk_cache()->create_count());
+}

net/http/http_network_transaction.h

@@ -54,6 +54,7 @@ class NET_EXPORT_PRIVATE HttpNetworkTransaction
 
   virtual int Read(IOBuffer* buf, int buf_len, CompletionCallback* callback);
   virtual void StopCaching() {}
+  virtual void DoneReading() {}
   virtual const HttpResponseInfo* GetResponseInfo() const;
   virtual LoadState GetLoadState() const;
   virtual uint64 GetUploadProgress() const;

net/http/http_transaction.h

@@ -96,6 +96,12 @@ class NET_EXPORT_PRIVATE HttpTransaction {
   // Stops further caching of this request by the HTTP cache, if there is any.
   virtual void StopCaching() = 0;
 
+  // Called to tell the transaction that we have successfully reached the end
+  // of the stream. This is equivalent to performing an extra Read() at the end
+  // that should return 0 bytes. This method should not be called if the
+  // transaction is busy processing a previous operation (like a pending Read).
+  virtual void DoneReading() = 0;
+
   // Returns the response info for this transaction or NULL if the response
   // info is not available.
   virtual const HttpResponseInfo* GetResponseInfo() const = 0;

net/http/http_transaction_unittest.cc

@@ -212,8 +212,10 @@ void TestTransactionConsumer::RunWithParams(const Tuple1<int>& params) {
 }
 
 
-MockNetworkTransaction::MockNetworkTransaction() :
-    ALLOW_THIS_IN_INITIALIZER_LIST(task_factory_(this)), data_cursor_(0) {
+MockNetworkTransaction::MockNetworkTransaction(MockNetworkLayer* factory)
+    : ALLOW_THIS_IN_INITIALIZER_LIST(task_factory_(this)),
+      data_cursor_(0),
+      transaction_factory_(factory->AsWeakPtr()) {
 }
 
 MockNetworkTransaction::~MockNetworkTransaction() {}
@@ -295,6 +297,11 @@ int MockNetworkTransaction::Read(net::IOBuffer* buf, int buf_len,
 
 void MockNetworkTransaction::StopCaching() {}
 
+void MockNetworkTransaction::DoneReading() {
+  if (transaction_factory_)
+    transaction_factory_->TransactionDoneReading();
+}
+
 const net::HttpResponseInfo* MockNetworkTransaction::GetResponseInfo() const {
   return &response_;
 }
@@ -320,14 +327,19 @@ void MockNetworkTransaction::RunCallback(net::CompletionCallback* callback,
   callback->Run(result);
 }
 
-MockNetworkLayer::MockNetworkLayer() : transaction_count_(0) {}
+MockNetworkLayer::MockNetworkLayer()
+    : transaction_count_(0), done_reading_called_(false) {}
 
 MockNetworkLayer::~MockNetworkLayer() {}
 
+void MockNetworkLayer::TransactionDoneReading() {
+  done_reading_called_ = true;
+}
+
 int MockNetworkLayer::CreateTransaction(
     scoped_ptr<net::HttpTransaction>* trans) {
   transaction_count_++;
-  trans->reset(new MockNetworkTransaction());
+  trans->reset(new MockNetworkTransaction(this));
   return net::OK;
 }
 

net/http/http_transaction_unittest.h

@@ -147,13 +147,15 @@ class TestTransactionConsumer : public CallbackRunner< Tuple1<int> > {
 //-----------------------------------------------------------------------------
 // mock network layer
 
+class MockNetworkLayer;
+
 // This transaction class inspects the available set of mock transactions to
 // find data for the request URL.  It supports IO operations that complete
 // synchronously or asynchronously to help exercise different code paths in the
 // HttpCache implementation.
 class MockNetworkTransaction : public net::HttpTransaction {
  public:
-  MockNetworkTransaction();
+  explicit MockNetworkTransaction(MockNetworkLayer* factory);
   virtual ~MockNetworkTransaction();
 
   virtual int Start(const net::HttpRequestInfo* request,
@@ -176,6 +178,8 @@ class MockNetworkTransaction : public net::HttpTransaction {
 
   virtual void StopCaching();
 
+  virtual void DoneReading();
+
   virtual const net::HttpResponseInfo* GetResponseInfo() const;
 
   virtual net::LoadState GetLoadState() const;
@@ -191,14 +195,18 @@ class MockNetworkTransaction : public net::HttpTransaction {
   std::string data_;
   int data_cursor_;
   int test_mode_;
+  base::WeakPtr<MockNetworkLayer> transaction_factory_;
 };
 
-class MockNetworkLayer : public net::HttpTransactionFactory {
+class MockNetworkLayer : public net::HttpTransactionFactory,
+                         public base::SupportsWeakPtr<MockNetworkLayer> {
  public:
   MockNetworkLayer();
   virtual ~MockNetworkLayer();
 
   int transaction_count() const { return transaction_count_; }
+  bool done_reading_called() const { return done_reading_called_; }
+  void TransactionDoneReading();
 
   // net::HttpTransactionFactory:
   virtual int CreateTransaction(scoped_ptr<net::HttpTransaction>* trans);
@@ -207,6 +215,7 @@ class MockNetworkLayer : public net::HttpTransactionFactory {
 
  private:
   int transaction_count_;
+  bool done_reading_called_;
 };
 
 //-----------------------------------------------------------------------------

net/net.gyp

@@ -1030,6 +1030,7 @@
         'tools/dump_cache/url_utilities_unittest.cc',
         'udp/udp_socket_unittest.cc',
         'url_request/url_request_job_factory_unittest.cc',
+        'url_request/url_request_job_unittest.cc',
         'url_request/url_request_throttler_simulation_unittest.cc',
         'url_request/url_request_throttler_test_support.cc',
         'url_request/url_request_throttler_test_support.h',

net/url_request/url_request_http_job.cc

@@ -1201,6 +1201,12 @@ void URLRequestHttpJob::StopCaching() {
     transaction_->StopCaching();
 }
 
+void URLRequestHttpJob::DoneReading() {
+  if (transaction_.get())
+    transaction_->DoneReading();
+  DoneWithRequest(FINISHED);
+}
+
 HostPortPair URLRequestHttpJob::GetSocketAddress() const {
   return response_info_ ? response_info_->socket_address : HostPortPair();
 }

net/url_request/url_request_http_job.h

@@ -65,29 +65,32 @@ class URLRequestHttpJob : public URLRequestJob {
                                   const string16& password);
 
   // Overridden from URLRequestJob:
-  virtual void SetUpload(UploadData* upload);
-  virtual void SetExtraRequestHeaders(const HttpRequestHeaders& headers);
-  virtual void Start();
-  virtual void Kill();
-  virtual LoadState GetLoadState() const;
-  virtual uint64 GetUploadProgress() const;
-  virtual bool GetMimeType(std::string* mime_type) const;
-  virtual bool GetCharset(std::string* charset);
-  virtual void GetResponseInfo(HttpResponseInfo* info);
-  virtual bool GetResponseCookies(std::vector<std::string>* cookies);
-  virtual int GetResponseCode() const;
-  virtual Filter* SetupFilter() const;
-  virtual bool IsSafeRedirect(const GURL& location);
-  virtual bool NeedsAuth();
-  virtual void GetAuthChallengeInfo(scoped_refptr<AuthChallengeInfo>*);
+  virtual void SetUpload(UploadData* upload) OVERRIDE;
+  virtual void SetExtraRequestHeaders(
+      const HttpRequestHeaders& headers) OVERRIDE;
+  virtual void Start() OVERRIDE;
+  virtual void Kill() OVERRIDE;
+  virtual LoadState GetLoadState() const OVERRIDE;
+  virtual uint64 GetUploadProgress() const OVERRIDE;
+  virtual bool GetMimeType(std::string* mime_type) const OVERRIDE;
+  virtual bool GetCharset(std::string* charset) OVERRIDE;
+  virtual void GetResponseInfo(HttpResponseInfo* info) OVERRIDE;
+  virtual bool GetResponseCookies(std::vector<std::string>* cookies) OVERRIDE;
+  virtual int GetResponseCode() const OVERRIDE;
+  virtual Filter* SetupFilter() const OVERRIDE;
+  virtual bool IsSafeRedirect(const GURL& location) OVERRIDE;
+  virtual bool NeedsAuth() OVERRIDE;
+  virtual void GetAuthChallengeInfo(scoped_refptr<AuthChallengeInfo>*) OVERRIDE;
   virtual void SetAuth(const string16& username,
-                       const string16& password);
-  virtual void CancelAuth();
-  virtual void ContinueWithCertificate(X509Certificate* client_cert);
-  virtual void ContinueDespiteLastError();
-  virtual bool ReadRawData(IOBuffer* buf, int buf_size, int *bytes_read);
-  virtual void StopCaching();
-  virtual HostPortPair GetSocketAddress() const;
+                       const string16& password) OVERRIDE;
+  virtual void CancelAuth() OVERRIDE;
+  virtual void ContinueWithCertificate(X509Certificate* client_cert) OVERRIDE;
+  virtual void ContinueDespiteLastError() OVERRIDE;
+  virtual bool ReadRawData(IOBuffer* buf, int buf_size,
+                           int *bytes_read) OVERRIDE;
+  virtual void StopCaching() OVERRIDE;
+  virtual void DoneReading() OVERRIDE;
+  virtual HostPortPair GetSocketAddress() const OVERRIDE;
 
   // Keep a reference to the url request context to be sure it's not deleted
   // before us.

net/url_request/url_request_job.cc

@@ -61,7 +61,7 @@ void URLRequestJob::DetachRequest() {
 bool URLRequestJob::Read(IOBuffer* buf, int buf_size, int *bytes_read) {
   bool rv = false;
 
-  DCHECK_LT(buf_size, 1000000);  // sanity check
+  DCHECK_LT(buf_size, 1000000);  // Sanity check.
   DCHECK(buf);
   DCHECK(bytes_read);
   DCHECK(filtered_read_buffer_ == NULL);
@@ -69,7 +69,7 @@ bool URLRequestJob::Read(IOBuffer* buf, int buf_size, int *bytes_read) {
 
   *bytes_read = 0;
 
-  // Skip Filter if not present
+  // Skip Filter if not present.
   if (!filter_.get()) {
     rv = ReadRawDataHelper(buf, buf_size, bytes_read);
   } else {
@@ -79,9 +79,15 @@ bool URLRequestJob::Read(IOBuffer* buf, int buf_size, int *bytes_read) {
     filtered_read_buffer_len_ = buf_size;
 
     if (ReadFilteredData(bytes_read)) {
-      rv = true;   // we have data to return
+      rv = true;   // We have data to return.
+
+      // It is fine to call DoneReading even if ReadFilteredData receives 0
+      // bytes from the net, but we avoid making that call if we know for
+      // sure that's the case (ReadRawDataHelper path).
+      if (*bytes_read == 0)
+        DoneReading();
     } else {
-      rv = false;  // error, or a new IO is pending
+      rv = false;  // Error, or a new IO is pending.
     }
   }
   if (rv && *bytes_read == 0)
@@ -358,6 +364,8 @@ void URLRequestJob::NotifyReadComplete(int bytes_read) {
     // Filter the data.
     int filter_bytes_read = 0;
     if (ReadFilteredData(&filter_bytes_read)) {
+      if (!filter_bytes_read)
+        DoneReading();
       request_->NotifyReadCompleted(filter_bytes_read);
     }
   } else {
@@ -454,6 +462,10 @@ bool URLRequestJob::ReadRawData(IOBuffer* buf, int buf_size,
   return true;
 }
 
+void URLRequestJob::DoneReading() {
+  // Do nothing.
+}
+
 void URLRequestJob::FilteredDataRead(int bytes_read) {
   DCHECK(filter_.get());  // don't add data if there is no filter
   filter_->FlushStreamBuffer(bytes_read);

net/url_request/url_request_job.h

@@ -251,6 +251,10 @@ class NET_EXPORT URLRequestJob : public base::RefCounted<URLRequestJob>,
   // info.
   virtual bool ReadRawData(IOBuffer* buf, int buf_size, int *bytes_read);
 
+  // Called to tell the job that a filter has successfully reached the end of
+  // the stream.
+  virtual void DoneReading();
+
   // Informs the filter that data has been read into its buffer
   void FilteredDataRead(int bytes_read);
 

net/url_request/url_request_job_unittest.cc

+// Copyright (c) 2011 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/url_request/url_request_job.h"
+
+#include "net/http/http_transaction_unittest.h"
+#include "net/url_request/url_request_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+// This is a header that signals the end of the data.
+const char kGzipGata[] = "\x1f\x08b\x08\0\0\0\0\0\0\3\3\0\0\0\0\0\0\0\0";
+
+void GZipServer(const net::HttpRequestInfo* request,
+                std::string* response_status, std::string* response_headers,
+                std::string* response_data) {
+  response_data->assign(kGzipGata, sizeof(kGzipGata));
+}
+
+const MockTransaction kGZip_Transaction = {
+  "http://www.google.com/gzyp",
+  "GET",
+  base::Time(),
+  "",
+  net::LOAD_NORMAL,
+  "HTTP/1.1 200 OK",
+  "Cache-Control: max-age=10000\n"
+  "Content-Encoding: gzip\n"
+  "Content-Length: 30\n",  // Intentionally wrong.
+  base::Time(),
+  "",
+  TEST_MODE_NORMAL,
+  &GZipServer,
+  0
+};
+
+}  // namespace
+
+TEST(URLRequestJob, TransactionNotifiedWhenDone) {
+  TestDelegate d;
+  TestURLRequest req(GURL(kGZip_Transaction.url), &d);
+  MockNetworkLayer network_layer;
+
+  AddMockTransaction(&kGZip_Transaction);
+
+  scoped_refptr<TestURLRequestContext> context(new TestURLRequestContext());
+  context->set_http_transaction_factory(&network_layer);
+  req.set_context(context);
+  req.set_method("GET");
+  req.Start();
+
+  MessageLoop::current()->Run();
+
+  EXPECT_TRUE(network_layer.done_reading_called());
+
+  RemoveMockTransaction(&kGZip_Transaction);
+}
+
+TEST(URLRequestJob, SyncTransactionNotifiedWhenDone) {
+  TestDelegate d;
+  TestURLRequest req(GURL(kGZip_Transaction.url), &d);
+  MockNetworkLayer network_layer;
+
+  MockTransaction transaction(kGZip_Transaction);
+  transaction.test_mode = TEST_MODE_SYNC_ALL;
+  AddMockTransaction(&transaction);
+
+  scoped_refptr<TestURLRequestContext> context(new TestURLRequestContext());
+  context->set_http_transaction_factory(&network_layer);
+  req.set_context(context);
+  req.set_method("GET");
+  req.Start();
+
+  MessageLoop::current()->Run();
+
+  EXPECT_TRUE(network_layer.done_reading_called());
+
+  RemoveMockTransaction(&transaction);
+}