Check that all RenderProcessHost(s) are terminated.

Check that all RenderProcessHost(s) are terminated.

BUG=608049
Committed: https://crrev.com/3d47eced8c022d3fd666c005c7a8b3f26f8c773e
Cr-Commit-Position: refs/heads/master@{#405320}

[alokp, 2016-07-08 17:30:14]

alokp@chromium.org changed reviewers:
+ dalecurtis@chromium.org, jam@chromium.org

[alokp, 2016-07-08 17:30:15]

https://codereview.chromium.org/2133813002/diff/1/content/browser/browser_mai...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2133813002/diff/1/content/browser/browser_mai...
content/browser/browser_main_loop.cc:1091:
CHECK(!RenderProcessHostImpl::HasAnyConnection());
jam@: Is this a valid assumption?

We are hitting a CHECK in AudioManager destructor which verifies that no audio
output stream outlives AudioManager. The streams owned by RenderProcessHostImpl
are released when IPC channel is reset. From the crash reports it seems that we
have instances of RPHI with valid IPC channel.

We explicitly cleanup other child process during shutdown:
https://cs.chromium.org/chromium/src/content/browser/browser_process_sub_thre...

But I did not find anything for renderer processes. Should I expect all
instances of RPHI be destroyed or at least disconnected by now?

[jam, 2016-07-09 02:52:10]

which member of RenderProcessHost specifically are you referring to
(audio_renderer_host_?) ?

For renderers, on browser shutdown we close all tabs which will lead to the
process host objects being destructed through its ref counting. But I don't know
if we have assumptions that the channel closed notification must have been
received before the main thread exits.

The referenced bug mentions fast shutdown. Perhaps if it's audio_renderer_host_,
it needs to be reset in RPHI::CleanUp?

[alokp, 2016-07-09 06:46:22]

On 2016/07/09 02:52:10, jam wrote:
> which member of RenderProcessHost specifically are you referring to
> (audio_renderer_host_?) ?

Both - audio_renderer_host_ and audio_input_renderer_host_.
 
> For renderers, on browser shutdown we close all tabs which will lead to the
> process host objects being destructed through its ref counting. But I don't
know
> if we have assumptions that the channel closed notification must have been
> received before the main thread exits.
> 
> The referenced bug mentions fast shutdown. Perhaps if it's
audio_renderer_host_,
> it needs to be reset in RPHI::CleanUp?

I suspect that IPC channel is not getting reset because of this condition:
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_pro...

If the IPC channel gets destroyed it will post
ChannelProxy::Context::OnChannelClosed task to the IO thread which will run when
IO thread is shutdown.

[jam, 2016-07-12 20:03:00]

Sorry for the delay, I just saw this. Please feel free to ping me if I take more
than  a few hours.

On 2016/07/09 06:46:22, alokp wrote:
> On 2016/07/09 02:52:10, jam wrote:
> > which member of RenderProcessHost specifically are you referring to
> > (audio_renderer_host_?) ?
> 
> Both - audio_renderer_host_ and audio_input_renderer_host_.
>  
> > For renderers, on browser shutdown we close all tabs which will lead to the
> > process host objects being destructed through its ref counting. But I don't
> know
> > if we have assumptions that the channel closed notification must have been
> > received before the main thread exits.
> > 
> > The referenced bug mentions fast shutdown. Perhaps if it's
> audio_renderer_host_,
> > it needs to be reset in RPHI::CleanUp?
> 
> I suspect that IPC channel is not getting reset because of this condition:
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_pro...

At shutdown, we should have already removed all tabs (which would get listeners_
to be empty) and workers (worker_ref_count_).

I still think we need to understand this issue better. Even if this cl lands and
the check fires, it won't help identify the culprit of what's keeping the
objects alive.

> 
> If the IPC channel gets destroyed it will post
> ChannelProxy::Context::OnChannelClosed task to the IO thread which will run
when
> IO thread is shutdown.

[alokp, 2016-07-13 06:10:42]

Description was changed from

==========
Check that all RenderProcessHost(s) have released IPC channel.

BUG=608049
==========

to

==========
Check that all RenderProcessHost(s) are terminated.

BUG=608049
==========

[alokp, 2016-07-13 06:11:21]

Description was changed from

==========
Check that all RenderProcessHost(s) are terminated.

BUG=608049
==========

to

==========
Check that all RenderProcessHost(s) are terminated.

BUG=608049
==========

[alokp, 2016-07-13 06:15:48]

> At shutdown, we should have already removed all tabs (which would get
listeners_
> to be empty) and workers (worker_ref_count_).
> 
> I still think we need to understand this issue better. Even if this cl lands
and
> the check fires, it won't help identify the culprit of what's keeping the
> objects alive.

Good point. I have added more specific CHECKs in the latest patch. PTAL.

[jam, 2016-07-13 21:08:09]

On 2016/07/13 06:15:48, alokp wrote:
> > At shutdown, we should have already removed all tabs (which would get
> listeners_
> > to be empty) and workers (worker_ref_count_).
> > 
> > I still think we need to understand this issue better. Even if this cl lands
> and
> > the check fires, it won't help identify the culprit of what's keeping the
> > objects alive.
> 
> Good point. I have added more specific CHECKs in the latest patch. PTAL.

ok to add this temporarily, please remove once we get data. lgtm

[DaleCurtis, 2016-07-13 21:09:51]

defer to jam@, but lgtm as a non-owner

[alokp, 2016-07-13 21:21:39]

The CQ bit was checked by alokp@chromium.org

[alokp, 2016-07-13 21:22:10]

> ok to add this temporarily, please remove once we get data. lgtm

ACK.

[commit-bot: I haz the power, 2016-07-13 21:22:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-13 22:47:02]

Description was changed from

==========
Check that all RenderProcessHost(s) are terminated.

BUG=608049
==========

to

==========
Check that all RenderProcessHost(s) are terminated.

BUG=608049
==========

[commit-bot: I haz the power, 2016-07-13 22:47:03]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-13 22:47:30]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 22:50:19]

Description was changed from

==========
Check that all RenderProcessHost(s) are terminated.

BUG=608049
==========

to

==========
Check that all RenderProcessHost(s) are terminated.

BUG=608049

Committed: https://crrev.com/3d47eced8c022d3fd666c005c7a8b3f26f8c773e
Cr-Commit-Position: refs/heads/master@{#405320}
==========

[commit-bot: I haz the power, 2016-07-13 22:50:21]

Patchset 3 (id:??) landed as
https://crrev.com/3d47eced8c022d3fd666c005c7a8b3f26f8c773e
Cr-Commit-Position: refs/heads/master@{#405320}

[alokp, 2016-07-21 20:01:09]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2169103002/ by alokp@chromium.org.

The reason for reverting is: This was temporarily added to collect data for
diagnosing crbug.com/608049. We have enough data points now to study. This can
now be reverted.

BUG=628192.