Implement error page commit policy in PlzNavigate.

This CL implements a new policy for which process do error pages commit.
When an error page is a result of a blocked request, it should be
committed in the same process as the document requesting the navigation.
Otherwise the error page should be committed in the process that would
render the destination URL.

BUG=685074
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2738643002
Cr-Commit-Position: refs/heads/master@{#457532}

chrome/test/data/extensions/api_test/webrequest/test_blocking.js

@@ -33,40 +33,6 @@ function toCharCodes(str) {
 }
 
 runTests([
-  // Navigates to a page with subresources, with a blocking handler that
-  // cancels the page request. The page will not load, and we should not
-  // see the subresources.
-  function complexLoadCancelled() {
-    expect(
-      [  // events
-        { label: "onBeforeRequest",
-          event: "onBeforeRequest",
-          details: {
-            type: "main_frame",
-            url: getURL("complexLoad/b.html"),
-            frameUrl: getURL("complexLoad/b.html")
-          },
-          retval: {cancel: true}
-        },
-        // Cancelling is considered an error.
-        { label: "onErrorOccurred",
-          event: "onErrorOccurred",
-          details: {
-            url: getURL("complexLoad/b.html"),
-            fromCache: false,
-            error: "net::ERR_BLOCKED_BY_CLIENT"
-            // Request to chrome-extension:// url has no IP.
-          }
-        },
-      ],
-      [  // event order
-        ["onBeforeRequest", "onErrorOccurred"]
-      ],
-      {urls: ["<all_urls>"]},  // filter
-      ["blocking"]);
-    navigateAndWait(getURL("complexLoad/b.html"));
-  },
-
   // Navigates to a page with subresources, with a blocking handler that
   // cancels the page request. The page will not load, and we should not
   // see the subresources.
@@ -125,6 +91,43 @@ runTests([
     navigateAndWait(getURLHttpSimpleLoad());
   },
 
+  // Navigates to a page with subresources, with a blocking handler that
+  // cancels the page request. The page will not load, and we should not
+  // see the subresources.
+  // TODO(nasko): This test was reordered in order to accommodate an
+  // unrelated failure (https://crbug.com/700514). Once that failure is fixed,
+  // the reordering should be reverted.
+  function complexLoadCancelled() {
+    expect(
+      [  // events
+        { label: "onBeforeRequest",
+          event: "onBeforeRequest",
+          details: {
+            type: "main_frame",
+            url: getURL("complexLoad/b.html"),
+            frameUrl: getURL("complexLoad/b.html")
+          },
+          retval: {cancel: true}
+        },
+        // Cancelling is considered an error.
+        { label: "onErrorOccurred",
+          event: "onErrorOccurred",
+          details: {
+            url: getURL("complexLoad/b.html"),
+            fromCache: false,
+            error: "net::ERR_BLOCKED_BY_CLIENT"
+            // Request to chrome-extension:// url has no IP.
+          }
+        },
+      ],
+      [  // event order
+        ["onBeforeRequest", "onErrorOccurred"]
+      ],
+      {urls: ["<all_urls>"]},  // filter
+      ["blocking"]);
+    navigateAndWait(getURL("complexLoad/b.html"));
+  },
+
   // Navigates to a page and provides invalid header information. The request
   // should continue as if the headers were not changed.
   function simpleLoadIgnoreOnBeforeSendHeadersInvalidHeaders() {

content/browser/frame_host/navigation_handle_impl_browsertest.cc

@@ -8,6 +8,7 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/common/browser_side_navigation_policy.h"
+#include "content/public/common/content_switches.h"
 #include "content/public/common/request_context_type.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/content_browser_test.h"
@@ -17,6 +18,7 @@
 #include "content/shell/browser/shell.h"
 #include "content/test/content_browser_test_utils_internal.h"
 #include "net/dns/mock_host_resolver.h"
+#include "net/test/url_request/url_request_failed_job.h"
 #include "ui/base/page_transition_types.h"
 #include "url/url_constants.h"
 
@@ -1118,4 +1120,86 @@ IN_PROC_BROWSER_TEST_F(NavigationHandleImplBrowserTest, BlockedOnRedirect) {
   EXPECT_EQ(finished_navigation, logger.finished_navigation_urls());
 }
 
+// This class allows running tests with PlzNavigate enabled, regardless of
+// default test configuration.
+class PlzNavigateNavigationHandleImplBrowserTest : public ContentBrowserTest {
+ public:
+  PlzNavigateNavigationHandleImplBrowserTest() {}
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    command_line->AppendSwitch(switches::kEnableBrowserSideNavigation);
+  }
+};
+
+// Test to verify that error pages caused by NavigationThrottle blocking a
+// request from being made are properly committed in the original process
+// that requested the navigation.
+IN_PROC_BROWSER_TEST_F(PlzNavigateNavigationHandleImplBrowserTest,
+                       ErrorPageBlockedNavigation) {
+  host_resolver()->AddRule("*", "127.0.0.1");
+  SetupCrossSiteRedirector(embedded_test_server());
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  GURL start_url(embedded_test_server()->GetURL("foo.com", "/title1.html"));
+  GURL blocked_url(embedded_test_server()->GetURL("bar.com", "/title2.html"));
+
+  {
+    NavigationHandleObserver observer(shell()->web_contents(), start_url);
+    EXPECT_TRUE(NavigateToURL(shell(), start_url));
+    EXPECT_TRUE(observer.has_committed());
+    EXPECT_FALSE(observer.is_error());
+  }
+
+  scoped_refptr<SiteInstance> site_instance =
+      shell()->web_contents()->GetMainFrame()->GetSiteInstance();
+
+  TestNavigationThrottleInstaller installer(
+      shell()->web_contents(), NavigationThrottle::BLOCK_REQUEST,
+      NavigationThrottle::PROCEED, NavigationThrottle::PROCEED);
+
+  {
+    NavigationHandleObserver observer(shell()->web_contents(), blocked_url);
+    EXPECT_FALSE(NavigateToURL(shell(), blocked_url));
+    EXPECT_TRUE(observer.has_committed());
+    EXPECT_TRUE(observer.is_error());
+    EXPECT_EQ(site_instance,
+              shell()->web_contents()->GetMainFrame()->GetSiteInstance());
+  }
+}
+
+// Test to verify that error pages caused by network error or other
+// recoverable error are properly committed in the process for the
+// destination URL.
+IN_PROC_BROWSER_TEST_F(PlzNavigateNavigationHandleImplBrowserTest,
+                       ErrorPageNetworkError) {
+  host_resolver()->AddRule("*", "127.0.0.1");
+  SetupCrossSiteRedirector(embedded_test_server());
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  GURL start_url(embedded_test_server()->GetURL("foo.com", "/title1.html"));
+  GURL error_url(
+      net::URLRequestFailedJob::GetMockHttpUrl(net::ERR_CONNECTION_RESET));
+  EXPECT_NE(start_url.host(), error_url.host());
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+                          base::Bind(&net::URLRequestFailedJob::AddUrlHandler));
+
+  {
+    NavigationHandleObserver observer(shell()->web_contents(), start_url);
+    EXPECT_TRUE(NavigateToURL(shell(), start_url));
+    EXPECT_TRUE(observer.has_committed());
+    EXPECT_FALSE(observer.is_error());
+  }
+
+  scoped_refptr<SiteInstance> site_instance =
+      shell()->web_contents()->GetMainFrame()->GetSiteInstance();
+  {
+    NavigationHandleObserver observer(shell()->web_contents(), error_url);
+    EXPECT_FALSE(NavigateToURL(shell(), error_url));
+    EXPECT_TRUE(observer.has_committed());
+    EXPECT_TRUE(observer.is_error());
+    EXPECT_NE(site_instance,
+              shell()->web_contents()->GetMainFrame()->GetSiteInstance());
+  }
+}
+
 }  // namespace content

content/browser/frame_host/navigation_request.cc

@@ -583,9 +583,23 @@ void NavigationRequest::OnRequestFailed(bool has_stale_copy_in_cache,
     return;
   }
 
-  // Select an appropriate renderer to show the error page.
-  RenderFrameHostImpl* render_frame_host =
+  // There are two types of error pages that need to be handled differently.
+  // * Error pages resulting from blocking the request, because the original
+  //   document wasn't even allowed to make the request. In such case,
+  //   the error pages should be committed in the process of the original
+  //   document, to avoid creating a process for the destination.
+  // * Error pages resulting from either network outage (no network, DNS
+  //   error, etc) or similar cases, where the user can reasonably expect that
+  //   a reload at a later point in time can be successful. Such error pages
+  //   do belong to the process that will host the destination URL, as a
+  //   reload will end up committing in that process anyway.
+  RenderFrameHostImpl* render_frame_host = nullptr;
+  if (net_error == net::ERR_BLOCKED_BY_CLIENT) {
+    render_frame_host = frame_tree_node_->current_frame_host();
+  } else {
+    render_frame_host =
         frame_tree_node_->render_manager()->GetFrameHostForNavigation(*this);
+  }
 
   // Don't ask the renderer to commit an URL if the browser will kill it when
   // it does.