Use process-per-app-instance for hosted apps without background permission.

Also update ExtensionProcessManager to map SiteInstances to extensions,
rather than extensions to processes.

BUG=87644
TEST=AppApiTest.AppProcessInstances

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

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

11 files changed, 193 insertions, 51 deletions

diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc
index a50ec9c..51177de 100644
--- a/chrome/browser/chrome_content_browser_client.cc
+++ b/chrome/browser/chrome_content_browser_client.cc
@@ -98,9 +98,11 @@ void InitRenderViewHostForExtensions(RenderViewHost* render_view_host) {
 
   site_instance->GetProcess()->mark_is_extension_process();
 
-  // Register the association between extension and process with
+  // Register the association between extension and SiteInstance with
   // ExtensionProcessManager.
-  process_manager->RegisterExtensionProcess(extension->id(), process->id());
+  // TODO(creis): Use this to replace SetInstalledAppForRenderer below.
+  process_manager->RegisterExtensionSiteInstance(site_instance->id(),
+                                                 extension->id());
 
   if (extension->is_app()) {
     render_view_host->Send(
@@ -194,6 +196,36 @@ GURL ChromeContentBrowserClient::GetEffectiveURL(Profile* profile,
   return extension->GetResourceURL(url.path());
 }
 
+bool ChromeContentBrowserClient::ShouldUseProcessPerSite(
+    Profile* profile,
+    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.
+  if (!effective_url.SchemeIs(chrome::kExtensionScheme))
+    return false;
+
+  if (!profile || !profile->GetExtensionService())
+    return false;
+
+  const Extension* extension =
+      profile->GetExtensionService()->GetExtensionByURL(effective_url);
+  if (!extension)
+    return false;
+
+  // If the URL is part of a hosted app that does not have the background
+  // permission, we want to give each instance its own process to improve
+  // responsiveness.
+  if (extension->GetType() == Extension::TYPE_HOSTED_APP &&
+      !extension->HasAPIPermission(ExtensionAPIPermission::kBackground))
+    return false;
+
+  // Hosted apps that have the background permission must use process per site,
+  // since all instances can make synchronous calls to the background window.
+  // Other extensions should use process per site as well.
+  return true;
+}
+
 bool ChromeContentBrowserClient::IsURLSameAsAnySiteInstance(const GURL& url) {
   return url == GURL(chrome::kChromeUICrashURL) ||
          url == GURL(chrome::kChromeUIKillURL) ||
diff --git a/chrome/browser/chrome_content_browser_client.h b/chrome/browser/chrome_content_browser_client.h
index 8e1b02d..8ab60c9 100644
--- a/chrome/browser/chrome_content_browser_client.h
+++ b/chrome/browser/chrome_content_browser_client.h
@@ -21,6 +21,8 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient {
   virtual void PluginProcessHostCreated(PluginProcessHost* host) OVERRIDE;
   virtual void WorkerProcessHostCreated(WorkerProcessHost* host) OVERRIDE;
   virtual content::WebUIFactory* GetWebUIFactory() OVERRIDE;
+  virtual bool ShouldUseProcessPerSite(Profile* profile,
+                                       const GURL& effective_url) OVERRIDE;
   virtual GURL GetEffectiveURL(Profile* profile, const GURL& url) OVERRIDE;
   virtual bool IsURLSameAsAnySiteInstance(const GURL& url) OVERRIDE;
   virtual std::string GetCanonicalEncodingNameByAliasName(
diff --git a/chrome/browser/extensions/app_process_apitest.cc b/chrome/browser/extensions/app_process_apitest.cc
index 898a16c..56679f6 100644
--- a/chrome/browser/extensions/app_process_apitest.cc
+++ b/chrome/browser/extensions/app_process_apitest.cc
@@ -109,9 +109,10 @@ IN_PROC_BROWSER_TEST_F(AppApiTest, MAYBE_AppProcess) {
                   is_extension_process());
   EXPECT_FALSE(browser()->GetTabContentsAt(3)->web_ui());
 
-  // The extension should have opened 3 new tabs. Including the original blank
-  // tab, we now have 4 tabs. Two should be part of the extension app, and
-  // grouped in the same process.
+  // We should have opened 3 new extension tabs. Including the original blank
+  // tab, we now have 4 tabs. Because the app_process app has the background
+  // permission, all of its instances are in the same process.  Thus two tabs
+  // should be part of the extension app and grouped in the same process.
   ASSERT_EQ(4, browser()->tab_count());
   RenderViewHost* host = browser()->GetTabContentsAt(1)->render_view_host();
 
@@ -154,6 +155,61 @@ IN_PROC_BROWSER_TEST_F(AppApiTest, MAYBE_AppProcess) {
   ASSERT_TRUE(windowOpenerValid);
 }
 
+// Test that hosted apps without the background permission use a process per app
+// instance model, such that separate instances are in separate processes.
+IN_PROC_BROWSER_TEST_F(AppApiTest, AppProcessInstances) {
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kDisablePopupBlocking);
+
+  host_resolver()->AddRule("*", "127.0.0.1");
+  ASSERT_TRUE(test_server()->Start());
+
+  ASSERT_TRUE(LoadExtension(
+      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);
+
+  // 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
+  // they have no elevated privileges and thus should not have 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_TRUE(browser()->GetTabContentsAt(1)->render_view_host()->process()->
+                  is_extension_process());
+  EXPECT_FALSE(browser()->GetTabContentsAt(1)->web_ui());
+  browser()->NewTab();
+  ui_test_utils::NavigateToURL(browser(), base_url.Resolve("path2/empty.html"));
+  EXPECT_TRUE(browser()->GetTabContentsAt(2)->render_view_host()->process()->
+                  is_extension_process());
+  EXPECT_FALSE(browser()->GetTabContentsAt(2)->web_ui());
+
+  // We should have opened 2 new extension tabs. Including the original blank
+  // tab, we now have 3 tabs. The two app tabs should not be in the same
+  // process, since they do not have the background permission.  (Thus, we want
+  // to separate them to improve responsiveness.)
+  ASSERT_EQ(3, browser()->tab_count());
+  RenderViewHost* host1 = browser()->GetTabContentsAt(1)->render_view_host();
+  RenderViewHost* host2 = browser()->GetTabContentsAt(2)->render_view_host();
+  EXPECT_NE(host1->process(), host2->process());
+
+  // Opening tabs with window.open should keep the page in the opener's process.
+  ASSERT_EQ(1u, BrowserList::GetBrowserCount(browser()->profile()));
+  WindowOpenHelper(browser(), host1,
+                   base_url.Resolve("path1/empty.html"), true);
+  WindowOpenHelper(browser(), host2,
+                   base_url.Resolve("path2/empty.html"), true);
+}
+
 // 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")
diff --git a/chrome/browser/extensions/extension_process_manager.cc b/chrome/browser/extensions/extension_process_manager.cc
index 65e55ff..1944478 100644
--- a/chrome/browser/extensions/extension_process_manager.cc
+++ b/chrome/browser/extensions/extension_process_manager.cc
@@ -37,6 +37,7 @@ class IncognitoExtensionProcessManager : public ExtensionProcessManager {
                                     const GURL& url);
   virtual SiteInstance* GetSiteInstanceForURL(const GURL& url);
   virtual RenderProcessHost* GetExtensionProcess(const GURL& url);
+  virtual const Extension* GetExtensionForSiteInstance(int site_instance_id);
 
  private:
   // NotificationObserver:
@@ -95,9 +96,7 @@ ExtensionProcessManager::ExtensionProcessManager(Profile* profile)
                  Source<Profile>(original_profile));
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED,
                  Source<Profile>(profile));
-  registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_TERMINATED,
-                 NotificationService::AllSources());
-  registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_CLOSED,
+  registrar_.Add(this, content::NOTIFICATION_SITE_INSTANCE_DELETED,
                  NotificationService::AllSources());
   registrar_.Add(this, content::NOTIFICATION_APP_TERMINATING,
                  NotificationService::AllSources());
@@ -221,28 +220,23 @@ ExtensionHost* ExtensionProcessManager::GetBackgroundHostForExtension(
   return NULL;
 }
 
-void ExtensionProcessManager::RegisterExtensionProcess(
-    const std::string& extension_id, int process_id) {
-  // TODO(mpcomplete): This is the only place we actually read process_ids_.
-  // Is it necessary?
-  ProcessIDMap::const_iterator it = process_ids_.find(extension_id);
-  if (it != process_ids_.end() && (*it).second == process_id)
+void ExtensionProcessManager::RegisterExtensionSiteInstance(
+    int site_instance_id, const std::string& extension_id) {
+  SiteInstanceIDMap::const_iterator it = extension_ids_.find(site_instance_id);
+  if (it != extension_ids_.end() && (*it).second == extension_id)
     return;
 
-  // Extension ids should get removed from the map before the process ids get
-  // reused from a dead renderer.
-  DCHECK(it == process_ids_.end());
-  process_ids_[extension_id] = process_id;
+  // SiteInstance ids should get removed from the map before the extension ids
+  // get used for a new SiteInstance.
+  DCHECK(it == extension_ids_.end());
+  extension_ids_[site_instance_id] = extension_id;
 }
 
-void ExtensionProcessManager::UnregisterExtensionProcess(int process_id) {
-  ProcessIDMap::iterator it = process_ids_.begin();
-  while (it != process_ids_.end()) {
-    if (it->second == process_id)
-      process_ids_.erase(it++);
-    else
-      ++it;
-  }
+void ExtensionProcessManager::UnregisterExtensionSiteInstance(
+    int site_instance_id) {
+  SiteInstanceIDMap::iterator it = extension_ids_.find(site_instance_id);
+  if (it != extension_ids_.end())
+    extension_ids_.erase(it++);
 }
 
 RenderProcessHost* ExtensionProcessManager::GetExtensionProcess(
@@ -262,6 +256,20 @@ RenderProcessHost* ExtensionProcessManager::GetExtensionProcess(
       Extension::GetBaseURLFromExtensionId(extension_id));
 }
 
+const Extension* ExtensionProcessManager::GetExtensionForSiteInstance(
+    int site_instance_id) {
+  SiteInstanceIDMap::const_iterator it = extension_ids_.find(site_instance_id);
+  if (it != extension_ids_.end()) {
+    // Look up the extension by ID, including disabled extensions in case
+    // this gets called while an old process is still around.
+    ExtensionService* service =
+        browsing_instance_->profile()->GetExtensionService();
+    return service->GetExtensionById(it->second, false);
+  }
+
+  return NULL;
+}
+
 SiteInstance* ExtensionProcessManager::GetSiteInstanceForURL(const GURL& url) {
   return browsing_instance_->GetSiteInstanceForURL(url);
 }
@@ -313,10 +321,9 @@ void ExtensionProcessManager::Observe(int type,
       break;
     }
 
-    case content::NOTIFICATION_RENDERER_PROCESS_TERMINATED:
-    case content::NOTIFICATION_RENDERER_PROCESS_CLOSED: {
-      RenderProcessHost* host = Source<RenderProcessHost>(source).ptr();
-      UnregisterExtensionProcess(host->id());
+    case content::NOTIFICATION_SITE_INSTANCE_DELETED: {
+      SiteInstance* site_instance = Source<SiteInstance>(source).ptr();
+      UnregisterExtensionSiteInstance(site_instance->id());
       break;
     }
 
@@ -422,6 +429,17 @@ RenderProcessHost* IncognitoExtensionProcessManager::GetExtensionProcess(
   }
 }
 
+const Extension* IncognitoExtensionProcessManager::GetExtensionForSiteInstance(
+    int site_instance_id) {
+  const Extension* extension =
+      ExtensionProcessManager::GetExtensionForSiteInstance(site_instance_id);
+  if (extension && extension->incognito_split_mode()) {
+    return extension;
+  } else {
+    return original_manager_->GetExtensionForSiteInstance(site_instance_id);
+  }
+}
+
 const Extension* IncognitoExtensionProcessManager::GetExtensionOrAppByURL(
     const GURL& url) {
   ExtensionService* service =
diff --git a/chrome/browser/extensions/extension_process_manager.h b/chrome/browser/extensions/extension_process_manager.h
index c2a7710..fd7682f 100644
--- a/chrome/browser/extensions/extension_process_manager.h
+++ b/chrome/browser/extensions/extension_process_manager.h
@@ -68,20 +68,25 @@ class ExtensionProcessManager : public NotificationObserver {
   // Returns the SiteInstance that the given URL belongs to.
   virtual SiteInstance* GetSiteInstanceForURL(const GURL& url);
 
-  // Registers an extension process by |extension_id| and specifying which
-  // |process_id| it belongs to.
-  void RegisterExtensionProcess(const std::string& extension_id,
-                                int process_id);
+  // Registers a SiteInstance with |site_instance_id| as hosting the extension
+  // with |extension_id|.
+  void RegisterExtensionSiteInstance(int site_instance_id,
+                                     const std::string& extension_id);
 
-  // Unregisters an extension process with specified |process_id|.
-  void UnregisterExtensionProcess(int process_id);
+  // Unregisters the extension associated with |site_instance_id|.
+  void UnregisterExtensionSiteInstance(int site_instance_id);
 
   // Returns the extension process that |url| is associated with if it exists.
+  // This is not valid for hosted apps without the background permission, since
+  // such apps may have multiple processes.
   virtual RenderProcessHost* GetExtensionProcess(const GURL& url);
 
   // Returns the process that the extension with the given ID is running in.
   RenderProcessHost* GetExtensionProcess(const std::string& extension_id);
 
+  // Returns the Extension associated with the given SiteInstance id, if any.
+  virtual const Extension* GetExtensionForSiteInstance(int site_instance_id);
+
   // Returns true if |host| is managed by this process manager.
   bool HasExtensionHost(ExtensionHost* host) const;
 
@@ -116,9 +121,9 @@ class ExtensionProcessManager : public NotificationObserver {
   // controls process grouping.
   scoped_refptr<BrowsingInstance> browsing_instance_;
 
-  // A map of extension ID to the render_process_id that the extension lives in.
-  typedef std::map<std::string, int> ProcessIDMap;
-  ProcessIDMap process_ids_;
+  // A map of site instance ID to the ID of the extension it hosts.
+  typedef std::map<int, std::string> SiteInstanceIDMap;
+  SiteInstanceIDMap extension_ids_;
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionProcessManager);
 };
diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index 5748626..8449ce3 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -2171,12 +2171,16 @@ const Extension* ExtensionService::GetExtensionByWebExtent(const GURL& url) {
 }
 
 bool ExtensionService::ExtensionBindingsAllowed(const GURL& url) {
-  // Allow bindings for all packaged extension.
-  if (GetExtensionByURL(url))
+  // Allow bindings for all packaged extensions.
+  // Note that GetExtensionByURL may return an Extension for hosted apps
+  // if the URL came from GetEffectiveURL.
+  const Extension* extension = GetExtensionByURL(url);
+  if (extension && extension->GetType() != Extension::TYPE_HOSTED_APP)
     return true;
 
   // Allow bindings for all component, hosted apps.
-  const Extension* extension = GetExtensionByWebExtent(url);
+  if (!extension)
+    extension = GetExtensionByWebExtent(url);
   return (extension && extension->location() == Extension::COMPONENT);
 }
 
diff --git a/chrome/browser/geolocation/chrome_geolocation_permission_context.cc b/chrome/browser/geolocation/chrome_geolocation_permission_context.cc
index c2db97f..6e8ed8e 100644
--- a/chrome/browser/geolocation/chrome_geolocation_permission_context.cc
+++ b/chrome/browser/geolocation/chrome_geolocation_permission_context.cc
@@ -22,7 +22,7 @@
 #include "chrome/common/pref_names.h"
 #include "content/browser/browser_thread.h"
 #include "content/browser/geolocation/geolocation_provider.h"
-#include "content/browser/renderer_host/render_process_host.h"
+#include "content/browser/renderer_host/render_view_host.h"
 #include "content/common/content_notification_types.h"
 #include "content/common/notification_registrar.h"
 #include "content/common/notification_source.h"
@@ -519,12 +519,18 @@ void ChromeGeolocationPermissionContext::RequestGeolocationPermission(
     if (!ext)
       ext = extensions->GetExtensionByWebExtent(requesting_frame);
     if (ext && ext->HasAPIPermission(ExtensionAPIPermission::kGeolocation)) {
-      ExtensionProcessManager* epm = profile_->GetExtensionProcessManager();
-      RenderProcessHost* process = epm->GetExtensionProcess(requesting_frame);
-      if (process && process->id() == render_process_id) {
-        NotifyPermissionSet(render_process_id, render_view_id, bridge_id,
-                            requesting_frame, true);
-        return;
+      // Make sure this matches the current extension.
+      RenderViewHost* rvh = RenderViewHost::FromID(render_process_id,
+                                                   render_view_id);
+      if (rvh && rvh->site_instance()) {
+        ExtensionProcessManager* epm = profile_->GetExtensionProcessManager();
+        const Extension* current_ext = epm->GetExtensionForSiteInstance(
+            rvh->site_instance()->id());
+        if (ext == current_ext) {
+          NotifyPermissionSet(render_process_id, render_view_id, bridge_id,
+                              requesting_frame, true);
+          return;
+        }
       }
     }
   }
diff --git a/chrome/test/data/extensions/api_test/app_process/manifest.json b/chrome/test/data/extensions/api_test/app_process/manifest.json
index 42a95fc..f8eb88a 100644
--- a/chrome/test/data/extensions/api_test/app_process/manifest.json
+++ b/chrome/test/data/extensions/api_test/app_process/manifest.json
@@ -1,7 +1,7 @@
 {
   "name": "app_process",
   "version": "0.1",
-  "description": "Tests that app URLs are grouped into the right process",
+  "description": "Tests that app URLs with the background permission are grouped into the same process.",
   "app": {
     "urls": [
       "http://localhost/files/extensions/api_test/app_process/path1",
@@ -13,5 +13,8 @@
     "launch": {
       "web_url": "http://localhost/files/extensions/api_test/app_process/path1/foo.html"
     }
-  }
+  },
+  "permissions": [
+    "background"
+  ]
 }
diff --git a/chrome/test/data/extensions/api_test/app_process_instances/manifest.json b/chrome/test/data/extensions/api_test/app_process_instances/manifest.json
new file mode 100644
index 0000000..f502071
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/app_process_instances/manifest.json
@@ -0,0 +1,14 @@
+{
+  "name": "app_process_instances",
+  "version": "0.1",
+  "description": "Tests that app URLs without the background permission are not consolidated.",
+  "app": {
+    "urls": [
+      "http://localhost/files/extensions/api_test/app_process_instances/path1",
+      "http://localhost/files/extensions/api_test/app_process_instances/path2"
+    ],
+    "launch": {
+      "web_url": "http://localhost/files/extensions/api_test/app_process_instances/path1/empty.html"
+    }
+  }
+}
diff --git a/chrome/test/data/extensions/api_test/app_process_instances/path1/empty.html b/chrome/test/data/extensions/api_test/app_process_instances/path1/empty.html
new file mode 100644
index 0000000..d3cdf0a
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/app_process_instances/path1/empty.html
@@ -0,0 +1 @@
+<title>Unmodified</title>
diff --git a/chrome/test/data/extensions/api_test/app_process_instances/path2/empty.html b/chrome/test/data/extensions/api_test/app_process_instances/path2/empty.html
new file mode 100644
index 0000000..d3cdf0a
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/app_process_instances/path2/empty.html
@@ -0,0 +1 @@
+<title>Unmodified</title>