Support running memory watch under vista, plus other tweaks

This version of memory_watcher can run under Vista, even though the
recursive calls that it handles are appearing often enough that there
is a performance penalty.  With this landed, it may be possible for other
folks to run the tool, and I can work on improving its performance.

This CL also resolves the problem with hanging processes.  Although
memory reporting can only be done once, and it leaves a pile of memory
"hanging around," the browser can be cleanly exited.

Tweaks include outputing the aggregate stacks such that the largest
stacks appear at the start of the output file.

This version avoids ongoing aggregation of stats in favor of only
doing the aggregation at dump-time.  This probably enhances performance
at run-time, although it is hidden (on Vista) by the recursive calling.
This also simplifies the tracking code a fair amount.

There is some evidence that a small number of duplicate calls are being
made to "track" the same memory region, without an intervening free (i.e.,
call to "untrack").  The code to better diagnose this is currently in
place, but ifdef'ed, as it is only useful under a debugger.  Exercise
of this code (turning a stack-frame list into a human readable stack
trace string) currently causes some corruption shortly after it triggers,
so I can't leave it on full time.


r=mbelshe
Review URL: http://codereview.chromium.org/366031

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@31299 0039d316-1c4b-4281-b951-d872f2087c98

1 files changed, 49 insertions, 26 deletions

