Move process-per-site logic from BrowsingInstance to RenderProcessHost.

Move process-per-site logic from BrowsingInstance to RenderProcessHost.
This avoids sharing SiteInstances across different BrowsingInstances.

BUG=11629
BUG=131676
TEST=See repro steps in http://crbug.com/131676.


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

[Charlie Reis, 2012-06-26 16:48:15]

Hey Albert--
  Can you review?  I'm still trying to understand the GetSetZoomLevel unit test
failure on Windows and ChromeOS, since it's not failing for me on my local
Windows box, but other than that it should be ready.

Thanks,
Charlie

[Charlie Reis, 2012-06-26 20:59:10]

New patch up that fixes the HostZoomMap test failure with a change to
site_instance_impl_unittest.cc (cleaning up RPHs I created).  The rest is the
same.

[awong, 2012-06-27 00:26:54]

The logic looks good.  Comments are mostly style with one suggestion about how
to attach the site instance map to a BrowserContext.

http://codereview.chromium.org/10575014/diff/35001/chrome/browser/ssl/ssl_bro...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

http://codereview.chromium.org/10575014/diff/35001/chrome/browser/ssl/ssl_bro...
chrome/browser/ssl/ssl_browser_tests.cc:843: // Create a new tab in the same
process (since we are using process-per-site).
This parenthetical doesn't explain enough to me.  If we were not using process
per site, how would the setup code look different?

http://codereview.chromium.org/10575014/diff/35001/content/browser/browsing_i...
File content/browser/browsing_instance.cc (right):

http://codereview.chromium.org/10575014/diff/35001/content/browser/browsing_i...
content/browser/browsing_instance.cc:26: SiteInstanceMap::iterator i =
site_instance_map_.find(site);
How about just doing

return site_instance_map_.find(site) != site_instance_map_.end();

http://codereview.chromium.org/10575014/diff/35001/content/browser/renderer_h...
File content/browser/renderer_host/render_process_host_impl.cc (right):

http://codereview.chromium.org/10575014/diff/35001/content/browser/renderer_h...
content/browser/renderer_host/render_process_host_impl.cc:1148:
&g_context_site_process_map_.Get()[host->GetBrowserContext()];
Having a global map of BrowserContext objects -> data seems odd.  Would it be
better to user browser_context->SetUserData()? I'm not sure what the rules are
around adding more keys into the context though.

http://codereview.chromium.org/10575014/diff/35001/content/browser/renderer_h...
content/browser/renderer_host/render_process_host_impl.cc:1157: // Remove them
from the map.
I'd just merge this with the previous comment. This is getting a bit too close
to redescribing what the code says.

http://codereview.chromium.org/10575014/diff/35001/content/browser/renderer_h...
content/browser/renderer_host/render_process_host_impl.cc:1311: GURL site_url =
SiteInstanceImpl::GetSiteForURL(browser_context, url);
std::string site = SiteInstanceImpl::GetSiteForURL(browser_context, url)
    .possibly_invalid_spec();

is what is used in browsing_instance.cc.  Make them consistent here and
elsewhere?  (I personally prefer not introducing the extra var, but up to you).

http://codereview.chromium.org/10575014/diff/35001/content/browser/renderer_h...
content/browser/renderer_host/render_process_host_impl.cc:1332: (*map)[site] =
process;
What happens if the site is empty?  The process is just never recorded?

http://codereview.chromium.org/10575014/diff/35001/content/browser/site_insta...
File content/browser/site_instance_impl.cc (right):

http://codereview.chromium.org/10575014/diff/35001/content/browser/site_insta...
content/browser/site_instance_impl.cc:74: // existing process that we would use
if GetProcess were called.
GetProcess -> GetProcess()

http://codereview.chromium.org/10575014/diff/35001/content/browser/site_insta...
content/browser/site_instance_impl.cc:127: DCHECK(process_);
Should this be a CHECK?  The code can't continue sanely anyways right?

http://codereview.chromium.org/10575014/diff/35001/content/browser/site_insta...
content/browser/site_instance_impl.cc:131: // at this time, we will register it
in SetSite.)
SetSite -> SetSite().

Though I understand the parenthetical.  Where are we checking if site is set?

http://codereview.chromium.org/10575014/diff/35001/content/browser/site_insta...
File content/browser/site_instance_impl_unittest.cc (right):

