New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 882495 link

Starred by 2 users
Status: Fixed
Owner:
alph@chromium.org
Closed: Sep 13
Cc:
█ picksi@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 2
Type: Bug-Regression
Hotlist-MemoryInfra



Sign in to add a comment

Deadlock in SamplingHeapProfiler::SampleAdded

Project Member Reported by alph@chromium.org, Sep 10 
A classic two locks deadlock. One lock comes from SamplingHeapProfiler another from PoissonAllocationSampler

Make observers iteration outside the lock?

(gdb) thread 41
[Switching to thread 41 (Thread 0x7fc9f6be3700 (LWP 71291))]
#0  __lll_lock_wait () at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
135	in ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S
(gdb) bt
#0  0x00007fca0c4d9f5c in __lll_lock_wait () at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
#1  0x00007fca0c4d3b95 in __GI___pthread_mutex_lock (mutex=0x562a24dbe990 <base::PoissonAllocationSampler::Get()::instance+16>)
    at ../nptl/pthread_mutex_lock.c:80
#2  0x0000562a1f440610 in Lock() () at ../../base/synchronization/lock_impl_posix.cc:102
#3  0x0000562a1f3b2ff0 in DoRecordAlloc() () at ../../base/synchronization/lock.h:45
#4  0x0000562a1f3b2ff0 in DoRecordAlloc() () at ../../base/synchronization/lock.h:113
#5  0x0000562a1f3b2ff0 in DoRecordAlloc() () at ../../base/sampling_heap_profiler/poisson_allocation_sampler.cc:370
#6  0x0000562a1f3b36a2 in base::(anonymous namespace)::AllocFn(base::allocator::AllocatorDispatch const*, unsigned long, void*) ()
    at ../../base/sampling_heap_profiler/poisson_allocation_sampler.cc:96
#7  0x0000562a1f445fde in operator new() () at ../../base/allocator/allocator_shim.cc:159
#8  0x0000562a1f445fde in operator new() () at ../../base/allocator/allocator_shim_override_cpp_symbols.h:19
#9  0x0000562a1f3b5247 in __push_back_slow_path<base::SamplingHeapProfiler::Sample const&>() () at ../../buildtools/third_party/libc++/trunk/include/new:259
#10 0x0000562a1f3b5247 in __push_back_slow_path<base::SamplingHeapProfiler::Sample const&>() ()
    at ../../buildtools/third_party/libc++/trunk/include/memory:1799
#11 0x0000562a1f3b5247 in __push_back_slow_path<base::SamplingHeapProfiler::Sample const&>() ()
    at ../../buildtools/third_party/libc++/trunk/include/memory:1548
#12 0x0000562a1f3b5247 in __push_back_slow_path<base::SamplingHeapProfiler::Sample const&>() ()
    at ../../buildtools/third_party/libc++/trunk/include/__split_buffer:311
#13 0x0000562a1f3b5247 in __push_back_slow_path<base::SamplingHeapProfiler::Sample const&>() ()
    at ../../buildtools/third_party/libc++/trunk/include/vector:1578
#14 0x0000562a1f3b4a92 in GetSamples() () at ../../buildtools/third_party/libc++/trunk/include/vector:1599
#15 0x0000562a1f3b4a92 in GetSamples() () at ../../base/sampling_heap_profiler/sampling_heap_profiler.cc:133
#16 0x0000562a1f470cd7 in RetrieveAndSendSnapshot() () at ../../chrome/common/heap_profiler_controller.cc:99
#17 0x0000562a1f470c8e in HeapProfilerController::TakeSnapshot() () at ../../chrome/common/heap_profiler_controller.cc:93
#18 0x0000562a1c72aa34 in RunOnce() () at ../../base/bind_internal.h:516
#19 0x0000562a1c72aa34 in RunOnce() () at ../../base/bind_internal.h:636
#20 0x0000562a1c72aa34 in RunOnce() () at ../../base/bind_internal.h:689
#21 0x0000562a1c72aa34 in RunOnce() () at ../../base/bind_internal.h:658
#22 0x0000562a1f38832d in RunTask() () at ../../base/callback.h:99
#23 0x0000562a1f38832d in RunTask() () at ../../base/debug/task_annotator.cc:101
#24 0x0000562a1f3f36f1 in RunOrSkipTask() () at ../../base/task/task_scheduler/task_tracker.cc:558
#25 0x0000562a1f444dcb in RunOrSkipTask() () at ../../base/task/task_scheduler/task_tracker_posix.cc:23
#26 0x0000562a1f3f2a9f in RunAndPopNextTask() () at ../../base/task/task_scheduler/task_tracker.cc:437
#27 0x0000562a1f3ee83d in RunWorker() () at ../../base/task/task_scheduler/scheduler_worker.cc:332
#28 0x0000562a1f3ee5c4 in base::internal::SchedulerWorker::RunPooledWorker() () at ../../base/task/task_scheduler/scheduler_worker.cc:224
#29 0x0000562a1f4456df in ThreadFunc() () at ../../base/threading/platform_thread_posix.cc:76
#30 0x00007fca0c4d1494 in start_thread (arg=0x7fc9f6be3700) at pthread_create.c:333
#31 0x00007fca06413a8f in clone () at /lib/x86_64-linux-gnu/libc.so.6
(gdb) p ((pthread_mutex_t*)0x562a24dbe990)->__data.__owner
$7 = 37915
(gdb) thread find 37915
Thread 23 has target id 'Thread 0x7fc9ead7e700 (LWP 37915)'
(gdb) thread 23
[Switching to thread 23 (Thread 0x7fc9ead7e700 (LWP 37915))]
#0  __lll_lock_wait () at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
135	in ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S
(gdb) bt
#0  0x00007fca0c4d9f5c in __lll_lock_wait () at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
#1  0x00007fca0c4d3b95 in __GI___pthread_mutex_lock (mutex=0x562a24dbea18 <base::SamplingHeapProfiler::Get()::instance+24>) at ../nptl/pthread_mutex_lock.c:80
#2  0x0000562a1f440610 in Lock() () at ../../base/synchronization/lock_impl_posix.cc:102
#3  0x0000562a1f3b4358 in SampleAdded() () at ../../base/synchronization/lock.h:45
#4  0x0000562a1f3b4358 in SampleAdded() () at ../../base/synchronization/lock.h:113
#5  0x0000562a1f3b4358 in SampleAdded() () at ../../base/sampling_heap_profiler/sampling_heap_profiler.cc:106
#6  0x0000562a1f3b30c0 in DoRecordAlloc() () at ../../base/sampling_heap_profiler/poisson_allocation_sampler.cc:376
#7  0x0000562a1f3b36a2 in base::(anonymous namespace)::AllocFn(base::allocator::AllocatorDispatch const*, unsigned long, void*) ()
    at ../../base/sampling_heap_profiler/poisson_allocation_sampler.cc:96
