Fix a pair of ResourceLoader cancellation/error bugs.

content/browser/loader/resource_loader_unittest.cc

Index: content/browser/loader/resource_loader_unittest.cc
diff --git a/content/browser/loader/resource_loader_unittest.cc b/content/browser/loader/resource_loader_unittest.cc
index 6ca2ac10da28978c4b8678b8264b62465151aa4a..0390508f5b4be45329bccefe67a7d171acdedc7e 100644
--- a/content/browser/loader/resource_loader_unittest.cc
+++ b/content/browser/loader/resource_loader_unittest.cc
@@ -22,6 +22,7 @@
 #include "content/browser/browser_thread_impl.h"
 #include "content/browser/loader/redirect_to_file_resource_handler.h"
 #include "content/browser/loader/resource_loader_delegate.h"
+#include "content/browser/loader/test_resource_handler.h"
 #include "content/public/browser/client_certificate_delegate.h"
 #include "content/public/browser/resource_request_info.h"
 #include "content/public/common/content_paths.h"
@@ -49,6 +50,7 @@
 #include "net/test/cert_test_util.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "net/test/test_data_directory.h"
+#include "net/test/url_request/url_request_failed_job.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_filter.h"
 #include "net/url_request/url_request_interceptor.h"
@@ -265,177 +267,6 @@ class MockHTTPSJobURLRequestInterceptor : public net::URLRequestInterceptor {
 };
 
 // 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_on_will_read_(false),
-        received_eof_(false),
-        received_response_completed_(false),
-        received_request_redirected_(false),
-        total_bytes_downloaded_(0),
-        observed_effective_connection_type_(
-            net::EFFECTIVE_CONNECTION_TYPE_UNKNOWN) {}
-
-  // 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(); }
-  ResourceResponse* redirect_response() const {
-    return redirect_response_.get();
-  }
-  bool received_response_completed() const {
-    return received_response_completed_;
-  }
-  bool received_request_redirected() const {
-    return received_request_redirected_;
-  }
-  const net::URLRequestStatus& status() const { return status_; }
-  int total_bytes_downloaded() const { return total_bytes_downloaded_; }
-
-  net::EffectiveConnectionType observed_effective_connection_type() const {
-    return observed_effective_connection_type_;
-  }
-
-  void Resume() {
-    controller()->Resume();
-  }
-
-  bool OnRequestRedirected(const net::RedirectInfo& redirect_info,
-                           ResourceResponse* response,
-                           bool* defer) override {
-    redirect_response_ = response;
-    received_request_redirected_ = true;
-    return true;
-  }
-
-  bool OnResponseStarted(ResourceResponse* response, bool* defer) override {
-    EXPECT_FALSE(response_.get());
-    response_ = response;
-    observed_effective_connection_type_ =
-        response->head.effective_connection_type;
-    return true;
-  }
-
-  bool OnWillStart(const GURL& url, bool* defer) override {
-    EXPECT_TRUE(start_url_.is_empty());
-    start_url_ = url;
-    if (defer_request_on_will_start_) {
-      *defer = true;
-      deferred_run_loop_.Quit();
-    }
-    return true;
-  }
-
-  bool OnWillRead(scoped_refptr<net::IOBuffer>* buf,
-                  int* buf_size,
-                  int min_size) override {
-    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;
-  }
-
-  bool OnReadCompleted(int bytes_read, bool* defer) override {
-    EXPECT_TRUE(received_on_will_read_);
-    EXPECT_TRUE(expect_reads_);
-    EXPECT_FALSE(received_response_completed_);
-
-    if (bytes_read == 0) {
-      received_eof_ = true;
-      if (defer_eof_) {
-        defer_eof_ = false;
-        *defer = true;
-        deferred_run_loop_.Quit();
-      }
-    }
-
-    // Need another OnWillRead() call before seeing an OnReadCompleted().
-    received_on_will_read_ = false;
-
-    return !cancel_on_read_completed_;
-  }
-
-  void OnResponseCompleted(const net::URLRequestStatus& status,
-                           bool* defer) override {
-    EXPECT_FALSE(received_response_completed_);
-    if (status.is_success() && expect_reads_)
-      EXPECT_TRUE(received_eof_);
-
-    received_response_completed_ = true;
-    status_ = status;
-    response_completed_run_loop_.Quit();
-  }
-
-  void OnDataDownloaded(int bytes_downloaded) override {
-    EXPECT_FALSE(expect_reads_);
-    total_bytes_downloaded_ += bytes_downloaded;
-  }
-
-  // Waits for the the first deferred step to run, if there is one.
-  void WaitForDeferredStep() {
-    DCHECK(defer_request_on_will_start_ || defer_eof_);
-    deferred_run_loop_.Run();
-  }
-
-  // Waits until the response has completed.
-  void WaitForResponseComplete() {
-    response_completed_run_loop_.Run();
-    EXPECT_TRUE(received_response_completed_);
-  }
-
- 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_;
-  scoped_refptr<ResourceResponse> redirect_response_;
-  bool received_on_will_read_;
-  bool received_eof_;
-  bool received_response_completed_;
-  bool received_request_redirected_;
-  net::URLRequestStatus status_;
-  int total_bytes_downloaded_;
-  base::RunLoop deferred_run_loop_;
-  base::RunLoop response_completed_run_loop_;
-  std::unique_ptr<base::RunLoop> wait_for_progress_run_loop_;
-  net::EffectiveConnectionType observed_effective_connection_type_;
-};
-
 // Test browser client that captures calls to SelectClientCertificates and
 // records the arguments of the most recent call for later inspection.
 class SelectCertificateBrowserClient : public TestContentBrowserClient {
@@ -565,7 +396,9 @@ class ResourceLoaderTest : public testing::Test,
     test_url_request_context_.Init();
   }
 
