Don't send two OnResponseCompleteds in ResourceLoader.

If the handler returns false in OnReadCompleted, we call OnResponseCompleted
twice. Add a test to ensure we don't do that. In addition, add a test for
deferring an EOF; this currently relies on URLRequest::Read being callable
after an EOF. The test will ensure we remove that assumption if it stops being
true.

BUG=361830

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

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

4 files changed, 106 insertions, 13 deletions

diff --git a/content/browser/loader/resource_dispatcher_host_unittest.cc b/content/browser/loader/resource_dispatcher_host_unittest.cc
index 6c09723..7717431 100644
--- a/content/browser/loader/resource_dispatcher_host_unittest.cc
+++ b/content/browser/loader/resource_dispatcher_host_unittest.cc
@@ -2038,12 +2038,14 @@ TEST_F(ResourceDispatcherHostTest, ForbiddenDownload) {
 
   // Flush all pending requests.
   while (net::URLRequestTestJob::ProcessOnePendingMessage()) {}
+  base::MessageLoop::current()->RunUntilIdle();
 
   // Sorts out all the messages we saw by request.
   ResourceIPCAccumulator::ClassifiedMessages msgs;
   accum_.GetClassifiedMessages(&msgs);
 
   // We should have gotten one RequestComplete message.
+  ASSERT_EQ(1U, msgs.size());
   ASSERT_EQ(1U, msgs[0].size());
   EXPECT_EQ(ResourceMsg_RequestComplete::ID, msgs[0][0].type());
 
diff --git a/content/browser/loader/resource_loader.cc b/content/browser/loader/resource_loader.cc
index 6bab366..fde1697 100644
--- a/content/browser/loader/resource_loader.cc
+++ b/content/browser/loader/resource_loader.cc
@@ -355,14 +355,23 @@ void ResourceLoader::OnReadCompleted(net::URLRequest* unused, int bytes_read) {
     return;
   }
 
-  CompleteRead(bytes_read);
-
-  if (is_deferred())
+  // If the handler cancelled or deferred the request, do not continue
+  // processing the read. If cancelled, the URLRequest has already been
+  // cancelled and will schedule an erroring OnReadCompleted later. If deferred,
+  // 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.
+  if (!CompleteRead(bytes_read))
     return;
 
-  if (request_->status().is_success() && bytes_read > 0) {
+  DCHECK(request_->status().is_success());
+  DCHECK(!is_deferred());
+  if (bytes_read > 0) {
     StartReading(true);  // Read the next chunk.
   } else {
+    // URLRequest reported an EOF. Call ResponseCompleted.
+    DCHECK_EQ(0, bytes_read);
     ResponseCompleted();
   }
 }
@@ -603,7 +612,7 @@ void ResourceLoader::ReadMore(int* bytes_read) {
   // inspecting the URLRequest's status.
 }
 
-void ResourceLoader::CompleteRead(int bytes_read) {
+bool ResourceLoader::CompleteRead(int bytes_read) {
   DCHECK(bytes_read >= 0);
   DCHECK(request_->status().is_success());
 
@@ -612,9 +621,12 @@ void ResourceLoader::CompleteRead(int bytes_read) {
   bool defer = false;
   if (!handler_->OnReadCompleted(info->GetRequestID(), bytes_read, &defer)) {
     Cancel();
+    return false;
   } else if (defer) {
     deferred_stage_ = DEFERRED_READ;  // Read next chunk when resumed.
+    return false;
   }
+  return true;
 }
 
 void ResourceLoader::ResponseCompleted() {
diff --git a/content/browser/loader/resource_loader.h b/content/browser/loader/resource_loader.h
index 7245429..806b526 100644
--- a/content/browser/loader/resource_loader.h
+++ b/content/browser/loader/resource_loader.h
@@ -98,7 +98,9 @@ class CONTENT_EXPORT ResourceLoader : public net::URLRequest::Delegate,
   void StartReading(bool is_continuation);
   void ResumeReading();
   void ReadMore(int* bytes_read);
-  void CompleteRead(int bytes_read);
+  // Passes a read result to the handler. Returns true to continue processing
+  // and false if the handler deferred or cancelled the request.
+  bool CompleteRead(int bytes_read);
   void ResponseCompleted();
   void CallDidFinishLoading();
   void RecordHistograms();
diff --git a/content/browser/loader/resource_loader_unittest.cc b/content/browser/loader/resource_loader_unittest.cc
index 3fff7be..dc5d027 100644
--- a/content/browser/loader/resource_loader_unittest.cc
+++ b/content/browser/loader/resource_loader_unittest.cc
@@ -18,6 +18,7 @@
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "content/test/test_content_browser_client.h"
 #include "ipc/ipc_message.h"
+#include "net/base/io_buffer.h"
 #include "net/base/mock_file_stream.h"
 #include "net/base/request_priority.h"
 #include "net/cert/x509_certificate.h"
@@ -77,21 +78,41 @@ class ClientCertStoreStub : public net::ClientCertStore {
   std::vector<std::string> requested_authorities_;
 };
 
+// Arbitrary read buffer size.
+const int kReadBufSize = 1024;
+
 // Dummy implementation of ResourceHandler, instance of which is needed to
 // initialize ResourceLoader.
 class ResourceHandlerStub : public ResourceHandler {
  public:
   explicit ResourceHandlerStub(net::URLRequest* request)
       : ResourceHandler(request),
+        read_buffer_(new net::IOBuffer(kReadBufSize)),
         defer_request_on_will_start_(false),
+        expect_reads_(true),
+        cancel_on_read_completed_(false),
+        defer_eof_(false),
         received_response_completed_(false),
         total_bytes_downloaded_(0) {
   }
 
+  // If true, defers the resource load in OnWillStart.
   void set_defer_request_on_will_start(bool defer_request_on_will_start) {
     defer_request_on_will_start_ = defer_request_on_will_start;
   }
 
+  // If true, expect OnWillRead / OnReadCompleted pairs for handling
+  // data. Otherwise, expect OnDataDownloaded.
+  void set_expect_reads(bool expect_reads) { expect_reads_ = expect_reads; }
+
+  // If true, cancel the request in OnReadCompleted by returning false.
+  void set_cancel_on_read_completed(bool cancel_on_read_completed) {
+    cancel_on_read_completed_ = cancel_on_read_completed;
+  }
+
+  // If true, cancel the request in OnReadCompleted by returning false.
+  void set_defer_eof(bool defer_eof) { defer_eof_ = defer_eof; }
+
   const GURL& start_url() const { return start_url_; }
   ResourceResponse* response() const { return response_.get(); }
   bool received_response_completed() const {
@@ -147,35 +168,57 @@ class ResourceHandlerStub : public ResourceHandler {
                           scoped_refptr<net::IOBuffer>* buf,
                           int* buf_size,
                           int min_size) OVERRIDE {
-    NOTREACHED();
-    return false;
+    if (!expect_reads_) {
+      ADD_FAILURE();
+      return false;
+    }
+
+    *buf = read_buffer_;
+    *buf_size = kReadBufSize;
+    return true;
   }
 
   virtual bool OnReadCompleted(int request_id,
                                int bytes_read,
                                bool* defer) OVERRIDE {
-    NOTREACHED();
-    return false;
+    if (!expect_reads_) {
+      ADD_FAILURE();
+      return false;
+    }
+
+    if (bytes_read == 0 && defer_eof_) {
+      // Only defer it once; on resumption there will be another EOF.
+      defer_eof_ = false;
+      *defer = true;
+    }
+
+    return !cancel_on_read_completed_;
   }
 
   virtual void OnResponseCompleted(int request_id,
                                    const net::URLRequestStatus& status,
                                    const std::string& security_info,
                                    bool* defer) OVERRIDE {
-    // TODO(davidben): This DCHECK currently fires everywhere. Fix the places in
-    // ResourceLoader where OnResponseCompleted is signaled twice.
-    // DCHECK(!received_response_completed_);
+    EXPECT_FALSE(received_response_completed_);
     received_response_completed_ = true;
     status_ = status;
   }
 
   virtual void OnDataDownloaded(int request_id,
                                 int bytes_downloaded) OVERRIDE {
+    if (expect_reads_)
+      ADD_FAILURE();
     total_bytes_downloaded_ += bytes_downloaded;
   }
 
  private:
+  scoped_refptr<net::IOBuffer> read_buffer_;
+
   bool defer_request_on_will_start_;
+  bool expect_reads_;
+  bool cancel_on_read_completed_;
+  bool defer_eof_;
+
   GURL start_url_;
   scoped_refptr<ResourceResponse> response_;
   bool received_response_completed_;
@@ -404,6 +447,38 @@ TEST_F(ResourceLoaderTest, ResumeCancelledRequest) {
   static_cast<ResourceController*>(loader_.get())->Resume();
 }
 
+// Tests that no invariants are broken if a ResourceHandler cancels during
+// OnReadCompleted.
+TEST_F(ResourceLoaderTest, CancelOnReadCompleted) {
+  raw_ptr_resource_handler_->set_cancel_on_read_completed(true);
+
+  loader_->StartRequest();
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(test_url(), raw_ptr_resource_handler_->start_url());
+  EXPECT_TRUE(raw_ptr_resource_handler_->received_response_completed());
+  EXPECT_EQ(net::URLRequestStatus::CANCELED,
+            raw_ptr_resource_handler_->status().status());
+}
+
+// Tests that no invariants are broken if a ResourceHandler defers EOF.
+TEST_F(ResourceLoaderTest, DeferEOF) {
+  raw_ptr_resource_handler_->set_defer_eof(true);
+
+  loader_->StartRequest();
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(test_url(), raw_ptr_resource_handler_->start_url());
+  EXPECT_FALSE(raw_ptr_resource_handler_->received_response_completed());
+
+  raw_ptr_resource_handler_->Resume();
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_TRUE(raw_ptr_resource_handler_->received_response_completed());
+  EXPECT_EQ(net::URLRequestStatus::SUCCESS,
+            raw_ptr_resource_handler_->status().status());
+}
+
 class ResourceLoaderRedirectToFileTest : public ResourceLoaderTest {
  public:
   ResourceLoaderRedirectToFileTest()
@@ -429,6 +504,8 @@ class ResourceLoaderRedirectToFileTest : public ResourceLoaderTest {
   virtual scoped_ptr<ResourceHandler> WrapResourceHandler(
       scoped_ptr<ResourceHandlerStub> leaf_handler,
       net::URLRequest* request) OVERRIDE {
+    leaf_handler->set_expect_reads(false);
+
     // Make a temporary file.
     CHECK(base::CreateTemporaryFile(&temp_path_));
     int flags = base::File::FLAG_WRITE | base::File::FLAG_TEMPORARY |