Print stack traces in child processes when browser tests failed.

base/debug/stack_trace_win.cc

Index: base/debug/stack_trace_win.cc
diff --git a/base/debug/stack_trace_win.cc b/base/debug/stack_trace_win.cc
index 55d55624088d59a0ac82f09da611b4ee48627ede..94c2e967363c218a0d7f875bf2657290ab033cab 100644
--- a/base/debug/stack_trace_win.cc
+++ b/base/debug/stack_trace_win.cc
@@ -26,6 +26,9 @@ namespace {
 // exception. Only used in unit tests.
 LPTOP_LEVEL_EXCEPTION_FILTER g_previous_filter = NULL;
 
+bool g_initialized_symbols = false;
+DWORD g_init_error = ERROR_SUCCESS;
+
 // Prints the exception call stack.
 // This is the unit tests exception filter.
 long WINAPI StackDumpExceptionFilter(EXCEPTION_POINTERS* info) {
@@ -42,6 +45,55 @@ FilePath GetExePath() {
   return FilePath(system_buffer);
 }
 
+bool InitializeSymbols() {
+  if (g_initialized_symbols)
+    return g_init_error == ERROR_SUCCESS;
+  g_initialized_symbols = true;
+  // Defer symbol load until they're needed, use undecorated names, and get line
+  // numbers.
+  SymSetOptions(SYMOPT_DEFERRED_LOADS |
+                SYMOPT_UNDNAME |
+                SYMOPT_LOAD_LINES);
+  if (!SymInitialize(GetCurrentProcess(), NULL, TRUE)) {

[Sébastien Marchand, 2016/01/06 21:42:54]

AFAIK the calls to SymInitialize should be paired with a call to SymCleanup to
avoid leaking resources. Not sure if it really matters here but I wanted to make
sure that it hasn't been overlooked.

Context: I'm rolling base/ deps in the Syzygy repo, and this is breaking all the
tests that were taking care of calling SymInitialize (you can only call it once
during a process lifetime). I'm trying to fix this in our codebase (i.e. stop
calling this function directly in our tests). This will probably trigger a bunch
of AppVerifier warning because we're leaking some resources. I know that
supporting third party project isn't a goal of base, but I just wanted to make
sure that it hasn't been overlooked.

+    g_init_error = GetLastError();
+    // TODO(awong): Handle error: SymInitialize can fail with
+    // ERROR_INVALID_PARAMETER.
+    // When it fails, we should not call debugbreak since it kills the current
+    // process (prevents future tests from running or kills the browser
+    // process).
+    DLOG(ERROR) << "SymInitialize failed: " << g_init_error;
+    return false;
+  }
+
+  // When transferring the binaries e.g. between bots, path put
+  // into the executable will get off. To still retrieve symbols correctly,
+  // add the directory of the executable to symbol search path.
+  // All following errors are non-fatal.
+  const size_t kSymbolsArraySize = 1024;
+  scoped_ptr<wchar_t[]> symbols_path(new wchar_t[kSymbolsArraySize]);
+
+  // Note: The below function takes buffer size as number of characters,
+  // not number of bytes!
+  if (!SymGetSearchPathW(GetCurrentProcess(),
+                         symbols_path.get(),
+                         kSymbolsArraySize)) {
+    DLOG(WARNING) << "SymGetSearchPath failed: " << g_init_error;
+    g_init_error = GetLastError();
+    return false;
+  }
+
+  std::wstring new_path(std::wstring(symbols_path.get()) +
+                        L";" + GetExePath().DirName().value());
+  if (!SymSetSearchPathW(GetCurrentProcess(), new_path.c_str())) {
+    g_init_error = GetLastError();
+    DLOG(WARNING) << "SymSetSearchPath failed." << g_init_error;
+    return false;
+  }
+
+  g_init_error = ERROR_SUCCESS;
+  return true;
+}
+
 // SymbolContext is a threadsafe singleton that wraps the DbgHelp Sym* family
 // of functions.  The Sym* family of functions may only be invoked by one
 // thread at a time.  SymbolContext code may access a symbol server over the
@@ -66,11 +118,6 @@ class SymbolContext {
       Singleton<SymbolContext, LeakySingletonTraits<SymbolContext> >::get();
   }
 
-  // Returns the error code of a failed initialization.
-  DWORD init_error() const {
-    return init_error_;
-  }
-
   // For the given trace, attempts to resolve the symbols, and output a trace
   // to the ostream os.  The format for each line of the backtrace is:
   //
@@ -132,51 +179,10 @@ class SymbolContext {
  private:
   friend struct DefaultSingletonTraits<SymbolContext>;
 
-  SymbolContext() : init_error_(ERROR_SUCCESS) {
-    // Initializes the symbols for the process.
-    // Defer symbol load until they're needed, use undecorated names, and
-    // get line numbers.
-    SymSetOptions(SYMOPT_DEFERRED_LOADS |
-                  SYMOPT_UNDNAME |
-                  SYMOPT_LOAD_LINES);
-    if (!SymInitialize(GetCurrentProcess(), NULL, TRUE)) {
-      init_error_ = GetLastError();
-      // TODO(awong): Handle error: SymInitialize can fail with
-      // ERROR_INVALID_PARAMETER.
-      // When it fails, we should not call debugbreak since it kills the current
-      // process (prevents future tests from running or kills the browser
-      // process).
-      DLOG(ERROR) << "SymInitialize failed: " << init_error_;
-      return;
-    }
-
-    init_error_ = ERROR_SUCCESS;
-
-    // When transferring the binaries e.g. between bots, path put
-    // into the executable will get off. To still retrieve symbols correctly,
-    // add the directory of the executable to symbol search path.
-    // All following errors are non-fatal.
-    const size_t kSymbolsArraySize = 1024;
-    scoped_ptr<wchar_t[]> symbols_path(new wchar_t[kSymbolsArraySize]);
-
-    // Note: The below function takes buffer size as number of characters,
-    // not number of bytes!
-    if (!SymGetSearchPathW(GetCurrentProcess(),
-                           symbols_path.get(),
-                           kSymbolsArraySize)) {
-      DLOG(WARNING) << "SymGetSearchPath failed: ";
-      return;
-    }
-
-    std::wstring new_path(std::wstring(symbols_path.get()) +
-                          L";" + GetExePath().DirName().value());
-    if (!SymSetSearchPathW(GetCurrentProcess(), new_path.c_str())) {
-      DLOG(WARNING) << "SymSetSearchPath failed.";
-      return;
-    }
+  SymbolContext() {
+    InitializeSymbols();
   }
 
-  DWORD init_error_;
   base::Lock lock_;
   DISALLOW_COPY_AND_ASSIGN(SymbolContext);
 };
@@ -187,8 +193,11 @@ bool EnableInProcessStackDumping() {
   // Add stack dumping support on exception on windows. Similar to OS_POSIX
   // signal() handling in process_util_posix.cc.
   g_previous_filter = SetUnhandledExceptionFilter(&StackDumpExceptionFilter);
-  RouteStdioToConsole();
-  return true;
+
+  // Need to initialize symbols early in the process or else this fails on
+  // swarming (since symbols are in different directory than in the exes) and
+  // also release x64.
+  return InitializeSymbols();
 }
 
 // Disable optimizations for the StackTrace::StackTrace function. It is
@@ -209,18 +218,12 @@ StackTrace::StackTrace() {
 #pragma optimize("", on)
 #endif
 
-StackTrace::StackTrace(const EXCEPTION_POINTERS* exception_pointers) {
-  // StackWalk64() may modify context record passed to it, so we will
-  // use a copy.
-  CONTEXT context_record = *exception_pointers->ContextRecord;
-  InitTrace(&context_record);
+StackTrace::StackTrace(EXCEPTION_POINTERS* exception_pointers) {
+  InitTrace(exception_pointers->ContextRecord);
 }
 
-StackTrace::StackTrace(const CONTEXT* context) {
-  // StackWalk64() may modify context record passed to it, so we will
-  // use a copy.
-  CONTEXT context_record = *context;
-  InitTrace(&context_record);
+StackTrace::StackTrace(CONTEXT* context) {
+  InitTrace(context);
 }
 
 void StackTrace::InitTrace(CONTEXT* context_record) {
@@ -266,9 +269,8 @@ void StackTrace::Print() const {
 
 void StackTrace::OutputToStream(std::ostream* os) const {
   SymbolContext* context = SymbolContext::GetInstance();
-  DWORD error = context->init_error();
-  if (error != ERROR_SUCCESS) {
-    (*os) << "Error initializing symbols (" << error
+  if (g_init_error != ERROR_SUCCESS) {
+    (*os) << "Error initializing symbols (" << g_init_error
           << ").  Dumping unresolved backtrace:\n";
     for (size_t i = 0; (i < count_) && os->good(); ++i) {
       (*os) << "\t" << trace_[i] << "\n";