[test] Provide control of stack trace generation in tests

Collecting and emitting stack traces is not free. Tests that run in
debug configurations may time out solely due to this cost. There are
many cases in which stacks traces may never be visible to the developer.
One such case is in a death test's child proc: the child process is
expected to crash, so any stack traces generated for this are expected
and hidden.

In https://crrev.com/1246434 we suppressed the generation and printing
of stack traces in CHECK/LOG(FATAL)/etc messages in death tests. In
https://crrev.com/1272204 we replaced this with a change to `StackTrace`
itself to suppress output in any case in death tests. In this change, we
go one step further as follows:

By default, `base::debug::StackTrace` will no longer collect a trace in
a death test, and printing an empty stack trace will no longer do any
processing. Tests that require a stack trace in a death test may use
base::debug::OverrideStackTraceOutputForTesting to force-enable
generation and output of traces in death tests. Additionally, some
non-death tests may also result in the generation and emission of stack
traces that will never be seen (e.g., those that use
`EXPECT_FATAL_FAILURE`). These tests can use the same test helper to
suppress the generation and output of stack traces in non-death tests.

Bug: 41490321
Change-Id: I036e333ec66731ee125f2e80c2e05b002b339e7a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5377866
Auto-Submit: Greg Thompson <grt@chromium.org>
Commit-Queue: Greg Thompson <grt@chromium.org>
Reviewed-by: Alberto Juarez <albertojuarez@google.com>
Reviewed-by: Francois Pierre Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1276095}

base/debug/stack_trace.cc

@@ -164,17 +164,10 @@ void* LinkStackFrames(void* fpp, void* parent_fp) {
 
 #endif  // BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS)
 
-// True if an OverrideSuppressedOutputForTesting instance is alive to force
+// True if an OverrideStackTraceOutputForTesting instance is alive to force
 // generation of symbolized stack traces in death tests.
-bool g_override_suppression = false;
-
-// Returns true if generation of symbolized stack traces is to be suppressed.
-bool ShouldSuppressOutput() {
-  // Backtraces are not visible in death test children, so do not waste
-  // resources by generating any unless an OverrideSuppressedOutputForTesting
-  // instance is alive.
-  return !g_override_suppression && ::base::internal::InDeathTestChild();
-}
+OverrideStackTraceOutputForTesting::Mode g_override_suppression =
+    OverrideStackTraceOutputForTesting::Mode::kUnset;
 
 }  // namespace
 
