Stop calling blink::shutdown

Stop calling blink::shutdown

RenderThreadImpl::Shutdown has been trying to shut down Blink and V8 gracefully,
but the graceful shutdown has caused tons of use-after-free bugs
(and many engineers has spent lots of time fixing ordering issues around the shutdown).

As discussed in blink-dev@ (https://groups.google.com/a/chromium.org/d/topic/blink-dev/kk4VX0xRB7I/discussion)
and platform-architecture-dev@ (https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/Zc12k91NTFk/discussion),
there is no reason we have to shut down the renderer gracefully.
It's just causing use-after-free bugs and wasting performance.
Hence, this CL stops calling blink::shutdown, which had been
shutting down *some things* in Blink and V8 gracefully.
(Remember that blink::shutdown hadn't been shutting down everything;
a lot of objects in Blink and V8 had already been left as is without getting destructed.)

Ideally we should just call ProcessDied() at an earlier stage of
RenderThreadImpl::Shutdown(), but I'd like to defer the change to a separate CL.

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

[haraken, 2016-08-17 04:23:13]

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

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

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-17 04:49:42]

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

[commit-bot: I haz the power, 2016-08-17 04:49:43]

Dry run: Exceeded global retry quota

[haraken, 2016-08-17 04:58:27]

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

[commit-bot: I haz the power, 2016-08-17 04:58:36]

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-17 05:01:30]

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

[commit-bot: I haz the power, 2016-08-17 05:01:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[haraken, 2016-08-17 06:11:29]

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

[commit-bot: I haz the power, 2016-08-17 06:11:52]

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-17 06:43:46]

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

[commit-bot: I haz the power, 2016-08-17 06:43:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[haraken, 2016-08-17 07:41:12]

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

[commit-bot: I haz the power, 2016-08-17 07:41:27]

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-17 08:14:54]

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

[commit-bot: I haz the power, 2016-08-17 08:14:54]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[haraken, 2016-08-17 08:34:00]

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

[commit-bot: I haz the power, 2016-08-17 08:34:18]

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-17 08:58:47]

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

[commit-bot: I haz the power, 2016-08-17 08:58:48]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[haraken, 2016-08-17 11:47:42]

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

[haraken, 2016-08-17 11:47:43]

tzik@: Feel free to take over this CL if you want.

[haraken, 2016-08-18 09:17:04]

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

[commit-bot: I haz the power, 2016-08-18 09:17:16]

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-18 11:02:20]

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

[commit-bot: I haz the power, 2016-08-18 11:02:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[haraken, 2016-08-18 13:29:44]

PS5 passes tests except linux asan :) Getting close.

[haraken, 2016-08-18 13:33:18]

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

[commit-bot: I haz the power, 2016-08-18 13:33:38]

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-18 13:59:29]

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

[commit-bot: I haz the power, 2016-08-18 13:59:30]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[haraken, 2016-08-19 06:44:39]

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

[commit-bot: I haz the power, 2016-08-19 06:45:05]

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-19 06:59:11]

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

[commit-bot: I haz the power, 2016-08-19 06:59:11]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[haraken, 2016-08-19 07:19:22]

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

[commit-bot: I haz the power, 2016-08-19 07:19:46]

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-19 09:02:14]

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

[commit-bot: I haz the power, 2016-08-19 09:02:15]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-08-19 09:41:54]

Description was changed from

==========
Stop calling blink::shutdown

BUG=
==========

to

==========
Stop calling blink::shutdown

RenderThreadImpl::Shutdown has been trying to shut down Blink and V8 gracefully,
but the graceful shutdown has caused tons of use-after-free bugs
(and many engineers has spent lots of time fixing ordering issues around the
shutdown).

