Add validations for error page commits.

With error page isolation, navigations that result in a chrome error
page (as opposed to HTTP errors such as 404) are isolated in their own
process.

This CL adds some validations at commit time to ensure that invariants
we expect are enforced by the browser. It only includes some of the
possible checks, as more investigations are needed to understand the
behavior of HTTP layer errors.

Bug: 866549
Change-Id: I8389629eb68213681983b37d1cfc945832e35ea7
Reviewed-on: https://chromium-review.googlesource.com/1160877
Commit-Queue: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580670}

content/browser/bad_message.h

@@ -230,6 +230,8 @@ enum BadMessageReason {
   PERMISSION_SERVICE_BAD_PERMISSION_DESCRIPTOR = 202,
   RFH_BLOB_URL_TOKEN_FOR_NON_BLOB_URL = 203,
   RFPH_BLOB_URL_TOKEN_FOR_NON_BLOB_URL = 204,
+  RFH_ERROR_PROCESS_NON_ERROR_COMMIT = 205,
+  RFH_ERROR_PROCESS_NON_UNIQUE_ORIGIN_COMMIT = 206,
 
   // Please add new elements here. The naming convention is abbreviated class
   // name (e.g. RenderFrameHost becomes RFH) plus a unique description of the

content/browser/frame_host/render_frame_host_impl.cc

@@ -5273,25 +5273,48 @@ bool RenderFrameHostImpl::ValidateDidCommitParams(
   // Error pages may sometimes commit a URL in the wrong process, which requires
   // an exception for the CanCommitURL checks.  This is ok as long as the origin
   // is unique.
-  // TODO(creis): Kill the renderer if it claims an error page has a non-unique
-  // origin.
   bool is_permitted_error_page = false;
-  if (validated_params->origin.unique()) {
-    if (SiteIsolationPolicy::IsErrorPageIsolationEnabled(
-            frame_tree_node_->IsMainFrame())) {
-      // With error page isolation, any URL can commit in an error page process.
-      if (GetSiteInstance()->GetSiteURL() ==
-          GURL(content::kUnreachableWebDataURL)) {
-        is_permitted_error_page = true;
+  if (SiteIsolationPolicy::IsErrorPageIsolationEnabled(
+          frame_tree_node_->IsMainFrame())) {
+    if (site_instance_->GetSiteURL() == GURL(content::kUnreachableWebDataURL)) {
+      // Commits in the error page process must only be failures, otherwise
+      // successful navigations could commit documents from origins different
+      // than the chrome-error://chromewebdata/ one and violate expectations.
+      if (!validated_params->url_is_unreachable) {
+        DEBUG_ALIAS_FOR_ORIGIN(origin_debug_alias, validated_params->origin);
+        bad_message::ReceivedBadMessage(
+            process, bad_message::RFH_ERROR_PROCESS_NON_ERROR_COMMIT);
+        return false;
       }
-    } else {
-      // Without error page isolation, a blocked navigation is expected to
-      // commit in the old renderer process.  This may be true for subframe
-      // navigations even when error page isolation is enabled for main frames.
-      if (GetNavigationHandle() && GetNavigationHandle()->GetNetErrorCode() ==
-                                       net::ERR_BLOCKED_BY_CLIENT) {
-        is_permitted_error_page = true;
+
+      // Error pages must commit in a unique origin. Terminate the renderer
+      // process if this is violated.
+      if (!validated_params->origin.unique()) {
+        DEBUG_ALIAS_FOR_ORIGIN(origin_debug_alias, validated_params->origin);
+        bad_message::ReceivedBadMessage(
+            process, bad_message::RFH_ERROR_PROCESS_NON_UNIQUE_ORIGIN_COMMIT);
+        return false;
       }
+
+      // With error page isolation, any URL can commit in an error page process.
+      is_permitted_error_page = true;
+    }
+  } else {
+    // Without error page isolation, a blocked navigation is expected to
+    // commit in the old renderer process.  This may be true for subframe
+    // navigations even when error page isolation is enabled for main frames.
+    if (GetNavigationHandle() && GetNavigationHandle()->GetNetErrorCode() ==
+                                     net::ERR_BLOCKED_BY_CLIENT) {
+      // Since this is known to be an error page commit, verify it happened in
+      // a unique origin, terminating the renderer process otherwise.
+      if (!validated_params->origin.unique()) {
+        DEBUG_ALIAS_FOR_ORIGIN(origin_debug_alias, validated_params->origin);
+        bad_message::ReceivedBadMessage(
+            process, bad_message::RFH_ERROR_PROCESS_NON_UNIQUE_ORIGIN_COMMIT);
+        return false;
+      }
+
+      is_permitted_error_page = true;
     }
   }
 

content/browser/site_instance_impl.cc

@@ -249,13 +249,15 @@ bool SiteInstanceImpl::HasWrongProcessForURL(const GURL& url) {
   if (IsRendererDebugURL(url))
     return false;
 
-  // Any process can host an about:blank URL.  This check avoids a process
-  // transfer for browser-initiated navigations to about:blank in a dedicated
-  // process; without it, IsSuitableHost would consider this process unsuitable
-  // for about:blank when it compares origin locks.  Renderer-initiated
-  // navigations will handle about:blank navigations elsewhere and leave them
-  // in the source SiteInstance, along with about:srcdoc and data:.
-  if (url == url::kAboutBlankURL)
+  // Any process can host an about:blank URL, except the one used for error
+  // pages, which should not commit successful navigations.  This check avoids a
+  // process transfer for browser-initiated navigations to about:blank in a
+  // dedicated process; without it, IsSuitableHost would consider this process
+  // unsuitable for about:blank when it compares origin locks.
+  // Renderer-initiated navigations will handle about:blank navigations
+  // elsewhere and leave them in the source SiteInstance, along with
+  // about:srcdoc and data:.
+  if (url.IsAboutBlank() && site_ != GURL(kUnreachableWebDataURL))
     return false;
 
   // If the site URL is an extension (e.g., for hosted apps or WebUI) but the

third_party/WebKit/LayoutTests/TestExpectations

@@ -2203,7 +2203,7 @@ crbug.com/665577 virtual/threaded/fast/scroll-behavior/overflow-scroll-root-fram
 crbug.com/665577 [ Linux Win ] virtual/threaded/fast/scroll-behavior/smooth-scroll/mousewheel-scroll-interrupted.html [ Pass Failure Timeout ]
 crbug.com/665577 [ Mac ] virtual/threaded/fast/scroll-behavior/smooth-scroll/track-scroll.html [ Pass Failure ]
 
-crbug.com/599670 [ Win ] http/tests/devtools/resource-parameters-ipv6.js [ Pass Failure ]
+crbug.com/599670 [ Win ] http/tests/devtools/resource-parameters-ipv6.js [ Pass Failure Crash ]
 crbug.com/472330 fast/borders/border-image-outset-split-inline-vertical-lr.html [ Failure ]
 crbug.com/472330 fast/writing-mode/box-shadow-vertical-lr.html [ Failure ]
 crbug.com/472330 fast/writing-mode/box-shadow-vertical-rl.html [ Failure ]
@@ -2652,7 +2652,7 @@ crbug.com/604875 external/wpt/css/css-images/tiled-gradients.html [ Failure ]
 crbug.com/808834 [ Linux Win ] external/wpt/css/css-pseudo/first-letter-001.html [ Failure ]
 
 
-crbug.com/723741 virtual/threaded/http/tests/devtools/tracing/idle-callback.js [ Failure Pass Timeout ]
+crbug.com/723741 virtual/threaded/http/tests/devtools/tracing/idle-callback.js [ Failure Crash Pass Timeout ]
 
 
 # Untriaged failures after https://crrev.com/c/543695/.

tools/metrics/histograms/enums.xml

@@ -3366,6 +3366,8 @@ uploading your change for review.  These are checked by presubmit scripts.
   <int value="202" label="PERMISSION_SERVICE_BAD_PERMISSION_DESCRIPTOR"/>
   <int value="203" label="RFH_BLOB_URL_TOKEN_FOR_NON_BLOB_URL"/>
   <int value="204" label="RFPH_BLOB_URL_TOKEN_FOR_NON_BLOB_URL"/>
+  <int value="205" label="RFH_ERROR_PROCESS_NON_ERROR_COMMIT"/>
+  <int value="206" label="RFH_ERROR_PROCESS_NON_UNIQUE_ORIGIN_COMMIT"/>
 </enum>
 
 <enum name="BadMessageReasonExtensions">