Revert of Print stack traces in child processes when browser tests failed. (patchset #5 id:80001 of https://codereview.chromium.org/1291553003/ )

Reason for revert:
This is unfortunately breaking Windows 10 in a major way. I did a local bisect and narrowed it down to this CL.

BUG=521242

Original issue's description:
> Print stack traces in child processes when browser tests failed.
>
> 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 child processes on all platforms (i.e. to be able to debug the large flakiness that occurred since last week). This is disabled for official builds.
>
> BUG=517488,358267
>
> Committed: https://crrev.com/8ba532e170befc312e66d032587fa2ad04bac975
> Cr-Commit-Position: refs/heads/master@{#343240}

TBR=scottmg@chromium.org,jln@chromium.org,wfh@chromium.org,jam@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=517488,358267

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

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

12 files changed, 125 insertions, 107 deletions

diff --git a/base/debug/stack_trace.h b/base/debug/stack_trace.h
index b0fa3b0..fb271b6 100644
--- a/base/debug/stack_trace.h
+++ b/base/debug/stack_trace.h
@@ -54,8 +54,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(_EXCEPTION_POINTERS* exception_pointers);
-  StackTrace(_CONTEXT* context);
+  StackTrace(const _EXCEPTION_POINTERS* exception_pointers);
+  StackTrace(const _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 8f07a0d..dbbec36 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(OFFICIAL_BUILD)
+#if !defined(NDEBUG)
     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(OFFICIAL_BUILD)
+#endif  // !defined(NDEBUG)
 
     return fd;
   }
@@ -619,9 +619,11 @@ 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 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)
+    // 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)
     // Open the object files for all read-only executable regions and cache
     // their file descriptors.
     std::vector<MappedMemoryRegion>::const_iterator it;
@@ -653,7 +655,7 @@ class SandboxSymbolizeHelper {
         }
       }
     }
-#endif  // !defined(OFFICIAL_BUILD)
+#endif  // !defined(NDEBUG)
   }
 
   // Initializes and installs the symbolization callback.