As discussed in blink-dev@
(https://groups.google.com/a/chromium.org/d/topic/blink-dev/kk4VX0xRB7I/discus...)
and platform-architecture-dev@
(https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/Zc...),
there is no reason we have to shut down the renderer gracefully.
It's just causing use-after-free bugs and wasting performance.
Hence, this CL stops calling blink::shutdown, which had been
shutting down *some things* in Blink and V8 gracefully.
(Remember that blink::shutdown hadn't been shutting down everything;
a lot of objects in Blink and V8 had already been left as is without getting
destructed.)

Ideally we should just call ProcessDied() at an earlier stage of
RenderThreadImpl::Shutdown(), but I'd like to defer the change to a separate CL.

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

[haraken, 2016-08-19 09:41:54]

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

[haraken, 2016-08-19 09:42:39]

Elliott: PTAL

Now all tests pass.

I'll send a PSA to blink-dev@ before landing.

[Torne, 2016-08-19 10:43:59]

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

[Torne, 2016-08-19 10:44:00]

What's going to happen in single process mode?

I've been trying to diagnose an issue where the single-process Android WebView
somehow ends up shutting down the in-process renderer and then attempts to start
it up again, resulting in a crash from the assertion failure in WTF::initialize
that forbids re-initializing WTF in a process that has called WTF::shutdown.
I've not yet been able to find the reason for this :(

I would *guess* that the effect of your change here would be that we still crash
in the second call to WTF::initialize because it also has an assert forbidding
calling initialize twice? If so then that's probably not a huge deal. However,
explicitly killing the process here in any way would be a problem, as that would
no longer "crash" in a way we can get reports about...

[haraken, 2016-08-19 10:58:16]

> I would *guess* that the effect of your change here would be that we still
crash
> in the second call to WTF::initialize because it also has an assert forbidding
> calling initialize twice? If so then that's probably not a huge deal.

Correct. This CL won't change the behavior.

On 2016/08/19 10:44:00, Torne wrote:
> What's going to happen in single process mode?
> 
> I've been trying to diagnose an issue where the single-process Android WebView
> somehow ends up shutting down the in-process renderer and then attempts to
start
> it up again, resulting in a crash from the assertion failure in
WTF::initialize
> that forbids re-initializing WTF in a process that has called WTF::shutdown.
> I've not yet been able to find the reason for this :(

That sounds like a definite bug... Also, remember that we're already forcibly
killing the renderer when IsFastShutdownPossible returns true. If that happens,
you already cannot receive the crash report you want to get.

[Torne, 2016-08-19 11:01:00]

On 2016/08/19 10:58:16, haraken wrote:
> > I would *guess* that the effect of your change here would be that we still
> crash
> > in the second call to WTF::initialize because it also has an assert
forbidding
> > calling initialize twice? If so then that's probably not a huge deal.
> 
> Correct. This CL won't change the behavior.
> 
> On 2016/08/19 10:44:00, Torne wrote:
> > What's going to happen in single process mode?
> > 
> > I've been trying to diagnose an issue where the single-process Android
WebView
> > somehow ends up shutting down the in-process renderer and then attempts to
> start
> > it up again, resulting in a crash from the assertion failure in
> WTF::initialize
> > that forbids re-initializing WTF in a process that has called WTF::shutdown.
> > I've not yet been able to find the reason for this :(
> 
> That sounds like a definite bug... Also, remember that we're already forcibly
> killing the renderer when IsFastShutdownPossible returns true. If that
happens,
> you already cannot receive the crash report you want to get.

Yes, it's a bug. We should never, ever enter the shutdown path in the first
place. It doesn't make sense to shut down the in-process renderer unless you are
also shutting down the browser process (at which point you would not then try to
re-initialize the in-process renderer afterward). However, I've been unable to
find the bug as of yet; if you have any ideas, please help ;)

I would hope that IsFastShutdownPossible never returns true in single-process
mode. That sounds like it would cause serious issues?

[haraken, 2016-08-19 11:03:50]

