diff options
author | creis@chromium.org <creis@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-07-12 02:13:55 +0000 |
---|---|---|
committer | creis@chromium.org <creis@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-07-12 02:13:55 +0000 |
commit | 056ad2a8ef3d941e6e116fbc7ef1a6d747a6c691 (patch) | |
tree | 342be06819c9bbfe9901c4ef1c93dc00fd8966cb | |
parent | 3e4aabc2c524579517ec69c4cc32ec2b3297faef (diff) | |
download | chromium_src-056ad2a8ef3d941e6e116fbc7ef1a6d747a6c691.zip chromium_src-056ad2a8ef3d941e6e116fbc7ef1a6d747a6c691.tar.gz chromium_src-056ad2a8ef3d941e6e116fbc7ef1a6d747a6c691.tar.bz2 |
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
16 files changed, 214 insertions, 52 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> diff --git a/content/browser/browsing_instance.cc b/content/browser/browsing_instance.cc index 7739d42..34bfa50 100644 --- a/content/browser/browsing_instance.cc +++ b/content/browser/browsing_instance.cc @@ -37,7 +37,8 @@ bool BrowsingInstance::ShouldUseProcessPerSite(const GURL& url) { // process creation logic in RenderProcessHost, so we do not need to worry // about it here. - if (url.SchemeIs(chrome::kExtensionScheme)) + if (content::GetContentClient()->browser()->ShouldUseProcessPerSite(profile_, + url)) return true; // DevTools pages have WebUI type but should not reuse the same host. diff --git a/content/browser/content_browser_client.h b/content/browser/content_browser_client.h index 84ddae1..5d55645 100644 --- a/content/browser/content_browser_client.h +++ b/content/browser/content_browser_client.h @@ -86,6 +86,11 @@ class ContentBrowserClient { // group different url schemes in the same SiteInstance. virtual GURL GetEffectiveURL(Profile* profile, const GURL& url) = 0; + // Returns whether all instances of the specified effective URL should be + // rendered by the same process, rather than using process-per-site-instance. + virtual bool ShouldUseProcessPerSite(Profile* profile, + const GURL& effective_url) = 0; + // Returns whether a specified URL is to be considered the same as any // SiteInstance. virtual bool IsURLSameAsAnySiteInstance(const GURL& url) = 0; diff --git a/content/browser/site_instance.cc b/content/browser/site_instance.cc index d69690e..442c807 100644 --- a/content/browser/site_instance.cc +++ b/content/browser/site_instance.cc @@ -42,6 +42,11 @@ SiteInstance::SiteInstance(BrowsingInstance* browsing_instance) } SiteInstance::~SiteInstance() { + NotificationService::current()->Notify( + content::NOTIFICATION_SITE_INSTANCE_DELETED, + Source<SiteInstance>(this), + NotificationService::NoDetails()); + // Now that no one is referencing us, we can safely remove ourselves from // the BrowsingInstance. Any future visits to a page from this site // (within the same BrowsingInstance) can safely create a new SiteInstance. diff --git a/content/browser/site_instance_unittest.cc b/content/browser/site_instance_unittest.cc index 40b9d8c..b1e55b5 100644 --- a/content/browser/site_instance_unittest.cc +++ b/content/browser/site_instance_unittest.cc @@ -42,6 +42,11 @@ class SiteInstanceTestBrowserClient : public content::MockContentBrowserClient { return &factory_; } + virtual bool ShouldUseProcessPerSite(Profile* profile, + const GURL& effective_url) OVERRIDE { + return false; + } + virtual bool IsURLSameAsAnySiteInstance(const GURL& url) OVERRIDE { return url == GURL(kSameAsAnyInstanceURL) || url == GURL(chrome::kAboutCrashURL); diff --git a/content/common/content_notification_types.h b/content/common/content_notification_types.h index 76006ad..b513e35 100644 --- a/content/common/content_notification_types.h +++ b/content/common/content_notification_types.h @@ -369,6 +369,10 @@ enum { // the new state is "visible." NOTIFICATION_RENDER_WIDGET_VISIBILITY_CHANGED, + // Sent from ~SiteInstance. The source is the SiteInstance, and the details + // are unused. + NOTIFICATION_SITE_INSTANCE_DELETED, + // The focused element inside a page has changed. The source is the // TabContents containing the render view host for the page. The details is // a Details<const bool> that indicates whether or not an editable node was |