Don't use process isolation for bookmark apps.

BUG=104636
TEST=No process swap on a link to/from a bookmark app.


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

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

chrome/browser/chrome_content_browser_client.cc

@@ -353,6 +353,11 @@ GURL ChromeContentBrowserClient::GetEffectiveURL(
   if (!extension)
     return url;
 
+  // Bookmark apps do not use the hosted app process model, and should be
+  // treated as normal URLs.
+  if (extension->from_bookmark())
+    return url;
+
   // If the URL is part of an extension's web extent, convert it to an
   // extension URL.
   return extension->GetResourceURL(url.path());
@@ -361,8 +366,8 @@ GURL ChromeContentBrowserClient::GetEffectiveURL(
 bool ChromeContentBrowserClient::ShouldUseProcessPerSite(
     content::BrowserContext* browser_context, const GURL& effective_url) {
   // Non-extension URLs should generally use process-per-site-instance.
-  // Because we expect to use the effective URL, hosted apps URLs should have
-  // an extension scheme by now.
+  // Because we expect to use the effective URL, URLs for hosted apps (apart
+  // from bookmark apps) should have an extension scheme by now.
   if (!effective_url.SchemeIs(chrome::kExtensionScheme))
     return false;
 

chrome/browser/extensions/app_process_apitest.cc

@@ -251,6 +251,76 @@ IN_PROC_BROWSER_TEST_F(AppApiTest, AppProcessInstances) {
   LOG(INFO) << "End of test.";
 }
 
+// Tests that bookmark apps do not use the app process model and are treated
+// like normal web pages instead.  http://crbug.com/104636.
+// TODO(creis): This test is disabled until we have a way to load a bookmark
+// app in browser_tests.  See http://crbug.com/104649.
+IN_PROC_BROWSER_TEST_F(AppApiTest, DISABLED_BookmarkAppGetsNormalProcess) {
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kDisablePopupBlocking);
+
+  extensions::ProcessMap* process_map =
+      browser()->profile()->GetExtensionService()->process_map();
+
+  host_resolver()->AddRule("*", "127.0.0.1");
+  ASSERT_TRUE(test_server()->Start());
+
+  // TODO(creis): We need a way to load an app in a test as a bookmark app.
+  // Until then, from_bookmark() will return false and this test will fail.
+  const Extension* extension =
+      LoadExtension(test_data_dir_.AppendASCII("app_process"));
+  ASSERT_TRUE(extension);
+  EXPECT_TRUE(extension->from_bookmark());
+  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, bookmark app tabs should be considered normal processes
+  // with no elevated privileges and no WebUI bindings.
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), base_url.Resolve("path1/empty.html"), NEW_FOREGROUND_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+  EXPECT_FALSE(process_map->Contains(
+      browser()->GetTabContentsAt(1)->render_view_host()->process()->GetID()));
+  EXPECT_FALSE(browser()->GetTabContentsAt(1)->web_ui());
+
+  ui_test_utils::WindowedNotificationObserver tab_added_observer(
+      content::NOTIFICATION_TAB_ADDED,
+      content::NotificationService::AllSources());
+  browser()->NewTab();
+  tab_added_observer.Wait();
+  ui_test_utils::NavigateToURL(browser(), base_url.Resolve("path2/empty.html"));
+  EXPECT_FALSE(process_map->Contains(
+      browser()->GetTabContentsAt(2)->render_view_host()->process()->GetID()));
+  EXPECT_FALSE(browser()->GetTabContentsAt(2)->web_ui());
+
+  // We should have opened 2 new bookmark app tabs. Including the original blank
+  // tab, we now have 3 tabs.  Because normal pages use the
+  // process-per-site-instance model, each should be in its own process.
+  ASSERT_EQ(3, browser()->tab_count());
+  RenderViewHost* host = browser()->GetTabContentsAt(1)->render_view_host();
+  EXPECT_NE(host->process(),
+            browser()->GetTabContentsAt(2)->render_view_host()->process());
+
+  // Now let's do the same using window.open. The same should happen.
+  ASSERT_EQ(1u, BrowserList::GetBrowserCount(browser()->profile()));
+  WindowOpenHelper(browser(), host,
+                   base_url.Resolve("path1/empty.html"), true);
+  WindowOpenHelper(browser(), host,
+                   base_url.Resolve("path2/empty.html"), true);
+
+  // Now let's have a tab navigate out of and back into the app's web
+  // extent. Neither navigation should switch processes.
+  const GURL& app_url(base_url.Resolve("path1/empty.html"));
+  const GURL& non_app_url(base_url.Resolve("path3/empty.html"));
+  RenderViewHost* host2 = browser()->GetTabContentsAt(2)->render_view_host();
+  NavigateTabHelper(browser()->GetTabContentsAt(2), non_app_url);
+  EXPECT_EQ(host2->process(),
+            browser()->GetTabContentsAt(2)->render_view_host()->process());
+  NavigateTabHelper(browser()->GetTabContentsAt(2), app_url);
+  EXPECT_EQ(host2->process(),
+            browser()->GetTabContentsAt(2)->render_view_host()->process());
+}
+
 // Tests that app process switching works properly in the following scenario:
 // 1. navigate to a page1 in the app
 // 2. page1 redirects to a page2 outside the app extent (ie, "/server-redirect")

chrome/browser/extensions/extension_service.cc

@@ -2196,7 +2196,7 @@ const Extension* ExtensionService::GetDisabledExtensionByWebExtent(
 bool ExtensionService::ExtensionBindingsAllowed(const GURL& url) {
   // Allow bindings for all packaged extensions.
   // Note that GetExtensionByURL may return an Extension for hosted apps
-  // if the URL came from GetEffectiveURL.
+  // (excluding bookmark apps) if the URL came from GetEffectiveURL.
   const Extension* extension = GetExtensionByURL(url);
   if (extension && extension->GetType() != Extension::TYPE_HOSTED_APP)
     return true;

chrome/browser/renderer_host/chrome_render_view_host_observer.cc

@@ -133,9 +133,9 @@ void ChromeRenderViewHostObserver::InitRenderViewForExtensions() {
 }
 
 const Extension* ChromeRenderViewHostObserver::GetExtension() {
-  // Note that due to ChromeContentBrowserClient::GetEffectiveURL(), even hosted
-  // apps will have a chrome-extension:// URL for their site, so we can ignore
-  // that wrinkle here.
+  // Note that due to ChromeContentBrowserClient::GetEffectiveURL(), hosted apps
+  // (excluding bookmark apps) will have a chrome-extension:// URL for their
+  // site, so we can ignore that wrinkle here.
   SiteInstance* site_instance = render_view_host()->site_instance();
   const GURL& site = site_instance->site();
 

chrome/renderer/chrome_content_renderer_client.cc

@@ -729,14 +729,26 @@ void ChromeContentRendererClient::SetExtensionDispatcher(
   extension_dispatcher_.reset(extension_dispatcher);
 }
 
+const Extension* ChromeContentRendererClient::GetNonBookmarkAppExtension(
+    const ExtensionSet* extensions, const GURL& url) {
+  // Exclude bookmark apps, which do not use the app process model.
+  const Extension* extension = extensions->GetByURL(url);
+  if (extension && extension->from_bookmark())
+    extension = NULL;
+  return extension;
+}
+
 bool ChromeContentRendererClient::CrossesExtensionExtents(
     WebFrame* frame,
     const GURL& new_url,
     bool is_initial_navigation) {
   const ExtensionSet* extensions = extension_dispatcher_->extensions();
-  bool is_extension_url = !!extensions->GetByURL(new_url);
   GURL old_url(frame->top()->document().url());
 
+  // Determine if the new URL is an extension (excluding bookmark apps).
+  const Extension* new_url_extension = GetNonBookmarkAppExtension(extensions,
+                                                                  new_url);
+
   // If old_url is still empty and this is an initial navigation, then this is
   // a window.open operation.  We should look at the opener URL.
   if (is_initial_navigation && old_url.is_empty() && frame->opener()) {
@@ -746,7 +758,7 @@ bool ChromeContentRendererClient::CrossesExtensionExtents(
     GURL opener_url = frame->opener()->document().url();
     bool opener_is_extension_url = !!extensions->GetByURL(opener_url);
     WebSecurityOrigin opener = frame->opener()->document().securityOrigin();
-    if (!is_extension_url &&
+    if (!new_url_extension &&
         !opener_is_extension_url &&
         extension_dispatcher_->is_extension_process() &&
         opener.canRequest(WebURL(new_url)))
@@ -758,15 +770,21 @@ bool ChromeContentRendererClient::CrossesExtensionExtents(
     old_url = frame->top()->opener()->top()->document().url();
   }
 
+  // Determine if the old URL is an extension (excluding bookmark apps).
+  const Extension* old_url_extension = GetNonBookmarkAppExtension(extensions,
+                                                                  old_url);
+
   // TODO(creis): Temporary workaround for crbug.com/59285: Only return true if
   // we would enter an extension app's extent from a non-app, or if we leave an
   // extension with no web extent.  We avoid swapping processes to exit a hosted
   // app for now, since we do not yet support postMessage calls from outside the
   // app back into it (e.g., as in Facebook OAuth 2.0).
-  bool old_url_is_hosted_app = extensions->GetByURL(old_url) &&
-       !extensions->GetByURL(old_url)->web_extent().is_empty();
-  return !extensions->InSameExtent(old_url, new_url) &&
-         !old_url_is_hosted_app;
+  bool old_url_is_hosted_app = old_url_extension &&
+      !old_url_extension->web_extent().is_empty();
+  if (old_url_is_hosted_app)
+    return false;
+
+  return old_url_extension != new_url_extension;
 }
 
 void ChromeContentRendererClient::OnPurgeMemory() {

chrome/renderer/chrome_content_renderer_client.h

@@ -14,7 +14,9 @@
 #include "content/public/renderer/content_renderer_client.h"
 
 class ChromeRenderProcessObserver;
+class Extension;
 class ExtensionDispatcher;
+class ExtensionSet;
 class RendererHistogramSnapshots;
 class RendererNetPredictor;
 class RendererTracking;
@@ -126,6 +128,11 @@ class ChromeContentRendererClient : public content::ContentRendererClient {
       bool is_blocked_for_prerendering,
       bool allow_loading);
 
+  // Returns the extension for the given URL.  Excludes extension objects for
+  // bookmark apps, which do not use the app process model.
+  const Extension* GetNonBookmarkAppExtension(const ExtensionSet* extensions,
+                                              const GURL& url);
+
   // Returns true if the frame is navigating to an URL either into or out of an
   // extension app's extent.
   bool CrossesExtensionExtents(WebKit::WebFrame* frame,