New WebContents created via ctrl-click should be in a new process.

chrome/browser/chrome_navigation_browsertest.cc

Index: chrome/browser/chrome_navigation_browsertest.cc
diff --git a/chrome/browser/chrome_navigation_browsertest.cc b/chrome/browser/chrome_navigation_browsertest.cc
index 0d243913e6a425fd033568733e1b86738e2d631c..b32384798fb215f2b8e0dfef0d18939072939015 100644
--- a/chrome/browser/chrome_navigation_browsertest.cc
+++ b/chrome/browser/chrome_navigation_browsertest.cc
@@ -189,6 +189,93 @@ IN_PROC_BROWSER_TEST_F(ChromeNavigationBrowserTest, TestViewFrameSource) {
             new_web_contents->GetTitle());
 }
 
+// Verify that ctrl-click results 1) open up in a new renderer process
+// (https://crbug.com/23815) and 2) are in a new browsing instance (e.g.
+// cannot find the opener's window by name - https://crbug.com/658386).
+IN_PROC_BROWSER_TEST_F(ChromeNavigationBrowserTest,
+                       CtrlClickShouldEndUpInNewProcess) {
+  // Navigate to the test page.
+  GURL main_url(embedded_test_server()->GetURL(
+      "/frame_tree/anchor_to_same_site_location.html"));
+  ui_test_utils::NavigateToURL(browser(), main_url);
+
+  // Verify that there is only 1 active tab (with the right contents committed).
+  EXPECT_EQ(0, browser()->tab_strip_model()->active_index());
+  content::WebContents* main_contents =
+      browser()->tab_strip_model()->GetWebContentsAt(0);
+  EXPECT_EQ(main_url, main_contents->GetLastCommittedURL());
+
+  // Ctrl-click the anchor/link in the page.
+  content::WebContents* new_contents = nullptr;
+  {
+    content::WebContentsAddedObserver new_tab_observer;
+#if defined(OS_MACOSX)
+    std::string new_tab_click_script =
+        "simulateClick(\"test-anchor-no-target\", { metaKey: true });";
+#else
+    std::string new_tab_click_script =
+        "simulateClick(\"test-anchor-no-target\", { ctrlKey: true });";
+#endif
+    EXPECT_TRUE(ExecuteScript(main_contents, new_tab_click_script));
+
+    // Wait for a new tab to appear (the whole point of this test).
+    new_contents = new_tab_observer.GetWebContents();
+  }
+
+  // Verify that the new tab has the right contents and is in the right, new
+  // place in the tab strip.
+  EXPECT_TRUE(WaitForLoadStop(new_contents));
+  EXPECT_EQ(2, browser()->tab_strip_model()->count());
+  EXPECT_EQ(new_contents, browser()->tab_strip_model()->GetWebContentsAt(1));
+  GURL expected_url(embedded_test_server()->GetURL("/title1.html"));
+  EXPECT_EQ(expected_url, new_contents->GetLastCommittedURL());
+
+  // Verify that the new tab is in a different process and SiteInstance from the
+  // old contents.
+  EXPECT_NE(main_contents->GetMainFrame()->GetProcess(),
+            new_contents->GetMainFrame()->GetProcess());
+  EXPECT_NE(main_contents->GetMainFrame()->GetSiteInstance(),
+            new_contents->GetMainFrame()->GetSiteInstance());
+
+  // Verify that |new_contents| truly is in a brand new browsing instance.

[alexmos, 2017/04/11 20:42:39]

Might be nice to also check that
main_instance->IsRelatedSiteInstance(new_instance) returns false

[Łukasz Anforowicz, 2017/04/11 23:10:37]

Done.

+  {
+    // Double-check that main_contents has expected window.name set.
+    // (this is a sanity check of test setup;  this is not a product test).
+    std::string name_of_main_contents_window;
+    ASSERT_TRUE(ExecuteScriptAndExtractString(
+        main_contents->GetMainFrame(),

[alexmos, 2017/04/11 20:42:39]

optional: this will also work if you just pass in main_contents (similarly
below)

[Łukasz Anforowicz, 2017/04/11 23:10:37]

Done.

+        "window.domAutomationController.send(window.name)",
+        &name_of_main_contents_window));
+    EXPECT_EQ("main_contents", name_of_main_contents_window);
+
+    // Verify that the new contents doesn't have a window.opener set.
+    bool window_opener_cast_to_bool;
+    ASSERT_TRUE(ExecuteScriptAndExtractBool(
+        new_contents->GetMainFrame(),
+        "window.domAutomationController.send(!!window.opener)",
+        &window_opener_cast_to_bool));
+    EXPECT_FALSE(window_opener_cast_to_bool);
+
+    // Verify that the new contents cannot find the old contents via
+    // window.open.
+    // (i.e. window.open should open a new window, rather than returning a
+    // reference to main_contents / old window).
+    content::WebContentsAddedObserver window_open_observer;
+    std::string location_of_opened_window;
+    ASSERT_TRUE(ExecuteScriptAndExtractString(
+        new_contents->GetMainFrame(),
+        "w = window.open('', 'main_contents');"
+        "window.domAutomationController.send(w.location.href);",
+        &location_of_opened_window));
+    content::WebContents* found_contents =
+        window_open_observer.GetWebContents();
+    // Expecting "false" -> expecting to be at a non-PAGE_TYPE_NORMAL page.

[alexmos, 2017/04/11 20:42:39]

I'm curious why this is the case?  I.e., why wouldn't it just be a normal
navigation to "about:blank"?

[Łukasz Anforowicz, 2017/04/11 23:10:37]

IsLastCommittedEntryOfPageType returns false, because
web_contents->GetController().GetLastCommittedEntry() returns nullptr, because
|last_committed_entry_index_ == -1|.

Does that make sense?  I am just reporting what I am seeing, I don't really know
if going to about:blank should count as a committed entry or not...

[alexmos, 2017/04/12 22:08:29]

Strange.  I'd expect the new popup to commit "about:blank" here.  Does it work
if you do window.open("/title1.html",...) rather than an empty URL?  It might be
that we're calling WaitForLoadStop after the new WC is created but before the
navigation we want to wait for is started, so it returns immediately?

(If the two windows *were* in the same BrowsingInstance, then
window.open('','main_contents') wouldn't commit anything, whereas
window.open('about:blank','main_contents') would commit about:blank in the
existing window.  But if window.open opens a new window, I'd expect both cases
to commit "about:blank".)

[Łukasz Anforowicz, 2017/04/12 22:40:04]

Good call:
- Previously found_contents->IsLoading() was false.
- Things work as expected if I pass an actual URL (i.e. "about:blank") to
window.open call.

I've wondered about removing WaitForLoadStop and URL comparisons below, but I
think they are somewhat desirable here (to make sure the opened window is a new
window, not the old window).

+    EXPECT_FALSE(WaitForLoadStop(found_contents));
+    EXPECT_EQ(GURL(), found_contents->GetLastCommittedURL());
+    EXPECT_EQ("about:blank", location_of_opened_window);
+  }
+}
+
 class ChromeNavigationPortMappedBrowserTest : public InProcessBrowserTest {
  public:
   ChromeNavigationPortMappedBrowserTest() {}