http://codereview.chromium.org/10575014/diff/35001/content/browser/site_insta...
content/browser/site_instance_impl_unittest.cc:466: // The processes will be
unregistered when the RPH scoped_ptrs go away.
Do we have a way to assert these two assumptions in a test?

If there is, then maybe a separate test should be make to exercise the
unregistration code.

http://codereview.chromium.org/10575014/diff/35001/content/browser/site_insta...
content/browser/site_instance_impl_unittest.cc:515:
EXPECT_TRUE(site_instance_a1.get() != NULL);
should we assert that site_instance_a1 != site_instance_a1_2?

http://codereview.chromium.org/10575014/diff/35001/content/browser/site_insta...
content/browser/site_instance_impl_unittest.cc:530: EXPECT_NE(process_a1.get(),
process_a2_3.get());
Same question about comparing the site instances...

http://codereview.chromium.org/10575014/diff/35001/content/browser/web_conten...
File content/browser/web_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/10575014/diff/35001/content/browser/web_conten...
content/browser/web_contents/render_view_host_manager.cc:470: //
GetRelatedSiteInstance for this, which will eagerly set the site and
GetRelatedSiteInstance -> GetRelatedSiteInstance()

http://codereview.chromium.org/10575014/diff/35001/content/public/browser/ren...
File content/public/browser/render_process_host.h (right):

http://codereview.chromium.org/10575014/diff/35001/content/public/browser/ren...
content/public/browser/render_process_host.h:242: // registered using
RegisterProcessHostForSite.
RegisterProcessHostForSite -> RegisterProcessHostForSite()

[Charlie Reis, 2012-06-27 20:53:43]

Thanks!  PTAL.

http://codereview.chromium.org/10575014/diff/35001/chrome/browser/ssl/ssl_bro...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

http://codereview.chromium.org/10575014/diff/35001/chrome/browser/ssl/ssl_bro...
chrome/browser/ssl/ssl_browser_tests.cc:843: // Create a new tab in the same
process (since we are using process-per-site).
On 2012/06/27 00:26:54, awong wrote:
> This parenthetical doesn't explain enough to me.  If we were not using process
> per site, how would the setup code look different?

Using the NEW_FOREGROUND_TAB disposition will not usually keep the new tab in a
new process by default.  It will work in process-per-site mode, though.  I've
updated the comment.

http://codereview.chromium.org/10575014/diff/35001/content/browser/browsing_i...
File content/browser/browsing_instance.cc (right):

http://codereview.chromium.org/10575014/diff/35001/content/browser/browsing_i...
content/browser/browsing_instance.cc:26: SiteInstanceMap::iterator i =
site_instance_map_.find(site);
On 2012/06/27 00:26:54, awong wrote:
> How about just doing
> 
> return site_instance_map_.find(site) != site_instance_map_.end();

Done.

http://codereview.chromium.org/10575014/diff/35001/content/browser/renderer_h...
File content/browser/renderer_host/render_process_host_impl.cc (right):

http://codereview.chromium.org/10575014/diff/35001/content/browser/renderer_h...
content/browser/renderer_host/render_process_host_impl.cc:1148:
&g_context_site_process_map_.Get()[host->GetBrowserContext()];
On 2012/06/27 00:26:54, awong wrote:
> Having a global map of BrowserContext objects -> data seems odd.  Would it be
> better to user browser_context->SetUserData()? I'm not sure what the rules are
> around adding more keys into the context though.

That would be possible, but it also requires adding a new class that inherits
from SupportsUserData::Data.  At the moment, I'm just moving the global state
from BrowsingInstance to RenderProcessHostImpl.

I'm inclined to leave it as is for the moment, but I can switch it if others
prefer.

http://codereview.chromium.org/10575014/diff/35001/content/browser/renderer_h...
content/browser/renderer_host/render_process_host_impl.cc:1157: // Remove them
from the map.
On 2012/06/27 00:26:54, awong wrote:
> I'd just merge this with the previous comment. This is getting a bit too close
> to redescribing what the code says.

Done.

http://codereview.chromium.org/10575014/diff/35001/content/browser/renderer_h...
content/browser/renderer_host/render_process_host_impl.cc:1311: GURL site_url =
SiteInstanceImpl::GetSiteForURL(browser_context, url);
On 2012/06/27 00:26:54, awong wrote:
> std::string site = SiteInstanceImpl::GetSiteForURL(browser_context, url)
>     .possibly_invalid_spec();
> 
> is what is used in browsing_instance.cc.  Make them consistent here and
> elsewhere?  (I personally prefer not introducing the extra var, but up to
you).

