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

Issue 764357 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Xoogler
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug
Hotlist-MemoryInfra

Blocking:
issue 757747



Sign in to add a comment

--enable-heap-profiling may cause deadlock on MemoryDumpManager's lock

Project Member Reported by kraynov@chromium.org, Sep 12 2017

Issue description

When we use --enable-heap-profiling in pseudo-stack mode, it may cause deadlock on MemoryDumpManager's lock.
Easily reproducible on Android.
This bot is red https://build.chromium.org/p/chromium.android.fyi/builders/Memory%20Infra%20Tester because of that.

The interesting and formated part os stack trace below:
base::AutoLock::ctor()                                                base/synchronization/lock.h:115
base::trace_event::MemoryDumpManager::RegisterDumpProviderInternal()  base/trace_event/memory_dump_manager.cc:402
base::trace_event::MemoryDumpManager::RegisterDumpProvider()          base/trace_event/memory_dump_manager.cc:362
base::trace_event::TraceLog::ctor()                                   base/trace_event/trace_log.cc:368
base::Singleton<>::get()                                              base/memory/singleton.h:258
base::trace_event::EnableFilteringForPseudoStackProfiling()           base/trace_event/memory_dump_manager.cc:117
base::trace_event::MemoryDumpManager::EnableHeapProfiling()           base/trace_event/memory_dump_manager.cc:259
base::trace_event::MemoryDumpManager::EnableHeapProfilingIfNeeded()   base/trace_event/memory_dump_manager.cc:227
tracing::EnableStartupTracingIfNeeded()                               components/tracing/common/trace_startup.cc:23

Basically we have that story:
1. Create MemoryDumpManager instance
2. Try to EnableHeapProfiling() when TraceLog is not created yet, MemoryDumpManager's lock got acquired
3. Create TraceLog as a part or lazy singleton dereference
4. TraceLog calls MemoryDumpManager::RegisterDumpProvider() which also acquires MemoryDumpManager's lock
5. Deadlock

I think would be reasonable to poke TraceLog's singleton before EnableHeapProfiling().
 
Blocking: 757747
add a call to  TraceLog::GetInstance() in EnableHeapProfilingIfNeeded so it gets initialized first?
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 13 2017

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

commit 0155e3e8f932f9e79202525001da661e7c48962a
Author: Greg Kraynov <kraynov@chromium.org>
Date: Wed Sep 13 16:45:40 2017

Avoid deadlock in MemoryDumpManager::EnableHeapProfiling.

Heap profiling may be requested to get enabled in stages when
TraceLog is not created yet. Since TraceLog calls other MDM's
lock-protected methods it can cause a deadlock.

Bug:  764357 
Change-Id: Ica08b4478aa09e03a3fa0761ec8ed787b1922cb6
Reviewed-on: https://chromium-review.googlesource.com/663722
Reviewed-by: Primiano Tucci <primiano@chromium.org>
Reviewed-by: Siddhartha S <ssid@chromium.org>
Commit-Queue: Grigoriy Kraynov <kraynov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501662}
[modify] https://crrev.com/0155e3e8f932f9e79202525001da661e7c48962a/components/tracing/common/trace_startup.cc

Status: Fixed (was: Assigned)

Sign in to add a comment