-  GURL test_url() const { return net::URLRequestTestJob::test_url_1(); }
+  GURL test_url() const {

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

nit, suggestion: Given the contrast below with test urls that don't redirect,
this wrapping strikes me as obscuring more than it helps.  Maybe get rid of it
and hoist the n:URLRTJ:t_u_r_t_u() call up, or calling it "test_redirect_url" or
something like it?

(Suggestion because of spending some time trying to understand the "this doesn't
need redirects" comment in ClientCertResourceLoaderTest::SetUp()). 

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

Done - called it test_redirect_url(), and hoisted the other URL up here, too,
calling it test_direct_url().

+    return net::URLRequestTestJob::test_url_redirect_to_url_1();
+  }
 
   TestNetworkQualityEstimator* network_quality_estimator() {
     return &network_quality_estimator_;
@@ -581,7 +414,7 @@ class ResourceLoaderTest : public testing::Test,
   }
 
   virtual std::unique_ptr<ResourceHandler> WrapResourceHandler(
-      std::unique_ptr<ResourceHandlerStub> leaf_handler,
+      std::unique_ptr<TestResourceHandler> leaf_handler,
       net::URLRequest* request) {
     return std::move(leaf_handler);
   }
@@ -604,8 +437,8 @@ class ResourceLoaderTest : public testing::Test,
         rfh->GetRoutingID(), belongs_to_main_frame,
         false /* parent_is_main_frame */, true /* allow_download */,
         false /* is_async */, false /* is_using_lofi_ */);
-    std::unique_ptr<ResourceHandlerStub> resource_handler(
-        new ResourceHandlerStub(request.get()));
+    std::unique_ptr<TestResourceHandler> resource_handler(
+        new TestResourceHandler(nullptr, nullptr));
     raw_ptr_resource_handler_ = resource_handler.get();
     loader_.reset(new ResourceLoader(
         std::move(request),
@@ -615,17 +448,14 @@ class ResourceLoaderTest : public testing::Test,
 
   void SetUp() override {
     job_factory_.SetProtocolHandler("test", CreateProtocolHandler());
+    net::URLRequestFailedJob::AddUrlHandler();
 
     browser_context_.reset(new TestBrowserContext());
     scoped_refptr<SiteInstance> site_instance =
         SiteInstance::Create(browser_context_.get());
     web_contents_.reset(
         TestWebContents::Create(browser_context_.get(), site_instance.get()));
-
-    std::unique_ptr<net::URLRequest> request(
-        resource_context_.GetRequestContext()->CreateRequest(
-            test_url(), net::DEFAULT_PRIORITY, nullptr /* delegate */));
-    SetUpResourceLoader(std::move(request), RESOURCE_TYPE_MAIN_FRAME, true);
+    SetUpResourceLoaderForUrl(test_url());
   }
 
   void TearDown() override {
@@ -633,9 +463,20 @@ class ResourceLoaderTest : public testing::Test,
     // |rvh_test_enabler_| and |thread_bundle_|. This lets asynchronous cleanup
     // tasks complete.
     web_contents_.reset();
+
+    // Clean up handlers.
+    net::URLRequestFilter::GetInstance()->ClearHandlers();
+
     base::RunLoop().RunUntilIdle();
   }
 
+  void SetUpResourceLoaderForUrl(const GURL& test_url) {
+    std::unique_ptr<net::URLRequest> request(
+        resource_context_.GetRequestContext()->CreateRequest(
+            test_url, net::DEFAULT_PRIORITY, nullptr /* delegate */));
+    SetUpResourceLoader(std::move(request), RESOURCE_TYPE_MAIN_FRAME, true);
+  }
+

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

nit, suggestion: It would feel more natural to me to have this next to
SetUpResourceLoader().

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

It's down here to it doesn't interrupt the two testing::Test overrides.  I've
moved it above SetUp, but happy to move elsewhere.

   void SetClientCertStore(std::unique_ptr<net::ClientCertStore> store) {
     dummy_cert_store_ = std::move(store);
   }
@@ -648,14 +489,36 @@ class ResourceLoaderTest : public testing::Test,
   }
   bool HandleExternalProtocol(ResourceLoader* loader,
                               const GURL& url) override {
+    EXPECT_EQ(loader, loader_.get());
     return false;
   }
-  void DidStartRequest(ResourceLoader* loader) override {}
+  void DidStartRequest(ResourceLoader* loader) override {
+    EXPECT_EQ(loader, loader_.get());
+    EXPECT_EQ(0, did_finish_loading_);
+    EXPECT_EQ(0, did_start_request_);
+    ++did_start_request_;
+  }
   void DidReceiveRedirect(ResourceLoader* loader,
                           const GURL& new_url,
-                          ResourceResponse* response) override {}
-  void DidReceiveResponse(ResourceLoader* loader) override {}
-  void DidFinishLoading(ResourceLoader* loader) override {}
+                          ResourceResponse* response) override {
+    EXPECT_EQ(loader, loader_.get());
+    EXPECT_EQ(0, did_finish_loading_);
+    EXPECT_EQ(0, did_receive_response_);
+    EXPECT_EQ(1, did_start_request_);
+    ++did_received_redirect_;
+  }
+  void DidReceiveResponse(ResourceLoader* loader) override {
+    EXPECT_EQ(loader, loader_.get());
+    EXPECT_EQ(0, did_finish_loading_);
+    EXPECT_EQ(0, did_receive_response_);
+    EXPECT_EQ(1, did_start_request_);
+    ++did_receive_response_;
+  }
+  void DidFinishLoading(ResourceLoader* loader) override {
+    EXPECT_EQ(loader, loader_.get());
+    EXPECT_EQ(0, did_finish_loading_);

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

Huh.  I presume given the abundance of EXPECTs that this means that a
DIdFinishLoading can be received without a previous DidStartRequest.  [Later]
And now I see the RequestFailsOnStart test below.  Is failing on start the only
case in which this happens?  Is there documentation somewhere about that
pathway?

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

There's no documentation.  I believe these calls were added just to avoid a
direct dependency from the ResourceLoader on the RDH, or maybe just so these
tests don't need a real RDH.  The calls between ResourceLoader and RDH were just
abstracted away.  DidStartRequest is just used for the little status display at
the lower left, to make sure the update timer is running (So unstarted requests
don't matter, though they don't really hurt).  DidFinishLoading does cleanup, so
always has to be called, or we have a leak.

I'd like to revisit this API a bit at some point, but one thing at a time.  :( 
DidStartRequest would probably be pretty easy to remove, at least, but probably
want to put that off until we decide how to handle things in a post-mojo world.

+    ++did_finish_loading_;
+  }
   std::unique_ptr<net::ClientCertStore> CreateClientCertStore(
       ResourceLoader* loader) override {
     return std::move(dummy_cert_store_);
@@ -664,6 +527,12 @@ class ResourceLoaderTest : public testing::Test,
   TestBrowserThreadBundle thread_bundle_;
   RenderViewHostTestEnabler rvh_test_enabler_;
 
+  // Record which ResourceDispatcherHostDelegate methods have been invoked.
+  int did_start_request_ = 0;
+  int did_received_redirect_ = 0;
+  int did_receive_response_ = 0;
+  int did_finish_loading_ = 0;
+
   net::URLRequestJobFactoryImpl job_factory_;
   TestNetworkQualityEstimator network_quality_estimator_;
   net::TestURLRequestContext test_url_request_context_;
@@ -673,7 +542,7 @@ class ResourceLoaderTest : public testing::Test,
   std::unique_ptr<net::ClientCertStore> dummy_cert_store_;
 
   // The ResourceLoader owns the URLRequest and the ResourceHandler.
-  ResourceHandlerStub* raw_ptr_resource_handler_;
+  TestResourceHandler* raw_ptr_resource_handler_;
   net::URLRequest* raw_ptr_to_request_;
   std::unique_ptr<ResourceLoader> loader_;
 };
