PlzNavigate: make all ResourceDispatcherHostTests pass

content/browser/loader/resource_dispatcher_host_unittest.cc

Index: content/browser/loader/resource_dispatcher_host_unittest.cc
diff --git a/content/browser/loader/resource_dispatcher_host_unittest.cc b/content/browser/loader/resource_dispatcher_host_unittest.cc
index c3cbb35379087937d7481a931ec2ab5045e865f5..a96ae79529cb6f562a89273d17d43e7b76d57451 100644
--- a/content/browser/loader/resource_dispatcher_host_unittest.cc
+++ b/content/browser/loader/resource_dispatcher_host_unittest.cc
@@ -23,14 +23,17 @@
 #include "content/browser/browser_thread_impl.h"
 #include "content/browser/cert_store_impl.h"
 #include "content/browser/child_process_security_policy_impl.h"
+#include "content/browser/frame_host/navigation_request_info.h"
 #include "content/browser/loader/cross_site_resource_handler.h"
 #include "content/browser/loader/detachable_resource_handler.h"
+#include "content/browser/loader/navigation_url_loader.h"
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
 #include "content/browser/loader/resource_loader.h"
 #include "content/browser/loader/resource_message_filter.h"
 #include "content/browser/loader/resource_request_info_impl.h"
 #include "content/common/appcache_interfaces.h"
 #include "content/common/child_process_host_impl.h"
+#include "content/common/navigation_params.h"
 #include "content/common/resource_messages.h"
 #include "content/common/ssl_status_serialization.h"
 #include "content/common/view_messages.h"
@@ -51,7 +54,9 @@
 #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_navigation_url_loader_delegate.h"
 #include "net/base/elements_upload_data_stream.h"
+#include "net/base/load_flags.h"
 #include "net/base/net_errors.h"
 #include "net/base/request_priority.h"
 #include "net/base/test_data_directory.h"
@@ -866,6 +871,20 @@ class MockCertStore : public content::CertStore {
   int default_cert_id_;
 };
 
+void CheckRequestCompleteErrorCode(const IPC::Message& message,
+                                   int expected_error_code) {
+  // Verify the expected error code was received.
+  int request_id;
+  int error_code;
+
+  ASSERT_EQ(ResourceMsg_RequestComplete::ID, message.type());
+
+  base::PickleIterator iter(message);
+  ASSERT_TRUE(IPC::ReadParam(&message, &iter, &request_id));
+  ASSERT_TRUE(IPC::ReadParam(&message, &iter, &error_code));
+  ASSERT_EQ(expected_error_code, error_code);
+}
+
 class ResourceDispatcherHostTest : public testing::TestWithParam<TestConfig>,
                                    public IPC::Sender {
  public:
@@ -876,7 +895,8 @@ class ResourceDispatcherHostTest : public testing::TestWithParam<TestConfig>,
       : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP),
         use_test_ssl_certificate_(false),
         old_factory_(NULL),
