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..cda1a544b5be20f5026f72d247ddb45010e3a0cf 100644
--- a/content/browser/loader/resource_loader_unittest.cc
+++ b/content/browser/loader/resource_loader_unittest.cc
@@ -7,8 +7,10 @@
 #include <stddef.h>
 #include <stdint.h>
 
+#include <deque>
 #include <memory>
 #include <utility>
+#include <vector>
 
 #include "base/files/file.h"
 #include "base/files/file_util.h"
@@ -501,7 +503,18 @@ class ResourceLoaderTest : public testing::Test,
   bool HandleExternalProtocol(ResourceLoader* loader,
                               const GURL& url) override {
     EXPECT_EQ(loader, loader_.get());
-    return false;
+    ++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());
+
+    bool return_value = handle_external_protocol_results_.front();
+    if (handle_external_protocol_results_.size() > 1)
+      handle_external_protocol_results_.pop_front();
+    return return_value;
   }
   void DidStartRequest(ResourceLoader* loader) override {
     EXPECT_EQ(loader, loader_.get());
@@ -548,6 +561,12 @@ class ResourceLoaderTest : public testing::Test,
   int did_received_redirect_ = 0;
   int did_receive_response_ = 0;
   int did_finish_loading_ = 0;
+  int handle_external_protocol_ = 0;
+
+  // Allows controlling the return values of sequential calls to
+  // HandleExternalProtocol. Values are removed by the measure they are used
+  // but the last one which is used for all following calls.
+  std::deque<bool> handle_external_protocol_results_{false};
 
   net::URLRequestJobFactoryImpl job_factory_;
   TestNetworkQualityEstimator network_quality_estimator_;
@@ -749,6 +768,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,12 +785,53 @@ 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());
   EXPECT_EQ(test_data(), raw_ptr_resource_handler_->body());
 }
 
+// Test the case where ResourceHandler defers nothing and the request is handled
+// as an external protocol on start.
+TEST_F(ResourceLoaderTest, SyncExternalProtocolHandlingOnStart) {
+  handle_external_protocol_results_ = {true};
+
+  loader_->StartRequest();
+  raw_ptr_resource_handler_->WaitUntilResponseComplete();
+  EXPECT_EQ(0, did_start_request_);
+  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(1, raw_ptr_resource_handler_->on_response_completed_called());
+  EXPECT_EQ(net::ERR_ABORTED,
+            raw_ptr_resource_handler_->final_status().error());
+  EXPECT_TRUE(raw_ptr_resource_handler_->body().empty());
+}
+
+// Test the case where ResourceHandler defers nothing and the request is handled
+// as an external protocol on redirect.
+TEST_F(ResourceLoaderTest, SyncExternalProtocolHandlingOnRedirect) {
+  handle_external_protocol_results_ = {false, true};
+
+  loader_->StartRequest();
+  raw_ptr_resource_handler_->WaitUntilResponseComplete();
+  EXPECT_EQ(1, did_start_request_);
+  EXPECT_EQ(1, did_received_redirect_);
+  EXPECT_EQ(0, did_receive_response_);
+  EXPECT_EQ(1, did_finish_loading_);
+  EXPECT_EQ(2, handle_external_protocol_);
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_will_start_called());
+  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::ERR_ABORTED,
+            raw_ptr_resource_handler_->final_status().error());
+  EXPECT_TRUE(raw_ptr_resource_handler_->body().empty());
+}
+
 // Test the case the ResourceHandler defers everything.
 TEST_F(ResourceLoaderTest, AsyncResourceHandler) {
   raw_ptr_resource_handler_->set_defer_on_will_start(true);
@@ -791,11 +852,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 +867,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 +895,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 +925,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 +949,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 +1007,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 +1018,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 +1038,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 +1161,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 +1181,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());
@@ -1204,6 +1277,67 @@ TEST_F(ResourceLoaderTest, AsyncCancelOnAsyncReceivedEof) {
   EXPECT_EQ(test_data(), raw_ptr_resource_handler_->body());
 }
 
+// Tests the request being deferred and then being handled as an external
+// protocol, both on start.
+TEST_F(ResourceLoaderTest, AsyncExternalProtocolHandlingOnStart) {
+  handle_external_protocol_results_ = {true};
+  raw_ptr_resource_handler_->set_defer_on_will_start(true);
+
+  loader_->StartRequest();
+  raw_ptr_resource_handler_->WaitUntilDeferred();
+  EXPECT_EQ(0, 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_response_completed_called());
+
+  raw_ptr_resource_handler_->Resume();
+  raw_ptr_resource_handler_->WaitUntilResponseComplete();
+  EXPECT_EQ(0, did_start_request_);
+  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());
+  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 the request being deferred and then being handled as an external
+// protocol, both on redirect.
+TEST_F(ResourceLoaderTest, AsyncExternalProtocolHandlingOnRedirect) {
+  handle_external_protocol_results_ = {false, true};
+  raw_ptr_resource_handler_->set_defer_on_request_redirected(true);
+
+  loader_->StartRequest();
+  raw_ptr_resource_handler_->WaitUntilDeferred();
+  EXPECT_EQ(1, did_start_request_);
+  EXPECT_EQ(1, did_received_redirect_);

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

In the process I created the async tests I noticed a difference in call order
between the handling of request start and redirects. This might be WAI but I
wanted to bring it up here.

On request start:
  1 handler_->OnWillStart then delegate_->HandleExternalProtocol
  2 delegate_->DidStartRequest 

On redirect:
  1 delegate_->DidReceiveRedirect
  2 handler_->OnRequestRedirected then delegate_->HandleExternalProtocol

WDYT?

[jam, 2016/12/20 20:55:56]

I suspect the order didn't matter and it wasn't intended. Seems orthogonal from
this cl though?

[carlosk, 2016/12/20 22:21:56]

It is orthogonal; I was just curious. Thanks.

+  EXPECT_EQ(0, did_finish_loading_);
+  EXPECT_EQ(1, handle_external_protocol_);
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_will_start_called());
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_request_redirected_called());
+  EXPECT_EQ(0, raw_ptr_resource_handler_->on_response_completed_called());
+
+  raw_ptr_resource_handler_->Resume();
+  raw_ptr_resource_handler_->WaitUntilResponseComplete();
+  EXPECT_EQ(1, did_start_request_);
+  EXPECT_EQ(1, did_received_redirect_);
+  EXPECT_EQ(0, did_receive_response_);
+  EXPECT_EQ(1, did_finish_loading_);
+  EXPECT_EQ(2, handle_external_protocol_);
+  EXPECT_EQ(1, raw_ptr_resource_handler_->on_will_start_called());
+  EXPECT_EQ(1, 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, RequestFailsOnStart) {
   SetUpResourceLoaderForUrl(
       net::URLRequestFailedJob::GetMockHttpUrlWithFailurePhase(
@@ -1215,6 +1349,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 +1409,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 +1417,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 +1438,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());