@@ -684,6 +553,12 @@ class ClientCertResourceLoaderTest : public ResourceLoaderTest {
   CreateProtocolHandler() override {
     return base::WrapUnique(new MockClientCertJobProtocolHandler);
   }
+
+  void SetUp() override {
+    ResourceLoaderTest::SetUp();
+    // These tests don't expect any redirects.
+    SetUpResourceLoaderForUrl(net::URLRequestTestJob::test_url_1());
+  }
 };
 
 // A ResourceLoaderTest that intercepts https://example.test and
@@ -740,7 +615,7 @@ TEST_F(ClientCertResourceLoaderTest, WithStoreLookup) {
   loader_->StartRequest();
   test_client.WaitForSelectCertificate();
 
-  EXPECT_FALSE(raw_ptr_resource_handler_->received_response_completed());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_response_completed_called());
 
   // Check if the test store was queried against correct |cert_authorities|.
   EXPECT_EQ(1, store_request_count);
@@ -754,8 +629,8 @@ TEST_F(ClientCertResourceLoaderTest, WithStoreLookup) {
 
   // Continue the request.
   test_client.ContinueWithCertificate(nullptr);
-  raw_ptr_resource_handler_->WaitForResponseComplete();
-  EXPECT_EQ(net::OK, raw_ptr_resource_handler_->status().error());
+  raw_ptr_resource_handler_->WaitUntilResponseComplete();
+  EXPECT_EQ(net::OK, raw_ptr_resource_handler_->final_status().error());
 
   // Restore the original content browser client.
   SetBrowserClientForTesting(old_client);
@@ -779,8 +654,8 @@ TEST_F(ClientCertResourceLoaderTest, WithNullStore) {
 
   // Continue the request.
   test_client.ContinueWithCertificate(nullptr);
-  raw_ptr_resource_handler_->WaitForResponseComplete();
-  EXPECT_EQ(net::OK, raw_ptr_resource_handler_->status().error());
+  raw_ptr_resource_handler_->WaitUntilResponseComplete();
+  EXPECT_EQ(net::OK, raw_ptr_resource_handler_->final_status().error());
 
   // Restore the original content browser client.
   SetBrowserClientForTesting(old_client);
@@ -803,9 +678,9 @@ TEST_F(ClientCertResourceLoaderTest, CancelSelection) {
 
   // Cancel the request.
   test_client.CancelCertificateSelection();
-  raw_ptr_resource_handler_->WaitForResponseComplete();
+  raw_ptr_resource_handler_->WaitUntilResponseComplete();
   EXPECT_EQ(net::ERR_SSL_CLIENT_AUTH_CERT_NEEDED,
-            raw_ptr_resource_handler_->status().error());
+            raw_ptr_resource_handler_->final_status().error());
 
   // Restore the original content browser client.
   SetBrowserClientForTesting(old_client);
@@ -822,12 +697,12 @@ TEST_F(ClientCertResourceLoaderTest, NoWebContents) {
 
   // Start the request and wait for it to complete.
   loader_->StartRequest();
-  raw_ptr_resource_handler_->WaitForResponseComplete();
+  raw_ptr_resource_handler_->WaitUntilResponseComplete();
 
   // Check that SelectClientCertificate wasn't called and the request aborted.
   EXPECT_EQ(0, test_client.call_count());
   EXPECT_EQ(net::ERR_SSL_CLIENT_AUTH_CERT_NEEDED,
-            raw_ptr_resource_handler_->status().error());
+            raw_ptr_resource_handler_->final_status().error());
 
   // Restore the original content browser client.
   SetBrowserClientForTesting(old_client);
@@ -850,41 +725,421 @@ TEST_F(ClientCertResourceLoaderTest, StoreAsyncCancel) {
   base::RunLoop().RunUntilIdle();
 }
 
-TEST_F(ResourceLoaderTest, ResumeCancelledRequest) {
-  raw_ptr_resource_handler_->set_defer_request_on_will_start(true);
+// Test the case the ResourceHandler defers nothing.
+TEST_F(ResourceLoaderTest, SyncResourceHandler) {
+  loader_->StartRequest();
+  raw_ptr_resource_handler_->WaitUntilResponseComplete();
+  EXPECT_EQ(1, did_start_request_);
+  EXPECT_EQ(1, did_received_redirect_);
+  EXPECT_EQ(1, did_receive_response_);
+  EXPECT_EQ(1, did_finish_loading_);
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_request_redirected_called());
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_response_completed_called());
+  EXPECT_EQ(net::OK, raw_ptr_resource_handler_->final_status().error());
+  EXPECT_EQ(test_data(), raw_ptr_resource_handler_->body());
+}
+
+// Test the case the ResourceHandler defers everything.
+TEST_F(ResourceLoaderTest, AsyncResourceHandler) {
+  raw_ptr_resource_handler_->set_defer_on_will_start(true);
+  raw_ptr_resource_handler_->set_defer_on_request_redirected(true);
+  raw_ptr_resource_handler_->set_defer_on_response_started(true);
+  raw_ptr_resource_handler_->set_defer_on_read_completed(true);
+  raw_ptr_resource_handler_->set_defer_on_read_eof(true);
+  raw_ptr_resource_handler_->set_defer_on_response_completed(true);
 
+  // Start and run until OnWillStart.
   loader_->StartRequest();
-  loader_->CancelRequest(true);
-  static_cast<ResourceController*>(loader_.get())->Resume();
+  raw_ptr_resource_handler_->WaitUntilDeferred();
+  // Spinning the message loop should not advance the state further.

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

Hmmmm.  This is testing to make sure that state doesn't go beyond the intended
final state by spinning the message loop, but is not testing that nothing
changes; it's possible that the WaitUntilDeferred() call didn't get as far as we
expect and that the second spin catches the state up.  

I'm not sure whether I want to request the obvious change (check the state
twice, before/after); it balloons the test, but given how wonky this code is,
I'm not convinced that the failure mode I describe is unreasonable.  Maybe think
about some test helper routine that checks for expected state on the test &
r_p_r_h variables all at once, so that only a couple of lines of code will be
duplicated?  

I'll leave this one up to you, but I do want to raise the issue.  I recognize
I'm being paranoid, it's just that that feels more justified than usual :-}.

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