Done.

http://codereview.chromium.org/10575014/diff/35001/content/browser/renderer_h...
content/browser/renderer_host/render_process_host_impl.cc:1332: (*map)[site] =
process;
On 2012/06/27 00:26:54, awong wrote:
> What happens if the site is empty?  The process is just never recorded?

Right.  I didn't think we would want to group all degenerate cases into the same
process.

Then again, we didn't have that check before in BrowsingInstance.  Perhaps (in
the interest of having a minimal functional change) I should leave the empty
check out.  Updated.

http://codereview.chromium.org/10575014/diff/35001/content/browser/site_insta...
File content/browser/site_instance_impl.cc (right):

http://codereview.chromium.org/10575014/diff/35001/content/browser/site_insta...
content/browser/site_instance_impl.cc:74: // existing process that we would use
if GetProcess were called.
On 2012/06/27 00:26:54, awong wrote:
> GetProcess -> GetProcess()

Done.

http://codereview.chromium.org/10575014/diff/35001/content/browser/site_insta...
content/browser/site_instance_impl.cc:127: DCHECK(process_);
On 2012/06/27 00:26:54, awong wrote:
> Should this be a CHECK?  The code can't continue sanely anyways right?

Done.

http://codereview.chromium.org/10575014/diff/35001/content/browser/site_insta...
content/browser/site_instance_impl.cc:131: // at this time, we will register it
in SetSite.)
On 2012/06/27 00:26:54, awong wrote:
> SetSite -> SetSite().
> 
> Though I understand the parenthetical.  Where are we checking if site is set?

Line 101 checks has_site_ before calling ShouldUseProcessPerSite.

http://codereview.chromium.org/10575014/diff/35001/content/browser/site_insta...
File content/browser/site_instance_impl_unittest.cc (right):

http://codereview.chromium.org/10575014/diff/35001/content/browser/site_insta...
content/browser/site_instance_impl_unittest.cc:466: // The processes will be
unregistered when the RPH scoped_ptrs go away.
On 2012/06/27 00:26:54, awong wrote:
> Do we have a way to assert these two assumptions in a test?
> 
> If there is, then maybe a separate test should be make to exercise the
> unregistration code.

Actually, we already have the first assumption tested in the
SiteInstanceDestructor test.  I've added a check for the second assumption in
TearDown, which should help a lot.

http://codereview.chromium.org/10575014/diff/35001/content/browser/site_insta...
content/browser/site_instance_impl_unittest.cc:515:
EXPECT_TRUE(site_instance_a1.get() != NULL);
On 2012/06/27 00:26:54, awong wrote:
> should we assert that site_instance_a1 != site_instance_a1_2?

Good call.  Added.

http://codereview.chromium.org/10575014/diff/35001/content/browser/site_insta...
content/browser/site_instance_impl_unittest.cc:530: EXPECT_NE(process_a1.get(),
process_a2_3.get());
On 2012/06/27 00:26:54, awong wrote:
> Same question about comparing the site instances...

Done.

http://codereview.chromium.org/10575014/diff/35001/content/browser/web_conten...
File content/browser/web_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/10575014/diff/35001/content/browser/web_conten...
content/browser/web_contents/render_view_host_manager.cc:470: //
GetRelatedSiteInstance for this, which will eagerly set the site and
On 2012/06/27 00:26:54, awong wrote:
> GetRelatedSiteInstance -> GetRelatedSiteInstance()

Done.

http://codereview.chromium.org/10575014/diff/35001/content/public/browser/ren...
File content/public/browser/render_process_host.h (right):

http://codereview.chromium.org/10575014/diff/35001/content/public/browser/ren...
content/public/browser/render_process_host.h:242: // registered using
RegisterProcessHostForSite.
On 2012/06/27 00:26:54, awong wrote:
> RegisterProcessHostForSite -> RegisterProcessHostForSite()

Done.

[awong, 2012-06-27 21:24:40]

LGTM w/ one comment about adding a comment

http://codereview.chromium.org/10575014/diff/35001/content/browser/renderer_h...
File content/browser/renderer_host/render_process_host_impl.cc (right):