On 2016/08/19 11:01:00, Torne wrote:
> On 2016/08/19 10:58:16, haraken wrote:
> > > I would *guess* that the effect of your change here would be that we still
> > crash
> > > in the second call to WTF::initialize because it also has an assert
> forbidding
> > > calling initialize twice? If so then that's probably not a huge deal.
> > 
> > Correct. This CL won't change the behavior.
> > 
> > On 2016/08/19 10:44:00, Torne wrote:
> > > What's going to happen in single process mode?
> > > 
> > > I've been trying to diagnose an issue where the single-process Android
> WebView
> > > somehow ends up shutting down the in-process renderer and then attempts to
> > start
> > > it up again, resulting in a crash from the assertion failure in
> > WTF::initialize
> > > that forbids re-initializing WTF in a process that has called
WTF::shutdown.
> > > I've not yet been able to find the reason for this :(
> > 
> > That sounds like a definite bug... Also, remember that we're already
forcibly
> > killing the renderer when IsFastShutdownPossible returns true. If that
> happens,
> > you already cannot receive the crash report you want to get.
> 
> Yes, it's a bug. We should never, ever enter the shutdown path in the first
> place. It doesn't make sense to shut down the in-process renderer unless you
are
> also shutting down the browser process (at which point you would not then try
to
> re-initialize the in-process renderer afterward). However, I've been unable to
> find the bug as of yet; if you have any ideas, please help ;)
> 
> I would hope that IsFastShutdownPossible never returns true in single-process
> mode. That sounds like it would cause serious issues?

I'm not familiar with how the single-thread mode is working, but maybe it's not
entering the shutdown path (including IsFastShutdownPossible) in the first
place, right?

[Torne, 2016-08-19 11:07:45]

On 2016/08/19 11:03:50, haraken wrote:
> On 2016/08/19 11:01:00, Torne wrote:
> > On 2016/08/19 10:58:16, haraken wrote:
> > > > I would *guess* that the effect of your change here would be that we
still
> > > crash
> > > > in the second call to WTF::initialize because it also has an assert
> > forbidding
> > > > calling initialize twice? If so then that's probably not a huge deal.
> > > 
> > > Correct. This CL won't change the behavior.
> > > 
> > > On 2016/08/19 10:44:00, Torne wrote:
> > > > What's going to happen in single process mode?
> > > > 
> > > > I've been trying to diagnose an issue where the single-process Android
> > WebView
> > > > somehow ends up shutting down the in-process renderer and then attempts
to
> > > start
> > > > it up again, resulting in a crash from the assertion failure in
> > > WTF::initialize
> > > > that forbids re-initializing WTF in a process that has called
> WTF::shutdown.
> > > > I've not yet been able to find the reason for this :(
> > > 
> > > That sounds like a definite bug... Also, remember that we're already
> forcibly
> > > killing the renderer when IsFastShutdownPossible returns true. If that
> > happens,
> > > you already cannot receive the crash report you want to get.
> > 
> > Yes, it's a bug. We should never, ever enter the shutdown path in the first
> > place. It doesn't make sense to shut down the in-process renderer unless you
> are
> > also shutting down the browser process (at which point you would not then
try
> to
> > re-initialize the in-process renderer afterward). However, I've been unable
to
> > find the bug as of yet; if you have any ideas, please help ;)
> > 
> > I would hope that IsFastShutdownPossible never returns true in
single-process
> > mode. That sounds like it would cause serious issues?
> 
> I'm not familiar with how the single-thread mode is working, but maybe it's
not
> entering the shutdown path (including IsFastShutdownPossible) in the first
> place, right?

Single-process mode in general *does* enter the shutdown path when the browser
is shutting down, and many tests that run in single-process mode rely on this
and don't pass otherwise (I tried to entirely nuke shutting down the renderer in
single-process to help debug this issue and couldn't submit the change as it
breaks many tests).

The Android WebView specifically doesn't *have* a browser shutdown path, though,
and so the renderer shutdown path should never happen either.

[haraken, 2016-08-22 09:33:01]

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

[haraken, 2016-08-22 09:33:02]

jochen: Would you take a look at this? Elliott is OOO.

I already sent a PSA to blink-dev@:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/LnhBbz6z0Y8

[jochen (gone - plz use gerrit), 2016-08-22 09:46:13]

lgtm

[haraken, 2016-08-22 09:46:40]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-08-22 09:46:50]

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-22 11:44:43]

Description was changed from

==========
Stop calling blink::shutdown

RenderThreadImpl::Shutdown has been trying to shut down Blink and V8 gracefully,
but the graceful shutdown has caused tons of use-after-free bugs
(and many engineers has spent lots of time fixing ordering issues around the
shutdown).