diff --git a/tools/memory_watcher/call_stack.cc b/tools/memory_watcher/call_stack.cc
index 490d3f5..460695d 100644
--- a/tools/memory_watcher/call_stack.cc
+++ b/tools/memory_watcher/call_stack.cc
@@ -33,16 +33,13 @@ typedef DWORD64 (__stdcall *t_SymLoadModule64)(HANDLE, HANDLE, PCSTR,
 typedef BOOL (__stdcall *t_SymGetModuleInfo64)(HANDLE, DWORD64,
                                                PIMAGEHLP_MODULE64);
 
-// According to http://msdn2.microsoft.com/en-us/library/ms680650(VS.85).aspx
-// "All DbgHelp functions, such as this one, are single threaded.  Therefore,
-// calls from more than one thread to this function will likely result in
-// unexpected behavior or memory corruption.  To avoid this, you must
-// synchromize all concurrent calls from one thread to this function."
-//
-// dbghelp_lock_ is used to serialize access across all calls to the DbgHelp
-// library.  This may be overly conservative (serializing them all together),
-// but does guarantee correctness.
-static Lock dbghelp_lock_;
+// static
+Lock CallStack::dbghelp_lock_;
+// static
+bool CallStack::dbghelp_loaded_ = false;
+// static
+DWORD CallStack::active_thread_id_ = 0;
+
 
 static t_StackWalk64 pStackWalk64 = NULL;
 static t_SymCleanup pSymCleanup = NULL;
@@ -62,15 +59,21 @@ static t_SymLoadModule64 pSymLoadModule64 = NULL;
   if (p##name == NULL) return false;                                    \
 } while (0)
 
-// Dynamically load the DbgHelp library and supporting routines that we
-// will use.
-static bool LoadDbgHelp() {
-  static bool loaded = false;
-  if (!loaded) {
+// This code has to be VERY careful to not induce any allocations, as memory
+// watching code may cause recursion, which may obscure the stack for the truly
+// offensive issue.  We use this function to break into a debugger, and it
+// is guaranteed to not do any allocations (in fact, not do anything).
+static void UltraSafeDebugBreak() {
+  _asm int(3);
+}
+
+// static
+bool CallStack::LoadDbgHelp() {
+  if (!dbghelp_loaded_) {
     AutoLock Lock(dbghelp_lock_);
 
     // Re-check if we've loaded successfully now that we have the lock.
-    if (loaded)
+    if (dbghelp_loaded_)
       return true;
 
     // Load dbghelp.dll, and obtain pointers to the exported functions that we
@@ -89,12 +92,13 @@ static bool LoadDbgHelp() {
       LOADPROC(dbghelp_module, SymGetModuleInfo64);
       LOADPROC(dbghelp_module, SymGetSearchPath);
       LOADPROC(dbghelp_module, SymLoadModule64);
-      loaded = true;
+      dbghelp_loaded_ = true;
     } else {
+      UltraSafeDebugBreak();
       return false;
     }
   }
-  return loaded;
+  return dbghelp_loaded_;
 }
 
 // Load the symbols for generating stack traces.
@@ -168,9 +172,14 @@ CallStack::CallStack() {
   frame_count_ = 0;
   hash_ = 0;
   id_ = InterlockedIncrement(&callstack_id);
+  valid_ = false;
+
+  if (!dbghelp_loaded_) {
+    UltraSafeDebugBreak();  // Initialize should have been called.
+    return;
+  }
 
-  LoadDbgHelp();
-  CHECK(GetStackTrace());
+  GetStackTrace();
 }
 
 bool CallStack::IsEqual(const CallStack &target) {
@@ -202,7 +211,17 @@ void CallStack::AddFrame(DWORD_PTR pc) {
   hash_ = hash_ ^ (pc >> 16);
 }
 
+bool CallStack::LockedRecursionDetected() const {
+  if (!active_thread_id_) return false;
+  DWORD thread_id = GetCurrentThreadId();
+  // TODO(jar): Perchance we should use atomic access to member.
+  return thread_id == active_thread_id_;
+}
+
 bool CallStack::GetStackTrace() {
+  if (LockedRecursionDetected())
+    return false;
+
   // Initialize the context record.
   CONTEXT context;
   memset(&context, 0, sizeof(context));
@@ -234,7 +253,7 @@ bool CallStack::GetStackTrace() {
   // Walk the stack.
   unsigned int count = 0;
   {
-    AutoLock lock(dbghelp_lock_);
+    AutoDbgHelpLock thread_monitoring_lock;
 
     while (count < kMaxTraceFrames) {
       count++;
@@ -255,11 +274,12 @@ bool CallStack::GetStackTrace() {
       // Push this frame's program counter onto the provided CallStack.
       AddFrame((DWORD_PTR)frame.AddrPC.Offset);
     }
+    valid_ = true;
   }
   return true;
 }
 
-void CallStack::ToString(std::string* output) {
+void CallStack::ToString(PrivateAllocatorString* output) {
   static const int kStackWalkMaxNameLen = MAX_SYM_NAME;
   HANDLE current_process = GetCurrentProcess();
 
@@ -272,12 +292,12 @@ void CallStack::ToString(std::string* output) {
 
   // Iterate through each frame in the call stack.
   for (int32 index = 0; index < frame_count_; index++) {
-    std::string line;
+    PrivateAllocatorString line;
 
     DWORD_PTR intruction_pointer = frame(index);
 
     SymbolCache::iterator it;
-    it = symbol_cache_->find( intruction_pointer );
+    it = symbol_cache_->find(intruction_pointer);
     if (it != symbol_cache_->end()) {
       line = it->second;
     } else {
@@ -311,14 +331,17 @@ void CallStack::ToString(std::string* output) {
               strstr(symbol->Name, "Perftools_") ||
               strstr(symbol->Name, "MemoryHook::") ) {
             // Just record a blank string.
-            (*symbol_cache_)[intruction_pointer] = std::string("");
+            (*symbol_cache_)[intruction_pointer] = "";
             continue;
           }
 
           line += "    ";
           line += static_cast<char*>(Line.FileName);
           line += " (";
-          line += IntToString(Line.LineNumber);
+          // TODO(jar): get something like this template to work :-/
+          // line += IntToCustomString<PrivateAllocatorString>(Line.LineNumber);
+          // ...and then delete this line, which uses std::string.
+          line += IntToString(Line.LineNumber).c_str();
           line += "): ";
           line += symbol->Name;
           line += "\n";