When determining whether or not to swap processes on navigation, check the top frame's URL.

Otherwise having an inner frame that matches the app's extent could mean that the process
is not swapped, even if we're not in an app process.

Difference from r94843 is that the popup window is 200x200, so that it results in a separate
Browser instance even on Chrome OS.

BUG=89272
TEST=no
R=creis@chromium.org

Review URL: http://codereview.chromium.org/7608015

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@96197 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/extensions/app_process_apitest.cc

@@ -16,6 +16,18 @@
 #include "net/base/mock_host_resolver.h"
 
 class AppApiTest : public ExtensionApiTest {
+ protected:
+  // Gets the base URL for files for a specific test, making sure that it uses
+  // "localhost" as the hostname, since that is what the extent is declared
+  // as in the test apps manifests.
+  GURL GetTestBaseURL(std::string test_directory) {
+    GURL::Replacements replace_host;
+    std::string host_str("localhost");  // must stay in scope with replace_host
+    replace_host.SetHostStr(host_str);
+    GURL base_url = test_server()->GetURL(
+        "files/extensions/api_test/" + test_directory + "/");
+    return base_url.ReplaceComponents(replace_host);
+  }
 };
 
 // Simulates a page calling window.open on an URL, and waits for the navigation.
@@ -79,15 +91,7 @@ IN_PROC_BROWSER_TEST_F(AppApiTest, MAYBE_AppProcess) {
   ASSERT_TRUE(LoadExtension(test_data_dir_.AppendASCII("app_process")));
 
   // Open two tabs in the app, one outside it.
-  GURL base_url = test_server()->GetURL(
-      "files/extensions/api_test/app_process/");
-
-  // The app under test acts on URLs whose host is "localhost",
-  // so the URLs we navigate to must have host "localhost".
-  GURL::Replacements replace_host;
-  std::string host_str("localhost");  // must stay in scope with replace_host
-  replace_host.SetHostStr(host_str);
-  base_url = base_url.ReplaceComponents(replace_host);
+  GURL base_url = GetTestBaseURL("app_process");
 
   // Test both opening a URL in a new tab, and opening a tab and then navigating
   // it.  Either way, app tabs should be considered extension processes, but
@@ -176,15 +180,7 @@ IN_PROC_BROWSER_TEST_F(AppApiTest, MAYBE_AppProcessInstances) {
       test_data_dir_.AppendASCII("app_process_instances")));
 
   // Open two tabs in the app, one outside it.
-  GURL base_url = test_server()->GetURL(
-      "files/extensions/api_test/app_process_instances/");
-
-  // The app under test acts on URLs whose host is "localhost",
-  // so the URLs we navigate to must have host "localhost".
-  GURL::Replacements replace_host;
-  std::string host_str("localhost");  // must stay in scope with replace_host
-  replace_host.SetHostStr(host_str);
-  base_url = base_url.ReplaceComponents(replace_host);
+  GURL base_url = GetTestBaseURL("app_process_instances");
 
   // Test both opening a URL in a new tab, and opening a tab and then navigating
   // it.  Either way, app tabs should be considered extension processes, but