@@ -675,7 +677,7 @@ class SandboxSymbolizeHelper {
 
   // Closes all file descriptors owned by this instance.
   void CloseObjectFiles() {
-#if !defined(OFFICIAL_BUILD)
+#if !defined(NDEBUG)
     std::map<std::string, int>::iterator it;
     for (it = modules_.begin(); it != modules_.end(); ++it) {
       int ret = IGNORE_EINTR(close(it->second));
@@ -683,18 +685,19 @@ class SandboxSymbolizeHelper {
       it->second = -1;
     }
     modules_.clear();
-#endif  // !defined(OFFICIAL_BUILD)
+#endif  // !defined(NDEBUG)
   }
 
   // Set to true upon successful initialization.
   bool is_initialized_;
 
-#if !defined(OFFICIAL_BUILD)
+#if !defined(NDEBUG)
   // 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 production builds.
+  // /proc/self/maps.  This code is not safe for release builds so
+  // this is only done for DEBUG builds.
   std::map<std::string, int> modules_;
-#endif  // !defined(OFFICIAL_BUILD)
+#endif  // !defined(NDEBUG)
 
   // Cache for the process memory regions.  Produced by parsing the contents
   // of /proc/self/maps cache.
diff --git a/base/debug/stack_trace_win.cc b/base/debug/stack_trace_win.cc
index ceccef5..55d5562 100644
--- a/base/debug/stack_trace_win.cc
+++ b/base/debug/stack_trace_win.cc
@@ -26,9 +26,6 @@ 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) {
@@ -45,55 +42,6 @@ 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
@@ -118,6 +66,11 @@ 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:
   //
@@ -179,10 +132,51 @@ class SymbolContext {
  private:
   friend struct DefaultSingletonTraits<SymbolContext>;
 
-  SymbolContext() {
-    InitializeSymbols();
+  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;
+    }
   }
 
+  DWORD init_error_;
   base::Lock lock_;
   DISALLOW_COPY_AND_ASSIGN(SymbolContext);
 };
@@ -194,11 +188,7 @@ bool EnableInProcessStackDumping() {
   // signal() handling in process_util_posix.cc.
   g_previous_filter = SetUnhandledExceptionFilter(&StackDumpExceptionFilter);
   RouteStdioToConsole();
-
-  // 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();
+  return true;
 }
 
 // Disable optimizations for the StackTrace::StackTrace function. It is
@@ -219,12 +209,18 @@ StackTrace::StackTrace() {
 #pragma optimize("", on)
 #endif
 
-StackTrace::StackTrace(EXCEPTION_POINTERS* exception_pointers) {
-  InitTrace(exception_pointers->ContextRecord);
+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(CONTEXT* context) {
-  InitTrace(context);
+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);
 }
 
 void StackTrace::InitTrace(CONTEXT* context_record) {
@@ -270,8 +266,9 @@ void StackTrace::Print() const {
 
 void StackTrace::OutputToStream(std::ostream* os) const {
   SymbolContext* context = SymbolContext::GetInstance();
-  if (g_init_error != ERROR_SUCCESS) {
-    (*os) << "Error initializing symbols (" << g_init_error
+  DWORD error = context->init_error();
+  if (error != ERROR_SUCCESS) {
+    (*os) << "Error initializing symbols (" << error
           << ").  Dumping unresolved backtrace:\n";
     for (size_t i = 0; (i < count_) && os->good(); ++i) {
       (*os) << "\t" << trace_[i] << "\n";
diff --git a/base/process/launch_win.cc b/base/process/launch_win.cc
index c17eba5..fa59f1a 100644
--- a/base/process/launch_win.cc
+++ b/base/process/launch_win.cc
@@ -64,22 +64,8 @@ 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) {
-    // _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 (_fileno(stdout) >= 0 || _fileno(stderr) >= 0)
+    return;
 
   if (!AttachConsole(ATTACH_PARENT_PROCESS)) {
     unsigned int result = GetLastError();
diff --git a/components/nacl/loader/nacl_main_platform_delegate_win.cc b/components/nacl/loader/nacl_main_platform_delegate_win.cc
index e4d0ad5..2681138 100644
--- a/components/nacl/loader/nacl_main_platform_delegate_win.cc
+++ b/components/nacl/loader/nacl_main_platform_delegate_win.cc
@@ -21,6 +21,12 @@ 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 55c6ac7..df069e8 100644
--- a/content/app/content_main_runner.cc
+++ b/content/app/content_main_runner.cc
@@ -10,7 +10,6 @@
 #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"
@@ -196,15 +195,6 @@ 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)
-  LoadLibraryA("dbghelp.dll");
-#endif
-#endif
 }
 
 class ContentClientInitializer {
diff --git a/content/common/sandbox_win.cc b/content/common/sandbox_win.cc
index f475db3..7627865 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 stack traces.
-#if !defined(OFFICIAL_BUILD)
+  // Add the policy for read-only PDB file access for AddressSanitizer.
+#if defined(ADDRESS_SANITIZER)
   base::FilePath exe;
   if (!PathService::Get(base::FILE_EXE, &exe))
     return false;
diff --git a/content/gpu/gpu_main.cc b/content/gpu/gpu_main.cc
index 8606afe..7820638 100644
--- a/content/gpu/gpu_main.cc
+++ b/content/gpu/gpu_main.cc
@@ -540,6 +540,13 @@ 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 6b2dc12..d220244 100644
--- a/content/ppapi_plugin/ppapi_thread.cc
+++ b/content/ppapi_plugin/ppapi_thread.cc
@@ -404,6 +404,12 @@ 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 3a34300..2d82597 100644
--- a/content/renderer/renderer_main.cc
+++ b/content/renderer/renderer_main.cc
@@ -6,6 +6,7 @@
 #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"
@@ -183,8 +184,17 @@ 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 3cf583d..2d769e8 100644
--- a/content/renderer/renderer_main_platform_delegate_win.cc
+++ b/content/renderer/renderer_main_platform_delegate_win.cc
@@ -109,6 +109,13 @@ 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/utility/utility_main.cc b/content/utility/utility_main.cc
index 60a5d27..742a476 100644
--- a/content/utility/utility_main.cc
+++ b/content/utility/utility_main.cc
@@ -47,6 +47,12 @@ 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.