Delegate decision what site instances can be rendered in what process to chrome

Delegate decision what site instances can be rendered in what process to chrome

BUG=89642, 90443
TEST=nothing breaks


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

[jochen (gone - plz use gerrit), 2011-09-23 20:49:05]

plz review

Aaron: extension_process_manager bits
John: rest

[jam, 2011-09-26 15:46:16]

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

http://codereview.chromium.org/8033001/diff/6001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_process_manager.cc:257:
std::set<int>::iterator it = process_ids_.find(host_id);
nit: you can also just write process_ids_.erase(host_id);

http://codereview.chromium.org/8033001/diff/6001/content/browser/content_brow...
File content/browser/content_browser_client.h (right):

http://codereview.chromium.org/8033001/diff/6001/content/browser/content_brow...
content/browser/content_browser_client.h:129: // special privileges, e.g.
special bindings.
nit: it's unclear to me reading this what special privileges means?

http://codereview.chromium.org/8033001/diff/6001/content/browser/renderer_hos...
File content/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/8033001/diff/6001/content/browser/renderer_hos...
content/browser/renderer_host/browser_render_process_host.cc:1003: bool
BrowserRenderProcessHost::IsSuitableHost(const GURL& site_url) {
it's unclear to me why you implement have this method here, and the rest in
RenderProcessHost. why not move this to RPH as well?

http://codereview.chromium.org/8033001/diff/6001/content/browser/renderer_hos...
File content/browser/renderer_host/mock_render_process_host.cc (right):

http://codereview.chromium.org/8033001/diff/6001/content/browser/renderer_hos...
content/browser/renderer_host/mock_render_process_host.cc:156: if
(!RenderProcessHost::IsSuitableHost(site_url))
since this is just a mock object, do you need all this? can't you always return
true?

[jochen (gone - plz use gerrit), 2011-09-26 15:56:04]

http://codereview.chromium.org/8033001/diff/6001/content/browser/content_brow...
File content/browser/content_browser_client.h (right):

http://codereview.chromium.org/8033001/diff/6001/content/browser/content_brow...
content/browser/content_browser_client.h:129: // special privileges, e.g.
special bindings.
On 2011/09/26 15:46:16, John Abd-El-Malek wrote:
> nit: it's unclear to me reading this what special privileges means?

I tried to avoid saying extension bindings, since content shouldn't know about
extension bindings.

any suggestions how to improve this comment?

[jam, 2011-09-26 16:28:14]

http://codereview.chromium.org/8033001/diff/6001/content/browser/site_instanc...
File content/browser/site_instance.cc (right):

http://codereview.chromium.org/8033001/diff/6001/content/browser/site_instanc...
content/browser/site_instance.cc:135: process_->is_extension_process();
note: it's not just kExtensionScheme that we don't want in content, it's
anything to do with extensions. so is_extension_process() etc needs to move out
as well..

this is maybe why you need the hard-to-understand PrivilegedProcess concept in
the API. perhaps you just need to ask the embedder if the given process is the
right one for the url.

[jochen (gone - plz use gerrit), 2011-09-26 19:08:39]

I have a follow up CL that moves is_extension_process to the
ExtensionProcessManager.

Addressed all comments

[jam, 2011-09-26 21:08:58]

http://codereview.chromium.org/8033001/diff/5007/content/browser/tab_contents...
File content/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/8033001/diff/5007/content/browser/tab_contents...
content/browser/tab_contents/render_view_host_manager.cc:369: if
(browser->ShouldRunInPrivilegedProcess(new_entry->url()))
I think this concept of privilieged processes is still hard to understand for
someone reading the content code.

I think it'll be easier if we can ask the embedder if two urls should be run in
the same process. chrome can then compare the urls to see if both are extension
urls or not. Ideally, we would have one function that would do this in
ContentBrowsrClient. Can we use IsSuitableHost() for this purpose?

[jochen (gone - plz use gerrit), 2011-09-27 18:34:28]

On 2011/09/26 21:08:58, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/8033001/diff/5007/content/browser/tab_contents...
> File content/browser/tab_contents/render_view_host_manager.cc (right):
> 
>
http://codereview.chromium.org/8033001/diff/5007/content/browser/tab_contents...
> content/browser/tab_contents/render_view_host_manager.cc:369: if
> (browser->ShouldRunInPrivilegedProcess(new_entry->url()))
> I think this concept of privilieged processes is still hard to understand for
> someone reading the content code.
> 
> I think it'll be easier if we can ask the embedder if two urls should be run
in
> the same process. chrome can then compare the urls to see if both are
extension
> urls or not. Ideally, we would have one function that would do this in
> ContentBrowsrClient. Can we use IsSuitableHost() for this purpose?

I've stripped out the render view host manager related bits, because the
remainder seems to be straight forward.

As for using IsSuitableHost: even if a process is suitable for hosting an
extension, we still want to swap the processes if the extensions differ

So maybe we need a method ShouldSwapProcessesForNavigation?

[jam, 2011-09-27 18:50:43]

lgtm

On 2011/09/27 18:34:28, jochen wrote:
> On 2011/09/26 21:08:58, John Abd-El-Malek wrote:
> >
>
http://codereview.chromium.org/8033001/diff/5007/content/browser/tab_contents...
> > File content/browser/tab_contents/render_view_host_manager.cc (right):
> > 
> >
>
http://codereview.chromium.org/8033001/diff/5007/content/browser/tab_contents...
> > content/browser/tab_contents/render_view_host_manager.cc:369: if
> > (browser->ShouldRunInPrivilegedProcess(new_entry->url()))
> > I think this concept of privilieged processes is still hard to understand
for
> > someone reading the content code.
> > 
> > I think it'll be easier if we can ask the embedder if two urls should be run
> in
> > the same process. chrome can then compare the urls to see if both are
> extension
> > urls or not. Ideally, we would have one function that would do this in
> > ContentBrowsrClient. Can we use IsSuitableHost() for this purpose?
> 
> I've stripped out the render view host manager related bits, because the
> remainder seems to be straight forward.
> 
> As for using IsSuitableHost: even if a process is suitable for hosting an
> extension, we still want to swap the processes if the extensions differ
> 
> So maybe we need a method ShouldSwapProcessesForNavigation?

that sounds better yes

[Aaron Boodman, 2011-09-27 19:26:49]

These thoughts might be a bit scattered, I'll look again after lunch.

http://codereview.chromium.org/8033001/diff/13005/chrome/browser/chrome_conte...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/8033001/diff/13005/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.cc:255: ExtensionProcessManager*
process_manager =
Nit: I would prefer to call this variable extension_process_manager, even though
it is longer. While reading the code, I kept forgetting that's what it was.

http://codereview.chromium.org/8033001/diff/13005/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.cc:263:
process_host->is_extension_process() ||
Eventually, this field is going to have to go. Will your design be able to
replace it?

http://codereview.chromium.org/8033001/diff/13005/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_process_manager.h (right):

http://codereview.chromium.org/8033001/diff/13005/chrome/browser/extensions/e...
chrome/browser/extensions/extension_process_manager.h:82: void
RegisterProcessHostWithBindings(int host_id);
Can we just call these methods "RegisterProcess", "UnregisterProcess", etc?

Also, we will want to eventually keep track of the actual Extension. Because
that way we can lock down processes to only accessing the apis that some
extension in their process has.

Have you thought at all about whether your design will grow to handle that
easily? I'm not sure if we'd store the Extension* or an extension ID... Or about
how we'd keep them in sync.

http://codereview.chromium.org/8033001/diff/13005/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/chrome_render_view_host_observer.cc (right):

http://codereview.chromium.org/8033001/diff/13005/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_render_view_host_observer.cc:97:
render_view_host()->AllowBindings(BindingsPolicy::EXTENSION);
I had hoped we'd be able to get rid of BindingsPolicy::EXTENSION and
ChildSecurityPolicy::GrantExtensionBindings().

Looking through the code, I'm not even sure if the RVH flag is used for anything
anymore. ExtensionFunctionDispatcher checks this in a more direct way (just by
looking at the URL).

And the process flag may be obsoleted by the stuff you just added to EPM.

[jochen (gone - plz use gerrit), 2011-09-27 20:41:40]

http://codereview.chromium.org/8033001/diff/13005/chrome/browser/chrome_conte...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/8033001/diff/13005/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.cc:255: ExtensionProcessManager*
process_manager =
On 2011/09/27 19:26:49, Aaron Boodman wrote:
> Nit: I would prefer to call this variable extension_process_manager, even
though
> it is longer. While reading the code, I kept forgetting that's what it was.

Done.

http://codereview.chromium.org/8033001/diff/13005/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.cc:263:
process_host->is_extension_process() ||
On 2011/09/27 19:26:49, Aaron Boodman wrote:
> Eventually, this field is going to have to go. Will your design be able to
> replace it?

currently this flag is set and queried only in chrome/ (with one exception in
browser render process host where based on this flag UMA metrics and the command
line of the browser are set, but both can be handled on the chrome/ site as well
I think)

So the plan is to register processes that have this flag with the EPM as well,
and query the EPM form everywhere in chrome/ where this flag is read.

http://codereview.chromium.org/8033001/diff/13005/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_process_manager.h (right):

http://codereview.chromium.org/8033001/diff/13005/chrome/browser/extensions/e...
chrome/browser/extensions/extension_process_manager.h:82: void
RegisterProcessHostWithBindings(int host_id);
On 2011/09/27 19:26:49, Aaron Boodman wrote:
> Can we just call these methods "RegisterProcess", "UnregisterProcess", etc?
> 
Done

> Also, we will want to eventually keep track of the actual Extension. Because
> that way we can lock down processes to only accessing the apis that some
> extension in their process has.
> 
> Have you thought at all about whether your design will grow to handle that
> easily? I'm not sure if we'd store the Extension* or an extension ID... Or
about
> how we'd keep them in sync.

At the point where the process is registered, we have the Extension object, so
basically we can keep track of the extension that is running in this process as
well.

For the IsSuitableHost call, we'd need to derive the extension from the URL, but
that's possible

http://codereview.chromium.org/8033001/diff/13005/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/chrome_render_view_host_observer.cc (right):

http://codereview.chromium.org/8033001/diff/13005/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_render_view_host_observer.cc:97:
render_view_host()->AllowBindings(BindingsPolicy::EXTENSION);
On 2011/09/27 19:26:49, Aaron Boodman wrote:
> I had hoped we'd be able to get rid of BindingsPolicy::EXTENSION and
> ChildSecurityPolicy::GrantExtensionBindings().
> 
> Looking through the code, I'm not even sure if the RVH flag is used for
anything
> anymore. ExtensionFunctionDispatcher checks this in a more direct way (just by
> looking at the URL).
> 
> And the process flag may be obsoleted by the stuff you just added to EPM.

True, looks like we could get rid of this entirely. I'll do this in a follow-up
CL. ok?

[Aaron Boodman, 2011-09-27 20:50:53]

http://codereview.chromium.org/8033001/diff/13005/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/chrome_render_view_host_observer.cc (right):

http://codereview.chromium.org/8033001/diff/13005/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_render_view_host_observer.cc:97:
render_view_host()->AllowBindings(BindingsPolicy::EXTENSION);
On 2011/09/27 19:26:49, Aaron Boodman wrote:
> I had hoped we'd be able to get rid of BindingsPolicy::EXTENSION and
> ChildSecurityPolicy::GrantExtensionBindings().
> 
> Looking through the code, I'm not even sure if the RVH flag is used for
anything
> anymore. ExtensionFunctionDispatcher checks this in a more direct way (just by
> looking at the URL).
> 
> And the process flag may be obsoleted by the stuff you just added to EPM.

Actually, now that I look at this, how come we can't just reuse
RegisterExtensionSiteInstance?.

It looks like it gets called everywhere this gets called, except that it also
runs for themes. And I don't think that even matters because we actually enforce
bindings (in ExtensionFunctionDispatcher) differently.

[Aaron Boodman, 2011-09-27 21:03:23]

LGTM

Per offline discussion, please send follow up changes for the remaining issues.

[commit-bot: I haz the power, 2011-09-27 21:04:10]

CQ is trying tha patch. Follow status at
https://chromium-status.appspot.com/cq/jochen@chromium.org/8033001/18001

[commit-bot: I haz the power, 2011-09-27 23:07:33]

Change committed as 103041