Avoid issuing a Read() after draining a request.

A URLRequest can signal the end of a stream by calling OnReadCompleted()
with a byte count of zero. No further reads should be issued after this.
However, ResourceLoader could issue a second Read() if the
ResourceHandler::OnReadCompleted() handler sets defer to true. I.e.

When a read completes:
  URLRequest calls
    -> ResourceLoader::OnReadCompleted(request, 0)
    -> ResourceHandler::OnReadCompleted(id, 0, &defer)
       (ResourceHandler sets defer to true)

Prior to this CL, the behavior on resumption was:

  ResourceHandler calls
    -> ResourceLoader::Resume()
    -> ResourceLoader::ResumeReading()
    -> ResourceLoader::StartReading()
    -> ResourceLoader::ReadMore()
    -> URLRequest::Read()

The new behavior is:

  ResourceHandler calls
    -> ResourceLoader::Resume()
    -> ResourceLoader::ResponseCompleted()

The new behavior is parallel to what happens if the ResourceHandler
didn't defer.

BUG=320394

Review URL: https://codereview.chromium.org/267563004

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

chrome/browser/download/download_browsertest.cc

@@ -3212,3 +3212,17 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, Resumption_MultipleAttempts) {
 
   EXPECT_FALSE(DidShowFileChooser());
 }
+
+// The file empty.bin is served with a MIME type of application/octet-stream.
+// The content body is empty. Make sure this case is handled properly and we
+// don't regress on http://crbug.com/320394.
+IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadTest_GZipWithNoContent) {
+  ASSERT_TRUE(test_server()->Start());
+  GURL url(test_server()->GetURL("files/downloads/empty.bin"));
+  // Downloading the same URL twice causes the second request to be served from
+  // cached (with a high probability). This test verifies that that doesn't
+  // happen regardless of whether the request is served via the cache or from
+  // the network.
+  DownloadAndWait(browser(), url);
+  DownloadAndWait(browser(), url);
+}

chrome/test/data/downloads/empty.bin.mock-http-headers

+HTTP/1.1 200 OK
+Date: Tue, 19 Nov 2013 02:16:10 GMT
+Accept-Ranges: bytes
+Last-Modified: Wed, 13 Nov 2013 04:27:07 GMT
+ETag: "COW-SAYS-MOO"
+Cache-Control: private
+Expires: -1
+Content-Type: application/octet-stream
+Vary: Accept-Encoding,User-Agent
+Content-Encoding: gzip
+Content-Length: 20

content/browser/download/download_browsertest.cc

@@ -1797,4 +1797,18 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest,
   ASSERT_TRUE(origin_two.ShutdownAndWaitUntilComplete());
 }
 
+// The file empty.bin is served with a MIME type of application/octet-stream.
+// The content body is empty. Make sure this case is handled properly and we
+// don't regress on http://crbug.com/320394.
+IN_PROC_BROWSER_TEST_F(DownloadContentTest, DownloadGZipWithNoContent) {
+  EmbeddedTestServer test_server;
+  ASSERT_TRUE(test_server.InitializeAndWaitUntilReady());
+
+  GURL url = test_server.GetURL("/empty.bin");
+  test_server.ServeFilesFromDirectory(GetTestFilePath("download", ""));
+
+  NavigateToURLAndWaitForDownload(shell(), url, DownloadItem::COMPLETE);
+  // That's it. This should work without crashing.
+}
+
 }  // namespace content

content/browser/loader/buffered_resource_handler.cc

