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

chrome/browser/chrome_navigation_browsertest.cc

 // Copyright 2016 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 "base/command_line.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
@@ skipping 171 matching lines @@
       browser()->tab_strip_model()->GetWebContentsAt(1);
   ASSERT_NE(new_web_contents, web_contents);
   WaitForLoadStop(new_web_contents);
 
   GURL view_frame_source_url(content::kViewSourceScheme + std::string(":") +
                              iframe_target_url.spec());
   EXPECT_EQ(url_formatter::FormatUrl(view_frame_source_url),
             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() {}
   ~ChromeNavigationPortMappedBrowserTest() override {}
 
   void SetUpCommandLine(base::CommandLine* command_line) override {
     ASSERT_TRUE(embedded_test_server()->Start());
 
     // Use the command line parameter for the host resolver, so URLs without
     // explicit port numbers can be mapped under the hood to the port number
@@ skipping 48 matching lines @@
       browser()->tab_strip_model()->GetWebContentsAt(
           browser()->tab_strip_model()->count() - 1);
   WaitForLoadStop(new_web_contents);
 
   // If the test is unsuccessful, the return value from GetLastCommittedURL
   // will be the virtual URL for the created NavigationEntry.
   // Note: Before the bug was fixed, the URL was the new_tab_url with a scheme
   // prepended and one less ":" character after the host.
   EXPECT_EQ(GURL(), new_web_contents->GetLastCommittedURL());
 }