Don't shutdown render "process" in single process mode

Don't shutdown render "process" in single process mode

This avoids races, and is no more overhead than the other many
long-lived browser threads that are kept about

BUG=158112
TEST=ran "chrome --single-process" on linux, open & close tabs.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170644

[jam, 2012-10-29 15:25:52]

why is cleanup getting called in single process mode if you're not exiting? that
seems like the cause of the bug, which this looks like it's masking over.

[joth, 2012-10-29 15:47:42]

AIUI if an embedding app creates a WebContents instance, then destroys it,
then some minutes later creates a new one. In the meantime, the render
process would have (attempted to) shutdown.
Should that scenario already be handled somewhere?


On 29 October 2012 15:25, <jam@chromium.org> wrote:

> why is cleanup getting called in single process mode if you're not
> exiting? that
> seems like the cause of the bug, which this looks like it's masking over.
>
>
https://codereview.chromium.**org/11314014/<https://codereview.chromium.org/1...
>
> --
>
>
>

[jam, 2012-10-29 19:23:14]

I see. perhaps Cleanup() shouldn't be called at all, i.e. the check should be in
RenderProcessHostImpl::Release

[joth, 2012-10-29 20:01:33]

That means it would need duplicating in
RenderProcessHostImpl::OnDumpHandlesDone()
(and anywhere else that might call it in future).
Alternatively, as the Cleanup() call is already a no-op unless all widgets
have been removed, we could rename it to MaybeCleanup() ?





On 29 October 2012 19:23, <jam@chromium.org> wrote:

> I see. perhaps Cleanup() shouldn't be called at all, i.e. the check should
> be in
> RenderProcessHostImpl::Release
>
>
https://codereview.chromium.**org/11314014/<https://codereview.chromium.org/1...
>

[jam, 2012-10-29 23:54:42]

On 2012/10/29 20:01:33, joth wrote:
> That means it would need duplicating in
> RenderProcessHostImpl::OnDumpHandlesDone()
> (and anywhere else that might call it in future).

not really, since OnDumpHandlesDone is called in response to an IPC sent in
Release(). so you only have to do it in Release, at line 1128
 
> Alternatively, as the Cleanup() call is already a no-op unless all widgets
> have been removed, we could rename it to MaybeCleanup() ?
> 
> 
> 
> 
> 
> On 29 October 2012 19:23, <mailto:jam@chromium.org> wrote:
> 
> > I see. perhaps Cleanup() shouldn't be called at all, i.e. the check should
> > be in
> > RenderProcessHostImpl::Release
> >
> >
>
https://codereview.chromium.**org/11314014/%3Chttps://codereview.chromium.org...>
> >

[joth, 2012-11-14 03:11:42]

Moved the check to Release()

Thanks

[joth, 2012-11-27 01:38:13]

John: Any other comments ?

Thanks

[joth, 2012-11-29 23:00:24]

https://codereview.chromium.org/11314014/diff/27002/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/11314014/diff/27002/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:1487: if
(pending_views_ || num_active_views > 1 || run_renderer_in_process())
I did consider doing this check in render process instead, but
ChildThread::OnProcessFinalRelease specifically comments the protocol is to
always defer to the host (browser) opinion on this, and also, at this point we
only want to alter the renderer behavior not all possible child processes.

[jam, 2012-11-30 23:54:16]

lgtm

[commit-bot: I haz the power, 2012-11-30 23:56:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joth@chromium.org/11314014/27002

[commit-bot: I haz the power, 2012-12-01 03:18:11]

Change committed as 170644

[Jeffrey Yasskin, 2012-12-14 22:10:51]

https://chromiumcodereview.appspot.com/11314014/diff/27002/content/browser/re...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://chromiumcodereview.appspot.com/11314014/diff/27002/content/browser/re...
content/browser/renderer_host/render_process_host_impl.cc:1130: // Keep the one
renderer thread around forever in single process mode.
I'm now seeing a DCHECK-failure when running `xvfb-run out/Debug/browser_tests
--single-process --gtest_filter=DeclarativeApiTest.DeclarativeApi` at
chrome/browser/profiles/profile_destroyer.cc:40: "DCHECK(hosts.empty() ||
profile->IsOffTheRecord());" It showed up about the time this CL was committed,
but I haven't rolled back to be sure. Any suggestions for how to fix it?

[joth, 2012-12-14 22:34:12]

ah sorry for the inconvenience in having to track this down!.

https://chromiumcodereview.appspot.com/11314014/diff/27002/content/browser/re...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://chromiumcodereview.appspot.com/11314014/diff/27002/content/browser/re...
content/browser/renderer_host/render_process_host_impl.cc:1130: // Keep the one
renderer thread around forever in single process mode.
On 2012/12/14 22:10:51, Jeffrey Yasskin wrote:
> I'm now seeing a DCHECK-failure when running `xvfb-run out/Debug/browser_tests
> --single-process --gtest_filter=DeclarativeApiTest.DeclarativeApi` at
> chrome/browser/profiles/profile_destroyer.cc:40: "DCHECK(hosts.empty() ||
> profile->IsOffTheRecord());" It showed up about the time this CL was
committed,
> but I haven't rolled back to be sure. Any suggestions for how to fix it?

Good question. Most of the logic in the DestroyProfileWhenAppropriate() function
is incorrect for single process mode, as single process does not support off the
record. (It cannot: it only supports one profile as there's only one renderer).
Also 'render' process shutdown was never reliable in single process mode (it was
race prone). The simplest 'solution' (?) maybe to add "if
(CommandLine::HasSwitch(kSingleProcess)) return;" prior to that DCHECK. (i.e.
leak the profile too when running in unsupported mdoe) .. I haven't tried that
though, maybe something else will break...

[Jeffrey Yasskin, 2012-12-14 23:40:36]

On Fri, Dec 14, 2012 at 2:34 PM,  <joth@chromium.org> wrote:
> ah sorry for the inconvenience in having to track this down!.

No worries; --single-process doesn't have a continuous builder, so
this sort of thing is bound to happen from time to time. It wasn't
that hard to find, either.