I've just copied the "most imporant" EXPECT_EQ(1...) test before the extra wait
on all of these, though I'm not sure they get us anything.  Seems like they're
only testing that the TestResourceHandler defers in the method we told it to.

Similar checks make no sense in the cancel tests - those all wait until
completion, which makes the on_response_completed_called check already pretty
redundant.

+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(0, did_start_request_);
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_will_start_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_request_redirected_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_response_completed_called());
+
+  // Resume and run until OnRequestRedirected.
+  raw_ptr_resource_handler_->Resume();
+  raw_ptr_resource_handler_->WaitUntilDeferred();
+  // Spinning the message loop should not advance the state further.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, did_received_redirect_);
+  EXPECT_EQ(0, did_receive_response_);
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_request_redirected_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_will_read_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_response_completed_called());
+
+  // Resume and run until OnResponseStarted.
+  raw_ptr_resource_handler_->Resume();
+  raw_ptr_resource_handler_->WaitUntilDeferred();
+  // Spinning the message loop should not advance the state further.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, did_receive_response_);
+  EXPECT_EQ(0, did_finish_loading_);
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_response_started_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_will_read_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_response_completed_called());
+
+  // Resume and run until OnReadCompleted.
+  raw_ptr_resource_handler_->Resume();
+  raw_ptr_resource_handler_->WaitUntilDeferred();
+  // Spinning the message loop should not advance the state further.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_will_read_called());
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_read_completed_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_read_eof());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_response_completed_called());
+
+  // Resume and run until the final 0-byte read, signalling EOF.
+  raw_ptr_resource_handler_->Resume();
+  raw_ptr_resource_handler_->WaitUntilDeferred();
+  // Spinning the message loop should not advance the state further.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_read_eof());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_response_completed_called());
+  EXPECT_EQ(test_data(), raw_ptr_resource_handler_->body());
+
+  // Resume and run until OnResponseCompleted is called, which again defers the
+  // request.
+  raw_ptr_resource_handler_->Resume();
+  raw_ptr_resource_handler_->WaitUntilResponseComplete();
+  EXPECT_EQ(0, did_finish_loading_);
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_response_completed_called());
+  EXPECT_EQ(net::OK, raw_ptr_resource_handler_->final_status().error());
+  EXPECT_EQ(test_data(), raw_ptr_resource_handler_->body());
+
+  // Resume and run until all pending tasks. Note that OnResponseCompleted was
+  // invoked in the previous section, so can't use RunUntilCompleted().
+  raw_ptr_resource_handler_->Resume();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, did_finish_loading_);
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_response_completed_called());
+  EXPECT_EQ(net::OK, raw_ptr_resource_handler_->final_status().error());
+  EXPECT_EQ(test_data(), raw_ptr_resource_handler_->body());
 }
 
-// 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);
+TEST_F(ResourceLoaderTest, SyncCancelOnWillStart) {
+  raw_ptr_resource_handler_->set_on_will_start_result(false);
 
   loader_->StartRequest();
-  raw_ptr_resource_handler_->WaitForResponseComplete();
+  raw_ptr_resource_handler_->WaitUntilResponseComplete();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(0, did_start_request_);
+  EXPECT_EQ(1, did_finish_loading_);
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_will_start_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_request_redirected_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_response_started_called());
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_response_completed_called());
+
+  EXPECT_EQ(net::ERR_ABORTED,
+            raw_ptr_resource_handler_->final_status().error());
+  EXPECT_EQ("", raw_ptr_resource_handler_->body());
+}
 
-  EXPECT_EQ(test_url(), raw_ptr_resource_handler_->start_url());
-  EXPECT_EQ(net::URLRequestStatus::CANCELED,
-            raw_ptr_resource_handler_->status().status());
+TEST_F(ResourceLoaderTest, SyncCancelOnRequestRedirected) {
+  raw_ptr_resource_handler_->set_on_request_redirected_result(false);
+
+  loader_->StartRequest();
+  raw_ptr_resource_handler_->WaitUntilResponseComplete();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, did_received_redirect_);
+  EXPECT_EQ(0, did_receive_response_);
+  EXPECT_EQ(1, did_finish_loading_);
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_request_redirected_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_will_read_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_read_completed_called());
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_response_completed_called());
+
+  EXPECT_EQ(net::ERR_ABORTED,
+            raw_ptr_resource_handler_->final_status().error());
+  EXPECT_EQ("", raw_ptr_resource_handler_->body());
 }
 
