Fix a pair of ResourceLoader cancellation/error bugs.

content/browser/loader/resource_loader_unittest.cc

 // Copyright (c) 2013 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 "content/browser/loader/resource_loader.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <memory>
 #include <utility>
 
 #include "base/files/file.h"
 #include "base/files/file_util.h"
 #include "base/location.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/weak_ptr.h"
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #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"
 #include "content/public/common/resource_response.h"
 #include "content/public/common/resource_type.h"
 #include "content/public/test/mock_resource_context.h"
 #include "content/public/test/test_browser_context.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "content/public/test/test_renderer_host.h"
 #include "content/test/test_content_browser_client.h"
 #include "content/test/test_web_contents.h"
 #include "ipc/ipc_message.h"
 #include "net/base/chunked_upload_data_stream.h"
 #include "net/base/io_buffer.h"
 #include "net/base/mock_file_stream.h"
 #include "net/base/net_errors.h"
 #include "net/base/request_priority.h"
 #include "net/base/upload_bytes_element_reader.h"
 #include "net/cert/x509_certificate.h"
 #include "net/nqe/effective_connection_type.h"
 #include "net/nqe/network_quality_estimator.h"
 #include "net/ssl/client_cert_store.h"
 #include "net/ssl/ssl_cert_request_info.h"
 #include "net/ssl/ssl_private_key.h"
 #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"
 #include "net/url_request/url_request_job_factory.h"
 #include "net/url_request/url_request_job_factory_impl.h"
 #include "net/url_request/url_request_test_job.h"
 #include "net/url_request/url_request_test_util.h"
 #include "storage/browser/blob/shareable_file_reference.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ skipping 196 matching lines @@
                   : net::URLRequestTestJob::test_headers();
     return new MockHTTPSURLRequestJob(request, network_delegate, headers,
                                       "dummy response", true);
   }
 
  private:
   bool redirect_;
 };
 
 // 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 {
  public:
   SelectCertificateBrowserClient() : call_count_(0) {}
 
   // Waits until the first call to SelectClientCertificate.
   void WaitForSelectCertificate() {
     select_certificate_run_loop_.Run();
     // Process any pending messages - just so tests can check if
@@ skipping 109 matching lines @@
         test_url_request_context_(true),
         resource_context_(&test_url_request_context_),
         raw_ptr_resource_handler_(NULL),
         raw_ptr_to_request_(NULL) {
     test_url_request_context_.set_job_factory(&job_factory_);
     test_url_request_context_.set_network_quality_estimator(
         &network_quality_estimator_);
     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_;
   }
 
   std::string test_data() const {
     return net::URLRequestTestJob::test_data_1();
   }
 
   virtual std::unique_ptr<net::URLRequestJobFactory::ProtocolHandler>
   CreateProtocolHandler() {
     return net::URLRequestTestJob::CreateProtocolHandler();
   }
 
   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);
   }
 
   // Replaces loader_ with a new one for |request|.
   void SetUpResourceLoader(std::unique_ptr<net::URLRequest> request,
                            ResourceType resource_type,
                            bool belongs_to_main_frame) {
     raw_ptr_to_request_ = request.get();
 
     // A request marked as a main frame request must also belong to a main
     // frame.
     ASSERT_TRUE((resource_type != RESOURCE_TYPE_MAIN_FRAME) ||
                 belongs_to_main_frame);
 
     RenderFrameHost* rfh = web_contents_->GetMainFrame();
     ResourceRequestInfo::AllocateForTesting(
         request.get(), resource_type, &resource_context_,
         rfh->GetProcess()->GetID(), rfh->GetRenderViewHost()->GetRoutingID(),
         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),
         WrapResourceHandler(std::move(resource_handler), raw_ptr_to_request_),
         this));
   }
 
   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 {
     // Destroy the WebContents and pump the event loop before destroying
     // |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);
   }
 
   // ResourceLoaderDelegate:
   ResourceDispatcherHostLoginDelegate* CreateLoginDelegate(
       ResourceLoader* loader,
       net::AuthChallengeInfo* auth_info) override {
     return NULL;
   }
   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_);
   }
 
   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_;
   MockResourceContext resource_context_;
   std::unique_ptr<TestBrowserContext> browser_context_;
   std::unique_ptr<TestWebContents> web_contents_;
   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_;
 };
 
 class ClientCertResourceLoaderTest : public ResourceLoaderTest {
  protected:
   std::unique_ptr<net::URLRequestJobFactory::ProtocolHandler>
   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
 // https://example-redirect.test URLs and sets SSL info on the
 // responses. The latter serves a Location: header in the response.
 class HTTPSSecurityInfoResourceLoaderTest : public ResourceLoaderTest {
  public:
   HTTPSSecurityInfoResourceLoaderTest()
       : ResourceLoaderTest(),
         test_https_url_("https://example.test"),
@@ skipping 36 matching lines @@
   SetClientCertStore(std::move(test_store));
 
   // Plug in test content browser client.
   SelectCertificateBrowserClient test_client;
   ContentBrowserClient* old_client = SetBrowserClientForTesting(&test_client);
 
   // Start the request and wait for it to pause.
   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);
   EXPECT_EQ(MockClientCertURLRequestJob::test_authorities(),
             store_requested_authorities);
 
   // Check if the retrieved certificates were passed to the content browser
   // client.
   EXPECT_EQ(1, test_client.call_count());
   EXPECT_EQ(dummy_certs, test_client.passed_certs());
 
   // 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);
 }
 
 // Tests that client certificates are requested on a platform with NULL
 // ClientCertStore.
 TEST_F(ClientCertResourceLoaderTest, WithNullStore) {
   // Plug in test content browser client.
   SelectCertificateBrowserClient test_client;
   ContentBrowserClient* old_client = SetBrowserClientForTesting(&test_client);
 
   // Start the request and wait for it to pause.
   loader_->StartRequest();
   test_client.WaitForSelectCertificate();
 
   // Check if the SelectClientCertificate was called on the content browser
   // client.
   EXPECT_EQ(1, test_client.call_count());
   EXPECT_EQ(net::CertificateList(), test_client.passed_certs());
 
   // 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);
 }
 
 // Tests that the ContentBrowserClient may cancel a certificate request.
 TEST_F(ClientCertResourceLoaderTest, CancelSelection) {
   // Plug in test content browser client.
   SelectCertificateBrowserClient test_client;
   ContentBrowserClient* old_client = SetBrowserClientForTesting(&test_client);
 
   // Start the request and wait for it to pause.
   loader_->StartRequest();
   test_client.WaitForSelectCertificate();
 
   // Check if the SelectClientCertificate was called on the content browser
   // client.
   EXPECT_EQ(1, test_client.call_count());
   EXPECT_EQ(net::CertificateList(), test_client.passed_certs());
 
   // 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);
 }
 
 // Verifies that requests without WebContents attached abort.
 TEST_F(ClientCertResourceLoaderTest, NoWebContents) {
   // Destroy the WebContents before starting the request.
   web_contents_.reset();
 
   // Plug in test content browser client.
   SelectCertificateBrowserClient test_client;
   ContentBrowserClient* old_client = SetBrowserClientForTesting(&test_client);
 
   // 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);
 }
 
 // Verifies that ClientCertStore's callback doesn't crash if called after the
 // loader is destroyed.
 TEST_F(ClientCertResourceLoaderTest, StoreAsyncCancel) {
   base::RunLoop loader_destroyed_run_loop;
   LoaderDestroyingCertStore* test_store =
       new LoaderDestroyingCertStore(&loader_,
                                     loader_destroyed_run_loop.QuitClosure());
   SetClientCertStore(base::WrapUnique(test_store));
 
   loader_->StartRequest();
   loader_destroyed_run_loop.Run();
   EXPECT_FALSE(loader_);
 
   // Pump the event loop to ensure nothing asynchronous crashes either.
   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();
+  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());
+}
+
+TEST_F(ResourceLoaderTest, SyncCancelOnWillStart) {
+  raw_ptr_resource_handler_->set_on_will_start_result(false);
+
+  loader_->StartRequest();
+  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());
+}
+
+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());
+}
+
+TEST_F(ResourceLoaderTest, SyncCancelOnResponseStarted) {
+  raw_ptr_resource_handler_->set_on_response_started_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_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());
+}
+
+TEST_F(ResourceLoaderTest, SyncCancelOnWillRead) {
+  raw_ptr_resource_handler_->set_on_will_read_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_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();
 }
 