http://codereview.chromium.org/10575014/diff/35001/content/browser/renderer_h...
content/browser/renderer_host/render_process_host_impl.cc:1148:
&g_context_site_process_map_.Get()[host->GetBrowserContext()];
On 2012/06/27 20:53:43, creis wrote:
> On 2012/06/27 00:26:54, awong wrote:
> > Having a global map of BrowserContext objects -> data seems odd.  Would it
be
> > better to user browser_context->SetUserData()? I'm not sure what the rules
are
> > around adding more keys into the context though.
> 
> That would be possible, but it also requires adding a new class that inherits
> from SupportsUserData::Data.  At the moment, I'm just moving the global state
> from BrowsingInstance to RenderProcessHostImpl.
> 
> I'm inclined to leave it as is for the moment, but I can switch it if others
> prefer.

sgtm

http://codereview.chromium.org/10575014/diff/35001/content/browser/renderer_h...
content/browser/renderer_host/render_process_host_impl.cc:1332: (*map)[site] =
process;
On 2012/06/27 20:53:43, creis wrote:
> On 2012/06/27 00:26:54, awong wrote:
> > What happens if the site is empty?  The process is just never recorded?
> 
> Right.  I didn't think we would want to group all degenerate cases into the
same
> process.
> 
> Then again, we didn't have that check before in BrowsingInstance.  Perhaps (in
> the interest of having a minimal functional change) I should leave the empty
> check out.  Updated.

Cool.  Also maybe a comment explaining why the behavior makes sense if
site.empty() would be helpful to future readers?

[Charlie Reis, 2012-06-27 21:42:30]

Thanks.  John, can you take a look for owner's approval?

Albert had one question about switching from a global map to using
SupportsUserData.  Which way would you prefer?

http://codereview.chromium.org/10575014/diff/35001/content/browser/renderer_h...
File content/browser/renderer_host/render_process_host_impl.cc (right):

http://codereview.chromium.org/10575014/diff/35001/content/browser/renderer_h...
content/browser/renderer_host/render_process_host_impl.cc:1332: (*map)[site] =
process;
On 2012/06/27 21:24:40, awong wrote:
> On 2012/06/27 20:53:43, creis wrote:
> > On 2012/06/27 00:26:54, awong wrote:
> > > What happens if the site is empty?  The process is just never recorded?
> > 
> > Right.  I didn't think we would want to group all degenerate cases into the
> same
> > process.
> > 
> > Then again, we didn't have that check before in BrowsingInstance.  Perhaps
(in
> > the interest of having a minimal functional change) I should leave the empty
> > check out.  Updated.
> 
> Cool.  Also maybe a comment explaining why the behavior makes sense if
> site.empty() would be helpful to future readers?

Not sure there's a clear answer, so I added a TODO comment to investigate.

[jam, 2012-06-28 00:12:52]

http://codereview.chromium.org/10575014/diff/51004/content/browser/renderer_h...
File content/browser/renderer_host/render_process_host_impl.h (right):

http://codereview.chromium.org/10575014/diff/51004/content/browser/renderer_h...
content/browser/renderer_host/render_process_host_impl.h:135: static void
UnregisterHost(int host_id, RenderProcessHost* host);
no need to pass |host| since it can be retrieved from the map?

http://codereview.chromium.org/10575014/diff/51004/content/public/browser/ren...
File content/public/browser/render_process_host.h (right):

