Move ResourceHandler deferred actions ahead of external protocol handling.

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 6515453be66ce231384b8924578637c0bfd2bba7..d8e829cbcfa25a1d86162ef39dec5bcc192f9e64 100644
--- a/content/browser/loader/resource_loader_unittest.cc
+++ b/content/browser/loader/resource_loader_unittest.cc
@@ -501,6 +501,13 @@ class ResourceLoaderTest : public testing::Test,
   bool HandleExternalProtocol(ResourceLoader* loader,
                               const GURL& url) override {
     EXPECT_EQ(loader, loader_.get());
+    ++handle_external_protocol_;
+
+    // Check that calls to HandleExternalProtocol always happen after the calls
+    // to the ResourceHandler's OnWillStart and OnRequestRedirected.
+    EXPECT_EQ(handle_external_protocol_,
+              raw_ptr_resource_handler_->on_will_start_called() +
+                  raw_ptr_resource_handler_->on_request_redirected_called());
     return false;

[mmenke, 2016/12/16 16:19:36]

Do we have any tests where HandleExternalProtocol returns true?  If not, should
have tests for all relevant paths:

1)  Sync OnWillStart successful completion -> HandleExternalProtocol returns
true -> cancels request, don't see other events (Other than the complete event.
2)  Same for sync OnWillRedirect successful completion.
3+4)  Async variants of 1) and 2)

Think we already have good coverage of all the cancellation cases, with the
extra EXPECTS you added.

[carlosk, 2016/12/20 19:23:31]