As discussed in blink-dev@
(https://groups.google.com/a/chromium.org/d/topic/blink-dev/kk4VX0xRB7I/discus...)
and platform-architecture-dev@
(https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/Zc...),
there is no reason we have to shut down the renderer gracefully.
It's just causing use-after-free bugs and wasting performance.
Hence, this CL stops calling blink::shutdown, which had been
shutting down *some things* in Blink and V8 gracefully.
(Remember that blink::shutdown hadn't been shutting down everything;
a lot of objects in Blink and V8 had already been left as is without getting
destructed.)

Ideally we should just call ProcessDied() at an earlier stage of
RenderThreadImpl::Shutdown(), but I'd like to defer the change to a separate CL.

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

to

==========
Stop calling blink::shutdown

RenderThreadImpl::Shutdown has been trying to shut down Blink and V8 gracefully,
but the graceful shutdown has caused tons of use-after-free bugs
(and many engineers has spent lots of time fixing ordering issues around the
shutdown).

As discussed in blink-dev@
(https://groups.google.com/a/chromium.org/d/topic/blink-dev/kk4VX0xRB7I/discus...)
and platform-architecture-dev@
(https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/Zc...),
there is no reason we have to shut down the renderer gracefully.
It's just causing use-after-free bugs and wasting performance.
Hence, this CL stops calling blink::shutdown, which had been
shutting down *some things* in Blink and V8 gracefully.
(Remember that blink::shutdown hadn't been shutting down everything;
a lot of objects in Blink and V8 had already been left as is without getting
destructed.)

Ideally we should just call ProcessDied() at an earlier stage of
RenderThreadImpl::Shutdown(), but I'd like to defer the change to a separate CL.

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

[commit-bot: I haz the power, 2016-08-22 11:44:44]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-08-22 11:46:16]

Description was changed from

==========
Stop calling blink::shutdown

RenderThreadImpl::Shutdown has been trying to shut down Blink and V8 gracefully,
but the graceful shutdown has caused tons of use-after-free bugs
(and many engineers has spent lots of time fixing ordering issues around the
shutdown).

As discussed in blink-dev@
(https://groups.google.com/a/chromium.org/d/topic/blink-dev/kk4VX0xRB7I/discus...)
and platform-architecture-dev@
(https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/Zc...),
there is no reason we have to shut down the renderer gracefully.
It's just causing use-after-free bugs and wasting performance.
Hence, this CL stops calling blink::shutdown, which had been
shutting down *some things* in Blink and V8 gracefully.
(Remember that blink::shutdown hadn't been shutting down everything;
a lot of objects in Blink and V8 had already been left as is without getting
destructed.)

Ideally we should just call ProcessDied() at an earlier stage of
RenderThreadImpl::Shutdown(), but I'd like to defer the change to a separate CL.

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

to

==========
Stop calling blink::shutdown

RenderThreadImpl::Shutdown has been trying to shut down Blink and V8 gracefully,
but the graceful shutdown has caused tons of use-after-free bugs
(and many engineers has spent lots of time fixing ordering issues around the
shutdown).

As discussed in blink-dev@
(https://groups.google.com/a/chromium.org/d/topic/blink-dev/kk4VX0xRB7I/discus...)
and platform-architecture-dev@
(https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/Zc...),
there is no reason we have to shut down the renderer gracefully.
It's just causing use-after-free bugs and wasting performance.
Hence, this CL stops calling blink::shutdown, which had been
shutting down *some things* in Blink and V8 gracefully.
(Remember that blink::shutdown hadn't been shutting down everything;
a lot of objects in Blink and V8 had already been left as is without getting
destructed.)

Ideally we should just call ProcessDied() at an earlier stage of
RenderThreadImpl::Shutdown(), but I'd like to defer the change to a separate CL.

BUG=639244

Committed: https://crrev.com/01cb51d26b17923ed2b5c3b59566f0fc9aed74ae
Cr-Commit-Position: refs/heads/master@{#413430}
==========

[commit-bot: I haz the power, 2016-08-22 11:46:17]

Patchset 9 (id:??) landed as
https://crrev.com/01cb51d26b17923ed2b5c3b59566f0fc9aed74ae
Cr-Commit-Position: refs/heads/master@{#413430}

[haraken, 2016-09-05 02:21:58]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/2312593002/ 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()..