-// Tests that no invariants are broken if a ResourceHandler defers EOF.
-TEST_F(ResourceLoaderTest, DeferEOF) {
-  raw_ptr_resource_handler_->set_defer_eof(true);
+TEST_F(ResourceLoaderTest, SyncCancelOnResponseStarted) {
+  raw_ptr_resource_handler_->set_on_response_started_result(false);
 
   loader_->StartRequest();
-  raw_ptr_resource_handler_->WaitForDeferredStep();
+  raw_ptr_resource_handler_->WaitUntilResponseComplete();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, did_receive_response_);
+  EXPECT_EQ(1, did_finish_loading_);
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_response_started_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_will_read_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_read_completed_called());
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_response_completed_called());
+
+  EXPECT_EQ(net::ERR_ABORTED,
+            raw_ptr_resource_handler_->final_status().error());
+  EXPECT_EQ("", raw_ptr_resource_handler_->body());
+}
 
-  EXPECT_EQ(test_url(), raw_ptr_resource_handler_->start_url());
-  EXPECT_FALSE(raw_ptr_resource_handler_->received_response_completed());
+TEST_F(ResourceLoaderTest, SyncCancelOnWillRead) {
+  raw_ptr_resource_handler_->set_on_will_read_result(false);
 
-  raw_ptr_resource_handler_->Resume();
-  raw_ptr_resource_handler_->WaitForResponseComplete();
-  EXPECT_EQ(net::URLRequestStatus::SUCCESS,
-            raw_ptr_resource_handler_->status().status());
+  loader_->StartRequest();
+  raw_ptr_resource_handler_->WaitUntilResponseComplete();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, did_receive_response_);
+  EXPECT_EQ(1, did_finish_loading_);
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_will_read_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_read_completed_called());
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_response_completed_called());
+
+  EXPECT_EQ(net::ERR_ABORTED,
+            raw_ptr_resource_handler_->final_status().error());
+  EXPECT_EQ("", raw_ptr_resource_handler_->body());
+}
+
+TEST_F(ResourceLoaderTest, SyncCancelOnReadCompleted) {
+  raw_ptr_resource_handler_->set_on_read_completed_result(false);
+
+  loader_->StartRequest();
+  raw_ptr_resource_handler_->WaitUntilResponseComplete();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, did_receive_response_);
+  EXPECT_EQ(1, did_finish_loading_);
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_read_completed_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_read_eof());
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_response_completed_called());
+
+  EXPECT_EQ(net::ERR_ABORTED,
+            raw_ptr_resource_handler_->final_status().error());
+  EXPECT_LT(0u, raw_ptr_resource_handler_->body().size());
+}
+
+// This test is broken because ResourceLoader assumes when
+// URLRequest::was_pending() is false, canceling the request will not result in
+// a completion notification.  This isn't the case - whether or not there's a
+// notification depends on whether URLRequestJob::NotifyDone() has been invoked
+// yet - something the URLRequest doesn't even know about, itself. As a result,
+// the ResourceLoader is notified of cancellation twice.
+// TODO(mmenke): Fix this.
+TEST_F(ResourceLoaderTest, DISABLED_SyncCancelOnReceivedEof) {
+  raw_ptr_resource_handler_->set_on_on_read_eof_result(false);
+
+  loader_->StartRequest();
+  raw_ptr_resource_handler_->WaitUntilResponseComplete();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, did_receive_response_);
+  EXPECT_EQ(1, did_finish_loading_);
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_read_eof());
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_response_completed_called());
+
+  EXPECT_EQ(net::ERR_ABORTED,
+            raw_ptr_resource_handler_->final_status().error());
+  EXPECT_EQ(test_data(), raw_ptr_resource_handler_->body());
+}
+
+TEST_F(ResourceLoaderTest, AsyncCancelOnWillStart) {

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

Suggestion, nitty nit: I'm wanting to be precise in my descriptions of all this
stuff since I'm finding it *really* easy to mis-think about it, and I'm
specifically wanting to be precise about Sync vs. Async in-band vs. Async
out-of-band.  These tests are (just barely) Async out of band, so I'd rather
call them AsyncCancelAfter* rather than AsyncCancelOn*.  But I'm fine if you
disagree--I'm mostly making the statement to keep the distinction clear in my
mind as I read.

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

I'd call these async in-band.  The async is triggered by the OnWillStart, but
asynchronously, and it's blocking the request in the meantime.

I consider OnWillStart to last from when the first ResourceHandler's OnWillStart
method is called to when the last one's OnWillStart method completes.

Neither view is right or wrong, just think using "after" is more confusing here.

+  raw_ptr_resource_handler_->set_defer_on_will_start(true);
+
+  loader_->StartRequest();
+  raw_ptr_resource_handler_->WaitUntilDeferred();
+  raw_ptr_resource_handler_->CancelWithError(net::ERR_FAILED);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(0, did_start_request_);
+  EXPECT_EQ(1, did_finish_loading_);
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_will_start_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_request_redirected_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_response_started_called());
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_response_completed_called());
+
+  EXPECT_EQ(net::ERR_FAILED, raw_ptr_resource_handler_->final_status().error());
+  EXPECT_EQ("", raw_ptr_resource_handler_->body());
+}
+
+TEST_F(ResourceLoaderTest, AsyncCancelOnRequestRedirected) {
+  raw_ptr_resource_handler_->set_defer_on_request_redirected(true);
+
+  loader_->StartRequest();
+  raw_ptr_resource_handler_->WaitUntilDeferred();
+  raw_ptr_resource_handler_->CancelWithError(net::ERR_FAILED);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, did_received_redirect_);
+  EXPECT_EQ(0, did_receive_response_);
+  EXPECT_EQ(1, did_finish_loading_);
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_request_redirected_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_will_read_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_read_completed_called());
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_response_completed_called());
+
+  EXPECT_EQ(net::ERR_FAILED, raw_ptr_resource_handler_->final_status().error());
+  EXPECT_EQ("", raw_ptr_resource_handler_->body());
+}
+
+TEST_F(ResourceLoaderTest, AsyncCancelOnResponseStarted) {
+  raw_ptr_resource_handler_->set_defer_on_response_started(true);
+
+  loader_->StartRequest();
+  raw_ptr_resource_handler_->WaitUntilDeferred();
+  raw_ptr_resource_handler_->CancelWithError(net::ERR_FAILED);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, did_receive_response_);
+  EXPECT_EQ(1, did_finish_loading_);
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_response_started_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_will_read_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_read_completed_called());
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_response_completed_called());
+
+  EXPECT_EQ(net::ERR_FAILED, raw_ptr_resource_handler_->final_status().error());
+  EXPECT_EQ("", raw_ptr_resource_handler_->body());
+}
+
+TEST_F(ResourceLoaderTest, AsyncCancelOnReadCompleted) {
+  raw_ptr_resource_handler_->set_defer_on_read_completed(true);
+
+  loader_->StartRequest();
+  raw_ptr_resource_handler_->WaitUntilDeferred();
+  raw_ptr_resource_handler_->CancelWithError(net::ERR_FAILED);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, did_receive_response_);
+  EXPECT_EQ(1, did_finish_loading_);
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_read_completed_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_read_eof());
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_response_completed_called());
+
+  EXPECT_EQ(net::ERR_FAILED, raw_ptr_resource_handler_->final_status().error());
+  EXPECT_LT(0u, raw_ptr_resource_handler_->body().size());
+}
+
+TEST_F(ResourceLoaderTest, AsyncCancelOnReceivedEof) {
+  raw_ptr_resource_handler_->set_defer_on_read_eof(true);
+
+  loader_->StartRequest();
+  raw_ptr_resource_handler_->WaitUntilDeferred();
+  // Have to spin the message loop for the test to pass - see comment on sync
+  // version of this test for explanation.

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

FWIW, I read over the explanation on the sync test twice and haven't figured out
how it explains the double spin.  I presume the first one is to move the request
through until Eof is reached, and the second is to handle the PostTask that
CancelWithError() generates?  And that we don't get a double completion because
we aren't going through the sync return pathway in the ResourceLoader, which is
what dispatches the extra completion message?

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

The issue is URLRequest/URLRequestJob.  If you look at
URLRequestJob::NotifyDone, which is called on completion and on failure, it
posts a task to run URLRequestJob::CompleteNotifyDone asynchronously. 
URLRequestJob::CompleteNotifyDone checks if the request failed, and if so, tells
us about a failed read.

So what happens it URLRequestJob::Read sees the request succeeded, it calls
NotifyDone() and returns telling us the read succeeded, with 0 bytes read, and
we're done.  NotifyDone() posts a task to call CompleteNotifyDone.  If the
request isn't canceled, CompleteNotifyDone sees the request succeeded, and does
nothing.  If the request is cancelled before CompleteNotifyDone runs,
URLRequestJob sees the error and tells us about the failure.  So the extra
message loop spin is to wait for CompleteNotifyDone to complete.

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

That makes sense, but I still think there's a lacuna in the commenting. 
Specifically, the comment on the Sync test doesn't explain that the notification
from the URLRequestJob is done through a PostTask, and thus that we have to spin
the message loop to get it.   Willing to add a note about that (can just be an
addendum here that mentions the notification mentioned in the sync case is done
through a PostTask)?

[mmenke, 2016/12/06 21:09:53]

The next CL in this chain removes the extra spin, and fixes the issue
(Amdittedly, there's still some ugliness in URLRequestJob to avoid this case,
which will hopefully go away once I rework the suspend mode stuff).

Anyhow, given that I already have a CL out for review that removes this extra
spin, doesn't seem worth commenting on.

+  // TODO(mmenke): Remove this line once that's fixed.
+  base::RunLoop().RunUntilIdle();
+  raw_ptr_resource_handler_->CancelWithError(net::ERR_FAILED);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, did_receive_response_);
+  EXPECT_EQ(1, did_finish_loading_);
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_read_eof());
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_response_completed_called());
+
+  EXPECT_EQ(net::ERR_FAILED, raw_ptr_resource_handler_->final_status().error());
+  EXPECT_EQ(test_data(), raw_ptr_resource_handler_->body());
+}
+
+TEST_F(ResourceLoaderTest, RequestFailsOnStart) {
+  SetUpResourceLoaderForUrl(
+      net::URLRequestFailedJob::GetMockHttpUrlWithFailurePhase(
+          net::URLRequestFailedJob::START, net::ERR_FAILED));
+
+  loader_->StartRequest();
+  raw_ptr_resource_handler_->WaitUntilResponseComplete();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(0, did_received_redirect_);
+  EXPECT_EQ(0, did_receive_response_);
+  EXPECT_EQ(1, did_finish_loading_);
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_will_start_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_request_redirected_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_response_started_called());
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_response_completed_called());
+  EXPECT_EQ(net::ERR_FAILED, raw_ptr_resource_handler_->final_status().error());
+  EXPECT_EQ("", raw_ptr_resource_handler_->body());
+}
+
+TEST_F(ResourceLoaderTest, RequestFailsOnReadSync) {
+  SetUpResourceLoaderForUrl(
+      net::URLRequestFailedJob::GetMockHttpUrlWithFailurePhase(
+          net::URLRequestFailedJob::READ_SYNC, net::ERR_FAILED));
+
+  loader_->StartRequest();
+  raw_ptr_resource_handler_->WaitUntilResponseComplete();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(0, did_received_redirect_);
+  EXPECT_EQ(1, did_receive_response_);
+  EXPECT_EQ(1, did_finish_loading_);
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_will_start_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_request_redirected_called());
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_response_started_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_read_completed_called());
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_response_completed_called());
+  EXPECT_EQ(net::ERR_FAILED, raw_ptr_resource_handler_->final_status().error());
+  EXPECT_EQ("", raw_ptr_resource_handler_->body());
+}
+
+TEST_F(ResourceLoaderTest, RequestFailsOnReadAsync) {
+  SetUpResourceLoaderForUrl(
+      net::URLRequestFailedJob::GetMockHttpUrlWithFailurePhase(
+          net::URLRequestFailedJob::READ_ASYNC, net::ERR_FAILED));
+
+  loader_->StartRequest();
+  raw_ptr_resource_handler_->WaitUntilResponseComplete();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(0, did_received_redirect_);
+  EXPECT_EQ(1, did_receive_response_);
+  EXPECT_EQ(1, did_finish_loading_);
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_will_start_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_request_redirected_called());
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_response_started_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_read_completed_called());
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_response_completed_called());
+  EXPECT_EQ(net::ERR_FAILED, raw_ptr_resource_handler_->final_status().error());
+  EXPECT_EQ("", raw_ptr_resource_handler_->body());
+}
+
+TEST_F(ResourceLoaderTest, OutOfBandCancelDuringStart) {
+  SetUpResourceLoaderForUrl(
+      net::URLRequestFailedJob::GetMockHttpUrlWithFailurePhase(
+          net::URLRequestFailedJob::START, net::ERR_IO_PENDING));
+
+  loader_->StartRequest();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_will_start_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_request_redirected_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_response_started_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_response_completed_called());
+
+  raw_ptr_resource_handler_->CancelWithError(net::ERR_FAILED);
+  raw_ptr_resource_handler_->WaitUntilResponseComplete();
+
+  EXPECT_EQ(0, did_received_redirect_);
+  EXPECT_EQ(0, did_receive_response_);
+  EXPECT_EQ(1, did_finish_loading_);
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_will_start_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_request_redirected_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_response_started_called());
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_response_completed_called());
+  EXPECT_EQ(net::ERR_FAILED, raw_ptr_resource_handler_->final_status().error());
+  EXPECT_EQ("", raw_ptr_resource_handler_->body());
+}
+
+TEST_F(ResourceLoaderTest, OutOfBandCancelDuringRead) {
+  SetUpResourceLoaderForUrl(
+      net::URLRequestFailedJob::GetMockHttpUrlWithFailurePhase(
+          net::URLRequestFailedJob::READ_SYNC, net::ERR_IO_PENDING));
+
+  loader_->StartRequest();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_will_start_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_request_redirected_called());
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_response_started_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_read_completed_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_response_completed_called());
+
+  raw_ptr_resource_handler_->CancelWithError(net::ERR_FAILED);
+  raw_ptr_resource_handler_->WaitUntilResponseComplete();
+  EXPECT_EQ(0, did_received_redirect_);
+  EXPECT_EQ(1, did_receive_response_);
+  EXPECT_EQ(1, did_finish_loading_);
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_will_start_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_request_redirected_called());
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_response_started_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_read_completed_called());
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_response_completed_called());
+  EXPECT_EQ(net::ERR_FAILED, raw_ptr_resource_handler_->final_status().error());
+  EXPECT_EQ("", raw_ptr_resource_handler_->body());
+}
+
+TEST_F(ResourceLoaderTest, ResumeCanceledRequest) {
+  raw_ptr_resource_handler_->set_defer_on_will_start(true);
+
+  loader_->StartRequest();
+  loader_->CancelRequest(true);
+  static_cast<ResourceController*>(loader_.get())->Resume();
 }
 
 class ResourceLoaderRedirectToFileTest : public ResourceLoaderTest {
@@ -916,9 +1171,9 @@ class ResourceLoaderRedirectToFileTest : public ResourceLoaderTest {
   }
 
   std::unique_ptr<ResourceHandler> WrapResourceHandler(
-      std::unique_ptr<ResourceHandlerStub> leaf_handler,
+      std::unique_ptr<TestResourceHandler> leaf_handler,
       net::URLRequest* request) override {
-    leaf_handler->set_expect_reads(false);
+    leaf_handler->set_expect_on_data_downloaded(true);
 
     // Make a temporary file.
     CHECK(base::CreateTemporaryFile(&temp_path_));
@@ -968,14 +1223,15 @@ class ResourceLoaderRedirectToFileTest : public ResourceLoaderTest {
 TEST_F(ResourceLoaderRedirectToFileTest, Basic) {
   // Run it to completion.
   loader_->StartRequest();
-  raw_ptr_resource_handler_->WaitForResponseComplete();
+  raw_ptr_resource_handler_->WaitUntilResponseComplete();
 
   // Check that the handler forwarded all information to the downstream handler.
-  EXPECT_EQ(temp_path(),
-            raw_ptr_resource_handler_->response()->head.download_file_path);
+  EXPECT_EQ(
+      temp_path(),
+      raw_ptr_resource_handler_->resource_response()->head.download_file_path);
   EXPECT_EQ(test_url(), raw_ptr_resource_handler_->start_url());
   EXPECT_EQ(net::URLRequestStatus::SUCCESS,
-            raw_ptr_resource_handler_->status().status());
+            raw_ptr_resource_handler_->final_status().status());
   EXPECT_EQ(test_data().size(), static_cast<size_t>(
       raw_ptr_resource_handler_->total_bytes_downloaded()));
 
@@ -995,11 +1251,11 @@ TEST_F(ResourceLoaderRedirectToFileTest, CreateTemporaryError) {
 
   // Run it to completion.
   loader_->StartRequest();
-  raw_ptr_resource_handler_->WaitForResponseComplete();
+  raw_ptr_resource_handler_->WaitUntilResponseComplete();
 
   // To downstream, the request was canceled.
   EXPECT_EQ(net::URLRequestStatus::CANCELED,
-            raw_ptr_resource_handler_->status().status());
+            raw_ptr_resource_handler_->final_status().status());
   EXPECT_EQ(0, raw_ptr_resource_handler_->total_bytes_downloaded());
 }
 
@@ -1009,15 +1265,16 @@ TEST_F(ResourceLoaderRedirectToFileTest, WriteError) {
 
   // Run it to completion.
   loader_->StartRequest();
-  raw_ptr_resource_handler_->WaitForResponseComplete();
+  raw_ptr_resource_handler_->WaitUntilResponseComplete();
 
   // To downstream, the request was canceled sometime after it started, but
   // before any data was written.
-  EXPECT_EQ(temp_path(),
-            raw_ptr_resource_handler_->response()->head.download_file_path);
+  EXPECT_EQ(
+      temp_path(),
+      raw_ptr_resource_handler_->resource_response()->head.download_file_path);
   EXPECT_EQ(test_url(), raw_ptr_resource_handler_->start_url());
   EXPECT_EQ(net::URLRequestStatus::CANCELED,
-            raw_ptr_resource_handler_->status().status());
+            raw_ptr_resource_handler_->final_status().status());
   EXPECT_EQ(0, raw_ptr_resource_handler_->total_bytes_downloaded());
 }
 
@@ -1027,15 +1284,16 @@ TEST_F(ResourceLoaderRedirectToFileTest, WriteErrorAsync) {
 
   // Run it to completion.
   loader_->StartRequest();
-  raw_ptr_resource_handler_->WaitForResponseComplete();
+  raw_ptr_resource_handler_->WaitUntilResponseComplete();
 
   // To downstream, the request was canceled sometime after it started, but
   // before any data was written.
-  EXPECT_EQ(temp_path(),
-            raw_ptr_resource_handler_->response()->head.download_file_path);
+  EXPECT_EQ(
+      temp_path(),
+      raw_ptr_resource_handler_->resource_response()->head.download_file_path);
   EXPECT_EQ(test_url(), raw_ptr_resource_handler_->start_url());
   EXPECT_EQ(net::URLRequestStatus::CANCELED,
-            raw_ptr_resource_handler_->status().status());
+            raw_ptr_resource_handler_->final_status().status());
   EXPECT_EQ(0, raw_ptr_resource_handler_->total_bytes_downloaded());
 }
 
