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

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

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=92099

[Charlie Reis, 2011-07-08 21:59:19]

Here's a change to put different instances of hosted apps in separate processes,
unless the app uses the background permission.

I needed to remove the unused process_ids_ map in ExtensionProcessManager
because there's no longer one process per extension (for hosted apps).

I also updated ExtensionService::ExtensionBindingsAllowed, which wasn't behaving
as expected if we passed in an effective URL (i.e., an extension:// URL for a
hosted app).

[Matt Perry, 2011-07-08 22:18:35]

LGTM

http://codereview.chromium.org/7328029/diff/2001/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/7328029/diff/2001/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:194: const GURL& url) {
rename url to effective_url to clarify expectations

http://codereview.chromium.org/7328029/diff/2001/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_process_apitest.cc (right):

http://codereview.chromium.org/7328029/diff/2001/chrome/browser/extensions/ap...
chrome/browser/extensions/app_process_apitest.cc:196: // The extension should
have opened 2 new tabs. Including the original blank
the extension didn't open the tabs, we did, right?

http://codereview.chromium.org/7328029/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/7328029/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_process_manager.cc:224:
browsing_instance_->GetSiteInstanceForURL(url));
What will happen here for hosted apps?

[Charlie Reis, 2011-07-08 22:49:50]

Thanks Matt-- responses below.

John, can you take a look for owners approval?  I've factored some of the
extension-specific logic out of BrowsingInstance into
ChromeContentBrowserClient.

http://codereview.chromium.org/7328029/diff/2001/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/7328029/diff/2001/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:194: const GURL& url) {
On 2011/07/08 22:18:35, Matt Perry wrote:
> rename url to effective_url to clarify expectations

Done.

http://codereview.chromium.org/7328029/diff/2001/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_process_apitest.cc (right):

http://codereview.chromium.org/7328029/diff/2001/chrome/browser/extensions/ap...
chrome/browser/extensions/app_process_apitest.cc:196: // The extension should
have opened 2 new tabs. Including the original blank
On 2011/07/08 22:18:35, Matt Perry wrote:
> the extension didn't open the tabs, we did, right?

Yep.  Fixed in the test above as well.

http://codereview.chromium.org/7328029/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/7328029/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_process_manager.cc:224:
browsing_instance_->GetSiteInstanceForURL(url));
On 2011/07/08 22:18:35, Matt Perry wrote:
> What will happen here for hosted apps?

This doesn't appear to be called for hosted apps-- they hit the logic in
BrowsingInstance::ShouldUseProcessPerSite.  Do you know of a case where it's
possible?

[Matt Perry, 2011-07-08 23:11:43]

On 2011/07/08 22:49:50, creis
wrote:http://codereview.chromium.org/7328029/diff/2001/chrome/browser/extensions/extension_process_manager.cc
> File chrome/browser/extensions/extension_process_manager.cc (right):
> 
>
http://codereview.chromium.org/7328029/diff/2001/chrome/browser/extensions/ex...
> chrome/browser/extensions/extension_process_manager.cc:224:
> browsing_instance_->GetSiteInstanceForURL(url));
> On 2011/07/08 22:18:35, Matt Perry wrote:
> > What will happen here for hosted apps?
> 
> This doesn't appear to be called for hosted apps-- they hit the logic in
> BrowsingInstance::ShouldUseProcessPerSite.  Do you know of a case where it's
> possible?

Based on
http://www.google.com/codesearch#search/&q=GetExtensionProcess%5C(&exact_pack...
it looks like geolocation may rely on it.

[Charlie Reis, 2011-07-11 20:27:12]

On 2011/07/08 23:11:43, Matt Perry wrote:
> > On 2011/07/08 22:18:35, Matt Perry wrote:
> > > What will happen here for hosted apps?
> > 
> > This doesn't appear to be called for hosted apps-- they hit the logic in
> > BrowsingInstance::ShouldUseProcessPerSite.  Do you know of a case where it's
> > possible?
> 
> Based on
>
http://www.google.com/codesearch#search/&q=GetExtensionProcess%255C%28&exact_...
> it looks like geolocation may rely on it.

I just debugged that case to take a closer look.  It doesn't find an Extension
for hosted apps because it uses ExtensionService::GetExtensionByURL on a normal
URL, not the app's effective URL.  I don't see how that ever worked for hosted
apps-- even if it did find the Extension object, the hosted app's
BrowsingInstance was never the one in ExtensionProcessManager.  And I can
confirm that my hosted app with the geolocation permission still prompts the
user, regardless of my change.

I noticed this case is being discussed in http://crbug.com/88550.  I'll post a
comment there.

Ok if I go ahead with this CL?

Thanks,
Charlie

[Matt Perry, 2011-07-11 20:29:29]