-        send_data_received_acks_(false) {
+        send_data_received_acks_(false),
+        auto_advance_(false) {
     browser_context_.reset(new TestBrowserContext());
     BrowserContext::EnsureResourceContextInitialized(browser_context_.get());
     base::RunLoop().RunUntilIdle();
@@ -1106,6 +1126,52 @@ class ResourceDispatcherHostTest : public testing::TestWithParam<TestConfig>,
     host_.OnRenderFrameDeleted(global_routing_id);
   }
 
+  // Creates and drive a main resource request until completion. Then asserts

[mmenke, 2016/04/08 15:19:12]

nit:  "Create and drive" or "creates and drives".  Whichever you use should
agree in tense with the next sentence as well.

[clamy, 2016/04/08 16:03:17]

Done.

+  // that the expected_error_code has been emitted for the request.
+  void CompleteFailingMainResourceRequest(const GURL& url,
+                                          int expected_error_code) {
+    if (IsBrowserSideNavigationEnabled()) {
+      auto_advance_ = true;
+
+      // Make a navigation request.
+      TestNavigationURLLoaderDelegate delegate;
+      BeginNavigationParams begin_params(std::string(), net::LOAD_NORMAL, false,
+                                         false, REQUEST_CONTEXT_TYPE_LOCATION);

[mmenke, 2016/04/08 15:19:12]

I assume "REQUEST_CONTEXT_TYPE_LOCATION" is the general case?  None of the
RequestContextTypes are documented.  :(

[clamy, 2016/04/08 16:03:17]

I think so. It's the clicking on a link case.

+      CommonNavigationParams common_params;
+      common_params.url = url;
+      scoped_ptr<NavigationRequestInfo> request_info(new NavigationRequestInfo(
+          common_params, begin_params, url, url::Origin(url), true, false, -1,
+          scoped_refptr<ResourceRequestBody>()));
+      scoped_ptr<NavigationURLLoader> test_loader = NavigationURLLoader::Create(
+          browser_context_.get(), std::move(request_info), nullptr, &delegate);
+
+      // The navigation should fail with the error code of ERR_INVALID_RESPONSE.

[carlosk, 2016/04/05 15:28:46]

As the expected error code is an input parameter this comment seems incorrect.

[clamy, 2016/04/08 16:03:17]

Done. Indeed.

+      delegate.WaitForRequestFailed();
+      ASSERT_EQ(expected_error_code, delegate.net_error());
+      return;
+    }
+
+    MakeTestRequestWithResourceType(filter_.get(), 0, 1, url,
+                                    RESOURCE_TYPE_MAIN_FRAME);
+
+    // Flush all pending requests.
+    while (net::URLRequestTestJob::ProcessOnePendingMessage()) {
+    }
+    base::MessageLoop::current()->RunUntilIdle();
+
+    // Sorts out all the messages we saw by request.
+    ResourceIPCAccumulator::ClassifiedMessages msgs;
+    accum_.GetClassifiedMessages(&msgs);
+
+    // We should have gotten one RequestComplete message.
+    ASSERT_EQ(1U, msgs.size());
+    ASSERT_EQ(1U, msgs[0].size());
+    EXPECT_EQ(ResourceMsg_RequestComplete::ID, msgs[0][0].type());
+
+    // The RequestComplete message should have had the expected error code.
+    CheckRequestCompleteErrorCode(msgs[0][0], expected_error_code);
+  }
+
   scoped_ptr<LoadInfoTestRequestInfo> loader_test_request_info_;
   scoped_ptr<base::RunLoop> wait_for_request_create_loop_;
 
@@ -1129,6 +1195,7 @@ class ResourceDispatcherHostTest : public testing::TestWithParam<TestConfig>,
   scoped_ptr<base::RunLoop> wait_for_request_complete_loop_;
   RenderViewHostTestEnabler render_view_host_test_enabler_;
   MockCertStore mock_cert_store_;
+  bool auto_advance_;
 };
 
 void ResourceDispatcherHostTest::MakeTestRequest(int render_view_id,
@@ -1238,20 +1305,6 @@ void ResourceDispatcherHostTest::CompleteStartRequest(
     URLRequestTestDelayedStartJob::CompleteStart(req);
 }
 
-void CheckRequestCompleteErrorCode(const IPC::Message& message,
-                                   int expected_error_code) {
-  // Verify the expected error code was received.
-  int request_id;
-  int error_code;
-
-  ASSERT_EQ(ResourceMsg_RequestComplete::ID, message.type());
-
-  base::PickleIterator iter(message);
-  ASSERT_TRUE(IPC::ReadParam(&message, &iter, &request_id));
-  ASSERT_TRUE(IPC::ReadParam(&message, &iter, &error_code));
-  ASSERT_EQ(expected_error_code, error_code);
-}
-
 testing::AssertionResult ExtractInlinedChunkData(
     const IPC::Message& message,
     std::string* leading_chunk_data) {
@@ -2467,32 +2520,23 @@ TEST_P(ResourceDispatcherHostTest, ForbiddenDownload) {
 
   HandleScheme("http");
 
-  // Only MAIN_FRAMEs can trigger a download.
-  MakeTestRequestWithResourceType(filter_.get(), 0, 1, GURL("http:bla"),
-                                  RESOURCE_TYPE_MAIN_FRAME);
-
-  // Flush all pending requests.
-  while (net::URLRequestTestJob::ProcessOnePendingMessage()) {}
-  base::MessageLoop::current()->RunUntilIdle();
-
-  // Sorts out all the messages we saw by request.
-  ResourceIPCAccumulator::ClassifiedMessages msgs;
-  accum_.GetClassifiedMessages(&msgs);
-
-  // We should have gotten one RequestComplete message.
-  ASSERT_EQ(1U, msgs.size());
-  ASSERT_EQ(1U, msgs[0].size());
-  EXPECT_EQ(ResourceMsg_RequestComplete::ID, msgs[0][0].type());
+  const int expected_error_code = net::ERR_INVALID_RESPONSE;
+  const GURL forbidden_download_url = GURL("http:bla");

[mmenke, 2016/04/08 15:19:12]

consts are pretty rare, unless you use kConstNamingStyle.  I'd just make thes
enon-const, to match the style of the std::strings above, but I'm happy, either
way.

[clamy, 2016/04/08 16:03:17]

Done.

 
-  // The RequestComplete message should have had the error code of
-  // ERR_INVALID_RESPONSE.
-  CheckRequestCompleteErrorCode(msgs[0][0], net::ERR_INVALID_RESPONSE);
+  CompleteFailingMainResourceRequest(forbidden_download_url,
+                                     expected_error_code);
 }
 
 // Test for http://crbug.com/76202 .  We don't want to destroy a
 // download request prematurely when processing a cancellation from
 // the renderer.
 TEST_P(ResourceDispatcherHostTest, IgnoreCancelForDownloads) {
+  // PlzNavigate: A request that ends up being a download is a main resource
+  // request. Hence, it has been initiated by the browser and is not associated
+  // with a renderer. Therefore, it cannot be canceled by a renderer IPC.
+  if (IsBrowserSideNavigationEnabled())

[carlosk, 2016/04/05 15:28:46]

nit: use the SUCCEED() << "Message" operator (which is still a nop BTW).

[clamy, 2016/04/08 16:03:17]

Done.

+    return;
+
   EXPECT_EQ(0, host_.pending_requests());
 
   int render_view_id = 0;
@@ -2548,29 +2592,63 @@ TEST_P(ResourceDispatcherHostTest, CancelRequestsForContext) {
   job_factory_->SetDelayedCompleteJobGeneration(true);
   HandleScheme("http");
 
-  MakeTestRequestWithResourceType(filter_.get(), render_view_id, request_id,
-                                  GURL("http://example.com/blah"),
-                                  RESOURCE_TYPE_MAIN_FRAME);
-  // Return some data so that the request is identified as a download
-  // and the proper resource handlers are created.
-  EXPECT_TRUE(net::URLRequestTestJob::ProcessOnePendingMessage());
+  const GURL download_url = GURL("http://example.com/blah");
 
-  // And now simulate a cancellation coming from the renderer.
-  ResourceHostMsg_CancelRequest msg(request_id);
-  host_.OnMessageReceived(msg, filter_.get());
+  if (IsBrowserSideNavigationEnabled()) {
+    // Create a NavigationRequest.
+    TestNavigationURLLoaderDelegate delegate;
+    BeginNavigationParams begin_params(std::string(), net::LOAD_NORMAL, false,
+                                       false, REQUEST_CONTEXT_TYPE_LOCATION);
+    CommonNavigationParams common_params;
+    common_params.url = download_url;
+    scoped_ptr<NavigationRequestInfo> request_info(new NavigationRequestInfo(
+        common_params, begin_params, download_url, url::Origin(download_url),
+        true, false, -1, scoped_refptr<ResourceRequestBody>()));
+    scoped_ptr<NavigationURLLoader> loader = NavigationURLLoader::Create(
+        browser_context_.get(), std::move(request_info), nullptr, &delegate);
+
+    // Wait until a response has been received and proceed with the response.
+    KickOffRequest();
+
+    // Return some data so that the request is identified as a download
+    // and the proper resource handlers are created.
+    EXPECT_TRUE(net::URLRequestTestJob::ProcessOnePendingMessage());
+    base::MessageLoop::current()->RunUntilIdle();
 
-  // Since the request had already started processing as a download,
-  // the cancellation above should have been ignored and the request
-  // should still be alive.
-  EXPECT_EQ(1, host_.pending_requests());
+    EXPECT_EQ(delegate.net_error(), net::ERR_ABORTED);

[mmenke, 2016/04/08 15:19:12]

Think this requires a comment.  I assume this is because the navigation should
be aborted once the request is identified as a download (but the request itself
should continue).

[clamy, 2016/04/08 16:03:17]

Done.

+    EXPECT_EQ(1, host_.pending_requests());
 
-  // Cancelling by other methods shouldn't work either.
-  host_.CancelRequestsForProcess(render_view_id);
-  EXPECT_EQ(1, host_.pending_requests());
+    // In PlzNavigate, the renderer cannot cancel the request directly.
+    // However, cancelling by context should work.
+    host_.CancelRequestsForContext(browser_context_->GetResourceContext());
+    EXPECT_EQ(0, host_.pending_requests());
 
-  // Cancelling by context should work.
-  host_.CancelRequestsForContext(filter_->resource_context());
-  EXPECT_EQ(0, host_.pending_requests());
+    base::MessageLoop::current()->RunUntilIdle();
+  } else {
+    MakeTestRequestWithResourceType(filter_.get(), render_view_id, request_id,
+                                    download_url, RESOURCE_TYPE_MAIN_FRAME);
+
+    // Return some data so that the request is identified as a download
+    // and the proper resource handlers are created.
+    EXPECT_TRUE(net::URLRequestTestJob::ProcessOnePendingMessage());
+
+    // And now simulate a cancellation coming from the renderer.
+    ResourceHostMsg_CancelRequest msg(request_id);
+    host_.OnMessageReceived(msg, filter_.get());
+
+    // Since the request had already started processing as a download,
+    // the cancellation above should have been ignored and the request
+    // should still be alive.
+    EXPECT_EQ(1, host_.pending_requests());
+
+    // Cancelling by other methods shouldn't work either.
+    host_.CancelRequestsForProcess(render_view_id);
+    EXPECT_EQ(1, host_.pending_requests());
+
+    // Cancelling by context should work.
+    host_.CancelRequestsForContext(filter_->resource_context());
+    EXPECT_EQ(0, host_.pending_requests());
+  }
 }
 
 TEST_P(ResourceDispatcherHostTest, CancelRequestsForContextDetached) {
@@ -3163,23 +3241,10 @@ TEST_P(ResourceDispatcherHostTest, UnknownURLScheme) {
 
   HandleScheme("http");
 
-  MakeTestRequestWithResourceType(filter_.get(), 0, 1, GURL("foo://bar"),
-                                  RESOURCE_TYPE_MAIN_FRAME);
+  const GURL invalid_sheme_url = GURL("foo://bar");
+  const int expected_error_code = net::ERR_UNKNOWN_URL_SCHEME;
 
-  // Flush all pending requests.
-  while (net::URLRequestTestJob::ProcessOnePendingMessage()) {}
-
-  // Sort all the messages we saw by request.
-  ResourceIPCAccumulator::ClassifiedMessages msgs;
-  accum_.GetClassifiedMessages(&msgs);
-
-  // We should have gotten one RequestComplete message.
-  ASSERT_EQ(1U, msgs[0].size());
-  EXPECT_EQ(ResourceMsg_RequestComplete::ID, msgs[0][0].type());
-
-  // The RequestComplete message should have the error code of
-  // ERR_UNKNOWN_URL_SCHEME.
-  CheckRequestCompleteErrorCode(msgs[0][0], net::ERR_UNKNOWN_URL_SCHEME);
+  CompleteFailingMainResourceRequest(invalid_sheme_url, expected_error_code);
 }
 
 TEST_P(ResourceDispatcherHostTest, DataReceivedACKs) {
@@ -3790,7 +3855,8 @@ net::URLRequestJob* TestURLRequestJobFactory::MaybeCreateJobWithProtocolHandler(
     } else if (scheme == "big-job") {
       return new URLRequestBigJob(request, network_delegate);
     } else {
-      return new net::URLRequestTestJob(request, network_delegate);
+      return new net::URLRequestTestJob(request, network_delegate,
+                                        test_fixture_->auto_advance_);
     }
   } else {
     if (delay_start_) {
@@ -3809,9 +3875,8 @@ net::URLRequestJob* TestURLRequestJobFactory::MaybeCreateJobWithProtocolHandler(
                                         test_fixture_->response_data_, false);
     } else {
       return new net::URLRequestTestJob(
-          request, network_delegate,
-          test_fixture_->response_headers_, test_fixture_->response_data_,
-          false);
+          request, network_delegate, test_fixture_->response_headers_,
+          test_fixture_->response_data_, test_fixture_->auto_advance_);
     }
   }
 }