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}

3 files changed, 78 insertions, 78 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";