Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2159123002:
Shutdown renderer main message loop before blink::shutdown() (Closed)
|
Created:
4 years, 5 months ago by tzik Modified:
4 years, 3 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, mlamouri+watch-content_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionShutdown renderer main message loop before blink::shutdown()
blink::Persistent requires all instances to be destroyed before ThreadState
shutdown, which happens in blink::shutdown(). However, some blink::Persistent
are held by MessageLoop as bound arguments. That causes renderer crash on the
destructor of blink::Persistent.
This CL moves MessageLoop shutdown before blink::Platform::shutdown, so that
message loop doesn't hold any task on ThreadState shutdown.
BUG=627004
Patch Set 1 #
Total comments: 2
Patch Set 2 : +ChildThreadImpl::main_task_runner() and replace message_loop() access #Patch Set 3 : Split out Platform::shutdown() from blink::shutdown() #Patch Set 4 : fix #Patch Set 5 : split out blink::Platform::initialize from blink::initialize #
Total comments: 8
Patch Set 6 : update #
Messages
Total messages: 60 (31 generated)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Shutdown renderer main message loop before blink::shutdown() BUG= ========== to ========== Shutdown renderer main message loop before blink::shutdown() blink::Persistent requires all instances to be destroyed before ThreadState shutdown, which happens in blink::shutdown(). However, some blink::Persistent are held by MessageLoop as bound arguments. That causes renderer crash on the destructor of blink::Persistent. This CL moves MessageLoop shutdown before blink::shutdown, so that message loop doesn't hold any task on ThreadState shutdown. BUG= ==========
Description was changed from ========== Shutdown renderer main message loop before blink::shutdown() blink::Persistent requires all instances to be destroyed before ThreadState shutdown, which happens in blink::shutdown(). However, some blink::Persistent are held by MessageLoop as bound arguments. That causes renderer crash on the destructor of blink::Persistent. This CL moves MessageLoop shutdown before blink::shutdown, so that message loop doesn't hold any task on ThreadState shutdown. BUG= ========== to ========== Shutdown renderer main message loop before blink::shutdown() blink::Persistent requires all instances to be destroyed before ThreadState shutdown, which happens in blink::shutdown(). However, some blink::Persistent are held by MessageLoop as bound arguments. That causes renderer crash on the destructor of blink::Persistent. This CL moves MessageLoop shutdown before blink::shutdown, so that message loop doesn't hold any task on ThreadState shutdown. BUG=627004 ==========
tzik@chromium.org changed reviewers: + esprehn@chromium.org, haraken@chromium.org
PTAL
https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (left): https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:996: // to the browser process. You're doing the opposite of what the comment says we have to do now, why is that correct? You now destroy the message loop before the discardable memory manager shuts down which means the IPCs won't make it out.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (left): https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:996: // to the browser process. On 2016/07/19 16:42:54, esprehn wrote: > You're doing the opposite of what the comment says we have to do now, why is > that correct? You now destroy the message loop before the discardable memory > manager shuts down which means the IPCs won't make it out. The comment looks inaccurate to me. ChildDiscardableSharedMemoryManager spins the message loop only for a sync IPC for allocation, and doesn't use it for shutdown. Though it uses the main thread task runner to check if the current thread is the main thread, the message loop itself is not needed for it.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/07/19 17:26:52, tzik wrote: > https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... > File content/renderer/render_thread_impl.cc (left): > > https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... > content/renderer/render_thread_impl.cc:996: // to the browser process. > On 2016/07/19 16:42:54, esprehn wrote: > > You're doing the opposite of what the comment says we have to do now, why is > > that correct? You now destroy the message loop before the discardable memory > > manager shuts down which means the IPCs won't make it out. > > The comment looks inaccurate to me. > ChildDiscardableSharedMemoryManager spins the message loop only for a sync IPC > for allocation, and doesn't use it for shutdown. > Though it uses the main thread task runner to check if the current thread is the > main thread, the message loop itself is not needed for it. I think the comment is still valid. The following scenario can happen: 1) blink::shutdown 2) Resource's destructor gets called 3) It unlocks itself from DiscardableMemory. This can issue IPC. The message loop is needed.
On 2016/07/20 08:34:34, haraken wrote: > On 2016/07/19 17:26:52, tzik wrote: > > > https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... > > File content/renderer/render_thread_impl.cc (left): > > > > > https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... > > content/renderer/render_thread_impl.cc:996: // to the browser process. > > On 2016/07/19 16:42:54, esprehn wrote: > > > You're doing the opposite of what the comment says we have to do now, why is > > > that correct? You now destroy the message loop before the discardable memory > > > manager shuts down which means the IPCs won't make it out. > > > > The comment looks inaccurate to me. > > ChildDiscardableSharedMemoryManager spins the message loop only for a sync IPC > > for allocation, and doesn't use it for shutdown. > > Though it uses the main thread task runner to check if the current thread is > the > > main thread, the message loop itself is not needed for it. > > I think the comment is still valid. The following scenario can happen: > > 1) blink::shutdown > 2) Resource's destructor gets called > 3) It unlocks itself from DiscardableMemory. This can issue IPC. The message > loop is needed. Can you try to process all the remaining contents in the message loop before calling blink::shutdown, instead of shutting down the message loop?
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
On 2016/07/20 08:34:34, haraken wrote: > On 2016/07/19 17:26:52, tzik wrote: > > > https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... > > File content/renderer/render_thread_impl.cc (left): > > > > > https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... > > content/renderer/render_thread_impl.cc:996: // to the browser process. > > On 2016/07/19 16:42:54, esprehn wrote: > > > You're doing the opposite of what the comment says we have to do now, why is > > > that correct? You now destroy the message loop before the discardable memory > > > manager shuts down which means the IPCs won't make it out. > > > > The comment looks inaccurate to me. > > ChildDiscardableSharedMemoryManager spins the message loop only for a sync IPC > > for allocation, and doesn't use it for shutdown. > > Though it uses the main thread task runner to check if the current thread is > the > > main thread, the message loop itself is not needed for it. > > I think the comment is still valid. The following scenario can happen: > > 1) blink::shutdown > 2) Resource's destructor gets called > 3) It unlocks itself from DiscardableMemory. This can issue IPC. The message > loop is needed. Ah, that's true though it sounds weird to me. ShutdownDiscardableSharedMemoryManager probably doesn't need the message loop, but other part of Blink does. So, can we split Platform::shutdown() from blink::shutdown(), and move the message loop shutdown between them?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/07/20 08:35:36, haraken wrote: > On 2016/07/20 08:34:34, haraken wrote: > > On 2016/07/19 17:26:52, tzik wrote: > > > > > > https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... > > > File content/renderer/render_thread_impl.cc (left): > > > > > > > > > https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... > > > content/renderer/render_thread_impl.cc:996: // to the browser process. > > > On 2016/07/19 16:42:54, esprehn wrote: > > > > You're doing the opposite of what the comment says we have to do now, why > is > > > > that correct? You now destroy the message loop before the discardable > memory > > > > manager shuts down which means the IPCs won't make it out. > > > > > > The comment looks inaccurate to me. > > > ChildDiscardableSharedMemoryManager spins the message loop only for a sync > IPC > > > for allocation, and doesn't use it for shutdown. > > > Though it uses the main thread task runner to check if the current thread is > > the > > > main thread, the message loop itself is not needed for it. > > > > I think the comment is still valid. The following scenario can happen: > > > > 1) blink::shutdown > > 2) Resource's destructor gets called > > 3) It unlocks itself from DiscardableMemory. This can issue IPC. The message > > loop is needed. > > Can you try to process all the remaining contents in the message loop before > calling blink::shutdown, instead of shutting down the message loop? It's already done as base::RunLoop().RunUntilIdle();.
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/07/20 09:25:25, tzik wrote: > On 2016/07/20 08:34:34, haraken wrote: > > On 2016/07/19 17:26:52, tzik wrote: > > > > > > https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... > > > File content/renderer/render_thread_impl.cc (left): > > > > > > > > > https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... > > > content/renderer/render_thread_impl.cc:996: // to the browser process. > > > On 2016/07/19 16:42:54, esprehn wrote: > > > > You're doing the opposite of what the comment says we have to do now, why > is > > > > that correct? You now destroy the message loop before the discardable > memory > > > > manager shuts down which means the IPCs won't make it out. > > > > > > The comment looks inaccurate to me. > > > ChildDiscardableSharedMemoryManager spins the message loop only for a sync > IPC > > > for allocation, and doesn't use it for shutdown. > > > Though it uses the main thread task runner to check if the current thread is > > the > > > main thread, the message loop itself is not needed for it. > > > > I think the comment is still valid. The following scenario can happen: > > > > 1) blink::shutdown > > 2) Resource's destructor gets called > > 3) It unlocks itself from DiscardableMemory. This can issue IPC. The message > > loop is needed. > > Ah, that's true though it sounds weird to me. > ShutdownDiscardableSharedMemoryManager probably doesn't need the message loop, > but other part of Blink does. Oh, I noticed that we already removed code that calls unlock() in Resource's destructor. Then the issue should be gone. See https://codereview.chromium.org/1647443002/ for more details. > So, can we split Platform::shutdown() from blink::shutdown(), and move the > message loop shutdown between them? That is doable but sounds a bit nasty to me. >> Can you try to process all the remaining contents in the message loop before >> calling blink::shutdown, instead of shutting down the message loop? > > It's already done as base::RunLoop().RunUntilIdle();. Then why do we still have the issue (i.e., how is it possible that the message loop holds tasks that retain Persistent handles when blink::shutdown gets called)?
On 2016/07/20 10:19:23, haraken wrote: > On 2016/07/20 09:25:25, tzik wrote: > > On 2016/07/20 08:34:34, haraken wrote: > > > On 2016/07/19 17:26:52, tzik wrote: > > > > > > > > > > https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... > > > > File content/renderer/render_thread_impl.cc (left): > > > > > > > > > > > > > > https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... > > > > content/renderer/render_thread_impl.cc:996: // to the browser process. > > > > On 2016/07/19 16:42:54, esprehn wrote: > > > > > You're doing the opposite of what the comment says we have to do now, > why > > is > > > > > that correct? You now destroy the message loop before the discardable > > memory > > > > > manager shuts down which means the IPCs won't make it out. > > > > > > > > The comment looks inaccurate to me. > > > > ChildDiscardableSharedMemoryManager spins the message loop only for a sync > > IPC > > > > for allocation, and doesn't use it for shutdown. > > > > Though it uses the main thread task runner to check if the current thread > is > > > the > > > > main thread, the message loop itself is not needed for it. > > > > > > I think the comment is still valid. The following scenario can happen: > > > > > > 1) blink::shutdown > > > 2) Resource's destructor gets called > > > 3) It unlocks itself from DiscardableMemory. This can issue IPC. The message > > > loop is needed. > > > > Ah, that's true though it sounds weird to me. > > ShutdownDiscardableSharedMemoryManager probably doesn't need the message loop, > > but other part of Blink does. > > Oh, I noticed that we already removed code that calls unlock() in Resource's > destructor. Then the issue should be gone. > > See https://codereview.chromium.org/1647443002/ for more details. Since Resource::allClientsAndObserversRemoved() calls Resource::unlock(), a destructor of some client class may causes Resource::unlock(). > > > So, can we split Platform::shutdown() from blink::shutdown(), and move the > > message loop shutdown between them? > > That is doable but sounds a bit nasty to me. > > >> Can you try to process all the remaining contents in the message loop before > >> calling blink::shutdown, instead of shutting down the message loop? > > > > It's already done as base::RunLoop().RunUntilIdle();. > > Then why do we still have the issue (i.e., how is it possible that the message > loop holds tasks that retain Persistent handles when blink::shutdown gets > called)? It's probably in delayed queue in the message queue, posted by destructors, or posted from other threads, e.g. IO thread.
On 2016/07/20 10:50:22, tzik wrote: > On 2016/07/20 10:19:23, haraken wrote: > > On 2016/07/20 09:25:25, tzik wrote: > > > On 2016/07/20 08:34:34, haraken wrote: > > > > On 2016/07/19 17:26:52, tzik wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... > > > > > File content/renderer/render_thread_impl.cc (left): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... > > > > > content/renderer/render_thread_impl.cc:996: // to the browser process. > > > > > On 2016/07/19 16:42:54, esprehn wrote: > > > > > > You're doing the opposite of what the comment says we have to do now, > > why > > > is > > > > > > that correct? You now destroy the message loop before the discardable > > > memory > > > > > > manager shuts down which means the IPCs won't make it out. > > > > > > > > > > The comment looks inaccurate to me. > > > > > ChildDiscardableSharedMemoryManager spins the message loop only for a > sync > > > IPC > > > > > for allocation, and doesn't use it for shutdown. > > > > > Though it uses the main thread task runner to check if the current > thread > > is > > > > the > > > > > main thread, the message loop itself is not needed for it. > > > > > > > > I think the comment is still valid. The following scenario can happen: > > > > > > > > 1) blink::shutdown > > > > 2) Resource's destructor gets called > > > > 3) It unlocks itself from DiscardableMemory. This can issue IPC. The > message > > > > loop is needed. > > > > > > Ah, that's true though it sounds weird to me. > > > ShutdownDiscardableSharedMemoryManager probably doesn't need the message > loop, > > > but other part of Blink does. > > > > Oh, I noticed that we already removed code that calls unlock() in Resource's > > destructor. Then the issue should be gone. > > > > See https://codereview.chromium.org/1647443002/ for more details. > > Since Resource::allClientsAndObserversRemoved() calls Resource::unlock(), a > destructor of some client class may causes Resource::unlock(). > > > > > > So, can we split Platform::shutdown() from blink::shutdown(), and move the > > > message loop shutdown between them? > > > > That is doable but sounds a bit nasty to me. > > > > >> Can you try to process all the remaining contents in the message loop > before > > >> calling blink::shutdown, instead of shutting down the message loop? > > > > > > It's already done as base::RunLoop().RunUntilIdle();. > > > > Then why do we still have the issue (i.e., how is it possible that the message > > loop holds tasks that retain Persistent handles when blink::shutdown gets > > called)? > > It's probably in delayed queue in the message queue, posted by destructors, or > posted from other threads, e.g. IO thread. Would you help me understand what Persistent handles you're talking about? Conceptually all non-main-thread Persistent handles must be explicitly cleared before content/ calls blink::shutdown. It looks weird that some destructor implicitly posts a task during blink::shutdown and creates new Persistent handles.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/07/20 10:56:47, haraken wrote: > On 2016/07/20 10:50:22, tzik wrote: > > On 2016/07/20 10:19:23, haraken wrote: > > > On 2016/07/20 09:25:25, tzik wrote: > > > > On 2016/07/20 08:34:34, haraken wrote: > > > > > On 2016/07/19 17:26:52, tzik wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... > > > > > > File content/renderer/render_thread_impl.cc (left): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... > > > > > > content/renderer/render_thread_impl.cc:996: // to the browser process. > > > > > > On 2016/07/19 16:42:54, esprehn wrote: > > > > > > > You're doing the opposite of what the comment says we have to do > now, > > > why > > > > is > > > > > > > that correct? You now destroy the message loop before the > discardable > > > > memory > > > > > > > manager shuts down which means the IPCs won't make it out. > > > > > > > > > > > > The comment looks inaccurate to me. > > > > > > ChildDiscardableSharedMemoryManager spins the message loop only for a > > sync > > > > IPC > > > > > > for allocation, and doesn't use it for shutdown. > > > > > > Though it uses the main thread task runner to check if the current > > thread > > > is > > > > > the > > > > > > main thread, the message loop itself is not needed for it. > > > > > > > > > > I think the comment is still valid. The following scenario can happen: > > > > > > > > > > 1) blink::shutdown > > > > > 2) Resource's destructor gets called > > > > > 3) It unlocks itself from DiscardableMemory. This can issue IPC. The > > message > > > > > loop is needed. > > > > > > > > Ah, that's true though it sounds weird to me. > > > > ShutdownDiscardableSharedMemoryManager probably doesn't need the message > > loop, > > > > but other part of Blink does. > > > > > > Oh, I noticed that we already removed code that calls unlock() in Resource's > > > destructor. Then the issue should be gone. > > > > > > See https://codereview.chromium.org/1647443002/ for more details. > > > > Since Resource::allClientsAndObserversRemoved() calls Resource::unlock(), a > > destructor of some client class may causes Resource::unlock(). > > > > > > > > > So, can we split Platform::shutdown() from blink::shutdown(), and move the > > > > message loop shutdown between them? > > > > > > That is doable but sounds a bit nasty to me. > > > > > > >> Can you try to process all the remaining contents in the message loop > > before > > > >> calling blink::shutdown, instead of shutting down the message loop? > > > > > > > > It's already done as base::RunLoop().RunUntilIdle();. > > > > > > Then why do we still have the issue (i.e., how is it possible that the > message > > > loop holds tasks that retain Persistent handles when blink::shutdown gets > > > called)? > > > > It's probably in delayed queue in the message queue, posted by destructors, or > > posted from other threads, e.g. IO thread. > > Would you help me understand what Persistent handles you're talking about? I don't have the concrete case for Persistent, here is a rerated example for WeakPersistent case: https://cluster-fuzz.appspot.com/testcase?key=6417573642240000, a destruction observer of the message loop touches a callback that has WeakPersistent. > > Conceptually all non-main-thread Persistent handles must be explicitly cleared > before content/ calls blink::shutdown. It looks weird that some destructor > implicitly posts a task during blink::shutdown and creates new Persistent > handles.
On 2016/07/20 12:04:33, tzik wrote: > On 2016/07/20 10:56:47, haraken wrote: > > On 2016/07/20 10:50:22, tzik wrote: > > > On 2016/07/20 10:19:23, haraken wrote: > > > > On 2016/07/20 09:25:25, tzik wrote: > > > > > On 2016/07/20 08:34:34, haraken wrote: > > > > > > On 2016/07/19 17:26:52, tzik wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... > > > > > > > File content/renderer/render_thread_impl.cc (left): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... > > > > > > > content/renderer/render_thread_impl.cc:996: // to the browser > process. > > > > > > > On 2016/07/19 16:42:54, esprehn wrote: > > > > > > > > You're doing the opposite of what the comment says we have to do > > now, > > > > why > > > > > is > > > > > > > > that correct? You now destroy the message loop before the > > discardable > > > > > memory > > > > > > > > manager shuts down which means the IPCs won't make it out. > > > > > > > > > > > > > > The comment looks inaccurate to me. > > > > > > > ChildDiscardableSharedMemoryManager spins the message loop only for > a > > > sync > > > > > IPC > > > > > > > for allocation, and doesn't use it for shutdown. > > > > > > > Though it uses the main thread task runner to check if the current > > > thread > > > > is > > > > > > the > > > > > > > main thread, the message loop itself is not needed for it. > > > > > > > > > > > > I think the comment is still valid. The following scenario can happen: > > > > > > > > > > > > 1) blink::shutdown > > > > > > 2) Resource's destructor gets called > > > > > > 3) It unlocks itself from DiscardableMemory. This can issue IPC. The > > > message > > > > > > loop is needed. > > > > > > > > > > Ah, that's true though it sounds weird to me. > > > > > ShutdownDiscardableSharedMemoryManager probably doesn't need the message > > > loop, > > > > > but other part of Blink does. > > > > > > > > Oh, I noticed that we already removed code that calls unlock() in > Resource's > > > > destructor. Then the issue should be gone. > > > > > > > > See https://codereview.chromium.org/1647443002/ for more details. > > > > > > Since Resource::allClientsAndObserversRemoved() calls Resource::unlock(), a > > > destructor of some client class may causes Resource::unlock(). > > > > > > > > > > > > So, can we split Platform::shutdown() from blink::shutdown(), and move > the > > > > > message loop shutdown between them? > > > > > > > > That is doable but sounds a bit nasty to me. > > > > > > > > >> Can you try to process all the remaining contents in the message loop > > > before > > > > >> calling blink::shutdown, instead of shutting down the message loop? > > > > > > > > > > It's already done as base::RunLoop().RunUntilIdle();. > > > > > > > > Then why do we still have the issue (i.e., how is it possible that the > > message > > > > loop holds tasks that retain Persistent handles when blink::shutdown gets > > > > called)? > > > > > > It's probably in delayed queue in the message queue, posted by destructors, > or > > > posted from other threads, e.g. IO thread. > > > > Would you help me understand what Persistent handles you're talking about? > > I don't have the concrete case for Persistent, here is a rerated example for > WeakPersistent case: > https://cluster-fuzz.appspot.com/testcase?key=6417573642240000, a destruction > observer of the message loop touches a callback that has WeakPersistent. Who posts the task to the message loop? Is it posted after content/ calls base::RunLoop().RunUntilIdle()?
On 2016/07/20 13:44:41, haraken wrote: > On 2016/07/20 12:04:33, tzik wrote: > > On 2016/07/20 10:56:47, haraken wrote: > > > On 2016/07/20 10:50:22, tzik wrote: > > > > On 2016/07/20 10:19:23, haraken wrote: > > > > > On 2016/07/20 09:25:25, tzik wrote: > > > > > > On 2016/07/20 08:34:34, haraken wrote: > > > > > > > On 2016/07/19 17:26:52, tzik wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... > > > > > > > > File content/renderer/render_thread_impl.cc (left): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... > > > > > > > > content/renderer/render_thread_impl.cc:996: // to the browser > > process. > > > > > > > > On 2016/07/19 16:42:54, esprehn wrote: > > > > > > > > > You're doing the opposite of what the comment says we have to do > > > now, > > > > > why > > > > > > is > > > > > > > > > that correct? You now destroy the message loop before the > > > discardable > > > > > > memory > > > > > > > > > manager shuts down which means the IPCs won't make it out. > > > > > > > > > > > > > > > > The comment looks inaccurate to me. > > > > > > > > ChildDiscardableSharedMemoryManager spins the message loop only > for > > a > > > > sync > > > > > > IPC > > > > > > > > for allocation, and doesn't use it for shutdown. > > > > > > > > Though it uses the main thread task runner to check if the current > > > > thread > > > > > is > > > > > > > the > > > > > > > > main thread, the message loop itself is not needed for it. > > > > > > > > > > > > > > I think the comment is still valid. The following scenario can > happen: > > > > > > > > > > > > > > 1) blink::shutdown > > > > > > > 2) Resource's destructor gets called > > > > > > > 3) It unlocks itself from DiscardableMemory. This can issue IPC. The > > > > message > > > > > > > loop is needed. > > > > > > > > > > > > Ah, that's true though it sounds weird to me. > > > > > > ShutdownDiscardableSharedMemoryManager probably doesn't need the > message > > > > loop, > > > > > > but other part of Blink does. > > > > > > > > > > Oh, I noticed that we already removed code that calls unlock() in > > Resource's > > > > > destructor. Then the issue should be gone. > > > > > > > > > > See https://codereview.chromium.org/1647443002/ for more details. > > > > > > > > Since Resource::allClientsAndObserversRemoved() calls Resource::unlock(), > a > > > > destructor of some client class may causes Resource::unlock(). > > > > > > > > > > > > > > > So, can we split Platform::shutdown() from blink::shutdown(), and move > > the > > > > > > message loop shutdown between them? > > > > > > > > > > That is doable but sounds a bit nasty to me. > > > > > > > > > > >> Can you try to process all the remaining contents in the message loop > > > > before > > > > > >> calling blink::shutdown, instead of shutting down the message loop? > > > > > > > > > > > > It's already done as base::RunLoop().RunUntilIdle();. > > > > > > > > > > Then why do we still have the issue (i.e., how is it possible that the > > > message > > > > > loop holds tasks that retain Persistent handles when blink::shutdown > gets > > > > > called)? > > > > > > > > It's probably in delayed queue in the message queue, posted by > destructors, > > or > > > > posted from other threads, e.g. IO thread. > > > > > > Would you help me understand what Persistent handles you're talking about? > > > > I don't have the concrete case for Persistent, here is a rerated example for > > WeakPersistent case: > > https://cluster-fuzz.appspot.com/testcase?key=6417573642240000, a destruction > > observer of the message loop touches a callback that has WeakPersistent. > > Who posts the task to the message loop? Is it posted after content/ calls > base::RunLoop().RunUntilIdle()? On that case, no one posted it. It just touches a WeakPersistent after the MessageLoop shutdown. Another case below is a potential failcase: https://cluster-fuzz.appspot.com/testcase?key=4572050421448704 RTCCertificateGeneratorRequest posts a task with blink::WebRTCCertificateCallback to a worker thread, and posts back another task to the main thread. This task chain passes a WebRTCCertificateObserver that contains a Persistent<ScriptPromiseResolver>, and the final PostTask can be done after the base::RunLoop().RunUntilIdle() call. https://cs.chromium.org/chromium/src/content/renderer/media/rtc_certificate_g...
On 2016/07/20 14:27:04, tzik wrote: > On 2016/07/20 13:44:41, haraken wrote: > > On 2016/07/20 12:04:33, tzik wrote: > > > On 2016/07/20 10:56:47, haraken wrote: > > > > On 2016/07/20 10:50:22, tzik wrote: > > > > > On 2016/07/20 10:19:23, haraken wrote: > > > > > > On 2016/07/20 09:25:25, tzik wrote: > > > > > > > On 2016/07/20 08:34:34, haraken wrote: > > > > > > > > On 2016/07/19 17:26:52, tzik wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... > > > > > > > > > File content/renderer/render_thread_impl.cc (left): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... > > > > > > > > > content/renderer/render_thread_impl.cc:996: // to the browser > > > process. > > > > > > > > > On 2016/07/19 16:42:54, esprehn wrote: > > > > > > > > > > You're doing the opposite of what the comment says we have to > do > > > > now, > > > > > > why > > > > > > > is > > > > > > > > > > that correct? You now destroy the message loop before the > > > > discardable > > > > > > > memory > > > > > > > > > > manager shuts down which means the IPCs won't make it out. > > > > > > > > > > > > > > > > > > The comment looks inaccurate to me. > > > > > > > > > ChildDiscardableSharedMemoryManager spins the message loop only > > for > > > a > > > > > sync > > > > > > > IPC > > > > > > > > > for allocation, and doesn't use it for shutdown. > > > > > > > > > Though it uses the main thread task runner to check if the > current > > > > > thread > > > > > > is > > > > > > > > the > > > > > > > > > main thread, the message loop itself is not needed for it. > > > > > > > > > > > > > > > > I think the comment is still valid. The following scenario can > > happen: > > > > > > > > > > > > > > > > 1) blink::shutdown > > > > > > > > 2) Resource's destructor gets called > > > > > > > > 3) It unlocks itself from DiscardableMemory. This can issue IPC. > The > > > > > message > > > > > > > > loop is needed. > > > > > > > > > > > > > > Ah, that's true though it sounds weird to me. > > > > > > > ShutdownDiscardableSharedMemoryManager probably doesn't need the > > message > > > > > loop, > > > > > > > but other part of Blink does. > > > > > > > > > > > > Oh, I noticed that we already removed code that calls unlock() in > > > Resource's > > > > > > destructor. Then the issue should be gone. > > > > > > > > > > > > See https://codereview.chromium.org/1647443002/ for more details. > > > > > > > > > > Since Resource::allClientsAndObserversRemoved() calls > Resource::unlock(), > > a > > > > > destructor of some client class may causes Resource::unlock(). > > > > > > > > > > > > > > > > > > So, can we split Platform::shutdown() from blink::shutdown(), and > move > > > the > > > > > > > message loop shutdown between them? > > > > > > > > > > > > That is doable but sounds a bit nasty to me. > > > > > > > > > > > > >> Can you try to process all the remaining contents in the message > loop > > > > > before > > > > > > >> calling blink::shutdown, instead of shutting down the message loop? > > > > > > > > > > > > > > It's already done as base::RunLoop().RunUntilIdle();. > > > > > > > > > > > > Then why do we still have the issue (i.e., how is it possible that the > > > > message > > > > > > loop holds tasks that retain Persistent handles when blink::shutdown > > gets > > > > > > called)? > > > > > > > > > > It's probably in delayed queue in the message queue, posted by > > destructors, > > > or > > > > > posted from other threads, e.g. IO thread. > > > > > > > > Would you help me understand what Persistent handles you're talking about? > > > > > > I don't have the concrete case for Persistent, here is a rerated example for > > > WeakPersistent case: > > > https://cluster-fuzz.appspot.com/testcase?key=6417573642240000, a > destruction > > > observer of the message loop touches a callback that has WeakPersistent. > > > > Who posts the task to the message loop? Is it posted after content/ calls > > base::RunLoop().RunUntilIdle()? > > On that case, no one posted it. It just touches a WeakPersistent after the > MessageLoop shutdown. > Another case below is a potential failcase: > https://cluster-fuzz.appspot.com/testcase?key=4572050421448704 > > RTCCertificateGeneratorRequest posts a task with > blink::WebRTCCertificateCallback to a worker thread, and posts back another task > to the main thread. > This task chain passes a WebRTCCertificateObserver that contains a > Persistent<ScriptPromiseResolver>, and the final PostTask can be done after the > base::RunLoop().RunUntilIdle() call. > https://cs.chromium.org/chromium/src/content/renderer/media/rtc_certificate_g... It looks weird that RTCCertificateGeneratorRequest is working (i.e., posting a task) while content/ is shutting down Blink. Can we stop RTCCertificateGeneratorRequest before content/ starts the shutdown sequence? My point is that content/ should stop anything that may access Blink before calling blink::shutdown. We should follow the rule here.
On 2016/07/20 16:33:36, haraken wrote: > On 2016/07/20 14:27:04, tzik wrote: > > On 2016/07/20 13:44:41, haraken wrote: > > > On 2016/07/20 12:04:33, tzik wrote: > > > > On 2016/07/20 10:56:47, haraken wrote: > > > > > On 2016/07/20 10:50:22, tzik wrote: > > > > > > On 2016/07/20 10:19:23, haraken wrote: > > > > > > > On 2016/07/20 09:25:25, tzik wrote: > > > > > > > > On 2016/07/20 08:34:34, haraken wrote: > > > > > > > > > On 2016/07/19 17:26:52, tzik wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... > > > > > > > > > > File content/renderer/render_thread_impl.cc (left): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... > > > > > > > > > > content/renderer/render_thread_impl.cc:996: // to the browser > > > > process. > > > > > > > > > > On 2016/07/19 16:42:54, esprehn wrote: > > > > > > > > > > > You're doing the opposite of what the comment says we have > to > > do > > > > > now, > > > > > > > why > > > > > > > > is > > > > > > > > > > > that correct? You now destroy the message loop before the > > > > > discardable > > > > > > > > memory > > > > > > > > > > > manager shuts down which means the IPCs won't make it out. > > > > > > > > > > > > > > > > > > > > The comment looks inaccurate to me. > > > > > > > > > > ChildDiscardableSharedMemoryManager spins the message loop > only > > > for > > > > a > > > > > > sync > > > > > > > > IPC > > > > > > > > > > for allocation, and doesn't use it for shutdown. > > > > > > > > > > Though it uses the main thread task runner to check if the > > current > > > > > > thread > > > > > > > is > > > > > > > > > the > > > > > > > > > > main thread, the message loop itself is not needed for it. > > > > > > > > > > > > > > > > > > I think the comment is still valid. The following scenario can > > > happen: > > > > > > > > > > > > > > > > > > 1) blink::shutdown > > > > > > > > > 2) Resource's destructor gets called > > > > > > > > > 3) It unlocks itself from DiscardableMemory. This can issue IPC. > > The > > > > > > message > > > > > > > > > loop is needed. > > > > > > > > > > > > > > > > Ah, that's true though it sounds weird to me. > > > > > > > > ShutdownDiscardableSharedMemoryManager probably doesn't need the > > > message > > > > > > loop, > > > > > > > > but other part of Blink does. > > > > > > > > > > > > > > Oh, I noticed that we already removed code that calls unlock() in > > > > Resource's > > > > > > > destructor. Then the issue should be gone. > > > > > > > > > > > > > > See https://codereview.chromium.org/1647443002/ for more details. > > > > > > > > > > > > Since Resource::allClientsAndObserversRemoved() calls > > Resource::unlock(), > > > a > > > > > > destructor of some client class may causes Resource::unlock(). > > > > > > > > > > > > > > > > > > > > > So, can we split Platform::shutdown() from blink::shutdown(), and > > move > > > > the > > > > > > > > message loop shutdown between them? > > > > > > > > > > > > > > That is doable but sounds a bit nasty to me. > > > > > > > > > > > > > > >> Can you try to process all the remaining contents in the message > > loop > > > > > > before > > > > > > > >> calling blink::shutdown, instead of shutting down the message > loop? > > > > > > > > > > > > > > > > It's already done as base::RunLoop().RunUntilIdle();. > > > > > > > > > > > > > > Then why do we still have the issue (i.e., how is it possible that > the > > > > > message > > > > > > > loop holds tasks that retain Persistent handles when blink::shutdown > > > gets > > > > > > > called)? > > > > > > > > > > > > It's probably in delayed queue in the message queue, posted by > > > destructors, > > > > or > > > > > > posted from other threads, e.g. IO thread. > > > > > > > > > > Would you help me understand what Persistent handles you're talking > about? > > > > > > > > I don't have the concrete case for Persistent, here is a rerated example > for > > > > WeakPersistent case: > > > > https://cluster-fuzz.appspot.com/testcase?key=6417573642240000, a > > destruction > > > > observer of the message loop touches a callback that has WeakPersistent. > > > > > > Who posts the task to the message loop? Is it posted after content/ calls > > > base::RunLoop().RunUntilIdle()? > > > > On that case, no one posted it. It just touches a WeakPersistent after the > > MessageLoop shutdown. > > Another case below is a potential failcase: > > https://cluster-fuzz.appspot.com/testcase?key=4572050421448704 > > > > RTCCertificateGeneratorRequest posts a task with > > blink::WebRTCCertificateCallback to a worker thread, and posts back another > task > > to the main thread. > > This task chain passes a WebRTCCertificateObserver that contains a > > Persistent<ScriptPromiseResolver>, and the final PostTask can be done after > the > > base::RunLoop().RunUntilIdle() call. > > > https://cs.chromium.org/chromium/src/content/renderer/media/rtc_certificate_g... > > It looks weird that RTCCertificateGeneratorRequest is working (i.e., posting a > task) while content/ is shutting down Blink. Can we stop > RTCCertificateGeneratorRequest before content/ starts the shutdown sequence? > > My point is that content/ should stop anything that may access Blink before > calling blink::shutdown. We should follow the rule here. Does that include destroying a Persistent<>? That requires major surgery, and looks hard to enforce. That implies we have to forbid to Bind any Persistent and any object that may contain Persintent into a Callback, since we don't have no way to clear posted task.
On 2016/07/20 18:01:55, tzik wrote: > On 2016/07/20 16:33:36, haraken wrote: > > On 2016/07/20 14:27:04, tzik wrote: > > > On 2016/07/20 13:44:41, haraken wrote: > > > > On 2016/07/20 12:04:33, tzik wrote: > > > > > On 2016/07/20 10:56:47, haraken wrote: > > > > > > On 2016/07/20 10:50:22, tzik wrote: > > > > > > > On 2016/07/20 10:19:23, haraken wrote: > > > > > > > > On 2016/07/20 09:25:25, tzik wrote: > > > > > > > > > On 2016/07/20 08:34:34, haraken wrote: > > > > > > > > > > On 2016/07/19 17:26:52, tzik wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... > > > > > > > > > > > File content/renderer/render_thread_impl.cc (left): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2159123002/diff/1/content/renderer/render_thr... > > > > > > > > > > > content/renderer/render_thread_impl.cc:996: // to the > browser > > > > > process. > > > > > > > > > > > On 2016/07/19 16:42:54, esprehn wrote: > > > > > > > > > > > > You're doing the opposite of what the comment says we have > > to > > > do > > > > > > now, > > > > > > > > why > > > > > > > > > is > > > > > > > > > > > > that correct? You now destroy the message loop before the > > > > > > discardable > > > > > > > > > memory > > > > > > > > > > > > manager shuts down which means the IPCs won't make it out. > > > > > > > > > > > > > > > > > > > > > > The comment looks inaccurate to me. > > > > > > > > > > > ChildDiscardableSharedMemoryManager spins the message loop > > only > > > > for > > > > > a > > > > > > > sync > > > > > > > > > IPC > > > > > > > > > > > for allocation, and doesn't use it for shutdown. > > > > > > > > > > > Though it uses the main thread task runner to check if the > > > current > > > > > > > thread > > > > > > > > is > > > > > > > > > > the > > > > > > > > > > > main thread, the message loop itself is not needed for it. > > > > > > > > > > > > > > > > > > > > I think the comment is still valid. The following scenario can > > > > happen: > > > > > > > > > > > > > > > > > > > > 1) blink::shutdown > > > > > > > > > > 2) Resource's destructor gets called > > > > > > > > > > 3) It unlocks itself from DiscardableMemory. This can issue > IPC. > > > The > > > > > > > message > > > > > > > > > > loop is needed. > > > > > > > > > > > > > > > > > > Ah, that's true though it sounds weird to me. > > > > > > > > > ShutdownDiscardableSharedMemoryManager probably doesn't need the > > > > message > > > > > > > loop, > > > > > > > > > but other part of Blink does. > > > > > > > > > > > > > > > > Oh, I noticed that we already removed code that calls unlock() in > > > > > Resource's > > > > > > > > destructor. Then the issue should be gone. > > > > > > > > > > > > > > > > See https://codereview.chromium.org/1647443002/ for more details. > > > > > > > > > > > > > > Since Resource::allClientsAndObserversRemoved() calls > > > Resource::unlock(), > > > > a > > > > > > > destructor of some client class may causes Resource::unlock(). > > > > > > > > > > > > > > > > > > > > > > > > So, can we split Platform::shutdown() from blink::shutdown(), > and > > > move > > > > > the > > > > > > > > > message loop shutdown between them? > > > > > > > > > > > > > > > > That is doable but sounds a bit nasty to me. > > > > > > > > > > > > > > > > >> Can you try to process all the remaining contents in the > message > > > loop > > > > > > > before > > > > > > > > >> calling blink::shutdown, instead of shutting down the message > > loop? > > > > > > > > > > > > > > > > > > It's already done as base::RunLoop().RunUntilIdle();. > > > > > > > > > > > > > > > > Then why do we still have the issue (i.e., how is it possible that > > the > > > > > > message > > > > > > > > loop holds tasks that retain Persistent handles when > blink::shutdown > > > > gets > > > > > > > > called)? > > > > > > > > > > > > > > It's probably in delayed queue in the message queue, posted by > > > > destructors, > > > > > or > > > > > > > posted from other threads, e.g. IO thread. > > > > > > > > > > > > Would you help me understand what Persistent handles you're talking > > about? > > > > > > > > > > I don't have the concrete case for Persistent, here is a rerated example > > for > > > > > WeakPersistent case: > > > > > https://cluster-fuzz.appspot.com/testcase?key=6417573642240000, a > > > destruction > > > > > observer of the message loop touches a callback that has WeakPersistent. > > > > > > > > Who posts the task to the message loop? Is it posted after content/ calls > > > > base::RunLoop().RunUntilIdle()? > > > > > > On that case, no one posted it. It just touches a WeakPersistent after the > > > MessageLoop shutdown. > > > Another case below is a potential failcase: > > > https://cluster-fuzz.appspot.com/testcase?key=4572050421448704 > > > > > > RTCCertificateGeneratorRequest posts a task with > > > blink::WebRTCCertificateCallback to a worker thread, and posts back another > > task > > > to the main thread. > > > This task chain passes a WebRTCCertificateObserver that contains a > > > Persistent<ScriptPromiseResolver>, and the final PostTask can be done after > > the > > > base::RunLoop().RunUntilIdle() call. > > > > > > https://cs.chromium.org/chromium/src/content/renderer/media/rtc_certificate_g... > > > > It looks weird that RTCCertificateGeneratorRequest is working (i.e., posting a > > task) while content/ is shutting down Blink. Can we stop > > RTCCertificateGeneratorRequest before content/ starts the shutdown sequence? > > > > My point is that content/ should stop anything that may access Blink before > > calling blink::shutdown. We should follow the rule here. > > Does that include destroying a Persistent<>? That requires major surgery, and > looks hard to enforce. We have enforced it so far. content/ should be responsible for clearing any client pointers (to Blink), shut down Blink things etc before calling blink::shutdown(). > That implies we have to forbid to Bind any Persistent and any object that may > contain Persintent into a Callback, since we don't have no way to clear posted > task. So my question is why content/ cannot make sure that there is no callback running before calling blink::shutdown(). Remember that this is not a problem limited to message loops. If you shut down Blink without stopping RTCCertificateGeneratorRequest, it may be possible that some destructor in Blink (called in Oilpan's shutdown GC) calls some destructor in content/, which may touch the still-live RTCCertificateGeneratorRequest, which touches Blink and then crashes (we've caused many crashes like this). The only reliable way to fix these crashes is to properly stop RTCCertificateGeneratorRequest before calling blink::shutdown.
Sorry, I forgot to review this CL... As discussed offline, let's land PS4 as a short-term fix. In long-term, we should remove the graceful shutdown sequence (as discussed in platform-architecture-dev@). Given that PS4 is splitting Platform::shutdown from blink::shutdown, shall we split Platform::initialize from blink::initialize for consistency?
What's the status here? :)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/06 01:20:00, esprehn wrote: > What's the status here? :) Let me resume this. > Given that PS4 is splitting Platform::shutdown from blink::shutdown, shall we > split Platform::initialize from blink::initialize for consistency? Ok, I did split out blink::Platform::initialize from blink::initialize. Could you take another look?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2159123002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebKit.cpp (right): https://codereview.chromium.org/2159123002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebKit.cpp:85: // Platform::initialize(platform); Remove this. https://codereview.chromium.org/2159123002/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebKit.h (right): https://codereview.chromium.org/2159123002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/web/WebKit.h:40: // Platform must be initialized before Blink initialization. Can we add an assert for this?
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Shutdown renderer main message loop before blink::shutdown() blink::Persistent requires all instances to be destroyed before ThreadState shutdown, which happens in blink::shutdown(). However, some blink::Persistent are held by MessageLoop as bound arguments. That causes renderer crash on the destructor of blink::Persistent. This CL moves MessageLoop shutdown before blink::shutdown, so that message loop doesn't hold any task on ThreadState shutdown. BUG=627004 ========== to ========== Shutdown renderer main message loop before blink::shutdown() blink::Persistent requires all instances to be destroyed before ThreadState shutdown, which happens in blink::shutdown(). However, some blink::Persistent are held by MessageLoop as bound arguments. That causes renderer crash on the destructor of blink::Persistent. This CL moves MessageLoop shutdown before blink::Platform::shutdown, so that message loop doesn't hold any task on ThreadState shutdown. BUG=627004 ==========
tzik@chromium.org changed reviewers: + dalecurtis@google.com
tzik@chromium.org changed reviewers: + dalecurtis@chromium.org - dalecurtis@google.com
tzik@chromium.org changed reviewers: + kinuko@chromium.org
PTAL to: kinuko: //content, if you have time. dalecurtis: //media tkent: public/web https://codereview.chromium.org/2159123002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebKit.cpp (right): https://codereview.chromium.org/2159123002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebKit.cpp:85: // Platform::initialize(platform); On 2016/08/08 10:50:19, haraken wrote: > > Remove this. Done. https://codereview.chromium.org/2159123002/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebKit.h (right): https://codereview.chromium.org/2159123002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/web/WebKit.h:40: // Platform must be initialized before Blink initialization. On 2016/08/08 10:50:19, haraken wrote: > > Can we add an assert for this? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2159123002/diff/80001/content/child/child_thr... File content/child/child_thread_impl.h (right): https://codereview.chromium.org/2159123002/diff/80001/content/child/child_thr... content/child/child_thread_impl.h:185: base::SingleThreadTaskRunner* main_task_runner() const { main_thread_->main_task_runner() looks slightly cryptic, what 'main' stands for here? https://codereview.chromium.org/2159123002/diff/80001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2159123002/diff/80001/content/renderer/render... content/renderer/render_thread_impl.cc:994: base::RunLoop().RunUntilIdle(); Hmm, I feel uneasy calling this in production code. Do we really have a guarantee that this finishes? In browser process we prefer just dropping tasks at least after processing limited # of tasks. How frequently does the crash happen?
https://codereview.chromium.org/2159123002/diff/80001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2159123002/diff/80001/content/renderer/render... content/renderer/render_thread_impl.cc:994: base::RunLoop().RunUntilIdle(); On 2016/08/09 14:12:18, kinuko wrote: > Hmm, I feel uneasy calling this in production code. Do we really have a > guarantee that this finishes? In browser process we prefer just dropping tasks > at least after processing limited # of tasks. > > How frequently does the crash happen? Note that we're already calling RunUntilIdle at line 982. So this RunUntileIdle won't make things that worse. On the other hand, I totally agree with your point and that's why I want to entirely remove the graceful shutdown sequence.
lgtm
https://codereview.chromium.org/2159123002/diff/80001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2159123002/diff/80001/content/renderer/render... content/renderer/render_thread_impl.cc:994: base::RunLoop().RunUntilIdle(); On 2016/08/09 15:46:58, haraken wrote: > On 2016/08/09 14:12:18, kinuko wrote: > > Hmm, I feel uneasy calling this in production code. Do we really have a > > guarantee that this finishes? In browser process we prefer just dropping tasks > > at least after processing limited # of tasks. > > > > How frequently does the crash happen? > > Note that we're already calling RunUntilIdle at line 982. So this RunUntileIdle > won't make things that worse. > > On the other hand, I totally agree with your point and that's why I want to > entirely remove the graceful shutdown sequence. I see that we're already calling it... thanks. Another question is if we are sure that running message loop until idle runs tasks that need to run (e.g. no delayed tasks would hold persistent handles)-- if not making this change as a stop-gap doesn't feel very convincing to me (depending on the severity of the original crash).
On 2016/08/11 06:10:56, kinuko wrote: > https://codereview.chromium.org/2159123002/diff/80001/content/renderer/render... > File content/renderer/render_thread_impl.cc (right): > > https://codereview.chromium.org/2159123002/diff/80001/content/renderer/render... > content/renderer/render_thread_impl.cc:994: base::RunLoop().RunUntilIdle(); > On 2016/08/09 15:46:58, haraken wrote: > > On 2016/08/09 14:12:18, kinuko wrote: > > > Hmm, I feel uneasy calling this in production code. Do we really have a > > > guarantee that this finishes? In browser process we prefer just dropping > tasks > > > at least after processing limited # of tasks. > > > > > > How frequently does the crash happen? > > > > Note that we're already calling RunUntilIdle at line 982. So this > RunUntileIdle > > won't make things that worse. > > > > On the other hand, I totally agree with your point and that's why I want to > > entirely remove the graceful shutdown sequence. > > I see that we're already calling it... thanks. Another question is if we are > sure that running message loop until idle runs tasks that need to run (e.g. no > delayed tasks would hold persistent handles)-- if not making this change as a > stop-gap doesn't feel very convincing to me (depending on the severity of the > original crash). Honestly speaking, I don't think it's productive to try to make this CL more correct :) If the original crash is not that severe, maybe should we try to invest our effort on removing blink::shutdown instead?
On 2016/08/11 12:23:13, haraken wrote: > On 2016/08/11 06:10:56, kinuko wrote: > > > https://codereview.chromium.org/2159123002/diff/80001/content/renderer/render... > > File content/renderer/render_thread_impl.cc (right): > > > > > https://codereview.chromium.org/2159123002/diff/80001/content/renderer/render... > > content/renderer/render_thread_impl.cc:994: base::RunLoop().RunUntilIdle(); > > On 2016/08/09 15:46:58, haraken wrote: > > > On 2016/08/09 14:12:18, kinuko wrote: > > > > Hmm, I feel uneasy calling this in production code. Do we really have a > > > > guarantee that this finishes? In browser process we prefer just dropping > > tasks > > > > at least after processing limited # of tasks. > > > > > > > > How frequently does the crash happen? > > > > > > Note that we're already calling RunUntilIdle at line 982. So this > > RunUntileIdle > > > won't make things that worse. > > > > > > On the other hand, I totally agree with your point and that's why I want to > > > entirely remove the graceful shutdown sequence. > > > > I see that we're already calling it... thanks. Another question is if we are > > sure that running message loop until idle runs tasks that need to run (e.g. no > > delayed tasks would hold persistent handles)-- if not making this change as a > > stop-gap doesn't feel very convincing to me (depending on the severity of the > > original crash). > > Honestly speaking, I don't think it's productive to try to make this CL more > correct :) If the original crash is not that severe, maybe should we try to > invest our effort on removing blink::shutdown instead? Yep, that's what I'm asking if that could be the case.
What's the status here? Can we close this now that we're removing graceful shutdown? :)
On 2016/08/30 23:35:05, esprehn wrote: > What's the status here? Can we close this now that we're removing graceful > shutdown? :) Yes, this should no longer be needed. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