#8  0x0000562a1f445fde in operator new() () at ../../base/allocator/allocator_shim.cc:159
#9  0x0000562a1f445fde in operator new() () at ../../base/allocator/allocator_shim_override_cpp_symbols.h:19
#10 0x0000562a1f400d45 in ConstructTlsVector() () at ../../base/threading/thread_local_storage.cc:197
#11 0x0000562a1f401330 in Set() () at ../../base/threading/thread_local_storage.cc:382
#12 0x0000562a1f3b2e9f in RecordAlloc() () at ../../base/sampling_heap_profiler/poisson_allocation_sampler.cc:355
#13 0x0000562a1f3b36a2 in base::(anonymous namespace)::AllocFn(base::allocator::AllocatorDispatch const*, unsigned long, void*) ()
    at ../../base/sampling_heap_profiler/poisson_allocation_sampler.cc:96
#14 0x0000562a1f445fde in operator new() () at ../../base/allocator/allocator_shim.cc:159
#15 0x0000562a1f445fde in operator new() () at ../../base/allocator/allocator_shim_override_cpp_symbols.h:19
#16 0x0000562a1ef7820b in append() () at ../../buildtools/third_party/libc++/trunk/include/new:259
#17 0x0000562a1ef7820b in append() () at ../../buildtools/third_party/libc++/trunk/include/memory:1799
#18 0x0000562a1ef7820b in append() () at ../../buildtools/third_party/libc++/trunk/include/memory:1548
#19 0x0000562a1ef7820b in append() () at ../../buildtools/third_party/libc++/trunk/include/string:1982
#20 0x0000562a1ef7820b in append() () at ../../buildtools/third_party/libc++/trunk/include/string:2247
#21 0x0000562a1f3bc8fb in AppendToString() () at ../../base/strings/string_piece.cc:77
#22 0x0000562a1f3bc8fb in AppendToString() () at ../../base/strings/string_piece.cc:81
#23 0x0000562a1f36c09c in Append() () at ../../base/strings/string_piece.h:289
#24 0x0000562a1f36c09c in Append() () at ../../base/files/file_path.cc:517
#25 0x0000562a1f3fc19a in SetThreadCgroupForThreadPriority() () at ../../base/threading/platform_thread_linux.cc:64
#26 0x0000562a1f3fbd9b in SetCurrentThreadPriorityForPlatform() () at ../../base/threading/platform_thread_linux.cc:76
#27 0x0000562a1f3fbd9b in SetCurrentThreadPriorityForPlatform() () at ../../base/threading/platform_thread_linux.cc:102
#28 0x0000562a1f445444 in SetCurrentThreadPriority() () at ../../base/threading/platform_thread_posix.cc:262
#29 0x0000562a1f4456a6 in ThreadFunc() () at ../../base/threading/platform_thread_posix.cc:68
#30 0x00007fca0c4d1494 in start_thread (arg=0x7fc9ead7e700) at pthread_create.c:333
#31 0x00007fca06413a8f in clone () at /lib/x86_64-linux-gnu/libc.so.6
(gdb) p ((pthread_mutex_t*)0x562a24dbea18)->__data.__owner
$8 = 71291
(gdb) thread find 71291
Thread 41 has target id 'Thread 0x7fc9f6be3700 (LWP 71291)'
(gdb)