@@ -1057,48 +1315,50 @@ TEST_F(ResourceLoaderRedirectToFileTest, DeferCompletion) {
 
   // However, the resource loader stack is stuck somewhere after receiving the
   // response.
-  EXPECT_EQ(temp_path(),
-            raw_ptr_resource_handler_->response()->head.download_file_path);
+  EXPECT_EQ(
+      temp_path(),
+      raw_ptr_resource_handler_->resource_response()->head.download_file_path);
   EXPECT_EQ(test_url(), raw_ptr_resource_handler_->start_url());
-  EXPECT_FALSE(raw_ptr_resource_handler_->received_response_completed());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_response_completed_called());
   EXPECT_EQ(0, raw_ptr_resource_handler_->total_bytes_downloaded());
 
   // Now, release the floodgates.
   file_stream()->ReleaseCallbacks();
-  raw_ptr_resource_handler_->WaitForResponseComplete();
+  raw_ptr_resource_handler_->WaitUntilResponseComplete();
 
   // Although the URLRequest was successful, the leaf handler sees a failure
   // because the write never completed.
   EXPECT_EQ(net::URLRequestStatus::CANCELED,
-            raw_ptr_resource_handler_->status().status());
+            raw_ptr_resource_handler_->final_status().status());
 }
 
 // Tests that a RedirectToFileResourceHandler behaves properly when the
 // downstream handler defers OnWillStart.
 TEST_F(ResourceLoaderRedirectToFileTest, DownstreamDeferStart) {
   // Defer OnWillStart.
-  raw_ptr_resource_handler_->set_defer_request_on_will_start(true);
+  raw_ptr_resource_handler_->set_defer_on_will_start(true);
 
   // Run as far as we'll go.
   loader_->StartRequest();
-  raw_ptr_resource_handler_->WaitForDeferredStep();
+  raw_ptr_resource_handler_->WaitUntilDeferred();
 
   // The request should have stopped at OnWillStart.
   EXPECT_EQ(test_url(), raw_ptr_resource_handler_->start_url());
-  EXPECT_FALSE(raw_ptr_resource_handler_->response());
-  EXPECT_FALSE(raw_ptr_resource_handler_->received_response_completed());
+  EXPECT_FALSE(raw_ptr_resource_handler_->resource_response());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_response_completed_called());
   EXPECT_EQ(0, raw_ptr_resource_handler_->total_bytes_downloaded());
 
   // Now resume the request. Now we complete.
   raw_ptr_resource_handler_->Resume();
