Remove a graceful shutdown sequence from the renderer |
||||||||
Issue descriptionRenderThreadImpl::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. We should get rid of the graceful shutdown sequence. The goal is to just call ProcessDied() at an earlier stage of RenderThreadImpl::Shutdown().
,
Aug 22 2016
Issue 638865 has been merged into this issue.
,
Aug 22 2016
,
Aug 22 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/01cb51d26b17923ed2b5c3b59566f0fc9aed74ae commit 01cb51d26b17923ed2b5c3b59566f0fc9aed74ae Author: haraken <haraken@chromium.org> Date: Mon Aug 22 11:44:26 2016 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 Review-Url: https://codereview.chromium.org/2249353002 Cr-Commit-Position: refs/heads/master@{#413430} [modify] https://crrev.com/01cb51d26b17923ed2b5c3b59566f0fc9aed74ae/content/renderer/render_thread_impl.cc [modify] https://crrev.com/01cb51d26b17923ed2b5c3b59566f0fc9aed74ae/third_party/WebKit/Source/wtf/ThreadSpecific.h
,
Aug 24 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e0817cfbfc3c7f8b13aa2ffc99c31f336ba1d29a commit e0817cfbfc3c7f8b13aa2ffc99c31f336ba1d29a Author: haraken <haraken@chromium.org> Date: Wed Aug 24 02:58:08 2016 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 Review-Url: https://codereview.chromium.org/2270543002 Cr-Commit-Position: refs/heads/master@{#413959} [modify] https://crrev.com/e0817cfbfc3c7f8b13aa2ffc99c31f336ba1d29a/content/renderer/render_thread_impl.cc
,
Aug 24 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/47f9c2d1ca781e1e4df5dc05b74200a6ba696afe commit 47f9c2d1ca781e1e4df5dc05b74200a6ba696afe Author: haraken <haraken@chromium.org> Date: Wed Aug 24 09:32:43 2016 Remove WTF::isShutdown() Now it's unused. BUG=639244 Review-Url: https://codereview.chromium.org/2277663002 Cr-Commit-Position: refs/heads/master@{#414030} [modify] https://crrev.com/47f9c2d1ca781e1e4df5dc05b74200a6ba696afe/third_party/WebKit/Source/wtf/WTF.cpp [modify] https://crrev.com/47f9c2d1ca781e1e4df5dc05b74200a6ba696afe/third_party/WebKit/Source/wtf/WTF.h
,
Aug 25 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5ef6c507623ec9b9536f2912bf3fb53244cd0601 commit 5ef6c507623ec9b9536f2912bf3fb53244cd0601 Author: haraken <haraken@chromium.org> Date: Thu Aug 25 00:55:35 2016 Stop calling blink::shutdown in content/ tests 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. This CL stops calling blink::shutdown in content/ tests. BUG=639244 Review-Url: https://codereview.chromium.org/2268283004 Cr-Commit-Position: refs/heads/master@{#414234} [modify] https://crrev.com/5ef6c507623ec9b9536f2912bf3fb53244cd0601/content/public/test/render_view_test.cc [modify] https://crrev.com/5ef6c507623ec9b9536f2912bf3fb53244cd0601/content/test/test_blink_web_unit_test_support.cc
,
Aug 25 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bc13dbaa3f99577e8929e27283d7929d8f844d03 commit bc13dbaa3f99577e8929e27283d7929d8f844d03 Author: haraken <haraken@chromium.org> Date: Thu Aug 25 04:17:34 2016 Remove a graceful shutdown sequence from TestingPlatformSupport 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. This CL removes a graceful shutdown sequence from TestingPlatformSupport. BUG=639244 Review-Url: https://codereview.chromium.org/2279563003 Cr-Commit-Position: refs/heads/master@{#414307} [modify] https://crrev.com/bc13dbaa3f99577e8929e27283d7929d8f844d03/third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp
,
Aug 25 2016
haraken, you seems to be working on this, so I'm assigning you. Feel free to change the status to "Started" and/or change the priority.
,
Aug 25 2016
,
Aug 29 2016
,
Aug 30 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9953069136019c8fb2489763f925dc291d94c7c2 commit 9953069136019c8fb2489763f925dc291d94c7c2 Author: haraken <haraken@chromium.org> Date: Tue Aug 30 15:07:54 2016 Stop calling Platform::shutdown when a PPAPI/utility thread stop 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. This CL stops calling Platform::shutdown() when a PPAPI/utility thread stop. BUG=639244 Review-Url: https://codereview.chromium.org/2274383002 Cr-Commit-Position: refs/heads/master@{#415297} [modify] https://crrev.com/9953069136019c8fb2489763f925dc291d94c7c2/content/ppapi_plugin/ppapi_thread.cc [modify] https://crrev.com/9953069136019c8fb2489763f925dc291d94c7c2/content/utility/utility_thread_impl.cc
,
Sep 5 2016
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().
,
Sep 5 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/556b4223eeea55a338bf38ca87b004ba23e1039a commit 556b4223eeea55a338bf38ca87b004ba23e1039a Author: haraken <haraken@chromium.org> Date: Mon Sep 05 00:32:02 2016 Revert of Remove Platform::current checks from Mojo's error handlers in geolocation (patchset #1 id:1 of https://codereview.chromium.org/2271953002/ ) 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: > Remove Platform::current checks from Mojo's error handlers in geolocation > > The checks were added in https://codereview.chromium.org/2163683004/ > but are no longer needed because I already removed blink::shutdown > from the renderer's shutdown sequence. > > BUG= 628999 ,639244 > > Committed: https://crrev.com/8435863a81be1eee4c28131a341f8a4bce04a7b4 > Cr-Commit-Position: refs/heads/master@{#414232} TBR=sammc@chromium.org BUG= 628999 ,639244 NOTRY=true Review-Url: https://codereview.chromium.org/2312553003 Cr-Commit-Position: refs/heads/master@{#416480} [modify] https://crrev.com/556b4223eeea55a338bf38ca87b004ba23e1039a/third_party/WebKit/Source/modules/geolocation/Geolocation.cpp
,
Sep 5 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2302bf7e2cf6bbaa8f6b43a966acebbe974b43b9 commit 2302bf7e2cf6bbaa8f6b43a966acebbe974b43b9 Author: haraken <haraken@chromium.org> Date: Mon Sep 05 00:35:15 2016 Revert of Remove WTF::isShutdown() (patchset #1 id:1 of https://codereview.chromium.org/2277663002/ ) 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: > Remove WTF::isShutdown() > > Now it's unused. > > BUG=639244 > > Committed: https://crrev.com/47f9c2d1ca781e1e4df5dc05b74200a6ba696afe > Cr-Commit-Position: refs/heads/master@{#414030} TBR=yutak@chromium.org BUG=639244 NOTRY=true Review-Url: https://codereview.chromium.org/2311713002 Cr-Commit-Position: refs/heads/master@{#416481} [modify] https://crrev.com/2302bf7e2cf6bbaa8f6b43a966acebbe974b43b9/third_party/WebKit/Source/wtf/WTF.cpp [modify] https://crrev.com/2302bf7e2cf6bbaa8f6b43a966acebbe974b43b9/third_party/WebKit/Source/wtf/WTF.h
,
Sep 5 2016
Done.
,
Sep 5 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6e79a75b0a1e9f99a1a0956fd1570968a5ec9348 commit 6e79a75b0a1e9f99a1a0956fd1570968a5ec9348 Author: haraken <haraken@chromium.org> Date: Mon Sep 05 01:33:50 2016 Revert of Stop shutting down the message loop when the renderer stops (patchset #1 id:1 of https://codereview.chromium.org/2270543002/ ) 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 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} TBR=jochen@chromium.org,torne@chromium.org,jam@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=639244 Review-Url: https://codereview.chromium.org/2312583002 Cr-Commit-Position: refs/heads/master@{#416486} [modify] https://crrev.com/6e79a75b0a1e9f99a1a0956fd1570968a5ec9348/content/renderer/render_thread_impl.cc
,
Sep 5 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/43cf914949a06e8fe3292ca60c5bf046e8a87cc9 commit 43cf914949a06e8fe3292ca60c5bf046e8a87cc9 Author: haraken <haraken@chromium.org> Date: Mon Sep 05 02:25:08 2016 Revert of Remove a Platform::current() check from Mojo's error handler in WebSocket (patchset #1 id:1 of https://codereview.chromium.org/2276433003/ ) 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: > Remove a Platform::current() check from Mojo's error handler in WebSocket > > The check was added in https://codereview.chromium.org/2258003003/ > but is no longer needed because I already removed blink::shutdown > from renderer's shutdown sequence. > > BUG=637949,639244 > > Committed: https://crrev.com/9a8f5c73b4158495cf3e35aee83e1e2b2fc7cb9c > Cr-Commit-Position: refs/heads/master@{#415196} TBR=yhirano@chromium.org BUG=637949,639244 Review-Url: https://codereview.chromium.org/2312573002 Cr-Commit-Position: refs/heads/master@{#416487} [modify] https://crrev.com/43cf914949a06e8fe3292ca60c5bf046e8a87cc9/third_party/WebKit/Source/modules/websockets/WebSocketHandleImpl.cpp
,
Sep 5 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/19a7c51a9ceedd811e7e01a996cff4412ff795a2 commit 19a7c51a9ceedd811e7e01a996cff4412ff795a2 Author: haraken <haraken@chromium.org> Date: Mon Sep 05 04:19:50 2016 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 # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=639244 Review-Url: https://codereview.chromium.org/2312593002 Cr-Commit-Position: refs/heads/master@{#416494} [modify] https://crrev.com/19a7c51a9ceedd811e7e01a996cff4412ff795a2/content/renderer/render_thread_impl.cc [modify] https://crrev.com/19a7c51a9ceedd811e7e01a996cff4412ff795a2/third_party/WebKit/Source/wtf/ThreadSpecific.h
,
Sep 6 2016
Your change meets the bar and is auto-approved for M54 (branch: 2840)
,
Sep 6 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/85a5be3cfe186fee3d41fd4883e600988da6f110 commit 85a5be3cfe186fee3d41fd4883e600988da6f110 Author: Kentaro Hara <haraken@chromium.org> Date: Tue Sep 06 05:11:33 2016 Revert of Remove Platform::current checks from Mojo's error handlers in geolocation (patchset #1 id:1 of https://codereview.chromium.org/2271953002/ ) 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: > Remove Platform::current checks from Mojo's error handlers in geolocation > > The checks were added in https://codereview.chromium.org/2163683004/ > but are no longer needed because I already removed blink::shutdown > from the renderer's shutdown sequence. > > BUG= 628999 ,639244 > > Committed: https://crrev.com/8435863a81be1eee4c28131a341f8a4bce04a7b4 > Cr-Commit-Position: refs/heads/master@{#414232} TBR=sammc@chromium.org BUG= 628999 ,639244 NOTRY=true Review-Url: https://codereview.chromium.org/2312553003 Cr-Commit-Position: refs/heads/master@{#416480} (cherry picked from commit 556b4223eeea55a338bf38ca87b004ba23e1039a) Review URL: https://codereview.chromium.org/2312833002 . Cr-Commit-Position: refs/branch-heads/2840@{#160} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/85a5be3cfe186fee3d41fd4883e600988da6f110/third_party/WebKit/Source/modules/geolocation/Geolocation.cpp
,
Sep 6 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/edb1c010cfff486712e10f7d41eb0974e7099a93 commit edb1c010cfff486712e10f7d41eb0974e7099a93 Author: Kentaro Hara <haraken@chromium.org> Date: Tue Sep 06 05:15:40 2016 Revert of Remove WTF::isShutdown() (patchset #1 id:1 of https://codereview.chromium.org/2277663002/ ) 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: > Remove WTF::isShutdown() > > Now it's unused. > > BUG=639244 > > Committed: https://crrev.com/47f9c2d1ca781e1e4df5dc05b74200a6ba696afe > Cr-Commit-Position: refs/heads/master@{#414030} TBR=yutak@chromium.org BUG=639244 NOTRY=true Review-Url: https://codereview.chromium.org/2311713002 Cr-Commit-Position: refs/heads/master@{#416481} (cherry picked from commit 2302bf7e2cf6bbaa8f6b43a966acebbe974b43b9) Review URL: https://codereview.chromium.org/2311983003 . Cr-Commit-Position: refs/branch-heads/2840@{#161} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/edb1c010cfff486712e10f7d41eb0974e7099a93/third_party/WebKit/Source/wtf/WTF.cpp [modify] https://crrev.com/edb1c010cfff486712e10f7d41eb0974e7099a93/third_party/WebKit/Source/wtf/WTF.h
,
Sep 6 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/15dcff3430db7c7673b4227fa8bdab17442a0a14 commit 15dcff3430db7c7673b4227fa8bdab17442a0a14 Author: Kentaro Hara <haraken@chromium.org> Date: Tue Sep 06 05:18:38 2016 Revert of Stop shutting down the message loop when the renderer stops (patchset #1 id:1 of https://codereview.chromium.org/2270543002/ ) 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 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} TBR=jochen@chromium.org,torne@chromium.org,jam@chromium.org BUG=639244 Review-Url: https://codereview.chromium.org/2312583002 Cr-Commit-Position: refs/heads/master@{#416486} (cherry picked from commit 6e79a75b0a1e9f99a1a0956fd1570968a5ec9348) Review URL: https://codereview.chromium.org/2314613002 . Cr-Commit-Position: refs/branch-heads/2840@{#162} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/15dcff3430db7c7673b4227fa8bdab17442a0a14/content/renderer/render_thread_impl.cc
,
Sep 6 2016
Merged the fixes to M54.
,
Sep 6 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a5114377e139dd021316120164432305d96d618d commit a5114377e139dd021316120164432305d96d618d Author: Kentaro Hara <haraken@chromium.org> Date: Tue Sep 06 05:28:55 2016 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) Review URL: https://codereview.chromium.org/2315573003 . Cr-Commit-Position: refs/branch-heads/2840@{#163} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/a5114377e139dd021316120164432305d96d618d/content/renderer/render_thread_impl.cc [modify] https://crrev.com/a5114377e139dd021316120164432305d96d618d/third_party/WebKit/Source/wtf/ThreadSpecific.h
,
Oct 7 2016
This is still happening, see https://build.chromium.org/p/chromium.memory.full/builders/Linux%20TSan%20Tests/builds/2393/steps/content_browsertests%20on%20Ubuntu-12.04/logs/stdio
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/85a5be3cfe186fee3d41fd4883e600988da6f110 commit 85a5be3cfe186fee3d41fd4883e600988da6f110 Author: Kentaro Hara <haraken@chromium.org> Date: Tue Sep 06 05:11:33 2016 Revert of Remove Platform::current checks from Mojo's error handlers in geolocation (patchset #1 id:1 of https://codereview.chromium.org/2271953002/ ) 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: > Remove Platform::current checks from Mojo's error handlers in geolocation > > The checks were added in https://codereview.chromium.org/2163683004/ > but are no longer needed because I already removed blink::shutdown > from the renderer's shutdown sequence. > > BUG= 628999 ,639244 > > Committed: https://crrev.com/8435863a81be1eee4c28131a341f8a4bce04a7b4 > Cr-Commit-Position: refs/heads/master@{#414232} TBR=sammc@chromium.org BUG= 628999 ,639244 NOTRY=true Review-Url: https://codereview.chromium.org/2312553003 Cr-Commit-Position: refs/heads/master@{#416480} (cherry picked from commit 556b4223eeea55a338bf38ca87b004ba23e1039a) Review URL: https://codereview.chromium.org/2312833002 . Cr-Commit-Position: refs/branch-heads/2840@{#160} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/85a5be3cfe186fee3d41fd4883e600988da6f110/third_party/WebKit/Source/modules/geolocation/Geolocation.cpp
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/edb1c010cfff486712e10f7d41eb0974e7099a93 commit edb1c010cfff486712e10f7d41eb0974e7099a93 Author: Kentaro Hara <haraken@chromium.org> Date: Tue Sep 06 05:15:40 2016 Revert of Remove WTF::isShutdown() (patchset #1 id:1 of https://codereview.chromium.org/2277663002/ ) 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: > Remove WTF::isShutdown() > > Now it's unused. > > BUG=639244 > > Committed: https://crrev.com/47f9c2d1ca781e1e4df5dc05b74200a6ba696afe > Cr-Commit-Position: refs/heads/master@{#414030} TBR=yutak@chromium.org BUG=639244 NOTRY=true Review-Url: https://codereview.chromium.org/2311713002 Cr-Commit-Position: refs/heads/master@{#416481} (cherry picked from commit 2302bf7e2cf6bbaa8f6b43a966acebbe974b43b9) Review URL: https://codereview.chromium.org/2311983003 . Cr-Commit-Position: refs/branch-heads/2840@{#161} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/edb1c010cfff486712e10f7d41eb0974e7099a93/third_party/WebKit/Source/wtf/WTF.cpp [modify] https://crrev.com/edb1c010cfff486712e10f7d41eb0974e7099a93/third_party/WebKit/Source/wtf/WTF.h
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/15dcff3430db7c7673b4227fa8bdab17442a0a14 commit 15dcff3430db7c7673b4227fa8bdab17442a0a14 Author: Kentaro Hara <haraken@chromium.org> Date: Tue Sep 06 05:18:38 2016 Revert of Stop shutting down the message loop when the renderer stops (patchset #1 id:1 of https://codereview.chromium.org/2270543002/ ) 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 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} TBR=jochen@chromium.org,torne@chromium.org,jam@chromium.org BUG=639244 Review-Url: https://codereview.chromium.org/2312583002 Cr-Commit-Position: refs/heads/master@{#416486} (cherry picked from commit 6e79a75b0a1e9f99a1a0956fd1570968a5ec9348) Review URL: https://codereview.chromium.org/2314613002 . Cr-Commit-Position: refs/branch-heads/2840@{#162} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/15dcff3430db7c7673b4227fa8bdab17442a0a14/content/renderer/render_thread_impl.cc
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a5114377e139dd021316120164432305d96d618d commit a5114377e139dd021316120164432305d96d618d Author: Kentaro Hara <haraken@chromium.org> Date: Tue Sep 06 05:28:55 2016 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) Review URL: https://codereview.chromium.org/2315573003 . Cr-Commit-Position: refs/branch-heads/2840@{#163} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/a5114377e139dd021316120164432305d96d618d/content/renderer/render_thread_impl.cc [modify] https://crrev.com/a5114377e139dd021316120164432305d96d618d/third_party/WebKit/Source/wtf/ThreadSpecific.h
,
Jan 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bbfdd9f0669c9856883ffbf2cd9909e2a4df9dcf commit bbfdd9f0669c9856883ffbf2cd9909e2a4df9dcf Author: haraken <haraken@chromium.org> Date: Thu Jan 12 07:14:04 2017 Remove RenderThreadImpl::Shutdown RenderThreadImpl::Shutdown has been shutting 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 chromium-dev@ (https://groups.google.com/a/chromium.org/d/topic/chromium-dev/OLS4JSZvowI/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 removes RenderThreadImpl::Shutdown and instead calls _exit(0) in ~ChildProcess. We need a couple of special handling in a single-process mode. See the comment in ~ChildProcess for more details. BUG=639244 Review-Url: https://codereview.chromium.org/2309153002 Cr-Commit-Position: refs/heads/master@{#443175} [modify] https://crrev.com/bbfdd9f0669c9856883ffbf2cd9909e2a4df9dcf/base/at_exit.cc [modify] https://crrev.com/bbfdd9f0669c9856883ffbf2cd9909e2a4df9dcf/base/at_exit.h [modify] https://crrev.com/bbfdd9f0669c9856883ffbf2cd9909e2a4df9dcf/content/child/child_process.cc [modify] https://crrev.com/bbfdd9f0669c9856883ffbf2cd9909e2a4df9dcf/content/child/child_thread_impl.cc [modify] https://crrev.com/bbfdd9f0669c9856883ffbf2cd9909e2a4df9dcf/content/child/child_thread_impl.h [modify] https://crrev.com/bbfdd9f0669c9856883ffbf2cd9909e2a4df9dcf/content/renderer/render_thread_impl.cc [modify] https://crrev.com/bbfdd9f0669c9856883ffbf2cd9909e2a4df9dcf/content/renderer/render_thread_impl.h [modify] https://crrev.com/bbfdd9f0669c9856883ffbf2cd9909e2a4df9dcf/content/renderer/render_thread_impl_browsertest.cc [modify] https://crrev.com/bbfdd9f0669c9856883ffbf2cd9909e2a4df9dcf/third_party/WebKit/Source/wtf/ThreadSpecific.h
,
Jan 12 2017
haraken@, will you remove blink::shutdown, ModulesInitializer::shutdown and CoreInitializer::shutdown, too? If so, it's great because they've blocked worker shutdown cleanup :)
,
Jan 12 2017
Yes, that's my plan. Though I want to wait for a couple of days to confirm that the CL is stabilized on ToT.
,
Jan 12 2017
I see. Thank you :)
,
Feb 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/940efb95e8eb38337c4df327792b71dcc2de2d84 commit 940efb95e8eb38337c4df327792b71dcc2de2d84 Author: haraken <haraken@chromium.org> Date: Wed Feb 08 05:58:15 2017 Win should call ::TerminateProcess to exit a renderer process Win should not call _exit(0) because the behavior is odd. (See more discussion in https://codereview.chromium.org/2309153002/.) Instead, Win should use ::TerminateProcess. BUG=639244 Review-Url: https://codereview.chromium.org/2629623003 Cr-Commit-Position: refs/heads/master@{#448916} [modify] https://crrev.com/940efb95e8eb38337c4df327792b71dcc2de2d84/base/process/process.h [modify] https://crrev.com/940efb95e8eb38337c4df327792b71dcc2de2d84/base/process/process_posix.cc [modify] https://crrev.com/940efb95e8eb38337c4df327792b71dcc2de2d84/base/process/process_unittest.cc [modify] https://crrev.com/940efb95e8eb38337c4df327792b71dcc2de2d84/base/process/process_win.cc [modify] https://crrev.com/940efb95e8eb38337c4df327792b71dcc2de2d84/content/renderer/render_thread_impl.cc
,
Oct 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8dbd83c66912ffb764d8a2b7e4eb3a88f9c9be16 commit 8dbd83c66912ffb764d8a2b7e4eb3a88f9c9be16 Author: Devlin Cronin <rdevlin.cronin@chromium.org> Date: Wed Oct 11 17:06:34 2017 [Content Cleanup] Remove RenderThreadObserver::OnRendererProcessShutdown RenderThreadObserver has a method OnRendererProcessShutdown that allows implementors to do some pre-everything-is-gone cleanup. However, the caller of this was removed when we stopped shutting down the renderer process in https://crrev.com/bbfdd9f0669c9856883ffbf2cd9909e2a4df9dcf. As such, all methods have not been called for 9 months. Remove the OnRendererProcessShutdown method and all its implementations. Bug: 639244 Change-Id: I1cf923359b63ab84235e328786ca8d1c91b12710 Reviewed-on: https://chromium-review.googlesource.com/707348 Reviewed-by: Kentaro Hara <haraken@chromium.org> Reviewed-by: Jochen Eisinger <jochen@chromium.org> Commit-Queue: Devlin <rdevlin.cronin@chromium.org> Cr-Commit-Position: refs/heads/master@{#508008} [modify] https://crrev.com/8dbd83c66912ffb764d8a2b7e4eb3a88f9c9be16/chrome/renderer/chrome_render_thread_observer.cc [modify] https://crrev.com/8dbd83c66912ffb764d8a2b7e4eb3a88f9c9be16/chrome/renderer/chrome_render_thread_observer.h [modify] https://crrev.com/8dbd83c66912ffb764d8a2b7e4eb3a88f9c9be16/chrome/renderer/leak_detector/leak_detector_remote_client.cc [modify] https://crrev.com/8dbd83c66912ffb764d8a2b7e4eb3a88f9c9be16/chrome/renderer/leak_detector/leak_detector_remote_client.h [modify] https://crrev.com/8dbd83c66912ffb764d8a2b7e4eb3a88f9c9be16/chrome/renderer/prerender/prerender_dispatcher.cc [modify] https://crrev.com/8dbd83c66912ffb764d8a2b7e4eb3a88f9c9be16/chrome/renderer/prerender/prerender_dispatcher.h [modify] https://crrev.com/8dbd83c66912ffb764d8a2b7e4eb3a88f9c9be16/chrome/test/base/chrome_render_view_test.cc [modify] https://crrev.com/8dbd83c66912ffb764d8a2b7e4eb3a88f9c9be16/content/public/renderer/render_thread_observer.h [modify] https://crrev.com/8dbd83c66912ffb764d8a2b7e4eb3a88f9c9be16/content/renderer/image_downloader/image_downloader_base.cc [modify] https://crrev.com/8dbd83c66912ffb764d8a2b7e4eb3a88f9c9be16/content/renderer/image_downloader/image_downloader_base.h [modify] https://crrev.com/8dbd83c66912ffb764d8a2b7e4eb3a88f9c9be16/content/shell/renderer/layout_test/layout_test_render_thread_observer.cc [modify] https://crrev.com/8dbd83c66912ffb764d8a2b7e4eb3a88f9c9be16/content/shell/renderer/layout_test/layout_test_render_thread_observer.h [modify] https://crrev.com/8dbd83c66912ffb764d8a2b7e4eb3a88f9c9be16/extensions/renderer/dispatcher.cc [modify] https://crrev.com/8dbd83c66912ffb764d8a2b7e4eb3a88f9c9be16/extensions/renderer/dispatcher.h |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by haraken@chromium.org
, Aug 19 2016