On 2011/07/11 20:27:12, creis wrote:
> On 2011/07/08 23:11:43, Matt Perry wrote:
> > > On 2011/07/08 22:18:35, Matt Perry wrote:
> > > > What will happen here for hosted apps?
> > > 
> > > This doesn't appear to be called for hosted apps-- they hit the logic in
> > > BrowsingInstance::ShouldUseProcessPerSite.  Do you know of a case where
it's
> > > possible?
> > 
> > Based on
> >
>
http://www.google.com/codesearch#search/&q=GetExtensionProcess%25255C%2528&ex...
> > it looks like geolocation may rely on it.
> 
> I just debugged that case to take a closer look.  It doesn't find an Extension
> for hosted apps because it uses ExtensionService::GetExtensionByURL on a
normal
> URL, not the app's effective URL.  I don't see how that ever worked for hosted
> apps-- even if it did find the Extension object, the hosted app's
> BrowsingInstance was never the one in ExtensionProcessManager.  And I can
> confirm that my hosted app with the geolocation permission still prompts the
> user, regardless of my change.
> 
> I noticed this case is being discussed in http://crbug.com/88550.  I'll post a
> comment there.
> 
> Ok if I go ahead with this CL?

Yeah, LGTM. We'll need to figure something out for geolocation. Perhaps return a
list of extension processes instead of just 1.

[Charlie Reis, 2011-07-11 20:34:57]

On 2011/07/11 20:29:29, Matt Perry wrote:
> On 2011/07/11 20:27:12, creis wrote:
> > On 2011/07/08 23:11:43, Matt Perry wrote:
> > > > On 2011/07/08 22:18:35, Matt Perry wrote:
> > > > > What will happen here for hosted apps?
> > > > 
> > > > This doesn't appear to be called for hosted apps-- they hit the logic in
> > > > BrowsingInstance::ShouldUseProcessPerSite.  Do you know of a case where
> it's
> > > > possible?
> > > 
> > > Based on
> > >
> >
>
http://www.google.com/codesearch#search/&q=GetExtensionProcess%2525255C%25252...
> > > it looks like geolocation may rely on it.
> > 
> > I just debugged that case to take a closer look.  It doesn't find an
Extension
> > for hosted apps because it uses ExtensionService::GetExtensionByURL on a
> normal
> > URL, not the app's effective URL.  I don't see how that ever worked for
hosted
> > apps-- even if it did find the Extension object, the hosted app's
> > BrowsingInstance was never the one in ExtensionProcessManager.  And I can
> > confirm that my hosted app with the geolocation permission still prompts the
> > user, regardless of my change.
> > 
> > I noticed this case is being discussed in http://crbug.com/88550.  I'll post
a
> > comment there.
> > 
> > Ok if I go ahead with this CL?
> 
> Yeah, LGTM. We'll need to figure something out for geolocation. Perhaps return
a
> list of extension processes instead of just 1.

Yeah, that sounds reasonable.

Also, I missed the part that it falls back to GetExtensionByWebExtent, but it's
still buggy-- the RequestGeolocationPermission function is still not finding the
Extension for the hosted app in my case.  That's because the "requesting_frame"
URL is just the origin, but the hosted app's extent is a sub-path within the
origin.  I still consider that (and the BrowsingInstance mismatch) part of bug
88550, so I'll post an update there.

Thanks,
Charlie

[Charlie Reis, 2011-07-11 21:46:56]

Drat.  I think I was mistaken-- if the web extent was origin-wide before, we did
find the correct process in RequestGeolocationPermission.  (Despite the fact
that EPM has its own BrowsingInstance, it uses the profile-wide map for all
extension URLs.)

That means this CL would introduce a regression for geolocation on hosted apps. 
I'll update this CL to fix it before landing.  (I think I can use a map of
SiteInstance ID -> extension ID, since the geolocation logic has access to the
SiteInstance.  Multiple SiteInstances can point to the same extension without a
problem.)

After that, GetExtensionProcess should only be used for extensions and
single-process hosted apps.

[Charlie Reis, 2011-07-11 23:48:15]

Thanks for the heads up about the geolocation logic, Matt.  This CL should fix
that by keeping track of a SiteInstance-to-Extension map, making it easy to find
out if the current SiteInstance is associated with the right extension.  Can you
take a look?

John, can you review for owners approval?  I think I forgot to put you in the
reviewer list last time.  I've factored some of the extension-specific logic out
of BrowsingInstance into ChromeContentBrowserClient.

Thanks!
Charlie

[Matt Perry, 2011-07-12 00:13:02]

LGTM with below change

http://codereview.chromium.org/7328029/diff/9005/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/7328029/diff/9005/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_process_manager.cc:432: const Extension*
IncognitoExtensionProcessManager::GetExtensionForSiteInstance(
The logic here should match GetExtensionProcess. First check if it exists in our
map (by calling the base class' method). If so, and it is split mode, return it.
Otherwise, call through to the original manager.

[Charlie Reis, 2011-07-12 00:50:12]

http://codereview.chromium.org/7328029/diff/9005/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/7328029/diff/9005/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_process_manager.cc:432: const Extension*
IncognitoExtensionProcessManager::GetExtensionForSiteInstance(
On 2011/07/12 00:13:02, Matt Perry wrote:
> The logic here should match GetExtensionProcess. First check if it exists in
our
> map (by calling the base class' method). If so, and it is split mode, return
it.
> Otherwise, call through to the original manager.

Thanks-- fixed.

[jam, 2011-07-12 01:05:32]

content lgtm