Prevent unused normal SiteInstances from being used for extensions.

Prevent unused normal SiteInstances from being used for extensions.
Instead, create a new SiteInstance with the appropriate process type.

BUG=43448
TEST=RenderProcessHostTest.ProcessOverflow
TEST=SessionRestoreUITest.ShareProcessesOnRestore

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

[Charlie Reis, 2011-02-08 03:54:41]

I still think we need the refactoring described in the bugs listed as blocking
43448, but I think this smaller change can prevent extension/WebUI URLs from
ending up in a TYPE_NORMAL process.

The downside is that we'll still create a normal process and immediately throw
it away in the case of the Bookmark Manager or similar extensions, but at least
those URLs won't end up in a normal process where they don't work.

[brettw, 2011-02-09 07:18:47]

http://codereview.chromium.org/6312199/diff/2001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/6312199/diff/2001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_host_manager.cc:387: if
(WebUIFactory::UseWebUIForURL(profile, dest_url))
Just to be sure, this doesn't affect "normal" new tab page WebUI loads does it?
I'm a bit confused how your bookmark manager bug will be different than the new
tab page, and we certainly don't want to be discarding processes in that case.

[Charlie Reis, 2011-02-09 07:25:39]

http://codereview.chromium.org/6312199/diff/2001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/6312199/diff/2001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_host_manager.cc:387: if
(WebUIFactory::UseWebUIForURL(profile, dest_url))
On 2011/02/09 07:18:47, brettw wrote:
> Just to be sure, this doesn't affect "normal" new tab page WebUI loads does
it?
> I'm a bit confused how your bookmark manager bug will be different than the
new
> tab page, and we certainly don't want to be discarding processes in that case.

The "normal" NTP case will almost always find an existing SiteInstance in line
381.  I believe this is true even if no NTPs are currently open, as long as one
has been created since browser startup.  (That's what I saw when debugging.)

The bookmark manager is different because it's rarely opened.  Admittedly, it is
possible to get the NTP to hit this logic the first time you view an NTP, but
we'll usually get that out of the way at startup.

[brettw, 2011-02-09 07:31:53]

http://codereview.chromium.org/6312199/diff/2001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/6312199/diff/2001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_host_manager.cc:387: if
(WebUIFactory::UseWebUIForURL(profile, dest_url))
The startup case is what worries me the most since it looks like we might
discard a process if this happens? Is there any possibility of creating a
renderer for the NTP and then discarding it and creating a new "WebUI" one?

[Charlie Reis, 2011-02-09 18:04:07]

http://codereview.chromium.org/6312199/diff/2001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/6312199/diff/2001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_host_manager.cc:387: if
(WebUIFactory::UseWebUIForURL(profile, dest_url))
On 2011/02/09 07:31:53, brettw wrote:
> The startup case is what worries me the most since it looks like we might
> discard a process if this happens? Is there any possibility of creating a
> renderer for the NTP and then discarding it and creating a new "WebUI" one?

Hmm, yes, that's unfortunate.

Our current behavior is still incorrect-- if the NTP isn't created until we hit
the process limit, it can end up sharing a process with normal pages.  That
process is granted WebUI privileges so that the NTP works.

It would really be better if the NTP case set the site on the SiteInstance much
earlier, so that we could use the current process without fear of having it
shared with normal tabs, and thus not throw it away here.

Let me take a look at browser_navigator.cc to see if that's feasible for WebUI
pages.

[Charlie Reis, 2011-02-18 01:53:38]

Ok, take two.  For WebUI URLs, we now create a SiteInstance with the correct
site up front, ensuring that we can use it for the NTP without wasting a
process.

The only place this has problems is if the WebUI URL redirects before the actual
navigation.  The Bookmark Manager happens to do that-- we create a process for
chrome://bookmarks and then do a process swap when it actually navigates to
chrome-extension://asdf.  The Bookmark Manager won't end up in the wrong type of
process, though, and it's not as common as the NTP issue with the previous
patch, so I think it's worthwhile.

My plan is to follow up with a CL that gives SiteInstance a type, plus a
CreateSiteInstanceForType factory method.  (RenderViewHostManager will be able
to verify the type is correct when navigating.)  That will let us avoid setting
the site too early (in browser_navigator), saving us the extra process in the
Bookmark Manager case.

Thoughts?

[brettw, 2011-02-18 23:21:37]

LGTM