[tracing] Add thread name to the pseudo stack of heap profiler

base/threading/thread_id_name_manager.cc

Index: base/threading/thread_id_name_manager.cc
diff --git a/base/threading/thread_id_name_manager.cc b/base/threading/thread_id_name_manager.cc
index 56cfa273a87d72df8ed3163bed0348e1d09203dc..51a11ff039cfc7bac2de509c42dd27ba9a337017 100644
--- a/base/threading/thread_id_name_manager.cc
+++ b/base/threading/thread_id_name_manager.cc
@@ -10,6 +10,7 @@
 #include "base/logging.h"
 #include "base/memory/singleton.h"
 #include "base/strings/string_util.h"
+#include "base/trace_event/heap_profiler_allocation_context_tracker.h"
 
 namespace base {
 namespace {
@@ -50,27 +51,34 @@ void ThreadIdNameManager::RegisterThread(PlatformThreadHandle::Handle handle,
 
 void ThreadIdNameManager::SetName(PlatformThreadId id,
                                   const std::string& name) {
-  AutoLock locked(lock_);
-  NameToInternedNameMap::iterator iter = name_to_interned_name_.find(name);
   std::string* leaked_str = NULL;
-  if (iter != name_to_interned_name_.end()) {
-    leaked_str = iter->second;
-  } else {
-    leaked_str = new std::string(name);
-    name_to_interned_name_[name] = leaked_str;
+  {
+    AutoLock locked(lock_);
+    NameToInternedNameMap::iterator iter = name_to_interned_name_.find(name);
+    if (iter != name_to_interned_name_.end()) {
+      leaked_str = iter->second;
+    } else {
+      leaked_str = new std::string(name);
+      name_to_interned_name_[name] = leaked_str;
+    }
+
+    ThreadIdToHandleMap::iterator id_to_handle_iter =
+        thread_id_to_handle_.find(id);
+
+    // The main thread of a process will not be created as a Thread object which
+    // means there is no PlatformThreadHandler registered.
+    if (id_to_handle_iter == thread_id_to_handle_.end()) {
+      main_process_name_ = leaked_str;
+      main_process_id_ = id;
+      return;
+    }
+    thread_handle_to_interned_name_[id_to_handle_iter->second] = leaked_str;
   }
 
-  ThreadIdToHandleMap::iterator id_to_handle_iter =
-      thread_id_to_handle_.find(id);
-
-  // The main thread of a process will not be created as a Thread object which
-  // means there is no PlatformThreadHandler registered.
-  if (id_to_handle_iter == thread_id_to_handle_.end()) {
-    main_process_name_ = leaked_str;
-    main_process_id_ = id;
-    return;
-  }
-  thread_handle_to_interned_name_[id_to_handle_iter->second] = leaked_str;
+  // Add the leaked thread name to heap profiler context tracker. The name added

[petrcermak, 2016/03/29 13:07:46]

Shouldn't you also do this for the main thread?

[ssid, 2016/03/29 18:19:11]

replied below.

+  // is valid for the lifetime of the process.
+  trace_event::AllocationContextTracker::SetCurrentThreadName(

[danakj, 2016/03/29 00:47:36]

Why not call GetName() when you need the name?

[ssid, 2016/03/29 18:19:11]

I cannot call GetName from AllocationTracker because it could cause a deadlock
at RegisterThread.
AllocationTracker gets initialized at first allocation in the thread (where I
should try to call GetName). The first allocation more often happens in
RegisterThread.
So, calling GetName tries to hold same lock as RegisterThread.

[danakj, 2016/03/29 20:12:41]

Sorry I'm not understanding this, but why is it a deadlock? If RegisterThread
and GetName happen at the same time, then one will wait for the other and then
continue. Deadlocks occur when 2 locks are held in different orders and one lock
ends up waiting on the other lock which waits on the first lock.

I only see 1 lock in your explanation tho, which would just provide single
access at a time?

It sounded like you're worried about RegisterThread and GetName happening from
the same thread at the same time? One does not call the other, right? Register
should happen before GetName for any given thread.

[Primiano Tucci (use gerrit), 2016/03/31 15:48:28]

Yeah that was my initial proposal, unfortunately won't work for this very
special case (more below).
I think what he means here is: deadlock by re-entrancy.
The magic bit here is that if he needs the name in the heap profiler code under
malloc hook. So if he tries to use GetName() he'd end up in the current
situation:

(some code) -> malloc -> TIDNameMgr::GetName().
Now, this would work most of the times, but won't work in the case where (some
code) == (some code in TIDNamemgr which has is holding the lock).
In other words, if the first malloc seen on one thread is due to some method
ThreadIdNameManager (which unfortunately seems to be the case), GetName would
re-entrantly try to re-acquire the lock. The lock is irrelevant to be honest,
the real problem is that he'd re-entrantly call within ThreadIdNameManager,
which should not happen even if it was lock-free, as re-entrant code is bad.

I think this is the reason why he's opting for a push-model.
I agree this is less nice from a layering viewpoint, but not sure is there any
other way around this (I'd love if there was a way to not touch base code
outside of trace_event).
Unless one calls malloc and the malloc hook calls GetName, which is the catch
here :)

+      leaked_str->c_str());
 }
 
 const char* ThreadIdNameManager::GetName(PlatformThreadId id) {