Comment 1 by alph@chromium.org, Sep 11

Labels: -Type-Bug Type-Bug-Regression
In short what happens is a classic deadlock:

T1: SHP::GetSamples():
T1:   lock(SHP::mutex_)                     // locks m1
T1:   allocate array for samples copy
T1:   PAS::DoRecordAlloc():
T1:     try lock(PAS::mutex_)               // waits for m2

T2: PAS::DoRecordAlloc():
T2:   lock(PAS::mutex_)                     // locks m2
T2:   observer->AddSample()
T2:     try lock(SHP::mutex_)               // waits for m1

It regressed when SHP class was split into SHP and PoissonAllocationSampler, with a mutex object being duplicated.
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 13 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/28cc68d3555a4ac15456107cd78c9eca0023ee4e

commit 28cc68d3555a4ac15456107cd78c9eca0023ee4e
Author: Alexei Filippov <alph@chromium.org>
Date: Thu Sep 13 07:58:36 2018

[sampling heap profiler] Fix deadlock in SHP::GetSamples

Introduce MuteThreadSamplesScope class to avoid reentrancy in
PoissonAllocationSampler clients. Once the object is in scope,
PAS will skip invocation of observer methods for this thread.
Other threads will still be reporting their samples.

BUG= 882495 

Change-Id: If41fd9be8f0310633ab235c773debe874b44512f
Reviewed-on: https://chromium-review.googlesource.com/1217855
Reviewed-by: Primiano Tucci <primiano@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Commit-Queue: Alexei Filippov <alph@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590945}
[modify] https://crrev.com/28cc68d3555a4ac15456107cd78c9eca0023ee4e/base/sampling_heap_profiler/poisson_allocation_sampler.cc
[modify] https://crrev.com/28cc68d3555a4ac15456107cd78c9eca0023ee4e/base/sampling_heap_profiler/poisson_allocation_sampler.h
[modify] https://crrev.com/28cc68d3555a4ac15456107cd78c9eca0023ee4e/base/sampling_heap_profiler/sampling_heap_profiler.cc
[modify] https://crrev.com/28cc68d3555a4ac15456107cd78c9eca0023ee4e/base/sampling_heap_profiler/sampling_heap_profiler.h
[modify] https://crrev.com/28cc68d3555a4ac15456107cd78c9eca0023ee4e/base/sampling_heap_profiler/sampling_heap_profiler_unittest.cc

Comment 3 by alph@chromium.org, Sep 13

Labels: Merge-Request-70 OS-Mac
Status: Fixed (was: Assigned)

Comment 4 by abdulsyed@chromium.org, Sep 13 

how safe is this merge?

Comment 5 by alph@chromium.org, Sep 14 

This one is pretty safe.
Project Member

Comment 6 by sheriffbot@chromium.org, Sep 14

Labels: -Merge-Request-70 Merge-Review-70 Hotlist-Merge-Review
This bug requires manual review: M70 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 7 by abdulsyed@chromium.org, Sep 14

Labels: -Merge-Review-70 Merge-Approved-70
branch:3538
Project Member

Comment 8 by bugdroid1@chromium.org, Sep 14

Labels: -merge-approved-70 merge-merged-3538
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/65a1d8f15a6761ffa110d55779046ade1a3b89c2

commit 65a1d8f15a6761ffa110d55779046ade1a3b89c2
Author: Alexei Filippov <alph@chromium.org>
Date: Fri Sep 14 20:30:20 2018

[sampling heap profiler] Fix deadlock in SHP::GetSamples

Introduce MuteThreadSamplesScope class to avoid reentrancy in
PoissonAllocationSampler clients. Once the object is in scope,
PAS will skip invocation of observer methods for this thread.
Other threads will still be reporting their samples.

BUG= 882495 
TBR=alph@chromium.org

(cherry picked from commit 28cc68d3555a4ac15456107cd78c9eca0023ee4e)

Change-Id: If41fd9be8f0310633ab235c773debe874b44512f
Reviewed-on: https://chromium-review.googlesource.com/1217855
Reviewed-by: Primiano Tucci <primiano@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Commit-Queue: Alexei Filippov <alph@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#590945}
Reviewed-on: https://chromium-review.googlesource.com/1227299
Reviewed-by: Alexei Filippov <alph@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#424}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/65a1d8f15a6761ffa110d55779046ade1a3b89c2/base/sampling_heap_profiler/poisson_allocation_sampler.cc
[modify] https://crrev.com/65a1d8f15a6761ffa110d55779046ade1a3b89c2/base/sampling_heap_profiler/poisson_allocation_sampler.h
[modify] https://crrev.com/65a1d8f15a6761ffa110d55779046ade1a3b89c2/base/sampling_heap_profiler/sampling_heap_profiler.cc
[modify] https://crrev.com/65a1d8f15a6761ffa110d55779046ade1a3b89c2/base/sampling_heap_profiler/sampling_heap_profiler.h
[modify] https://crrev.com/65a1d8f15a6761ffa110d55779046ade1a3b89c2/base/sampling_heap_profiler/sampling_heap_profiler_unittest.cc
►

Sign in to add a comment