Try to diagnose blink reinitialisation in WebView.

Try to diagnose blink reinitialisation in WebView.

WebView somehow ends up shutting down and reinitialising blink in the
same process sometimes, which isn't supported. We have no idea how to
reproduce this, so this change adds assertions that verify that we never
shut down InProcessRendererThread unless the browser is already being
shut down. WebView only exits by the process being SIGKILLed, so the
shutdown path is intended to be unreachable.

This is a temporary change that will be reverted once we've found the
stack that leads to the shutdown being initiated.

BUG=514141
Committed: https://crrev.com/58dcbf986997c2ca09b182eb6b3015de5c0e1fe9
Cr-Commit-Position: refs/heads/master@{#391207}

[Torne, 2016-04-18 14:25:11]

torne@chromium.org changed reviewers:
+ johnme@chromium.org

[Torne, 2016-04-18 14:25:11]

John, any more places you think I could usefully put this? I think it's expected
that the only way we get here is the destructor; the others are fallbacks to
catch it happening if I missed it, but probably won't produce a useful stack as
they'll crash on the thread itself..

[Torne, 2016-04-18 15:24:06]

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

[commit-bot: I haz the power, 2016-04-18 15:24:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1900513003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1900513003/20001

[commit-bot: I haz the power, 2016-04-18 17:30:51]

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

[commit-bot: I haz the power, 2016-04-18 17:30:52]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[johnme, 2016-04-19 14:25:10]

I'd suggest putting another in ChildThreadImpl::OnProcessFinalRelease (or
ChildProcess::ReleaseProcess) to get a stack trace before the async
ChildProcessHostMsg_ShutdownRequest -> ChildProcessMsg_Shutdown ping-pong (which
leads to ChildThreadImpl::OnShutdown that might quit the
InProcessRendererThread).

You might also want one in ChildThreadImpl::OnChannelError (since it uses
MessageLoop::QuitWhenIdle, it wouldn't show up directly in your other
stacktraces).

https://codereview.chromium.org/1900513003/diff/20001/content/renderer/in_pro...
File content/renderer/in_process_renderer_thread.cc (right):

https://codereview.chromium.org/1900513003/diff/20001/content/renderer/in_pro...
content/renderer/in_process_renderer_thread.cc:29:
CHECK(!base::CommandLine::ForCurrentProcess()->HasSwitch(
This check triggers frequently in content_browsertests:
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...

[Torne, 2016-04-19 14:36:24]

On 2016/04/19 14:25:10, johnme wrote:
> I'd suggest putting another in ChildThreadImpl::OnProcessFinalRelease (or
> ChildProcess::ReleaseProcess) to get a stack trace before the async
> ChildProcessHostMsg_ShutdownRequest -> ChildProcessMsg_Shutdown ping-pong
(which
> leads to ChildThreadImpl::OnShutdown that might quit the
> InProcessRendererThread).
> 
> You might also want one in ChildThreadImpl::OnChannelError (since it uses
> MessageLoop::QuitWhenIdle, it wouldn't show up directly in your other
> stacktraces).

Will look into these, thanks.

>
https://codereview.chromium.org/1900513003/diff/20001/content/renderer/in_pro...
> File content/renderer/in_process_renderer_thread.cc (right):
> 
>
https://codereview.chromium.org/1900513003/diff/20001/content/renderer/in_pro...
> content/renderer/in_process_renderer_thread.cc:29:
> CHECK(!base::CommandLine::ForCurrentProcess()->HasSwitch(
> This check triggers frequently in content_browsertests:
>
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...

Yes, unfortunately. It's probably unavoidable that this change *will* cause test
failures, because tests are allowed to rely on exactly the thing I'm trying to
prevent. I could come up with some hacky way to not run this check in tests..

[Torne, 2016-04-28 15:59:11]

Description was changed from

==========
Try to diagnose blink reinitialisation in WebView.

WebView somehow ends up shutting down and reinitialising blink in the
same process sometimes, which isn't supported. We have no idea how to
reproduce this, so this change adds assertions that verify that in
single-process mode we never shut down InProcessRendererThread. WebView
only exits by the process being SIGKILLed, so the shutdown path is
intended to be unreachable.

The kSingleProcess switch checks are probably redundant when in
InProcessRendererThread (which isn't used by multiprocess mode) but just
in case.

This is a temporary change that will be reverted once we've found the
stack that leads to the shutdown being initiated.

BUG=514141
==========

to

==========
Try to diagnose blink reinitialisation in WebView.

WebView somehow ends up shutting down and reinitialising blink in the
same process sometimes, which isn't supported. We have no idea how to
reproduce this, so this change adds assertions that verify that we never
shut down InProcessRendererThread unless the browser is already being
shut down. WebView only exits by the process being SIGKILLed, so the
shutdown path is intended to be unreachable.

This is a temporary change that will be reverted once we've found the
stack that leads to the shutdown being initiated.

BUG=514141
==========

[Torne, 2016-04-28 16:00:24]

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

[commit-bot: I haz the power, 2016-04-28 16:00:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1900513003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1900513003/40001

[Torne, 2016-04-28 16:00:40]

Okay, this version should actually avoid breaking tests. Sending for another dry
run to see if I'm right ;)

[commit-bot: I haz the power, 2016-04-28 16:51:52]

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

[commit-bot: I haz the power, 2016-04-28 16:51:53]

Dry run: This issue passed the CQ dry run.

[Torne, 2016-04-29 11:35:52]

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

[Torne, 2016-04-29 11:35:53]

Adding jochen@ for content OWNERS. Some context if you don't want to read
through the (long) bug: webview (which runs in single-process) is fairly
frequently crashing in the field because the renderer is being shut down and
then restarted inside a single process, which isn't supported and hits a Blink
assertion. However the stack for the assertion failure is just the renderer
thread, and so we don't know *why* the renderer thread was asked to stop. The
goal of this CL is to crash the *browser* instead if it ever asks the renderer
to stop when the browser isn't also stopping (which happens in tests, but never
in real android builds which don't shut down cleanly ever).

This version now doesn't regress any tests, and catches just the death of
InProcessRendererThread, which I think should be sufficient. It uses just a bool
to know whether the browser main loop is shutting down, and permits the renderer
to go away if so (which makes all the tests pass). I know defining a global for
this is gross, but since the two parts are in browser vs renderer and it only
has to work in single process mode, doing something more complicated doesn't
seem worth it for a temporary CL. It's also all ifdef'ed just for OS_ANDROID
(and chrome for android doesn't run in single process, so shouldn't be
affected).

So, the plan would be to land this in trunk now to get some android canary/dev
coverage to make sure I haven't broken anything, ship in the early M52 webview
betas to try to get this assertion to fail for webview users in the field so
that we get crash stacks reported, and once I've got some stacks, revert it in
M52 before stable, and also revert it in trunk.

What do you think?

[johnme, 2016-04-29 12:55:09]

lgtm to catch the crash.

Might want a TODO to remind you to revert this somewhat hacky code later :)

Might also, in a separate patch, want to make sure that
ChildThreadImpl::OnChannelError crashes the browser process in single process
mode.

[jochen (gone - plz use gerrit), 2016-05-02 17:06:26]

lgtm

[Torne, 2016-05-03 11:20:03]

The CQ bit was checked by torne@chromium.org

[commit-bot: I haz the power, 2016-05-03 11:20:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1900513003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1900513003/40001

[Torne, 2016-05-03 12:03:10]

https://codereview.chromium.org/1942273002 crashes in single-process mode in
RenderThreadImpl::OnChannelError, to try and catch this other possibility. This
seems like a sensible change to commit permanently and for all OSes, though, so
I'm doing it seperately. Will see if it breaks any tests :)

[commit-bot: I haz the power, 2016-05-03 12:18:28]

Description was changed from

==========
Try to diagnose blink reinitialisation in WebView.

WebView somehow ends up shutting down and reinitialising blink in the
same process sometimes, which isn't supported. We have no idea how to
reproduce this, so this change adds assertions that verify that we never
shut down InProcessRendererThread unless the browser is already being
shut down. WebView only exits by the process being SIGKILLed, so the
shutdown path is intended to be unreachable.

This is a temporary change that will be reverted once we've found the
stack that leads to the shutdown being initiated.

BUG=514141
==========

to

==========
Try to diagnose blink reinitialisation in WebView.

WebView somehow ends up shutting down and reinitialising blink in the
same process sometimes, which isn't supported. We have no idea how to
reproduce this, so this change adds assertions that verify that we never
shut down InProcessRendererThread unless the browser is already being
shut down. WebView only exits by the process being SIGKILLed, so the
shutdown path is intended to be unreachable.

This is a temporary change that will be reverted once we've found the
stack that leads to the shutdown being initiated.

BUG=514141
==========

[commit-bot: I haz the power, 2016-05-03 12:18:29]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-03 12:20:16]

Description was changed from

==========
Try to diagnose blink reinitialisation in WebView.

WebView somehow ends up shutting down and reinitialising blink in the
same process sometimes, which isn't supported. We have no idea how to
reproduce this, so this change adds assertions that verify that we never
shut down InProcessRendererThread unless the browser is already being
shut down. WebView only exits by the process being SIGKILLed, so the
shutdown path is intended to be unreachable.

This is a temporary change that will be reverted once we've found the
stack that leads to the shutdown being initiated.

BUG=514141
==========

to

==========
Try to diagnose blink reinitialisation in WebView.

WebView somehow ends up shutting down and reinitialising blink in the
same process sometimes, which isn't supported. We have no idea how to
reproduce this, so this change adds assertions that verify that we never
shut down InProcessRendererThread unless the browser is already being
shut down. WebView only exits by the process being SIGKILLed, so the
shutdown path is intended to be unreachable.

This is a temporary change that will be reverted once we've found the
stack that leads to the shutdown being initiated.

BUG=514141

Committed: https://crrev.com/58dcbf986997c2ca09b182eb6b3015de5c0e1fe9
Cr-Commit-Position: refs/heads/master@{#391207}
==========

[commit-bot: I haz the power, 2016-05-03 12:20:18]

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