@@ -459,7 +459,7 @@ bool BufferedResourceHandler::HasSupportingPlugin(bool* stale) {
 }
 
 bool BufferedResourceHandler::CopyReadBufferToNextHandler(int request_id) {
-  if (!bytes_read_)
+  if (!read_buffer_.get())
     return true;
 
   scoped_refptr<net::IOBuffer> buf;

content/browser/loader/resource_handler.h

@@ -97,7 +97,8 @@ class CONTENT_EXPORT ResourceHandler
   // Data (*bytes_read bytes) was written into the buffer provided by
   // OnWillRead.  A return value of false cancels the request, true continues
   // reading data.  Set |*defer| to true to defer reading more response data.
-  // Call controller()->Resume() to continue reading response data.
+  // Call controller()->Resume() to continue reading response data.  A zero
+  // |bytes_read| signals that no further data is available.
   virtual bool OnReadCompleted(int request_id, int bytes_read,
                                bool* defer) = 0;
 

content/browser/loader/resource_loader.cc

@@ -168,7 +168,12 @@ void ResourceLoader::MarkAsTransferring() {
 }
 
 void ResourceLoader::CompleteTransfer() {
-  DCHECK_EQ(DEFERRED_READ, deferred_stage_);
+  // Although CrossSiteResourceHandler defers at OnResponseStarted
+  // (DEFERRED_READ), it may be seeing a replay of events via
+  // BufferedResourceHandler, and so the request itself is actually deferred at
+  // a later read stage.
+  DCHECK(DEFERRED_READ == deferred_stage_ ||
+         DEFERRED_RESPONSE_COMPLETE == deferred_stage_);
 
   is_transferring_ = false;
   GetRequestInfo()->cross_site_handler()->ResumeResponse();
@@ -363,7 +368,7 @@ void ResourceLoader::OnReadCompleted(net::URLRequest* unused, int bytes_read) {
   // do nothing until resumed.
   //
   // Note: if bytes_read is 0 (EOF) and the handler defers, resumption will call
-  // Read() on the URLRequest again and get a second EOF.
+  // ResponseCompleted().
   if (is_deferred() || !request_->status().is_success())
     return;
 
@@ -426,6 +431,12 @@ void ResourceLoader::Resume() {
           base::Bind(&ResourceLoader::ResumeReading,
                      weak_ptr_factory_.GetWeakPtr()));
       break;
+    case DEFERRED_RESPONSE_COMPLETE:
+      base::MessageLoop::current()->PostTask(
+          FROM_HERE,
+          base::Bind(&ResourceLoader::ResponseCompleted,
+                     weak_ptr_factory_.GetWeakPtr()));
+      break;
     case DEFERRED_FINISH:
       // Delay self-destruction since we don't know how we were reached.
       base::MessageLoop::current()->PostTask(
@@ -622,7 +633,8 @@ void ResourceLoader::CompleteRead(int bytes_read) {
   if (!handler_->OnReadCompleted(info->GetRequestID(), bytes_read, &defer)) {
     Cancel();
   } else if (defer) {
-    deferred_stage_ = DEFERRED_READ;  // Read next chunk when resumed.
+    deferred_stage_ =
+        bytes_read > 0 ? DEFERRED_READ : DEFERRED_RESPONSE_COMPLETE;
   }
 
   // Note: the request may still have been cancelled while OnReadCompleted

content/browser/loader/resource_loader.h

@@ -123,6 +123,7 @@ class CONTENT_EXPORT ResourceLoader : public net::URLRequest::Delegate,
     DEFERRED_NETWORK_START,
     DEFERRED_REDIRECT,
     DEFERRED_READ,
+    DEFERRED_RESPONSE_COMPLETE,
     DEFERRED_FINISH
   };
   DeferredStage deferred_stage_;

content/browser/loader/resource_loader_unittest.cc

@@ -92,6 +92,8 @@ class ResourceHandlerStub : public ResourceHandler {
         expect_reads_(true),
         cancel_on_read_completed_(false),
         defer_eof_(false),
+        received_on_will_read_(false),
+        received_eof_(false),
         received_response_completed_(false),
         total_bytes_downloaded_(0) {
   }
@@ -168,30 +170,35 @@ class ResourceHandlerStub : public ResourceHandler {
                           scoped_refptr<net::IOBuffer>* buf,
                           int* buf_size,
                           int min_size) OVERRIDE {
-    if (!expect_reads_) {
-      ADD_FAILURE();
-      return false;
-    }
+    EXPECT_TRUE(expect_reads_);
+    EXPECT_FALSE(received_on_will_read_);
+    EXPECT_FALSE(received_eof_);
+    EXPECT_FALSE(received_response_completed_);
 
     *buf = read_buffer_;
     *buf_size = kReadBufSize;
+    received_on_will_read_ = true;
     return true;
   }
 
   virtual bool OnReadCompleted(int request_id,
                                int bytes_read,
                                bool* defer) OVERRIDE {
-    if (!expect_reads_) {
-      ADD_FAILURE();
-      return false;
-    }
+    EXPECT_TRUE(received_on_will_read_);
+    EXPECT_TRUE(expect_reads_);
+    EXPECT_FALSE(received_response_completed_);
 
-    if (bytes_read == 0 && defer_eof_) {
-      // Only defer it once; on resumption there will be another EOF.
-      defer_eof_ = false;
-      *defer = true;
+    if (bytes_read == 0) {
+      received_eof_ = true;
+      if (defer_eof_) {
+        defer_eof_ = false;
+        *defer = true;
+      }
     }
 
+    // Need another OnWillRead() call before seeing an OnReadCompleted().
+    received_on_will_read_ = false;
+
     return !cancel_on_read_completed_;
   }
 
@@ -200,14 +207,16 @@ class ResourceHandlerStub : public ResourceHandler {
                                    const std::string& security_info,
                                    bool* defer) OVERRIDE {
     EXPECT_FALSE(received_response_completed_);
+    if (status.is_success() && expect_reads_)
+      EXPECT_TRUE(received_eof_);
+
     received_response_completed_ = true;
     status_ = status;
   }
 
   virtual void OnDataDownloaded(int request_id,
                                 int bytes_downloaded) OVERRIDE {
-    if (expect_reads_)
-      ADD_FAILURE();
+    EXPECT_FALSE(expect_reads_);
     total_bytes_downloaded_ += bytes_downloaded;
   }
 
@@ -221,6 +230,8 @@ class ResourceHandlerStub : public ResourceHandler {
 
   GURL start_url_;
   scoped_refptr<ResourceResponse> response_;
+  bool received_on_will_read_;
+  bool received_eof_;
   bool received_response_completed_;
   net::URLRequestStatus status_;
   int total_bytes_downloaded_;

content/test/data/download/empty.bin.mock-http-headers

+HTTP/1.1 200 OK
+Date: Tue, 19 Nov 2013 02:16:10 GMT
+Accept-Ranges: bytes
+Last-Modified: Wed, 13 Nov 2013 04:27:07 GMT
+ETag: "COW-SAYS-MOO"
+Cache-Control: private
+Expires: -1
+Content-Type: application/octet-stream
+Vary: Accept-Encoding,User-Agent
+Content-Encoding: gzip
+Content-Length: 20