Don't swap processes for chrome:// subframes.

content/browser/frame_host/render_frame_host_manager_browsertest.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <memory>
 #include <set>
 
@@ skipping 3013 matching lines @@
   // With OOPIFs, this navigation used a cross-process transfer.  Ensure that
   // the iframe's old RFH still has correct origin, even though it's pending
   // deletion.
   if (AreAllSitesIsolatedForTesting()) {
     EXPECT_FALSE(child_rfh_b->is_active());
     EXPECT_NE(child_rfh_b, child->current_frame_host());
     EXPECT_EQ(url::Origin(url_b), child_rfh_b->GetLastCommittedOrigin());
   }
 }
 
+// Verify that with Site Isolation enabled, chrome:// pages with subframes
+// to other chrome:// URLs all stay in the same process.
+IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest,
+                       ChromeSchemeSubframesStayInProcessWithParent) {
+  // Enable Site Isolation so subframes with different chrome:// URLs will be
+  // treated as cross-site.
+  IsolateAllSitesForTesting(base::CommandLine::ForCurrentProcess());
+  StartEmbeddedServer();
+
+  GURL chrome_top_url = GURL(std::string(kChromeUIScheme) + "://" +
+                             std::string(kChromeUIBlobInternalsHost));
+  GURL chrome_child_url = GURL(std::string(kChromeUIScheme) + "://" +
+                               std::string(kChromeUIHistogramHost));
+  GURL regular_web_url(embedded_test_server()->GetURL("/title1.html"));
+
+  NavigationControllerImpl& controller = static_cast<NavigationControllerImpl&>(
+      shell()->web_contents()->GetController());
+  FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents())
+                            ->GetFrameTree()
+                            ->root();
+
+  // Navigate the main frame to the top chrome:// URL.
+  NavigateToURL(shell(), chrome_top_url);
+
+  // Inject a frame in the page and navigate it to a chrome:// URL as well.
+  {
+    std::string script = base::StringPrintf(
+        "var frame = document.createElement('iframe');\n"
+        "frame.src = '%s';\n"
+        "document.body.appendChild(frame);\n",
+        chrome_child_url.spec().c_str());

[Charlie Reis, 2017/01/20 23:32:11]

How did you get around the CSP problem you mentioned?

[nasko, 2017/01/20 23:51:58]

Found pages with less strict CSP :(.

+
+    TestNavigationObserver navigation_observer(shell()->web_contents());
+    EXPECT_TRUE(ExecuteScript(shell(), script));
+    navigation_observer.Wait();
+    EXPECT_EQ(1U, root->child_count());
+
+    // Ensure the subframe navigated to the expected URL and that it is in the
+    // same SiteInstance as the parent frame.
+    NavigationEntryImpl* entry = controller.GetLastCommittedEntry();
+    ASSERT_EQ(1U, entry->root_node()->children.size());
+    EXPECT_EQ(chrome_child_url,
+              entry->root_node()->children[0]->frame_entry->url());
+    EXPECT_EQ(root->current_frame_host()->GetSiteInstance(),
+              root->child_at(0)->current_frame_host()->GetSiteInstance());
+  }
+
+  // Ensure that non-chrome:// pages get a different SiteInstance and process.
+  {
+    std::string script = base::StringPrintf(
+        "var frame = document.createElement('iframe');\n"
+        "frame.src = '%s';\n"
+        "document.body.appendChild(frame);\n",
+        regular_web_url.spec().c_str());
+
+    TestNavigationObserver navigation_observer(shell()->web_contents());
+    EXPECT_TRUE(ExecuteScript(shell(), script));
+    navigation_observer.Wait();
+    EXPECT_EQ(2U, root->child_count());
+
+    // Ensure the subframe navigated to the expected URL and that it is in a
+    // different SiteInstance from the parent frame.

[Charlie Reis, 2017/01/20 23:32:11]

WAT??!  I'm shocked this is allowed.  (And I'm glad you tested it!)  We should
not be letting a WebUI page load a web iframe, should we?

I hope this crashes after sammc's https://codereview.chromium.org/2566583002/
lands.  We should probably upgrade his DCHECK_EQ in
RenderFrameHostImpl::AllowBindings to a CHECK_EQ (separate from his CL to avoid
any risk of getting it reverted).

[nasko, 2017/01/20 23:51:58]

We shouldn't :).
I think it is also worthwhile to add an explicit check for this as a
NavigationThrottle and ensure we just outright block the navigation. Feel free
to file a bug and assign it to me to take care of.

[Charlie Reis, 2017/01/20 23:59:31]

Agreed.  I'll file it.

+    NavigationEntryImpl* entry = controller.GetLastCommittedEntry();
+    ASSERT_EQ(2U, entry->root_node()->children.size());
+    EXPECT_EQ(regular_web_url,
+              entry->root_node()->children[1]->frame_entry->url());
+    EXPECT_NE(root->current_frame_host()->GetSiteInstance(),
+              root->child_at(1)->current_frame_host()->GetSiteInstance());
+  }
+}
+
 }  // namespace content