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..f48e44ee4d610624609e0d7e12a90746ce396cea 100644
--- a/chrome/browser/chrome_navigation_browsertest.cc
+++ b/chrome/browser/chrome_navigation_browsertest.cc
@@ -189,6 +189,91 @@ 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

[Charlie Reis, 2017/04/14 22:28:33]

nit: different process, SiteInstance, and BrowsingInstance from
(Line 239 explicitly checks the BrowsingInstance.)

[Łukasz Anforowicz, 2017/04/18 16:50:22]

Done.

+  // old contents.
+  EXPECT_NE(main_contents->GetMainFrame()->GetProcess(),
+            new_contents->GetMainFrame()->GetProcess());
+  EXPECT_NE(main_contents->GetMainFrame()->GetSiteInstance(),
+            new_contents->GetMainFrame()->GetSiteInstance());
+  EXPECT_FALSE(main_contents->GetSiteInstance()->IsRelatedSiteInstance(
+      new_contents->GetSiteInstance()));
+
+  // Verify that |new_contents| truly is in a brand new browsing instance.

[Charlie Reis, 2017/04/14 22:28:33]

Let's say: Verify that the new BrowsingInstance can't see windows from the old
one.

+  {
+    // Double-check that main_contents has expected window.name set.
+    // (this is a sanity check of test setup;  this is not a product test).

[Charlie Reis, 2017/04/14 22:28:33]

nit: Capitalize first "this", remove extra space after semicolon.

[Łukasz Anforowicz, 2017/04/18 16:50:22]

Done.

+    std::string name_of_main_contents_window;
+    ASSERT_TRUE(ExecuteScriptAndExtractString(
+        main_contents, "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, "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,
+        "w = window.open('about:blank', 'main_contents');"

[Charlie Reis, 2017/04/14 22:28:33]

This looks wrong to me-- my manual testing shows that passing about:blank here
will force the window to navigate to about:blank even if it's in the same
BrowsingInstance, so I'm guessing this part of the test will never fail.  Did
you mean to pass the empty string instead?


Ah, from your earlier conversation, maybe this was so that you got a commit
event in the popup?  I don't think you'll get one with the empty string, but I
do think we need to use the empty string here if we can.

[Łukasz Anforowicz, 2017/04/18 16:50:22]

Argh... you're right - I should be using an empty string.

[alexmos, 2017/04/18 17:26:27]

I'm a bit confused here. :)  Isn't the point of this to check whether or not the
a WebContents would be created?  If "main_contents" resolves to an existing
window (as would be the case for the same BrowsingInstance), we will time out on
the GetWebContents() call.  If it doesn't, it seems we'll get a new window
whether we use an empty URL or "about:blank".

That said, your new approach without the observer is even better. :)

[Charlie Reis, 2017/04/19 20:11:10]

Right-- I'm happy with the new approach.  Alex is probably right that the
GetWebContents() call would hang, but the EXPECT_EQ(url::kAboutBlankURL...) line
below stood out to me as something that would never fail.  Glad you've updated
it.

+        "window.domAutomationController.send(w.location.href);",
+        &location_of_opened_window));
+    content::WebContents* found_contents =
+        window_open_observer.GetWebContents();
+    EXPECT_TRUE(WaitForLoadStop(found_contents));
+    EXPECT_TRUE(found_contents->GetLastCommittedURL().IsAboutBlank());
+    EXPECT_EQ(url::kAboutBlankURL, location_of_opened_window);
+  }
+}
+
 class ChromeNavigationPortMappedBrowserTest : public InProcessBrowserTest {
  public:
   ChromeNavigationPortMappedBrowserTest() {}