Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(395)

Issue 2315573003: Revert of Stop calling blink::shutdown (patchset #9 id:160001 of https://codereview.chromium.org/22… (Closed)

Created:
4 years, 3 months ago by haraken
Modified:
4 years, 3 months ago
CC:
chromium-reviews
Target Ref:
refs/pending/branch-heads/2840
Project:
chromium
Visibility:
Public.

Description

Revert of Stop calling blink::shutdown (patchset #9 id:160001 of https://codereview.chromium.org/2249353002/ ) Reason for revert: 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(). Original issue's description: > 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} TBR=jochen@chromium.org,esprehn@chromium.org,torne@chromium.org,tzik@chromium.org BUG=639244 Review-Url: https://codereview.chromium.org/2312593002 Cr-Commit-Position: refs/heads/master@{#416494} (cherry picked from commit 19a7c51a9ceedd811e7e01a996cff4412ff795a2)

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+17 lines, -10 lines) Patch
M content/renderer/render_thread_impl.cc View 1 chunk +16 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/wtf/ThreadSpecific.h View 5 chunks +1 line, -7 lines 0 comments Download

Messages

Total messages: 1 (0 generated)
haraken
4 years, 3 months ago (2016-09-06 05:31:57 UTC) #1
Message was sent while issue was closed.
Committed patchset #1 (id:1) to pending queue manually as
ec77f957b4d32b620485e74a105800b23084be1a.

Powered by Google App Engine
This is Rietveld 408576698