http://codereview.chromium.org/10575014/diff/51004/content/public/browser/ren...
content/public/browser/render_process_host.h:237: static bool
ShouldUseProcessPerSite(content::BrowserContext* browser_context,
these functions appear to be only used in content, in which case they shouldn't
be exposed through the content api.  just put them in RenderProcessHostImpl?

[Charlie Reis, 2012-06-28 00:49:15]

After looking at the SupportsUserData approach, I kind of like it a bit more,
actually.  I've added that as well.

Albert and John, can you take another look?

Thanks!
Charlie

http://codereview.chromium.org/10575014/diff/51004/content/browser/renderer_h...
File content/browser/renderer_host/render_process_host_impl.h (right):

http://codereview.chromium.org/10575014/diff/51004/content/browser/renderer_h...
content/browser/renderer_host/render_process_host_impl.h:135: static void
UnregisterHost(int host_id, RenderProcessHost* host);
On 2012/06/28 00:12:52, John Abd-El-Malek wrote:
> no need to pass |host| since it can be retrieved from the map?

Good call.  Done.

http://codereview.chromium.org/10575014/diff/51004/content/public/browser/ren...
File content/public/browser/render_process_host.h (right):

http://codereview.chromium.org/10575014/diff/51004/content/public/browser/ren...
content/public/browser/render_process_host.h:237: static bool
ShouldUseProcessPerSite(content::BrowserContext* browser_context,
On 2012/06/28 00:12:52, John Abd-El-Malek wrote:
> these functions appear to be only used in content, in which case they
shouldn't
> be exposed through the content api.  just put them in RenderProcessHostImpl?

Done.

[Charlie Reis, 2012-06-28 18:14:39]

Added a small fix to test_renderer_host.cc to ensure that MockRenderProcessHosts
are deleted before their BrowserContext.

[awong, 2012-06-28 19:38:13]

http://codereview.chromium.org/10575014/diff/71001/content/test/test_renderer...
File content/test/test_renderer_host.cc (right):

http://codereview.chromium.org/10575014/diff/71001/content/test/test_renderer...
content/test/test_renderer_host.cc:148: rvh_test_enabler_.rph_factory_.reset();
Do these need to be deleted on any particular thread?

[Charlie Reis, 2012-06-28 19:41:57]

http://codereview.chromium.org/10575014/diff/71001/content/test/test_renderer...
File content/test/test_renderer_host.cc (right):

http://codereview.chromium.org/10575014/diff/71001/content/test/test_renderer...
content/test/test_renderer_host.cc:148: rvh_test_enabler_.rph_factory_.reset();
On 2012/06/28 19:38:13, awong wrote:
> Do these need to be deleted on any particular thread?

The RenderProcessHosts are created on the main test thread (see
OneClickSigninHelperTest.CanOffer for an example), so I think deleting them here
would be right.

[awong, 2012-06-28 19:44:36]

LGTM

On Thu, Jun 28, 2012 at 12:41 PM, <creis@chromium.org> wrote:

>
> http://codereview.chromium.**org/10575014/diff/71001/**
>
content/test/test_renderer_**host.cc<http://codereview.chromium.org/10575014/diff/71001/content/test/test_renderer_host.cc>
> File content/test/test_renderer_**host.cc (right):
>
> http://codereview.chromium.**org/10575014/diff/71001/**
>
content/test/test_renderer_**host.cc#newcode148<http://codereview.chromium.org/10575014/diff/71001/content/test/test_renderer_host.cc#newcode148>
> content/test/test_renderer_**host.cc:148:
> rvh_test_enabler_.rph_factory_**.reset();
> On 2012/06/28 19:38:13, awong wrote:
>
>> Do these need to be deleted on any particular thread?
>>
>
> The RenderProcessHosts are created on the main test thread (see
> OneClickSigninHelperTest.**CanOffer for an example), so I think deleting
> them here would be right.
>
>
http://codereview.chromium.**org/10575014/<http://codereview.chromium.org/105...
>

[jam, 2012-06-28 21:25:05]

lgtm

http://codereview.chromium.org/10575014/diff/71001/content/browser/renderer_h...
File content/browser/renderer_host/render_process_host_impl.h (right):

http://codereview.chromium.org/10575014/diff/71001/content/browser/renderer_h...
content/browser/renderer_host/render_process_host_impl.h:138: static bool
ShouldUseProcessPerSite(content::BrowserContext* browser_context,
nit: here and below, get rid of "content::" since this class is in the content
namespace

[Charlie Reis, 2012-06-28 21:30:19]

Thanks!  Scott, can you take a look at new_tab_ui_browsertest.cc for OWNERs
approval?

http://codereview.chromium.org/10575014/diff/71001/content/browser/renderer_h...
File content/browser/renderer_host/render_process_host_impl.h (right):

http://codereview.chromium.org/10575014/diff/71001/content/browser/renderer_h...
content/browser/renderer_host/render_process_host_impl.h:138: static bool
ShouldUseProcessPerSite(content::BrowserContext* browser_context,
On 2012/06/28 21:25:05, John Abd-El-Malek wrote:
> nit: here and below, get rid of "content::" since this class is in the content
> namespace

Done.

[sky, 2012-06-28 22:20:25]

LGTM

[commit-bot: I haz the power, 2012-06-29 15:02:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/10575014/51011

[commit-bot: I haz the power, 2012-06-29 16:34:38]

Change committed as 144911