Print stack traces in browser tests when any process crashes, or an assert fires.

The functionality to do this opens up security holes. Currently this was working only for debug Linux builds. However our trybots are release builds, and we need to be able to see stack traces from processes on all platforms and not just Linux (i.e. to be able to debug the large flakiness that occurred last week). This is disabled for official builds.

Also make release (non-official) builds print the callstack on asserts, just like debug builds. This makes it easier to debug test failures on the CQ (for example, DCHECKs for non-threadsafe usage of pointers).

Add a regression test that both renderer and browser process crashes print the callstack.

BUG=517488,358267,521148
NOPRESUBMIT=true

Committed: https://crrev.com/8ba532e170befc312e66d032587fa2ad04bac975
Cr-Commit-Position: refs/heads/master@{#343240}

Review URL: https://codereview.chromium.org/1291553003

Cr-Commit-Position: refs/heads/master@{#343626}

20 files changed, 351 insertions, 238 deletions

diff --git a/base/debug/stack_trace.h b/base/debug/stack_trace.h
index fb271b6..85c9177 100644
--- a/base/debug/stack_trace.h
+++ b/base/debug/stack_trace.h
@@ -26,16 +26,13 @@ namespace debug {
 // Enables stack dump to console output on exception and signals.
 // When enabled, the process will quit immediately. This is meant to be used in
 // unit_tests only! This is not thread-safe: only call from main thread.
-BASE_EXPORT bool EnableInProcessStackDumping();
-
-// A different version of EnableInProcessStackDumping that also works for
-// sandboxed processes.  For more details take a look at the description
-// of EnableInProcessStackDumping.
+// In sandboxed processes, this has to be called before the sandbox is turned
+// on.
 // Calling this function on Linux opens /proc/self/maps and caches its
-// contents. In DEBUG builds, this function also opens the object files that
-// are loaded in memory and caches their file descriptors (this cannot be
+// contents. In non-official builds, this function also opens the object files
+// that are loaded in memory and caches their file descriptors (this cannot be
 // done in official builds because it has security implications).
-BASE_EXPORT bool EnableInProcessStackDumpingForSandbox();
+BASE_EXPORT bool EnableInProcessStackDumping();
 
 // A stacktrace can be helpful in debugging. For example, you can include a
 // stacktrace member in a object (probably around #ifndef NDEBUG) so that you
@@ -54,8 +51,8 @@ class BASE_EXPORT StackTrace {
   // Creates a stacktrace for an exception.
   // Note: this function will throw an import not found (StackWalk64) exception
   // on system without dbghelp 5.1.
-  StackTrace(const _EXCEPTION_POINTERS* exception_pointers);
-  StackTrace(const _CONTEXT* context);
+  StackTrace(_EXCEPTION_POINTERS* exception_pointers);
+  StackTrace(_CONTEXT* context);
 #endif
 
   // Copying and assignment are allowed with the default functions.
diff --git a/base/debug/stack_trace_posix.cc b/base/debug/stack_trace_posix.cc
index dbbec36..d8eb005 100644
--- a/base/debug/stack_trace_posix.cc
+++ b/base/debug/stack_trace_posix.cc
@@ -512,7 +512,7 @@ class SandboxSymbolizeHelper {
   int GetFileDescriptor(const char* file_path) {
     int fd = -1;
 
-#if !defined(NDEBUG)
+#if !defined(OFFICIAL_BUILD)
     if (file_path) {
       // The assumption here is that iterating over std::map<std::string, int>
       // using a const_iterator does not allocate dynamic memory, hense it is
@@ -533,7 +533,7 @@ class SandboxSymbolizeHelper {
         fd = -1;
       }
     }
-#endif  // !defined(NDEBUG)
+#endif  // !defined(OFFICIAL_BUILD)
 
     return fd;
   }
@@ -619,11 +619,9 @@ class SandboxSymbolizeHelper {
   // Opens all object files and caches their file descriptors.
   void OpenSymbolFiles() {
     // Pre-opening and caching the file descriptors of all loaded modules is
-    // not considered safe for retail builds.  Hence it is only done in debug
-    // builds.  For more details, take a look at: http://crbug.com/341966
-    // Enabling this to release mode would require approval from the security
-    // team.
-#if !defined(NDEBUG)
+    // not safe for production builds.  Hence it is only done in non-official
+    // builds.  For more details, take a look at: http://crbug.com/341966.
+#if !defined(OFFICIAL_BUILD)
     // Open the object files for all read-only executable regions and cache
     // their file descriptors.
     std::vector<MappedMemoryRegion>::const_iterator it;
@@ -655,7 +653,7 @@ class SandboxSymbolizeHelper {
         }
       }
     }
-#endif  // !defined(NDEBUG)
+#endif  // !defined(OFFICIAL_BUILD)
   }
 
   // Initializes and installs the symbolization callback.
@@ -677,7 +675,7 @@ class SandboxSymbolizeHelper {
 
   // Closes all file descriptors owned by this instance.
   void CloseObjectFiles() {
-#if !defined(NDEBUG)
+#if !defined(OFFICIAL_BUILD)
     std::map<std::string, int>::iterator it;
     for (it = modules_.begin(); it != modules_.end(); ++it) {
       int ret = IGNORE_EINTR(close(it->second));
@@ -685,19 +683,18 @@ class SandboxSymbolizeHelper {
       it->second = -1;
     }
     modules_.clear();
-#endif  // !defined(NDEBUG)
+#endif  // !defined(OFFICIAL_BUILD)
   }
 
   // Set to true upon successful initialization.
   bool is_initialized_;
 
-#if !defined(NDEBUG)
+#if !defined(OFFICIAL_BUILD)
   // Mapping from file name to file descriptor.  Includes file descriptors
   // for all successfully opened object files and the file descriptor for
-  // /proc/self/maps.  This code is not safe for release builds so
-  // this is only done for DEBUG builds.
+  // /proc/self/maps.  This code is not safe for production builds.
   std::map<std::string, int> modules_;
-#endif  // !defined(NDEBUG)
+#endif  // !defined(OFFICIAL_BUILD)
 
   // Cache for the process memory regions.  Produced by parsing the contents
   // of /proc/self/maps cache.
@@ -707,15 +704,11 @@ class SandboxSymbolizeHelper {
 };
 #endif  // USE_SYMBOLIZE
 
-bool EnableInProcessStackDumpingForSandbox() {
+bool EnableInProcessStackDumping() {
 #if defined(USE_SYMBOLIZE)
   SandboxSymbolizeHelper::GetInstance();
 #endif  // USE_SYMBOLIZE
 
-  return EnableInProcessStackDumping();
-}
-
-bool EnableInProcessStackDumping() {
   // When running in an application, our code typically expects SIGPIPE
   // to be ignored.  Therefore, when testing that same code, it should run
   // with SIGPIPE ignored as well.
diff --git a/base/debug/stack_trace_win.cc b/base/debug/stack_trace_win.cc
index 55d5562..94c2e96 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)) {
+    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";
diff --git a/base/logging.cc b/base/logging.cc
index 559cb08..ea73161 100644
--- a/base/logging.cc
+++ b/base/logging.cc
@@ -543,7 +543,7 @@ LogMessage::LogMessage(const char* file, int line, LogSeverity severity,
 }
 
 LogMessage::~LogMessage() {
-#if !defined(NDEBUG) && !defined(OS_NACL) && !defined(__UCLIBC__)
+#if !defined(OFFICIAL_BUILD) && !defined(OS_NACL) && !defined(__UCLIBC__)
   if (severity_ == LOG_FATAL) {
     // Include a stack trace on a fatal.
     base::debug::StackTrace trace;
diff --git a/base/process/launch.h b/base/process/launch.h
index 0e42cd0..e17cee5 100644
--- a/base/process/launch.h
+++ b/base/process/launch.h
@@ -236,7 +236,7 @@ BASE_EXPORT bool SetJobObjectLimitFlags(HANDLE job_object, DWORD limit_flags);
 
 // Output multi-process printf, cout, cerr, etc to the cmd.exe console that ran
 // chrome. This is not thread-safe: only call from main thread.
-BASE_EXPORT void RouteStdioToConsole();
+BASE_EXPORT void RouteStdioToConsole(bool create_console_if_not_found);
 #endif  // defined(OS_WIN)
 
 // Executes the application specified by |cl| and wait for it to exit. Stores
@@ -245,6 +245,10 @@ BASE_EXPORT void RouteStdioToConsole();
 // indicating success).
 BASE_EXPORT bool GetAppOutput(const CommandLine& cl, std::string* output);
 
+// Like GetAppOutput, but also includes stderr.
+BASE_EXPORT bool GetAppOutputAndError(const CommandLine& cl,
+                                      std::string* output);
+
 #if defined(OS_WIN)
 // A Windows-specific version of GetAppOutput that takes a command line string
 // instead of a CommandLine object. Useful for situations where you need to
diff --git a/base/process/launch_posix.cc b/base/process/launch_posix.cc
index 99d8e3a..0ebf984 100644
--- a/base/process/launch_posix.cc
+++ b/base/process/launch_posix.cc
@@ -530,7 +530,8 @@ enum GetAppOutputInternalResult {
 // path for the application; in that case, |envp| must be null, and it will use
 // the current environment. If |do_search_path| is false, |argv[0]| should fully
 // specify the path of the application, and |envp| will be used as the
-// environment. Redirects stderr to /dev/null.
+// environment. If |include_stderr| is true, includes stderr otherwise redirects
+// it to /dev/null.
 // If we successfully start the application and get all requested output, we
 // return GOT_MAX_OUTPUT, or if there is a problem starting or exiting
 // the application we return RUN_FAILURE. Otherwise we return EXECUTE_SUCCESS.
@@ -543,6 +544,7 @@ enum GetAppOutputInternalResult {
 static GetAppOutputInternalResult GetAppOutputInternal(
     const std::vector<std::string>& argv,
     char* const envp[],
+    bool include_stderr,
     std::string* output,
     size_t max_output,
     bool do_search_path,
@@ -597,7 +599,9 @@ static GetAppOutputInternalResult GetAppOutputInternal(
         base::type_profiler::Controller::Stop();
 
         fd_shuffle1.push_back(InjectionArc(pipe_fd[1], STDOUT_FILENO, true));
-        fd_shuffle1.push_back(InjectionArc(dev_null, STDERR_FILENO, true));
+        fd_shuffle1.push_back(InjectionArc(
+            include_stderr ? pipe_fd[1] : dev_null,
+            STDERR_FILENO, true));
         fd_shuffle1.push_back(InjectionArc(dev_null, STDIN_FILENO, true));
         // Adding another element here? Remeber to increase the argument to
         // reserve(), above.
@@ -666,11 +670,20 @@ bool GetAppOutput(const std::vector<std::string>& argv, std::string* output) {
   // Run |execve()| with the current environment and store "unlimited" data.
   int exit_code;
   GetAppOutputInternalResult result = GetAppOutputInternal(
-      argv, NULL, output, std::numeric_limits<std::size_t>::max(), true,
+      argv, NULL, false, output, std::numeric_limits<std::size_t>::max(), true,
       &exit_code);
   return result == EXECUTE_SUCCESS && exit_code == EXIT_SUCCESS;
 }
 
+bool GetAppOutputAndError(const CommandLine& cl, std::string* output) {
+  // Run |execve()| with the current environment and store "unlimited" data.
+  int exit_code;
+  GetAppOutputInternalResult result = GetAppOutputInternal(
+      cl.argv(), NULL, true, output, std::numeric_limits<std::size_t>::max(),
+      true, &exit_code);
+  return result == EXECUTE_SUCCESS && exit_code == EXIT_SUCCESS;
+}
+
 // TODO(viettrungluu): Conceivably, we should have a timeout as well, so we
 // don't hang if what we're calling hangs.
 bool GetAppOutputRestricted(const CommandLine& cl,
@@ -679,7 +692,7 @@ bool GetAppOutputRestricted(const CommandLine& cl,
   char* const empty_environ = NULL;
   int exit_code;
   GetAppOutputInternalResult result = GetAppOutputInternal(
-      cl.argv(), &empty_environ, output, max_output, false, &exit_code);
+      cl.argv(), &empty_environ, false, output, max_output, false, &exit_code);
   return result == GOT_MAX_OUTPUT || (result == EXECUTE_SUCCESS &&
                                       exit_code == EXIT_SUCCESS);
 }
@@ -689,8 +702,8 @@ bool GetAppOutputWithExitCode(const CommandLine& cl,
                               int* exit_code) {
   // Run |execve()| with the current environment and store "unlimited" data.
   GetAppOutputInternalResult result = GetAppOutputInternal(
-      cl.argv(), NULL, output, std::numeric_limits<std::size_t>::max(), true,
-      exit_code);
+      cl.argv(), NULL, false, output, std::numeric_limits<std::size_t>::max(),
+      true, exit_code);
   return result == EXECUTE_SUCCESS;
 }
 
diff --git a/base/process/launch_win.cc b/base/process/launch_win.cc
index fa59f1a..324eee7 100644
--- a/base/process/launch_win.cc
+++ b/base/process/launch_win.cc
@@ -46,9 +46,91 @@ namespace {
 // process goes away.
 const DWORD kProcessKilledExitCode = 1;
 
+bool GetAppOutputInternal(const StringPiece16& cl,
+                          bool include_stderr,
+                          std::string* output) {
+  HANDLE out_read = NULL;
+  HANDLE out_write = NULL;
+
+  SECURITY_ATTRIBUTES sa_attr;
+  // Set the bInheritHandle flag so pipe handles are inherited.
+  sa_attr.nLength = sizeof(SECURITY_ATTRIBUTES);
+  sa_attr.bInheritHandle = TRUE;
+  sa_attr.lpSecurityDescriptor = NULL;
+
+  // Create the pipe for the child process's STDOUT.
+  if (!CreatePipe(&out_read, &out_write, &sa_attr, 0)) {
+    NOTREACHED() << "Failed to create pipe";
+    return false;
+  }
+
+  // Ensure we don't leak the handles.
+  win::ScopedHandle scoped_out_read(out_read);
+  win::ScopedHandle scoped_out_write(out_write);
+
+  // Ensure the read handles to the pipes are not inherited.
+  if (!SetHandleInformation(out_read, HANDLE_FLAG_INHERIT, 0)) {
+    NOTREACHED() << "Failed to disabled pipe inheritance";
+    return false;
+  }
+
+  FilePath::StringType writable_command_line_string;
+  writable_command_line_string.assign(cl.data(), cl.size());
+
+  STARTUPINFO start_info = {};
+
+  start_info.cb = sizeof(STARTUPINFO);
+  start_info.hStdOutput = out_write;
+  // Keep the normal stdin.
+  start_info.hStdInput = GetStdHandle(STD_INPUT_HANDLE);
+  if (include_stderr) {
+    start_info.hStdError = out_write;
+  } else {
+    start_info.hStdError = GetStdHandle(STD_ERROR_HANDLE);
+  }
+  start_info.dwFlags |= STARTF_USESTDHANDLES;
+
+  // Create the child process.
+  PROCESS_INFORMATION temp_process_info = {};
+  if (!CreateProcess(NULL,
+                     &writable_command_line_string[0],
+                     NULL, NULL,
+                     TRUE,  // Handles are inherited.
+                     0, NULL, NULL, &start_info, &temp_process_info)) {
+    NOTREACHED() << "Failed to start process";
+    return false;
+  }
+  base::win::ScopedProcessInformation proc_info(temp_process_info);
+
+  // Close our writing end of pipe now. Otherwise later read would not be able
+  // to detect end of child's output.
+  scoped_out_write.Close();
+
+  // Read output from the child process's pipe for STDOUT
+  const int kBufferSize = 1024;
+  char buffer[kBufferSize];
+
+  for (;;) {
+    DWORD bytes_read = 0;
+    BOOL success = ReadFile(out_read, buffer, kBufferSize, &bytes_read, NULL);
+    if (!success || bytes_read == 0)
+      break;
+    output->append(buffer, bytes_read);
+  }
+
+  // Let's wait for the process to finish.
+  WaitForSingleObject(proc_info.process_handle(), INFINITE);
+
+  int exit_code;
+  base::TerminationStatus status = GetTerminationStatus(
+      proc_info.process_handle(), &exit_code);
+  return status != base::TERMINATION_STATUS_PROCESS_CRASHED &&
+         status != base::TERMINATION_STATUS_ABNORMAL_TERMINATION;
+}
+
 }  // namespace
 
-void RouteStdioToConsole() {
+void RouteStdioToConsole(bool create_console_if_not_found) {
   // Don't change anything if stdout or stderr already point to a
   // valid stream.
   //
@@ -64,8 +146,22 @@ void RouteStdioToConsole() {
   // stdout/stderr on startup (before the handle IDs can be reused).
   // _fileno(stdout) will return -2 (_NO_CONSOLE_FILENO) if stdout was
   // invalid.
-  if (_fileno(stdout) >= 0 || _fileno(stderr) >= 0)
-    return;
+  if (_fileno(stdout) >= 0 || _fileno(stderr) >= 0) {
+    // _fileno was broken for SUBSYSTEM:WINDOWS from VS2010 to VS2012/2013.
+    // http://crbug.com/358267. Confirm that the underlying HANDLE is valid
+    // before aborting.
+
+    // This causes NaCl tests to hang on XP for reasons unclear, perhaps due
+    // to not being able to inherit handles. Since it's only for debugging,
+    // and redirecting still works, punt for now.
+    if (base::win::GetVersion() < base::win::VERSION_VISTA)
+      return;
+
+    intptr_t stdout_handle = _get_osfhandle(_fileno(stdout));
+    intptr_t stderr_handle = _get_osfhandle(_fileno(stderr));
+    if (stdout_handle >= 0 || stderr_handle >= 0)
+      return;
+  }
 
   if (!AttachConsole(ATTACH_PARENT_PROCESS)) {
     unsigned int result = GetLastError();
@@ -76,10 +172,12 @@ void RouteStdioToConsole() {
     // parent process is invalid (eg: crashed).
     if (result == ERROR_GEN_FAILURE)
       return;
-    // Make a new console if attaching to parent fails with any other error.
-    // It should be ERROR_INVALID_HANDLE at this point, which means the browser
-    // was likely not started from a console.
-    AllocConsole();
+    if (create_console_if_not_found) {
+      // Make a new console if attaching to parent fails with any other error.
+      // It should be ERROR_INVALID_HANDLE at this point, which means the
+      // browser was likely not started from a console.
+      AllocConsole();
+    }
   }
 
   // Arbitrary byte count to use when buffering output lines.  More
@@ -274,76 +372,12 @@ bool GetAppOutput(const CommandLine& cl, std::string* output) {
   return GetAppOutput(cl.GetCommandLineString(), output);
 }
 
-bool GetAppOutput(const StringPiece16& cl, std::string* output) {
-  HANDLE out_read = NULL;
-  HANDLE out_write = NULL;
-
-  SECURITY_ATTRIBUTES sa_attr;
-  // Set the bInheritHandle flag so pipe handles are inherited.
-  sa_attr.nLength = sizeof(SECURITY_ATTRIBUTES);
-  sa_attr.bInheritHandle = TRUE;
-  sa_attr.lpSecurityDescriptor = NULL;
-
-  // Create the pipe for the child process's STDOUT.
-  if (!CreatePipe(&out_read, &out_write, &sa_attr, 0)) {
-    NOTREACHED() << "Failed to create pipe";
-    return false;
-  }
-
-  // Ensure we don't leak the handles.
-  win::ScopedHandle scoped_out_read(out_read);
-  win::ScopedHandle scoped_out_write(out_write);
-
-  // Ensure the read handle to the pipe for STDOUT is not inherited.
-  if (!SetHandleInformation(out_read, HANDLE_FLAG_INHERIT, 0)) {
-    NOTREACHED() << "Failed to disabled pipe inheritance";
-    return false;
-  }
-
-  FilePath::StringType writable_command_line_string;
-  writable_command_line_string.assign(cl.data(), cl.size());
-
-  STARTUPINFO start_info = {};
-
-  start_info.cb = sizeof(STARTUPINFO);
-  start_info.hStdOutput = out_write;
-  // Keep the normal stdin and stderr.
-  start_info.hStdInput = GetStdHandle(STD_INPUT_HANDLE);
-  start_info.hStdError = GetStdHandle(STD_ERROR_HANDLE);
-  start_info.dwFlags |= STARTF_USESTDHANDLES;
-
-  // Create the child process.
-  PROCESS_INFORMATION temp_process_info = {};
-  if (!CreateProcess(NULL,
-                     &writable_command_line_string[0],
-                     NULL, NULL,
-                     TRUE,  // Handles are inherited.
-                     0, NULL, NULL, &start_info, &temp_process_info)) {
-    NOTREACHED() << "Failed to start process";
-    return false;
-  }
-  base::win::ScopedProcessInformation proc_info(temp_process_info);
-
-  // Close our writing end of pipe now. Otherwise later read would not be able
-  // to detect end of child's output.
-  scoped_out_write.Close();
-
-  // Read output from the child process's pipe for STDOUT
-  const int kBufferSize = 1024;
-  char buffer[kBufferSize];
-
-  for (;;) {
-    DWORD bytes_read = 0;
-    BOOL success = ReadFile(out_read, buffer, kBufferSize, &bytes_read, NULL);
-    if (!success || bytes_read == 0)
-      break;
-    output->append(buffer, bytes_read);
-  }
-
-  // Let's wait for the process to finish.
-  WaitForSingleObject(proc_info.process_handle(), INFINITE);
+bool GetAppOutputAndError(const CommandLine& cl, std::string* output) {
+  return GetAppOutputInternal(cl.GetCommandLineString(), true, output);
+}
 
-  return true;
+bool GetAppOutput(const StringPiece16& cl, std::string* output) {
+  return GetAppOutputInternal(cl, false, output);
 }
 
 void RaiseProcessToHighPriority() {
diff --git a/base/test/test_suite.cc b/base/test/test_suite.cc
index 2910466..fee7b68 100644
--- a/base/test/test_suite.cc
+++ b/base/test/test_suite.cc
@@ -17,6 +17,7 @@
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/path_service.h"
+#include "base/process/launch.h"
 #include "base/process/memory.h"
 #include "base/test/gtest_xml_unittest_result_printer.h"
 #include "base/test/gtest_xml_util.h"
@@ -312,6 +313,7 @@ void TestSuite::Initialize() {
 
   CHECK(debug::EnableInProcessStackDumping());
 #if defined(OS_WIN)
+  RouteStdioToConsole(true);
   // Make sure we run with high resolution timer to minimize differences
   // between production code and test code.
   Time::EnableHighResolutionTimer(true);
diff --git a/components/nacl/loader/nacl_helper_win_64.cc b/components/nacl/loader/nacl_helper_win_64.cc
index d67b7b3..91bb8ce 100644
--- a/components/nacl/loader/nacl_helper_win_64.cc
+++ b/components/nacl/loader/nacl_helper_win_64.cc
@@ -61,7 +61,7 @@ int NaClWin64Main() {
   content::SetupCRT(command_line);
   // Route stdio to parent console (if any) or create one.
   if (command_line.HasSwitch(switches::kEnableLogging))
-    base::RouteStdioToConsole();
+    base::RouteStdioToConsole(true);
 
   // Initialize the sandbox for this process.
   bool sandbox_initialized_ok = content::InitializeSandbox(&sandbox_info);
diff --git a/components/nacl/loader/nacl_main_platform_delegate_win.cc b/components/nacl/loader/nacl_main_platform_delegate_win.cc
index 2681138..e4d0ad5 100644
--- a/components/nacl/loader/nacl_main_platform_delegate_win.cc
+++ b/components/nacl/loader/nacl_main_platform_delegate_win.cc
@@ -21,12 +21,6 @@ void NaClMainPlatformDelegate::EnableSandbox(
   ::GetUserDefaultLangID();
   ::GetUserDefaultLCID();
 
-#if defined(ADDRESS_SANITIZER)
-    // Bind and leak dbghelp.dll before the token is lowered, otherwise
-    // AddressSanitizer will crash when trying to symbolize a report.
-    CHECK(LoadLibraryA("dbghelp.dll"));
-#endif
-
   // Turn the sandbox on.
   target_services->LowerToken();
 }
diff --git a/content/app/content_main_runner.cc b/content/app/content_main_runner.cc
index df069e8..c45b553 100644
--- a/content/app/content_main_runner.cc
+++ b/content/app/content_main_runner.cc
@@ -10,6 +10,7 @@
 #include "base/at_exit.h"
 #include "base/command_line.h"
 #include "base/debug/debugger.h"
+#include "base/debug/stack_trace.h"
 #include "base/files/file_path.h"
 #include "base/i18n/icu_util.h"
 #include "base/lazy_instance.h"
@@ -195,6 +196,16 @@ void CommonSubprocessInit(const std::string& process_type) {
   // surface UI -- but it's likely they get this wrong too so why not.
   setlocale(LC_NUMERIC, "C");
 #endif
+
+#if !defined(OFFICIAL_BUILD)
+  // Print stack traces to stderr when crashes occur. This opens up security
+  // holes so it should never be enabled for official builds.
+  base::debug::EnableInProcessStackDumping();
+#if defined(OS_WIN)
+  base::RouteStdioToConsole(false);
+  LoadLibraryA("dbghelp.dll");
+#endif
+#endif
 }
 
 class ContentClientInitializer {
@@ -603,7 +614,7 @@ class ContentMainRunnerImpl : public ContentMainRunner {
 #if defined(OS_WIN)
     // Route stdio to parent console (if any) or create one.
     if (command_line.HasSwitch(switches::kEnableLogging))
-      base::RouteStdioToConsole();
+      base::RouteStdioToConsole(true);
 #endif
 
     // Enable startup tracing asap to avoid early TRACE_EVENT calls being
diff --git a/content/common/sandbox_linux/sandbox_linux.cc b/content/common/sandbox_linux/sandbox_linux.cc
index 952772e..15472c3 100644
--- a/content/common/sandbox_linux/sandbox_linux.cc
+++ b/content/common/sandbox_linux/sandbox_linux.cc
@@ -140,13 +140,6 @@ void LinuxSandbox::PreinitializeSandbox() {
   sanitizer_args_.reset();
 #endif
 
-#if !defined(NDEBUG) || (defined(CFI_ENFORCEMENT) && !defined(OFFICIAL_BUILD))
-  // The in-process stack dumping needs to open /proc/self/maps and cache
-  // its contents before the sandbox is enabled.  It also pre-opens the
-  // object files that are already loaded in the process address space.
-  base::debug::EnableInProcessStackDumpingForSandbox();
-#endif  // !defined(NDEBUG)
-
   // Open proc_fd_. It would break the security of the setuid sandbox if it was
   // not closed.
   // If LinuxSandbox::PreinitializeSandbox() runs, InitializeSandbox() must run
diff --git a/content/common/sandbox_win.cc b/content/common/sandbox_win.cc
index 7627865..110edb8 100644
--- a/content/common/sandbox_win.cc
+++ b/content/common/sandbox_win.cc
@@ -328,8 +328,8 @@ bool AddGenericPolicy(sandbox::TargetPolicy* policy) {
     return false;
 #endif  // NDEBUG
 
-  // Add the policy for read-only PDB file access for AddressSanitizer.
-#if defined(ADDRESS_SANITIZER)
+  // Add the policy for read-only PDB file access for stack traces.
+#if !defined(OFFICIAL_BUILD)
   base::FilePath exe;
   if (!PathService::Get(base::FILE_EXE, &exe))
     return false;
@@ -741,12 +741,12 @@ base::Process StartSandboxedProcess(
     return base::Process();
   }
 
-  if (browser_command_line.HasSwitch(switches::kEnableLogging)) {
-    // If stdout/stderr point to a Windows console, these calls will
-    // have no effect.
-    policy->SetStdoutHandle(GetStdHandle(STD_OUTPUT_HANDLE));
-    policy->SetStderrHandle(GetStdHandle(STD_ERROR_HANDLE));
-  }
+#if !defined(OFFICIAL_BUILD)
+  // If stdout/stderr point to a Windows console, these calls will
+  // have no effect.
+  policy->SetStdoutHandle(GetStdHandle(STD_OUTPUT_HANDLE));
+  policy->SetStderrHandle(GetStdHandle(STD_ERROR_HANDLE));
+#endif
 
   if (delegate) {
     bool success = true;
diff --git a/content/gpu/gpu_main.cc b/content/gpu/gpu_main.cc
index 7820638..8606afe 100644
--- a/content/gpu/gpu_main.cc
+++ b/content/gpu/gpu_main.cc
@@ -540,13 +540,6 @@ bool StartSandboxWindows(const sandbox::SandboxInterfaceInfo* sandbox_info) {
   // content.
   sandbox::TargetServices* target_services = sandbox_info->target_services;
   if (target_services) {
-#if defined(ADDRESS_SANITIZER)
-    // Bind and leak dbghelp.dll before the token is lowered, otherwise
-    // AddressSanitizer will crash when trying to symbolize a report.
-    if (!LoadLibraryA("dbghelp.dll"))
-      return false;
-#endif
-
     target_services->LowerToken();
     return true;
   }
diff --git a/content/ppapi_plugin/ppapi_thread.cc b/content/ppapi_plugin/ppapi_thread.cc
index d220244..6b2dc12 100644
--- a/content/ppapi_plugin/ppapi_thread.cc
+++ b/content/ppapi_plugin/ppapi_thread.cc
@@ -404,12 +404,6 @@ void PpapiThread::OnLoadPlugin(const base::FilePath& path,
 
     WarmupWindowsLocales(permissions);
 
-#if defined(ADDRESS_SANITIZER)
-    // Bind and leak dbghelp.dll before the token is lowered, otherwise
-    // AddressSanitizer will crash when trying to symbolize a report.
-    LoadLibraryA("dbghelp.dll");
-#endif
-
     g_target_services->LowerToken();
   }
 #endif
diff --git a/content/renderer/renderer_main.cc b/content/renderer/renderer_main.cc
index 2d82597..3a34300 100644
--- a/content/renderer/renderer_main.cc
+++ b/content/renderer/renderer_main.cc
@@ -6,7 +6,6 @@
 #include "base/command_line.h"
 #include "base/debug/debugger.h"
 #include "base/debug/leak_annotations.h"
-#include "base/debug/stack_trace.h"
 #include "base/i18n/rtl.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/field_trial.h"
@@ -184,17 +183,8 @@ int RendererMain(const MainFunctionParams& parameters) {
                              renderer_scheduler.Pass());
 #endif
     bool run_loop = true;
-    if (!no_sandbox) {
+    if (!no_sandbox)
       run_loop = platform.EnableSandbox();
-    } else {
-      LOG(ERROR) << "Running without renderer sandbox";
-#if !defined(NDEBUG) || (defined(CFI_ENFORCEMENT) && !defined(OFFICIAL_BUILD))
-      // For convenience, we print the stack traces for crashes.  When sandbox
-      // is enabled, the in-process stack dumping is enabled as part of the
-      // EnableSandbox() call.
-      base::debug::EnableInProcessStackDumping();
-#endif
-    }
 #if defined(OS_POSIX) && !defined(OS_MACOSX)
     RenderProcessImpl render_process;
     RenderThreadImpl::Create(main_message_loop.Pass(),
diff --git a/content/renderer/renderer_main_platform_delegate_win.cc b/content/renderer/renderer_main_platform_delegate_win.cc
index 2d769e8..3cf583d 100644
--- a/content/renderer/renderer_main_platform_delegate_win.cc
+++ b/content/renderer/renderer_main_platform_delegate_win.cc
@@ -109,13 +109,6 @@ bool RendererMainPlatformDelegate::EnableSandbox() {
     ::GetUserDefaultLangID();
     ::GetUserDefaultLCID();
 
-#if defined(ADDRESS_SANITIZER)
-    // Bind and leak dbghelp.dll before the token is lowered, otherwise
-    // AddressSanitizer will crash when trying to symbolize a report.
-    if (!LoadLibraryA("dbghelp.dll"))
-      return false;
-#endif
-
     target_services->LowerToken();
     return true;
   }
diff --git a/content/test/content_browser_test_test.cc b/content/test/content_browser_test_test.cc
index f574f0c..051a1325 100644
--- a/content/test/content_browser_test_test.cc
+++ b/content/test/content_browser_test_test.cc
@@ -6,23 +6,125 @@
 
 #include "base/command_line.h"
 #include "base/location.h"
+#include "base/process/launch.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/launcher/test_launcher.h"
 #include "base/thread_task_runner_handle.h"
+#include "content/public/browser/render_process_host.h"
+#include "content/public/browser/render_process_host_observer.h"
+#include "content/public/browser/web_contents.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/content_browser_test_utils.h"
+#include "content/public/test/test_launcher.h"
 #include "content/public/test/test_utils.h"
 #include "content/shell/browser/shell.h"
+#include "content/shell/common/shell_switches.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace content {
 
+// On Android symbolization happens in one step after all the tests ran, so this
+// test doesn't work there.
+// TODO(mac): figure out why symbolization doesn't happen in the renderer.
+#if !defined(OS_ANDROID) && !defined(OS_MACOSX)
+
 IN_PROC_BROWSER_TEST_F(ContentBrowserTest, MANUAL_ShouldntRun) {
   // Ensures that tests with MANUAL_ prefix don't run automatically.
   ASSERT_TRUE(false);
 }
 
+class CrashObserver : public RenderProcessHostObserver {
+ public:
+  CrashObserver(const base::Closure& quit_closure)
+      : quit_closure_(quit_closure) {}
+  void RenderProcessExited(RenderProcessHost* host,
+                           base::TerminationStatus status,
+                           int exit_code) override {
+    ASSERT_TRUE(status == base::TERMINATION_STATUS_PROCESS_CRASHED ||
+                status == base::TERMINATION_STATUS_ABNORMAL_TERMINATION);
+    quit_closure_.Run();
+  }
+
+ private:
+  base::Closure quit_closure_;
+};
+
+IN_PROC_BROWSER_TEST_F(ContentBrowserTest, MANUAL_RendererCrash) {
+  scoped_refptr<MessageLoopRunner> message_loop_runner = new MessageLoopRunner;
+  CrashObserver crash_observer(message_loop_runner->QuitClosure());
+  shell()->web_contents()->GetRenderProcessHost()->AddObserver(&crash_observer);
+
+  NavigateToURL(shell(), GURL("chrome:crash"));
+  message_loop_runner->Run();
+}
+
+// Tests that browser tests print the callstack when a child process crashes.
+IN_PROC_BROWSER_TEST_F(ContentBrowserTest, RendererCrashCallStack) {
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  base::CommandLine new_test =
+      base::CommandLine(base::CommandLine::ForCurrentProcess()->GetProgram());
+  new_test.AppendSwitchASCII(base::kGTestFilterFlag,
+                             "ContentBrowserTest.MANUAL_RendererCrash");
+  new_test.AppendSwitch(kRunManualTestsFlag);
+  new_test.AppendSwitch(kSingleProcessTestsFlag);
+
+#if defined(ADDRESS_SANITIZER)
+  // Per https://www.chromium.org/developers/testing/addresssanitizer, there are
+  // ASAN bots that run without the sandbox which this test will pass for. The
+  // other ones pipe the output to a symbolizer script.
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kNoSandbox)) {
+    new_test.AppendSwitch(switches::kNoSandbox);
+  } else {
+    LOG(INFO) << "Couldn't run ContentBrowserTest.RendererCrashCallStack since "
+              << "sandbox is enabled and ASAN requires piping to an external "
+              << "script.";
+    return;
+  }
+#endif
+
+  std::string output;
+  base::GetAppOutputAndError(new_test, &output);
+
+  std::string crash_string =
+      "content::RenderFrameImpl::PrepareRenderViewForNavigation";
+
+  if (output.find(crash_string) == std::string::npos) {
+    GTEST_FAIL() << "Couldn't find\n" << crash_string << "\n in output\n "
+                 << output;
+  }
+}
+
+IN_PROC_BROWSER_TEST_F(ContentBrowserTest, MANUAL_BrowserCrash) {
+  CHECK(false);
+}
+
+// Tests that browser tests print the callstack on asserts.
+IN_PROC_BROWSER_TEST_F(ContentBrowserTest, BrowserCrashCallStack) {
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  base::CommandLine new_test =
+      base::CommandLine(base::CommandLine::ForCurrentProcess()->GetProgram());
+  new_test.AppendSwitchASCII(base::kGTestFilterFlag,
+                             "ContentBrowserTest.MANUAL_BrowserCrash");
+  new_test.AppendSwitch(kRunManualTestsFlag);
+  new_test.AppendSwitch(kSingleProcessTestsFlag);
+  std::string output;
+  base::GetAppOutputAndError(new_test, &output);
+
+  std::string crash_string =
+      "content::ContentBrowserTest_MANUAL_BrowserCrash_Test::RunTestOnMainThread";
+
+  if (output.find(crash_string) == std::string::npos) {
+    GTEST_FAIL() << "Couldn't find\n" << crash_string << "\n in output\n "
+                 << output;
+  }
+}
+
+#endif
+
 class ContentBrowserTestSanityTest : public ContentBrowserTest {
  public:
   void SetUpCommandLine(base::CommandLine* command_line) override {
@@ -59,7 +161,7 @@ void CallbackChecker(bool* non_nested_task_ran) {
 
 }  // namespace
 
-IN_PROC_BROWSER_TEST_F(ContentBrowserTestSanityTest, NonNestableTask) {
+IN_PROC_BROWSER_TEST_F(ContentBrowserTest, NonNestableTask) {
   bool non_nested_task_ran = false;
   base::ThreadTaskRunnerHandle::Get()->PostNonNestableTask(
       FROM_HERE, base::Bind(&CallbackChecker, &non_nested_task_ran));
diff --git a/content/utility/utility_main.cc b/content/utility/utility_main.cc
index 742a476..60a5d27 100644
--- a/content/utility/utility_main.cc
+++ b/content/utility/utility_main.cc
@@ -47,12 +47,6 @@ int UtilityMain(const MainFunctionParams& parameters) {
         parameters.sandbox_info->target_services;
     if (!target_services)
       return false;
-#if defined(ADDRESS_SANITIZER)
-    // Bind and leak dbghelp.dll before the token is lowered, otherwise
-    // AddressSanitizer will crash when trying to symbolize a report.
-    if (!LoadLibraryA("dbghelp.dll"))
-      return false;
-#endif
     char buffer;
     // Ensure RtlGenRandom is warm before the token is lowered; otherwise,
     // base::RandBytes() will CHECK fail when v8 is initialized.
diff --git a/mojo/application/public/cpp/lib/application_runner.cc b/mojo/application/public/cpp/lib/application_runner.cc
index 3127892..12125ff 100644
--- a/mojo/application/public/cpp/lib/application_runner.cc
+++ b/mojo/application/public/cpp/lib/application_runner.cc
@@ -9,6 +9,7 @@
 #include "base/debug/stack_trace.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
+#include "base/process/launch.h"
 #include "base/threading/worker_pool.h"
 #include "mojo/application/public/cpp/application_delegate.h"
 #include "mojo/application/public/cpp/application_impl.h"
@@ -46,8 +47,11 @@ MojoResult ApplicationRunner::Run(MojoHandle application_request_handle,
   if (init_base) {
     InitBaseCommandLine();
     at_exit.reset(new base::AtExitManager);
-#ifndef NDEBUG
+#ifndef OFFICIAL_BUILD
     base::debug::EnableInProcessStackDumping();
+#if defined(OS_WIN)
+    base::RouteStdioToConsole(false);
+#endif
 #endif
   }