Done.

   }
   void DidStartRequest(ResourceLoader* loader) override {
@@ -549,6 +556,8 @@ class ResourceLoaderTest : public testing::Test,
   int did_receive_response_ = 0;
   int did_finish_loading_ = 0;
 
+  int handle_external_protocol_ = 0;
+
   net::URLRequestJobFactoryImpl job_factory_;
   TestNetworkQualityEstimator network_quality_estimator_;
   net::TestURLRequestContext test_url_request_context_;
@@ -749,6 +758,7 @@ TEST_F(ResourceLoaderTest, SyncResourceHandler) {
   EXPECT_EQ(1, did_received_redirect_);
   EXPECT_EQ(1, did_receive_response_);
   EXPECT_EQ(1, did_finish_loading_);
+  EXPECT_EQ(2, handle_external_protocol_);
   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());
@@ -765,6 +775,7 @@ TEST_F(ResourceLoaderTest, SyncResourceHandlerAsyncReads) {
   EXPECT_EQ(0, did_received_redirect_);
   EXPECT_EQ(1, did_receive_response_);
   EXPECT_EQ(1, did_finish_loading_);
+  EXPECT_EQ(1, handle_external_protocol_);
   EXPECT_EQ(0, 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());
@@ -791,11 +802,13 @@ TEST_F(ResourceLoaderTest, AsyncResourceHandler) {
   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());
+  EXPECT_EQ(0, handle_external_protocol_);
 
   // Resume and run until OnRequestRedirected.
   raw_ptr_resource_handler_->Resume();
   raw_ptr_resource_handler_->WaitUntilDeferred();
   EXPECT_EQ(1, raw_ptr_resource_handler_->on_request_redirected_called());
+  EXPECT_EQ(1, handle_external_protocol_);
 
   // Spinning the message loop should not advance the state further.
   base::RunLoop().RunUntilIdle();
@@ -804,11 +817,13 @@ TEST_F(ResourceLoaderTest, AsyncResourceHandler) {
   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());
+  EXPECT_EQ(1, handle_external_protocol_);
 
   // Resume and run until OnResponseStarted.
   raw_ptr_resource_handler_->Resume();
   raw_ptr_resource_handler_->WaitUntilDeferred();
   EXPECT_EQ(1, raw_ptr_resource_handler_->on_response_started_called());
+  EXPECT_EQ(2, handle_external_protocol_);
 
   // Spinning the message loop should not advance the state further.
   base::RunLoop().RunUntilIdle();
@@ -830,7 +845,7 @@ TEST_F(ResourceLoaderTest, AsyncResourceHandler) {
   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.
+  // Resume and run until the final 0-byte read, signaling EOF.
   raw_ptr_resource_handler_->Resume();
   raw_ptr_resource_handler_->WaitUntilDeferred();
   EXPECT_EQ(1, raw_ptr_resource_handler_->on_read_eof());
@@ -860,6 +875,7 @@ TEST_F(ResourceLoaderTest, AsyncResourceHandler) {
   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());
+  EXPECT_EQ(2, handle_external_protocol_);
 }
 
 // Same as above, except reads complete asynchronously and there's no redirect.
@@ -883,11 +899,13 @@ TEST_F(ResourceLoaderTest, AsyncResourceHandlerAsyncReads) {
   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());
+  EXPECT_EQ(0, handle_external_protocol_);
 
   // Resume and run until OnResponseStarted.
   raw_ptr_resource_handler_->Resume();
   raw_ptr_resource_handler_->WaitUntilDeferred();
   EXPECT_EQ(1, raw_ptr_resource_handler_->on_response_started_called());
+  EXPECT_EQ(1, handle_external_protocol_);
 
   // Spinning the message loop should not advance the state further.
   base::RunLoop().RunUntilIdle();
@@ -939,6 +957,7 @@ TEST_F(ResourceLoaderTest, AsyncResourceHandlerAsyncReads) {
   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());
+  EXPECT_EQ(1, handle_external_protocol_);
 }
 
 TEST_F(ResourceLoaderTest, SyncCancelOnWillStart) {
@@ -949,6 +968,7 @@ TEST_F(ResourceLoaderTest, SyncCancelOnWillStart) {
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(0, did_start_request_);
   EXPECT_EQ(1, did_finish_loading_);
+  EXPECT_EQ(0, handle_external_protocol_);
   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());
@@ -968,6 +988,7 @@ TEST_F(ResourceLoaderTest, SyncCancelOnRequestRedirected) {
   EXPECT_EQ(1, did_received_redirect_);
   EXPECT_EQ(0, did_receive_response_);
   EXPECT_EQ(1, did_finish_loading_);
+  EXPECT_EQ(1, handle_external_protocol_);
   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());
@@ -1090,6 +1111,7 @@ TEST_F(ResourceLoaderTest, AsyncCancelOnWillStart) {
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(0, did_start_request_);
   EXPECT_EQ(1, did_finish_loading_);
+  EXPECT_EQ(0, handle_external_protocol_);
   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());
@@ -1109,6 +1131,7 @@ TEST_F(ResourceLoaderTest, AsyncCancelOnRequestRedirected) {
   EXPECT_EQ(1, did_received_redirect_);
   EXPECT_EQ(0, did_receive_response_);
   EXPECT_EQ(1, did_finish_loading_);
+  EXPECT_EQ(1, handle_external_protocol_);
   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());
@@ -1215,6 +1238,7 @@ TEST_F(ResourceLoaderTest, RequestFailsOnStart) {
   EXPECT_EQ(0, did_received_redirect_);
   EXPECT_EQ(0, did_receive_response_);
   EXPECT_EQ(1, did_finish_loading_);
+  EXPECT_EQ(1, handle_external_protocol_);
   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());
@@ -1274,6 +1298,7 @@ TEST_F(ResourceLoaderTest, OutOfBandCancelDuringStart) {
   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());
+  EXPECT_EQ(1, handle_external_protocol_);
 
   raw_ptr_resource_handler_->CancelWithError(net::ERR_FAILED);
   raw_ptr_resource_handler_->WaitUntilResponseComplete();
@@ -1281,6 +1306,7 @@ TEST_F(ResourceLoaderTest, OutOfBandCancelDuringStart) {
   EXPECT_EQ(0, did_received_redirect_);
   EXPECT_EQ(0, did_receive_response_);
   EXPECT_EQ(1, did_finish_loading_);
+  EXPECT_EQ(1, handle_external_protocol_);
   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());
@@ -1301,12 +1327,14 @@ TEST_F(ResourceLoaderTest, OutOfBandCancelDuringRead) {
   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());
+  EXPECT_EQ(1, handle_external_protocol_);
 
   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, handle_external_protocol_);
   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());