@@ -230,15 +223,17 @@ uintptr_t GetStackEnd() {
 
 StackTrace::StackTrace() : StackTrace(std::size(trace_)) {}
 
-StackTrace::StackTrace(size_t count) {
-  count_ = CollectStackTrace(trace_, std::min(count, std::size(trace_)));
-}
+StackTrace::StackTrace(size_t count)
+    : count_(ShouldSuppressOutput()
+                 ? 0
+                 : CollectStackTrace(trace_,
+                                     std::min(count, std::size(trace_)))) {}
 
-StackTrace::StackTrace(const void* const* trace, size_t count) {
-  count = std::min(count, std::size(trace_));
-  if (count)
-    memcpy(trace_, trace, count * sizeof(trace_[0]));
-  count_ = count;
+StackTrace::StackTrace(const void* const* trace, size_t count)
+    : count_(std::min(count, std::size(trace_))) {
+  if (count_) {
+    memcpy(trace_, trace, count_ * sizeof(trace_[0]));
+  }
 }
 
 // static
@@ -279,10 +274,6 @@ void StackTrace::Print() const {
 }
 
 void StackTrace::OutputToStream(std::ostream* os) const {
-  if (ShouldSuppressOutput()) {
-    (*os) << "Backtrace suppressed.";
-    return;
-  }
   OutputToStreamWithPrefix(os, nullptr);
 }
 
@@ -292,14 +283,22 @@ std::string StackTrace::ToString() const {
 std::string StackTrace::ToStringWithPrefix(const char* prefix_string) const {
   std::stringstream stream;
 #if !defined(__UCLIBC__) && !defined(_AIX)
-  if (ShouldSuppressOutput()) {
-    return "Backtrace suppressed.";
-  }
   OutputToStreamWithPrefix(&stream, prefix_string);
 #endif
   return stream.str();
 }
 
+// static
+bool StackTrace::ShouldSuppressOutput() {
+  using Mode = OverrideStackTraceOutputForTesting::Mode;
+  // Backtraces are not visible in death test children, so do not waste
+  // resources by generating any unless an OverrideStackTraceOutputForTesting
+  // instance is alive.
+  return g_override_suppression != Mode::kUnset
+             ? (g_override_suppression == Mode::kSuppressOutput)
+             : ::base::internal::InDeathTestChild();
+}
+
 std::ostream& operator<<(std::ostream& os, const StackTrace& s) {
 #if !defined(__UCLIBC__) && !defined(_AIX)
   s.OutputToStream(&os);
@@ -309,14 +308,16 @@ std::ostream& operator<<(std::ostream& os, const StackTrace& s) {
   return os;
 }
 
-OverrideSuppressedOutputForTesting::OverrideSuppressedOutputForTesting() {
-  CHECK(!g_override_suppression);  // Nesting not supported.
-  g_override_suppression = true;
+OverrideStackTraceOutputForTesting::OverrideStackTraceOutputForTesting(
+    Mode mode) {
+  CHECK_NE(mode, Mode::kUnset);
+  CHECK_EQ(g_override_suppression, Mode::kUnset);  // Nesting not supported.
+  g_override_suppression = mode;
 }
 
-OverrideSuppressedOutputForTesting::~OverrideSuppressedOutputForTesting() {
-  CHECK(g_override_suppression);  // Nesting not supported.
-  g_override_suppression = false;
+OverrideStackTraceOutputForTesting::~OverrideStackTraceOutputForTesting() {
+  CHECK_NE(g_override_suppression, Mode::kUnset);  // Nesting not supported.
+  g_override_suppression = Mode::kUnset;
 }
 
 #if BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS)

base/debug/stack_trace.h

@@ -135,14 +135,17 @@ class BASE_EXPORT StackTrace {
   std::string ToStringWithPrefix(const char* prefix_string) const;
 
  private:
+  // Returns true if generation of symbolized stack traces is to be suppressed.
+  static bool ShouldSuppressOutput();
+
 #if BUILDFLAG(IS_WIN)
   void InitTrace(const _CONTEXT* context_record);
 #endif
 
   const void* trace_[kMaxTraces];
 
-  // The number of valid frames in |trace_|.
-  size_t count_;
+  // The number of valid frames in |trace_|, or 0 if collection was suppressed.
+  size_t count_ = 0;
 };
 
 // Forwards to StackTrace::OutputToStream().
@@ -152,16 +155,22 @@ BASE_EXPORT std::ostream& operator<<(std::ostream& os, const StackTrace& s);
 // number of frames read.
 BASE_EXPORT size_t CollectStackTrace(const void** trace, size_t count);
 
-// A helper for death tests that must override the default suppression of
-// symbolized stack traces.
-class BASE_EXPORT OverrideSuppressedOutputForTesting {
+// A helper for tests that must either override the default suppression of
+// symbolized stack traces in death tests, or the default generation of them in
+// normal tests.
+class BASE_EXPORT OverrideStackTraceOutputForTesting {
  public:
-  OverrideSuppressedOutputForTesting();
-  ~OverrideSuppressedOutputForTesting();
-  OverrideSuppressedOutputForTesting(
-      const OverrideSuppressedOutputForTesting&) = delete;
-  OverrideSuppressedOutputForTesting& operator=(
-      const OverrideSuppressedOutputForTesting&) = delete;
+  enum class Mode {
+    kUnset,
+    kForceOutput,
+    kSuppressOutput,
+  };
+  explicit OverrideStackTraceOutputForTesting(Mode mode);
+  OverrideStackTraceOutputForTesting(
+      const OverrideStackTraceOutputForTesting&) = delete;
+  OverrideStackTraceOutputForTesting& operator=(
+      const OverrideStackTraceOutputForTesting&) = delete;
+  ~OverrideStackTraceOutputForTesting();
 };
 
 #if BUILDFLAG(CAN_UNWIND_WITH_FRAME_POINTERS)

base/debug/stack_trace_android.cc

@@ -91,6 +91,10 @@ void StackTrace::PrintWithPrefix(const char* prefix_string) const {
 // symbolize and demangle (e.g., addr2line, c++filt).
 void StackTrace::OutputToStreamWithPrefix(std::ostream* os,
                                           const char* prefix_string) const {
+  if (!count_) {
+    return;
+  }
+
   std::string proc_maps;
   std::vector<MappedMemoryRegion> regions;
   // Allow IO to read /proc/self/maps. Reading this file doesn't hit the disk

base/debug/stack_trace_fuchsia.cc

@@ -245,6 +245,9 @@ void StackTrace::PrintWithPrefix(const char* prefix_string) const {
 // https://fuchsia.googlesource.com/zircon/+/master/docs/symbolizer_markup.md
 void StackTrace::OutputToStreamWithPrefix(std::ostream* os,
                                           const char* prefix_string) const {
+  if (!count_) {
+    return;
+  }
   SymbolMap map;
 
   int module_id = 0;

base/debug/stack_trace_posix.cc

@@ -1045,7 +1045,9 @@ size_t CollectStackTrace(const void** trace, size_t count) {
 void StackTrace::PrintWithPrefix(const char* prefix_string) const {
 // NOTE: This code MUST be async-signal safe (it's used by in-process
 // stack dumping signal handler). NO malloc or stdio is allowed here.
-
+if (!count_) {
+  return;
+}
 #if defined(HAVE_BACKTRACE)
   PrintBacktraceOutputHandler handler;
   ProcessBacktrace(trace_, count_, prefix_string, &handler);
@@ -1055,6 +1057,9 @@ void StackTrace::PrintWithPrefix(const char* prefix_string) const {
 #if defined(HAVE_BACKTRACE)
 void StackTrace::OutputToStreamWithPrefix(std::ostream* os,
                                           const char* prefix_string) const {
+  if (!count_) {
+    return;
+  }
   StreamBacktraceOutputHandler handler(os);
   ProcessBacktrace(trace_, count_, prefix_string, &handler);
 }

base/debug/stack_trace_win.cc

@@ -17,6 +17,7 @@
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/memory/singleton.h"
+#include "base/ranges/algorithm.h"
 #include "base/strings/strcat_win.h"
 #include "base/strings/string_util.h"
 #include "base/synchronization/lock.h"
@@ -338,6 +339,12 @@ StackTrace::StackTrace(const CONTEXT* context) {
 }
 
 void StackTrace::InitTrace(const CONTEXT* context_record) {
+  if (ShouldSuppressOutput()) {
+    CHECK_EQ(count_, 0U);
+    base::ranges::fill(trace_, nullptr);
+    return;
+  }
+
   // StackWalk64 modifies the register context in place, so we have to copy it
   // so that downstream exception handlers get the right context.  The incoming
   // context may have had more register state (YMM, etc) than we need to unwind
@@ -389,6 +396,9 @@ void StackTrace::PrintWithPrefix(const char* prefix_string) const {
 
 void StackTrace::OutputToStreamWithPrefix(std::ostream* os,
                                           const char* prefix_string) const {
+  if (!count_) {
+    return;
+  }
   SymbolContext* context = SymbolContext::GetInstance();
   if (g_init_error != ERROR_SUCCESS) {
     (*os) << "Error initializing symbols (" << g_init_error

base/gtest_prod_util.cc

@@ -8,16 +8,15 @@ namespace base::internal {
 
 namespace {
 
-InDeathTestChildFn g_in_death_test_fn = nullptr;
-
+bool g_in_death_test = false;
 }
 
 bool InDeathTestChild() {
-  return g_in_death_test_fn && (*g_in_death_test_fn)();
+  return g_in_death_test;
 }
 
-void SetInDeathTestChildFn(InDeathTestChildFn in_death_test_child_fn) {
-  g_in_death_test_fn = in_death_test_child_fn;
+void SetInDeathTestChildForTesting(bool in_death_test_child) {
+  g_in_death_test = in_death_test_child;
 }
 
 }  // namespace base::internal

base/gtest_prod_util.h

@@ -68,18 +68,15 @@ namespace base::internal {
 
 // Returns true if executing within the context of a death test child process.
 // This is an internal utility. You do not want to call this. This is provided
-// for the sole purpose of suppressing expensive diagnostic logging in these
-// child processes, as this logging is ordinarily not exposed to developers.
+// for the purpose of suppressing expensive diagnostic logging in these child
+// processes, as this logging is ordinarily not exposed to developers.
 bool InDeathTestChild();
 
-// Sets a function that may be called to detect whether or not execution is
-// within the context of a death test child process. You do not want to call
-// this. This is provided so that base::TestSuite can make Google Test's
-// InDeathTestChild implementation available for very specific use in production
-// code; see above.
-using InDeathTestChildFn = bool (*)();
-BASE_EXPORT void SetInDeathTestChildFn(
-    InDeathTestChildFn in_death_test_child_fn);
+// Sets whether or not execution is within the context of a death test child
+// process. You do not want to call this. This is provided so that
+// base::TestSuite can provide the result of Google Test's InDeathTestChild
+// function for very specific use in production code; see above.
+BASE_EXPORT void SetInDeathTestChildForTesting(bool in_death_test_child);
 
 }  // namespace base::internal
 

base/test/test_suite.cc

@@ -504,6 +504,18 @@ void TestSuite::Initialize() {
 
   InitializeFromCommandLine(&argc_, argv_);
 
+#if GTEST_HAS_DEATH_TEST
+  if (::testing::internal::InDeathTestChild()) {
+    // For death tests using the "threadsafe" style (which includes all such
+    // tests on Windows and Fuchsia, and is the default for all Chromium tests
+    // on all platforms except Android; see `PreInitialize`),
+    //
+    // For more information, see
+    // https://github.com/google/googletest/blob/main/docs/advanced.md#death-test-styles.
+    internal::SetInDeathTestChildForTesting(true);
+  }
+#endif
+
   // Logging must be initialized before any thread has a chance to call logging
   // functions.
   InitializeLogging();
@@ -612,10 +624,6 @@ void TestSuite::Initialize() {
 }
 
 void TestSuite::InitializeFromCommandLine(int* argc, char** argv) {
-#if GTEST_HAS_DEATH_TEST
-  internal::SetInDeathTestChildFn(&::testing::internal::InDeathTestChild);
-#endif
-
   // CommandLine::Init() is called earlier from PreInitialize().
   testing::InitGoogleTest(argc, argv);
   testing::InitGoogleMock(argc, argv);

base/threading/thread_restrictions_unittest.cc

@@ -181,7 +181,8 @@ TEST_F(ThreadRestrictionsTest, DisallowUnresponsiveTasks) {
     defined(GTEST_HAS_DEATH_TEST)
 
 TEST_F(ThreadRestrictionsTest, BlockingCheckEmitsStack) {
-  debug::OverrideSuppressedOutputForTesting enable_stacks_in_death_tests;
+  debug::OverrideStackTraceOutputForTesting enable_stacks_in_death_tests(
+      debug::OverrideStackTraceOutputForTesting::Mode::kForceOutput);
   ScopedDisallowBlocking scoped_disallow_blocking;
   // The above ScopedDisallowBlocking should be on the blame list for who set
   // the ban.
@@ -205,7 +206,8 @@ class TestCustomDisallow {
 };
 
 TEST_F(ThreadRestrictionsTest, NestedAllowRestoresPreviousStack) {
-  debug::OverrideSuppressedOutputForTesting enable_stacks_in_death_tests;
+  debug::OverrideStackTraceOutputForTesting enable_stacks_in_death_tests(
+      debug::OverrideStackTraceOutputForTesting::Mode::kForceOutput);
   TestCustomDisallow custom_disallow;
   {
     ScopedAllowBlocking scoped_allow;

components/reporting/util/status_macros_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "components/reporting/util/status_macros.h"
 
+#include "base/debug/stack_trace.h"
 #include "base/types/expected.h"
 #include "components/reporting/util/status.h"
 #include "components/reporting/util/statusor.h"
@@ -141,6 +142,15 @@ void AssertOKErrorStatusOr() {
 }
 
 TEST(StatusMacros, AssertOKOnStatusOr) {
+  // The assertion failure in AssertOKErrorStatusOr will generate a stack trace.
+  // This is desirable for the normal case, where the trace can help the
+  // developer understand the failure. In this test, however, the failure is
+  // expected and its output is swallowed. Suppress generation of stack traces
+  // so that the cost of generating them does not lead to flaky timeouts in
+  // configurations where the collection and emission of stack traces is
+  // expensive (e.g., in debug builds).
+  base::debug::OverrideStackTraceOutputForTesting suppress_stacks(
+      base::debug::OverrideStackTraceOutputForTesting::Mode::kSuppressOutput);
   StatusOr<int> status_or(2);
   ASSERT_OK(status_or);
   ASSERT_OK(status_or) << "error message";
@@ -154,6 +164,15 @@ void ExpectOKErrorStatusOr() {
 }
 
 TEST(StatusMacros, ExpectOKOnStatusOr) {
+  // The assertion failure in ExpectOKErrorStatusOr will generate a stack trace.
+  // This is desirable for the normal case, where the trace can help the
+  // developer understand the failure. In this test, however, the failure is
+  // expected and its output is swallowed. Suppress generation of stack traces
+  // so that the cost of generating them does not lead to flaky timeouts in
+  // configurations where the collection and emission of stack traces is
+  // expensive (e.g., in debug builds).
+  base::debug::OverrideStackTraceOutputForTesting suppress_stacks(
+      base::debug::OverrideStackTraceOutputForTesting::Mode::kSuppressOutput);
   StatusOr<int> status_or(2);
   EXPECT_OK(status_or);
   EXPECT_OK(status_or) << "error message";