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}
CrOS-Libchrome-Original-Commit: 79dc59ac7602413181079ecb463873e29a1d7d0a
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 @@
// 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 @@
// 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 @@
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 @@
fd = -1;
}
}
-#endif // !defined(NDEBUG)
+#endif // !defined(OFFICIAL_BUILD)
return fd;
}
@@ -619,11 +619,9 @@
// 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 @@
}
}
}
-#endif // !defined(NDEBUG)
+#endif // !defined(OFFICIAL_BUILD)
}
// Initializes and installs the symbolization callback.
@@ -677,7 +675,7 @@
// 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 @@
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 @@
};
#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/logging.cc b/base/logging.cc
index 559cb08..ea73161 100644
--- a/base/logging.cc
+++ b/base/logging.cc
@@ -543,7 +543,7 @@
}
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 @@
// 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 @@
// 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 @@
// 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 @@
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 @@
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 @@
// 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 @@
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 @@
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/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 @@
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/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 @@
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
}