-  raw_ptr_resource_handler_->WaitForResponseComplete();
+  raw_ptr_resource_handler_->WaitUntilResponseComplete();
 
   // Check that the handler forwarded all information to the downstream handler.
-  EXPECT_EQ(temp_path(),
-            raw_ptr_resource_handler_->response()->head.download_file_path);
+  EXPECT_EQ(
+      temp_path(),
+      raw_ptr_resource_handler_->resource_response()->head.download_file_path);
   EXPECT_EQ(test_url(), raw_ptr_resource_handler_->start_url());
   EXPECT_EQ(net::URLRequestStatus::SUCCESS,
-            raw_ptr_resource_handler_->status().status());
+            raw_ptr_resource_handler_->final_status().status());
   EXPECT_EQ(test_data().size(), static_cast<size_t>(
       raw_ptr_resource_handler_->total_bytes_downloaded()));
 
@@ -1126,12 +1386,12 @@ class EffectiveConnectionTypeResourceLoaderTest : public ResourceLoaderTest {
 
     // Send the request and wait until it completes.
     loader_->StartRequest();
-    raw_ptr_resource_handler_->WaitForResponseComplete();
+    raw_ptr_resource_handler_->WaitUntilResponseComplete();
     ASSERT_EQ(net::URLRequestStatus::SUCCESS,
               raw_ptr_to_request_->status().status());
 
-    EXPECT_EQ(expected_type,
-              raw_ptr_resource_handler_->observed_effective_connection_type());
+    EXPECT_EQ(expected_type, raw_ptr_resource_handler_->resource_response()
+                                 ->head.effective_connection_type);
   }
 };