-// 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();
-  raw_ptr_resource_handler_->WaitForResponseComplete();
-
-  EXPECT_EQ(test_url(), raw_ptr_resource_handler_->start_url());
-  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();
-  raw_ptr_resource_handler_->WaitForDeferredStep();
-
-  EXPECT_EQ(test_url(), raw_ptr_resource_handler_->start_url());
-  EXPECT_FALSE(raw_ptr_resource_handler_->received_response_completed());
-
-  raw_ptr_resource_handler_->Resume();
-  raw_ptr_resource_handler_->WaitForResponseComplete();
-  EXPECT_EQ(net::URLRequestStatus::SUCCESS,
-            raw_ptr_resource_handler_->status().status());
-}
-
 class ResourceLoaderRedirectToFileTest : public ResourceLoaderTest {
  public:
   ResourceLoaderRedirectToFileTest()
       : file_stream_(NULL),
         redirect_to_file_resource_handler_(NULL) {
   }
 
   ~ResourceLoaderRedirectToFileTest() override {
     // Releasing the loader should result in destroying the file asynchronously.
     file_stream_ = nullptr;
     deletable_file_ = nullptr;
     loader_.reset();
 
     // Wait for the task to delete the file to run, and make sure the file is
     // cleaned up.
     base::RunLoop().RunUntilIdle();
     EXPECT_FALSE(base::PathExists(temp_path()));
   }
 
   base::FilePath temp_path() const { return temp_path_; }
   ShareableFileReference* deletable_file() const {
     return deletable_file_.get();
   }
   net::testing::MockFileStream* file_stream() const { return file_stream_; }
   RedirectToFileResourceHandler* redirect_to_file_resource_handler() const {
     return redirect_to_file_resource_handler_;
   }
 
   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_));
     int flags = base::File::FLAG_WRITE | base::File::FLAG_TEMPORARY |
                 base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_ASYNC;
     base::File file(temp_path_, flags);
     CHECK(file.IsValid());
 
     // Create mock file streams and a ShareableFileReference.
     std::unique_ptr<net::testing::MockFileStream> file_stream(
@@ skipping 29 matching lines @@
   // These are owned by the ResourceLoader.
   net::testing::MockFileStream* file_stream_;
   RedirectToFileResourceHandler* redirect_to_file_resource_handler_;
 };
 
 // Tests that a RedirectToFileResourceHandler works and forwards everything
 // downstream.
 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()));
 
   // Check that the data was written to the file.
   std::string contents;
   ASSERT_TRUE(base::ReadFileToString(temp_path(), &contents));
   EXPECT_EQ(test_data(), contents);
 }
 
 // Tests that RedirectToFileResourceHandler handles errors in creating the
 // temporary file.
 TEST_F(ResourceLoaderRedirectToFileTest, CreateTemporaryError) {
   // Swap out the create temporary function.
   redirect_to_file_resource_handler()->
       SetCreateTemporaryFileStreamFunctionForTesting(
           base::Bind(&CreateTemporaryError, base::File::FILE_ERROR_FAILED));
 
   // 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());
 }
 
 // Tests that RedirectToFileResourceHandler handles synchronous write errors.
 TEST_F(ResourceLoaderRedirectToFileTest, WriteError) {
   file_stream()->set_forced_error(net::ERR_FAILED);
 
   // 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());
 }
 
 // Tests that RedirectToFileResourceHandler handles asynchronous write errors.
 TEST_F(ResourceLoaderRedirectToFileTest, WriteErrorAsync) {
   file_stream()->set_forced_error_async(net::ERR_FAILED);
 
   // 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());
 }
 
 // Tests that RedirectToFileHandler defers completion if there are outstanding
 // writes and accounts for errors which occur in that time.
 TEST_F(ResourceLoaderRedirectToFileTest, DeferCompletion) {
   // Program the MockFileStream to error asynchronously, but throttle the
   // callback.
   file_stream()->set_forced_error_async(net::ERR_FAILED);
   file_stream()->ThrottleCallbacks();
 
   // Run it as far as it will go.
   loader_->StartRequest();
   base::RunLoop().RunUntilIdle();
 
   // At this point, the request should have completed.
   EXPECT_EQ(net::URLRequestStatus::SUCCESS,
             raw_ptr_to_request_->status().status());
 
   // 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()));
 
   // Check that the data was written to the file.
   std::string contents;
   ASSERT_TRUE(base::ReadFileToString(temp_path(), &contents));
   EXPECT_EQ(test_data(), contents);
 }
 
 class EffectiveConnectionTypeResourceLoaderTest : public ResourceLoaderTest {
  public:
   void VerifyEffectiveConnectionType(
       ResourceType resource_type,
       bool belongs_to_main_frame,
       net::EffectiveConnectionType set_type,
       net::EffectiveConnectionType expected_type) {
     network_quality_estimator()->set_effective_connection_type(set_type);
 
     // Start the request and wait for it to finish.
     std::unique_ptr<net::URLRequest> request(
         resource_context_.GetRequestContext()->CreateRequest(
             test_url(), net::DEFAULT_PRIORITY, nullptr /* delegate */));
     SetUpResourceLoader(std::move(request), resource_type,
                         belongs_to_main_frame);
 
     // 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);
   }
 };
 
 // Tests that the effective connection type is set on main frame requests.
 TEST_F(EffectiveConnectionTypeResourceLoaderTest, Slow2G) {
   VerifyEffectiveConnectionType(RESOURCE_TYPE_MAIN_FRAME, true,
                                 net::EFFECTIVE_CONNECTION_TYPE_SLOW_2G,
                                 net::EFFECTIVE_CONNECTION_TYPE_SLOW_2G);
 }
 
@@ skipping 14 matching lines @@
 
 // Tests that the effective connection type is not set on non-main frame
 // requests.
 TEST_F(EffectiveConnectionTypeResourceLoaderTest, DoesNotBelongToMainFrame) {
   VerifyEffectiveConnectionType(RESOURCE_TYPE_OBJECT, false,
                                 net::EFFECTIVE_CONNECTION_TYPE_3G,
                                 net::EFFECTIVE_CONNECTION_TYPE_UNKNOWN);
 }
 
 }  // namespace content