@@ -234,15 +230,7 @@ IN_PROC_BROWSER_TEST_F(AppApiTest, AppProcessRedirectBack) {
   ASSERT_TRUE(LoadExtension(test_data_dir_.AppendASCII("app_process")));
 
   // Open two tabs in the app.
-  GURL base_url = test_server()->GetURL(
-      "files/extensions/api_test/app_process/");
-
-  // The app under test acts on URLs whose host is "localhost",
-  // so the URLs we navigate to must have host "localhost".
-  GURL::Replacements replace_host;
-  std::string host_str("localhost");  // must stay in scope with replace_host
-  replace_host.SetHostStr(host_str);
-  base_url = base_url.ReplaceComponents(replace_host);
+  GURL base_url = GetTestBaseURL("app_process");
 
   browser()->NewTab();
   ui_test_utils::NavigateToURL(browser(), base_url.Resolve("path1/empty.html"));
@@ -273,12 +261,7 @@ IN_PROC_BROWSER_TEST_F(AppApiTest, ReloadIntoAppProcess) {
 
   // The app under test acts on URLs whose host is "localhost",
   // so the URLs we navigate to must have host "localhost".
-  GURL::Replacements replace_host;
-  std::string host_str("localhost");  // must stay in scope with replace_host
-  replace_host.SetHostStr(host_str);
-  GURL base_url = test_server()->GetURL(
-      "files/extensions/api_test/app_process/");
-  base_url = base_url.ReplaceComponents(replace_host);
+  GURL base_url = GetTestBaseURL("app_process");
 
   // Load an app URL before loading the app.
   ui_test_utils::NavigateToURL(browser(), base_url.Resolve("path1/empty.html"));
@@ -311,3 +294,47 @@ IN_PROC_BROWSER_TEST_F(AppApiTest, ReloadIntoAppProcess) {
   ui_test_utils::WaitForNavigation(&contents->controller());
   EXPECT_FALSE(contents->render_view_host()->process()->is_extension_process());
 }
+
+
+// Tests that if we have a non-app process (path3/container.html) that has an
+// iframe with  a URL in the app's extent (path1/iframe.html), then opening a
+// link from that iframe to a new window to a URL in the app's extent (path1/
+// empty.html) results in the new window being in an app process. See
+// http://crbug.com/89272 for more details.
+IN_PROC_BROWSER_TEST_F(AppApiTest, OpenAppFromIframe) {
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kDisablePopupBlocking);
+
+  host_resolver()->AddRule("*", "127.0.0.1");
+  ASSERT_TRUE(test_server()->Start());
+
+  GURL base_url = GetTestBaseURL("app_process");
+
+  // Load app and start URL (not in the app).
+  const Extension* app =
+      LoadExtension(test_data_dir_.AppendASCII("app_process"));
+  ASSERT_TRUE(app);
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(),
+      base_url.Resolve("path3/container.html"),
+      CURRENT_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION |
+          ui_test_utils::BROWSER_TEST_WAIT_FOR_BROWSER);
+  EXPECT_FALSE(browser()->GetTabContentsAt(0)->render_view_host()->process()->
+      is_extension_process());
+
+  // Wait for popup window to appear.
+  GURL app_url = base_url.Resolve("path1/empty.html");
+  Browser* last_active_browser = BrowserList::GetLastActive();
+  EXPECT_TRUE(last_active_browser);
+  ASSERT_NE(browser(), last_active_browser);
+  TabContents* newtab = last_active_browser->GetSelectedTabContents();
+  EXPECT_TRUE(newtab);
+  if (!newtab->controller().GetLastCommittedEntry() ||
+      newtab->controller().GetLastCommittedEntry()->url() != app_url)
+    ui_test_utils::WaitForNavigation(&newtab->controller());
+
+  // Popup window should be in the app's process.
+  EXPECT_TRUE(last_active_browser->GetTabContentsAt(0)->render_view_host()->
+      process()->is_extension_process());
+}

chrome/renderer/chrome_content_renderer_client.cc

@@ -671,13 +671,14 @@ bool ChromeContentRendererClient::CrossesExtensionExtents(WebFrame* frame,
                                                           const GURL& new_url) {
   const ExtensionSet* extensions = extension_dispatcher_->extensions();
   // If the URL is still empty, this is a window.open navigation. Check the
-  // opener's URL.
+  // opener's URL. In all cases we use the top frame's URL (as opposed to our
+  // frame's) since that's what determines the type of process.
   // TODO(abarth): This code is super sketchy! Are you sure looking at the
   // opener is correct here?  This appears to let me steal my opener's
   // privileges if I can make my URL be "empty."
-  GURL old_url(frame->document().url());
+  GURL old_url(frame->top()->document().url());
   if (old_url.is_empty() && frame->opener())
-    old_url = frame->opener()->document().url();
+    old_url = frame->top()->opener()->top()->document().url();
 
   // If this is a reload, check whether it has the wrong process type.  We
   // should send it to the browser if it's an extension URL (e.g., hosted app)

chrome/test/data/extensions/api_test/app_process/path1/iframe.html

+<script>
+  window.open(
+      '/files/extensions/api_test/app_process/path1/empty.html',
+      '',
+      // Small enough that a popup will be created even on Chrome OS
+      'width=200,height=200');
+</script>

chrome/test/data/extensions/api_test/app_process/path3/container.html

+<iframe src="/files/extensions/api_test/app_process/path1/iframe.html"></iframe>