Stop shutting down the message loop when the renderer stops

Stop shutting down the message loop when the renderer stops

This CL stops processing remaining tasks at the end of the shutdown sequence.
This has been a source of use-after-free crashes because base::RunLoop().RunUntilIdle()
runs arbitrary tasks after many things have been shut down.
main_message_loop_.reset() was also problematic because it can trigger connection error handlers of Mojo,
which called code in Blink.

BUG=639244
Committed: https://crrev.com/e0817cfbfc3c7f8b13aa2ffc99c31f336ba1d29a
Cr-Commit-Position: refs/heads/master@{#413959}

[haraken, 2016-08-22 23:53:07]

The CQ bit was checked by haraken@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 23:53:57]

Dry run: 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-08-23 01:04:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-23 01:04:17]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-08-23 11:34:10]

Description was changed from

==========
Stop shutting down the message loop when the renderer stops

BUG=639244
==========

to

==========
Stop shutting down the message loop when the renderer stops

This CL stops processing remaining tasks at the end of the shutdown sequence.
This has been a source of use-after-free crashes because
base::RunLoop().RunUntilIdle()
runs arbitrary tasks after many things have been shut down.
main_message_loop_.reset() was also problematic because it can trigger
connection error handlers of Mojo,
which called code in Blink.

BUG=639244
==========

[haraken, 2016-08-23 11:34:10]

haraken@chromium.org changed reviewers:
+ jochen@chromium.org

[haraken, 2016-08-23 11:34:18]

PTAL

[jochen (gone - plz use gerrit), 2016-08-23 12:05:37]

hum, won't the render_thread_impl dtor kill the message loop anyways?

how does that work in single-process mode?

Maybe jam@ should have a look at this

[haraken, 2016-08-23 12:19:13]

haraken@chromium.org changed reviewers:
+ jam@chromium.org, torne@chromium.org

[haraken, 2016-08-23 12:19:14]

On 2016/08/23 12:05:37, jochen wrote:
> hum, won't the render_thread_impl dtor kill the message loop anyways?

Right. I begin to think that removing message_loop.reset() is not the point of
this CL. The destructor has triggered Mojo's connection error handlers and
caused use-after-frees by touching Blink objects, but the issue was already
resolved by us removing blink::shutdown.

So the point of this CL is to remove RunUntilIdle(), which may run arbitrary
tasks after many things have shut down.

> 
> how does that work in single-process mode?

At least, WebView never enters RenderThreadImpl::Shutdown.

+torne@

> 
> Maybe jam@ should have a look at this

+jam: PTAL :)

[Torne, 2016-08-23 12:45:31]

On 2016/08/23 12:19:14, haraken wrote:
> On 2016/08/23 12:05:37, jochen wrote:
> > hum, won't the render_thread_impl dtor kill the message loop anyways?
> 
> Right. I begin to think that removing message_loop.reset() is not the point of
> this CL. The destructor has triggered Mojo's connection error handlers and
> caused use-after-frees by touching Blink objects, but the issue was already
> resolved by us removing blink::shutdown.
> 
> So the point of this CL is to remove RunUntilIdle(), which may run arbitrary
> tasks after many things have shut down.
> 
> > 
> > how does that work in single-process mode?
> 
> At least, WebView never enters RenderThreadImpl::Shutdown.
> 
> +torne@

It's never *intended* to enter RenderThreadImpl::Shutdown but I'm not sure that
it doesn't: crbug.com/514141 continues to exist in the field and we are, indeed,
shutting down and then attempting to restart the render thread in single process
mode in webview in some cases (which crashes, but I have no idea how we got
there yet). Any help welcomed :)

> > 
> > Maybe jam@ should have a look at this
> 
> +jam: PTAL :)

[haraken, 2016-08-23 12:56:04]

On 2016/08/23 12:45:31, Torne wrote:
> On 2016/08/23 12:19:14, haraken wrote:
> > On 2016/08/23 12:05:37, jochen wrote:
> > > hum, won't the render_thread_impl dtor kill the message loop anyways?
> > 
> > Right. I begin to think that removing message_loop.reset() is not the point
of
> > this CL. The destructor has triggered Mojo's connection error handlers and
> > caused use-after-frees by touching Blink objects, but the issue was already
> > resolved by us removing blink::shutdown.
> > 
> > So the point of this CL is to remove RunUntilIdle(), which may run arbitrary
> > tasks after many things have shut down.
> > 
> > > 
> > > how does that work in single-process mode?
> > 
> > At least, WebView never enters RenderThreadImpl::Shutdown.
> > 
> > +torne@
> 
> It's never *intended* to enter RenderThreadImpl::Shutdown but I'm not sure
that
> it doesn't: crbug.com/514141 continues to exist in the field and we are,
indeed,
> shutting down and then attempting to restart the render thread in single
process
> mode in webview in some cases (which crashes, but I have no idea how we got
> there yet). Any help welcomed :)

However, that would be totally a different issue from this CL. Whether we have
RunUntilIdle() or not won't change the situation :)

> 
> > > 
> > > Maybe jam@ should have a look at this
> > 
> > +jam: PTAL :)

[Torne, 2016-08-23 13:28:18]

