Fix a pair of ResourceLoader cancellation/error bugs.

content/browser/loader/test_resource_handler.cc

Index: content/browser/loader/test_resource_handler.cc
diff --git a/content/browser/loader/test_resource_handler.cc b/content/browser/loader/test_resource_handler.cc
index 0ee72ae6744d140e22a9c3cf27a251259851c5b5..4e85ef59d467fb881d092438ed732c6f977666f5 100644
--- a/content/browser/loader/test_resource_handler.cc
+++ b/content/browser/loader/test_resource_handler.cc
@@ -5,14 +5,18 @@
 #include "content/browser/loader/test_resource_handler.h"
 
 #include "base/logging.h"
-#include "net/url_request/url_request_status.h"
+#include "content/public/browser/resource_controller.h"
+#include "content/public/common/resource_response.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace content {
 
 TestResourceHandler::TestResourceHandler(net::URLRequestStatus* request_status,
                                          std::string* body)
-    : ResourceHandler(nullptr), request_status_(request_status), body_(body) {
+    : ResourceHandler(nullptr),
+      request_status_ptr_(request_status),
+      body_ptr_(body),
+      deferred_run_loop_(new base::RunLoop()) {
   SetBufferSize(2048);
 }
 
@@ -21,68 +25,120 @@ TestResourceHandler::TestResourceHandler()
 
 TestResourceHandler::~TestResourceHandler() {}
 
-void TestResourceHandler::SetController(ResourceController* controller) {}
+void TestResourceHandler::SetController(ResourceController* controller) {
+  controller_ = controller;
+}
 
 bool TestResourceHandler::OnRequestRedirected(
     const net::RedirectInfo& redirect_info,
     ResourceResponse* response,
     bool* defer) {
-  NOTREACHED() << "Redirects are not supported by the TestResourceHandler.";
-  return false;
+  EXPECT_FALSE(canceled_);
+  EXPECT_EQ(1, on_will_start_called_);
+  EXPECT_EQ(0, on_response_started_called_);
+  EXPECT_EQ(0, on_response_completed_called_);
+  ++on_request_redirected_called_;
+
+  if (!on_request_redirected_result_)
+    return false;
+
+  *defer = defer_on_request_redirected_;
+  defer_on_request_redirected_ = false;
+  if (*defer)
+    deferred_run_loop_->Quit();
+  return true;
 }
 
 bool TestResourceHandler::OnResponseStarted(ResourceResponse* response,
                                             bool* defer) {
+  EXPECT_FALSE(canceled_);
   EXPECT_EQ(1, on_will_start_called_);
   EXPECT_EQ(0, on_response_started_called_);
   EXPECT_EQ(0, on_response_completed_called_);
   ++on_response_started_called_;
 
-  if (!on_response_started_result_)
+  EXPECT_FALSE(resource_response_);
+  resource_response_ = response;
+
+  if (!on_response_started_result_) {
+    canceled_ = true;
     return false;
+  }
+
   *defer = defer_on_response_started_;
   defer_on_response_started_ = false;
+  if (*defer)
+    deferred_run_loop_->Quit();
   return true;
 }
 
 bool TestResourceHandler::OnWillStart(const GURL& url, bool* defer) {
+  EXPECT_FALSE(canceled_);
   EXPECT_EQ(0, on_response_started_called_);
   EXPECT_EQ(0, on_will_start_called_);
   EXPECT_EQ(0, on_response_completed_called_);
   ++on_will_start_called_;
 
-  if (!on_will_start_result_)
+  start_url_ = url;
+
+  if (!on_will_start_result_) {
+    canceled_ = true;
     return false;
+  }
 
   *defer = defer_on_will_start_;
+  if (*defer)
+    deferred_run_loop_->Quit();
   return true;
 }
 
 bool TestResourceHandler::OnWillRead(scoped_refptr<net::IOBuffer>* buf,
                                      int* buf_size,
                                      int min_size) {
+  EXPECT_FALSE(canceled_);
+  EXPECT_FALSE(expect_on_data_downloaded_);
   EXPECT_EQ(0, on_response_completed_called_);
   ++on_will_read_called_;
 
   *buf = buffer_;
   *buf_size = buffer_size_;
   memset(buffer_->data(), '\0', buffer_size_);
+  if (!on_will_read_result_)
+    canceled_ = true;
   return on_will_read_result_;
 }
 
 bool TestResourceHandler::OnReadCompleted(int bytes_read, bool* defer) {
+  EXPECT_FALSE(canceled_);
+  EXPECT_FALSE(expect_on_data_downloaded_);
   EXPECT_EQ(1, on_will_start_called_);
   EXPECT_EQ(1, on_response_started_called_);
   EXPECT_EQ(0, on_response_completed_called_);
+  EXPECT_EQ(0, on_read_eof_);
+
   ++on_read_completed_called_;
+  if (bytes_read == 0)
+    ++on_read_eof_;
 
   EXPECT_LE(static_cast<size_t>(bytes_read), buffer_size_);
-  if (body_)
-    body_->append(buffer_->data(), bytes_read);
-  if (!on_read_completed_result_)
+  if (body_ptr_)
+    body_ptr_->append(buffer_->data(), bytes_read);
+  body_.append(buffer_->data(), bytes_read);
+
+  if (!on_read_completed_result_ ||
+      (!on_on_read_eof_result_ && bytes_read == 0)) {
+    canceled_ = true;
     return false;
+  }
+
   *defer = defer_on_read_completed_;
   defer_on_read_completed_ = false;
+  if (bytes_read == 0 && defer_on_read_eof_)
+    *defer = true;
+
+  if (*defer)
+    deferred_run_loop_->Quit();
+
   return true;
 }
 
@@ -90,16 +146,33 @@ void TestResourceHandler::OnResponseCompleted(
     const net::URLRequestStatus& status,
     bool* defer) {
   EXPECT_EQ(0, on_response_completed_called_);
+  if (status.is_success() && !expect_on_data_downloaded_ && expect_eof_read_)
+    EXPECT_EQ(1, on_read_eof_);
+
   ++on_response_completed_called_;
 
-  if (request_status_)
-    *request_status_ = status;
+  if (request_status_ptr_)
+    *request_status_ptr_ = status;
+  final_status_ = status;
   *defer = defer_on_response_completed_;
   defer_on_response_completed_ = false;
+
+  if (*defer)
+    deferred_run_loop_->Quit();
+  response_complete_run_loop_.Quit();
 }
 
 void TestResourceHandler::OnDataDownloaded(int bytes_downloaded) {
-  NOTREACHED() << "Saving to file is not supported by the TestResourceHandler.";
+  EXPECT_TRUE(expect_on_data_downloaded_);
+  total_bytes_downloaded_ += bytes_downloaded;
+}
+
+void TestResourceHandler::Resume() {
+  controller_->Resume();

[Randy Smith (Not in Mondays), 2016/12/02 20:38:55]

Would it make sense to have an assertion/expectation that the resource handler
previously deferred?

[mmenke, 2016/12/02 21:26:52]

Maybe in a followup CL?  This one is already kinda big, and I have a 3rd CL
where I beef up testing a little.  Or do you want it before landing my
ResourceLoader changes?

[mmenke, 2016/12/02 21:39:54]

I mean my ResourceLoader in this CL - would want to make the change before my
major refactor...  Though my refactor would remove all those checks, anyways. 
Maybe not worth it?

[Randy Smith (Not in Mondays), 2016/12/06 20:46:44]

Hmmm.  Good point.  Let's not worry about it until after your refactor.  

+}
+
+void TestResourceHandler::CancelWithError(net::Error net_error) {
+  controller_->CancelWithError(net_error);

[Randy Smith (Not in Mondays), 2016/12/02 20:38:55]

Should this set canceled_?

[mmenke, 2016/12/02 21:26:52]

Done (And missed doing that in the redirect case, too)

 }
 
 void TestResourceHandler::SetBufferSize(int buffer_size) {
@@ -108,4 +181,13 @@ void TestResourceHandler::SetBufferSize(int buffer_size) {
   memset(buffer_->data(), '\0', buffer_size);
 }
 
+void TestResourceHandler::WaitUntilDeferred() {
+  deferred_run_loop_->Run();
+  deferred_run_loop_.reset(new base::RunLoop());
+}
+
+void TestResourceHandler::WaitUntilResponseComplete() {
+  response_complete_run_loop_.Run();
+}
+
 }  // namespace content