Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(8)

Issue 2973723003: Avoid race at shutdown between browser process' IO thread and GPU thread (Closed)

Created:
4 months, 3 weeks ago by hugoh_UTC2
Modified:
4 months, 2 weeks ago
CC:
chromium-reviews, jam, darin-cc_chromium.org, piman+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Avoid race at shutdown between browser process' IO thread and GPU thread Background: - When the GPU thread is *in-process* it runs inside the browser process. - GpuChildThread is a ChildThreadImpl and a IPC::Listener. - When GpuChildThread sees an error (or shutdown) of its IPC channel, it quits its message loop (= stops the thread). Problem: The browser process' IOThread calls InProcessGpuThread::StopSoon() while the in-process GPU thread destructs itself. When InProcessGpuThread::StopSoon asks for task_runner() the in-process GPU thread has - sometimes - already destructed itself and its MessageQueue. The IOThread then sees task_runner() == null and crashes: [FATAL:ref_counted.h(484)] Assert failed: ptr_ != nullptr. 0 base::debug::StackTrace::StackTrace() 1 base::debug::StackTrace::StackTrace() 2 logging::LogMessage::~LogMessage() 3 base::Thread::StopSoon() 4 base::Thread::Stop() 5 content::InProcessGpuThread::~InProcessGpuThread() 6 content::InProcessGpuThread::~InProcessGpuThread() 7 content::GpuProcessHost::~GpuProcessHost() 8 content::GpuProcessHost::~GpuProcessHost() 9 content::BrowserChildProcessHostImpl::TerminateAll() 10 content::BrowserProcessSubThread::IOThreadPreCleanUp() 11 content::BrowserProcessSubThread::CleanUp() 12 base::Thread::ThreadMain() 13 base::(anonymous namespace)::ThreadFunc() Solution: If a thread runs in the browser process, only Thread::Stop should stop its message loop. Otherwise, QuitWhenIdle could race Thread::Stop. BUG=258935 Review-Url: https://codereview.chromium.org/2973723003 Cr-Commit-Position: refs/heads/master@{#485895} Committed: https://chromium.googlesource.com/chromium/src/+/f1c501fa3022cc7de41048cce085a3e0ade8d225

Patch Set 1 : Change destruction order #

Patch Set 2 : Move CHECK to parent class #

Patch Set 3 : Only let Thread::Stop stop child threads inside the browser process #

Messages

Total messages: 34 (16 generated)
hugoh_UTC2
PTAL
4 months, 3 weeks ago (2017-07-06 08:26:13 UTC) #2
piman
I don't think I understand the race. What causes the in-process GPU thread to "destroy ...
4 months, 3 weeks ago (2017-07-06 20:11:04 UTC) #4
hugoh_UTC2
On 2017/07/06 20:11:04, piman wrote: > I don't think I understand the race. What causes ...
4 months, 3 weeks ago (2017-07-06 21:19:30 UTC) #5
hugoh_UTC2
Oh that wasn't very readable. Reformat: [5119:5234 browser_child_process_host_impl.cc(207)] BrowserChildProcessHostImpl deletes: 0x2495270dfc20 [5119:5234 in_process_gpu_thread.cc(52)] InProcessGpuThread::~InProcessGpuThread() [5119:5234 ...
4 months, 3 weeks ago (2017-07-06 21:29:52 UTC) #6
chromium-reviews
On Thu, Jul 6, 2017 at 2:29 PM, <hugoh@opera.com> wrote: > Oh that wasn't very ...
4 months, 3 weeks ago (2017-07-06 22:34:29 UTC) #7
hugoh_UTC2
On 2017/07/06 22:34:29, chromium-reviews wrote: > On Thu, Jul 6, 2017 at 2:29 PM, <mailto:hugoh@opera.com> ...
4 months, 2 weeks ago (2017-07-07 13:50:18 UTC) #8
chromium-reviews
On Fri, Jul 7, 2017 at 6:50 AM, <hugoh@opera.com> wrote: > On 2017/07/06 22:34:29, chromium-reviews ...
4 months, 2 weeks ago (2017-07-07 18:14:52 UTC) #9
hugoh_UTC2
On 2017/07/07 18:14:52, chromium-reviews wrote: > On Fri, Jul 7, 2017 at 6:50 AM, <mailto:hugoh@opera.com> ...
4 months, 2 weeks ago (2017-07-07 21:40:46 UTC) #10
piman
On 2017/07/07 21:40:46, hugoh_UTC2 wrote: > On 2017/07/07 18:14:52, chromium-reviews wrote: > > On Fri, ...
4 months, 2 weeks ago (2017-07-07 21:51:25 UTC) #11
hugoh_UTC2
On 2017/07/07 21:51:25, piman wrote: > On 2017/07/07 21:40:46, hugoh_UTC2 wrote: > > On 2017/07/07 ...
4 months, 2 weeks ago (2017-07-10 13:00:54 UTC) #16
chromium-reviews
On Mon, Jul 10, 2017 at 6:00 AM, <hugoh@opera.com> wrote: > On 2017/07/07 21:51:25, piman ...
4 months, 2 weeks ago (2017-07-10 20:18:47 UTC) #17
chromium-reviews
On Mon, Jul 10, 2017 at 6:00 AM, <hugoh@opera.com> wrote: > On 2017/07/07 21:51:25, piman ...
4 months, 2 weeks ago (2017-07-10 20:35:10 UTC) #18
hugoh_UTC2
Ok, thanks for clarifying. PTAL at PS3.
4 months, 2 weeks ago (2017-07-11 07:22:11 UTC) #23
piman
lgtm
4 months, 2 weeks ago (2017-07-11 17:31:37 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2973723003/100001
4 months, 2 weeks ago (2017-07-11 19:12:56 UTC) #27
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/498298)
4 months, 2 weeks ago (2017-07-11 22:30:06 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2973723003/100001
4 months, 2 weeks ago (2017-07-12 07:28:20 UTC) #31
commit-bot: I haz the power
4 months, 2 weeks ago (2017-07-12 08:10:50 UTC) #34
Message was sent while issue was closed.
Committed patchset #3 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/f1c501fa3022cc7de41048cce085...

Powered by Google App Engine
This is Rietveld efc10ee0f