On 2016/08/23 12:56:04, haraken wrote:
> On 2016/08/23 12:45:31, Torne wrote:
> > On 2016/08/23 12:19:14, haraken wrote:
> > > On 2016/08/23 12:05:37, jochen wrote:
> > > > hum, won't the render_thread_impl dtor kill the message loop anyways?
> > > 
> > > Right. I begin to think that removing message_loop.reset() is not the
point
> of
> > > this CL. The destructor has triggered Mojo's connection error handlers and
> > > caused use-after-frees by touching Blink objects, but the issue was
already
> > > resolved by us removing blink::shutdown.
> > > 
> > > So the point of this CL is to remove RunUntilIdle(), which may run
arbitrary
> > > tasks after many things have shut down.
> > > 
> > > > 
> > > > how does that work in single-process mode?
> > > 
> > > At least, WebView never enters RenderThreadImpl::Shutdown.
> > > 
> > > +torne@
> > 
> > It's never *intended* to enter RenderThreadImpl::Shutdown but I'm not sure
> that
> > it doesn't: crbug.com/514141 continues to exist in the field and we are,
> indeed,
> > shutting down and then attempting to restart the render thread in single
> process
> > mode in webview in some cases (which crashes, but I have no idea how we got
> > there yet). Any help welcomed :)
> 
> However, that would be totally a different issue from this CL. Whether we have
> RunUntilIdle() or not won't change the situation :)

Probably, but I'd really like to figure out why nonetheless ;p

> > 
> > > > 
> > > > Maybe jam@ should have a look at this
> > > 
> > > +jam: PTAL :)

[jam, 2016-08-23 17:34:55]

On 2016/08/23 12:19:14, haraken wrote:
> On 2016/08/23 12:05:37, jochen wrote:
> > hum, won't the render_thread_impl dtor kill the message loop anyways?
> 
> Right. I begin to think that removing message_loop.reset() is not the point of
> this CL. The destructor has triggered Mojo's connection error handlers and
> caused use-after-frees by touching Blink objects, but the issue was already
> resolved by us removing blink::shutdown.
> 
> So the point of this CL is to remove RunUntilIdle(), which may run arbitrary
> tasks after many things have shut down.

Agreed this is what will make the difference since the destructor is going to
run right after anyways. I think also removing the reset call, while not making
a difference, is fine to do.

lgtm

> 
> > 
> > how does that work in single-process mode?
> 
> At least, WebView never enters RenderThreadImpl::Shutdown.
> 
> +torne@
> 
> > 
> > Maybe jam@ should have a look at this
> 
> +jam: PTAL :)

[haraken, 2016-08-24 02:01:16]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-08-24 02:02:39]

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-08-24 02:58:33]

Description was changed from

==========
Stop shutting down the message loop when the renderer stops

This CL stops processing remaining tasks at the end of the shutdown sequence.
This has been a source of use-after-free crashes because
base::RunLoop().RunUntilIdle()
runs arbitrary tasks after many things have been shut down.
main_message_loop_.reset() was also problematic because it can trigger
connection error handlers of Mojo,
which called code in Blink.

BUG=639244
==========

to

==========
Stop shutting down the message loop when the renderer stops

This CL stops processing remaining tasks at the end of the shutdown sequence.
This has been a source of use-after-free crashes because
base::RunLoop().RunUntilIdle()
runs arbitrary tasks after many things have been shut down.
main_message_loop_.reset() was also problematic because it can trigger
connection error handlers of Mojo,
which called code in Blink.

BUG=639244
==========

[commit-bot: I haz the power, 2016-08-24 02:58:34]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-08-24 03:00:50]

Description was changed from

==========
Stop shutting down the message loop when the renderer stops

This CL stops processing remaining tasks at the end of the shutdown sequence.
This has been a source of use-after-free crashes because
base::RunLoop().RunUntilIdle()
runs arbitrary tasks after many things have been shut down.
main_message_loop_.reset() was also problematic because it can trigger
connection error handlers of Mojo,
which called code in Blink.

BUG=639244
==========

to

==========
Stop shutting down the message loop when the renderer stops

This CL stops processing remaining tasks at the end of the shutdown sequence.
This has been a source of use-after-free crashes because
base::RunLoop().RunUntilIdle()
runs arbitrary tasks after many things have been shut down.
main_message_loop_.reset() was also problematic because it can trigger
connection error handlers of Mojo,
which called code in Blink.

BUG=639244

Committed: https://crrev.com/e0817cfbfc3c7f8b13aa2ffc99c31f336ba1d29a
Cr-Commit-Position: refs/heads/master@{#413959}
==========

[commit-bot: I haz the power, 2016-08-24 03:00:52]

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

[haraken, 2016-09-05 00:30:42]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2312583002/ by haraken@chromium.org.

The reason for reverting is: I'll revert r413430 and its dependent CLs because
r413430 caused issue 642072.

The problem is that we cannot simply remove blink::shutdown because the
following scenario can happen:

1) blink::shutdown is not called. Workers are still running.
2) RenderThreadImpl gets destructed. MessageLoop gets destructed.
3) The workers may access the RenderThreadImpl and MessageLoop.

To fix the problem, we need to call ProcessDied() and forcibly kill the renderer
process at the end of RenderThreadImpl::Shutdown().
.