Block execution in ~HangWatchScope() when a hang is in progress

This gives more actionable hang reports as it allows to keep the
execution from advancing to unrelated parts of the code when we
are in the progress of capturing a hang.

Bug: 1034046
Change-Id: Ie4d5f31515fec829c527cf92ee9f723e5f940f5f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2068450
Commit-Queue: Oliver Li <olivierli@google.com>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#745775}

base/threading/hang_watcher.cc

@@ -5,6 +5,7 @@
 #include "base/threading/hang_watcher.h"
 
 #include <algorithm>
+#include <atomic>
 #include <utility>
 
 #include "base/bind.h"
@@ -71,6 +72,7 @@ HangWatchScope::HangWatchScope(TimeDelta timeout) {
 
 HangWatchScope::~HangWatchScope() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+
   internal::HangWatchState* current_hang_watch_state =
       internal::HangWatchState::GetHangWatchStateForCurrentThread()->Get();
 
@@ -80,6 +82,10 @@ HangWatchScope::~HangWatchScope() {
     return;
   }
 
+  // If a hang is currently being captured we should block here so execution
+  // stops and the relevant stack frames are recorded.
+  base::HangWatcher::GetInstance()->BlockIfCaptureInProgress();
+
 #if DCHECK_IS_ON()
   // Verify that no Scope was destructed out of order.
   DCHECK_EQ(this, current_hang_watch_state->GetCurrentHangWatchScope());
@@ -194,9 +200,14 @@ void HangWatcher::Monitor() {
   }
 
   if (must_invoke_hang_closure) {
+    capture_in_progress.store(true, std::memory_order_relaxed);
+    base::AutoLock scope_lock(capture_lock_);
+
     // Invoke the closure outside the scope of |watch_state_lock_|
     // to prevent lock reentrancy.
     on_hang_closure_.Run();
+
+    capture_in_progress.store(false, std::memory_order_relaxed);
   }
 
   if (after_monitor_closure_for_testing_) {
@@ -217,6 +228,16 @@ void HangWatcher::SignalMonitorEventForTesting() {
   monitor_event_.Signal();
 }
 
+void HangWatcher::BlockIfCaptureInProgress() {
+  // Makes a best-effort attempt to block execution if a hang is currently being
+  // captured.Only block on |capture_lock| if |capture_in_progress| hints that
+  // it's already held to avoid serializing all threads on this function when no
+  // hang capture is in-progress.
+  if (capture_in_progress.load(std::memory_order_relaxed)) {
+    base::AutoLock hang_lock(capture_lock_);
+  }
+}
+
 void HangWatcher::UnregisterThread() {
   AutoLock auto_lock(watch_state_lock_);
 

base/threading/hang_watcher.h

@@ -13,6 +13,7 @@
 #include "base/callback.h"
 #include "base/callback_helpers.h"
 #include "base/feature_list.h"
+#include "base/synchronization/lock.h"
 #include "base/threading/simple_thread.h"
 #include "base/threading/thread_checker.h"
 #include "base/threading/thread_local.h"
@@ -118,6 +119,11 @@ class BASE_EXPORT HangWatcher : public DelegateSimpleThread::Delegate {
   // HangWatcher thread is sleeping. Use only for testing.
   void SignalMonitorEventForTesting();
 
+  // Use to block until the hang is recorded. Allows the caller to halt
+  // execution so it does not overshoot the hang watch target and result in a
+  // non-actionable stack trace in the crash recorded.
+  void BlockIfCaptureInProgress();
+
  private:
   THREAD_CHECKER(thread_checker_);
 
@@ -160,6 +166,9 @@ class BASE_EXPORT HangWatcher : public DelegateSimpleThread::Delegate {
 
   base::RepeatingClosure after_monitor_closure_for_testing_;
 
+  base::Lock capture_lock_;
+  std::atomic<bool> capture_in_progress;
+
   FRIEND_TEST_ALL_PREFIXES(HangWatcherTest, NestedScopes);
 };
 

base/threading/hang_watcher_unittest.cc

@@ -264,4 +264,104 @@ TEST_F(HangWatcherRealTimeTest, PeriodicCallsCount) {
   ASSERT_FALSE(hang_event_.IsSignaled());
 }
 
+class HangWatchScopeBlockingTest : public testing::Test {
+ public:
+  void SetUp() override {
+    // Start the HangWatcher.
+    hang_watcher_ =
+        std::make_unique<HangWatcher>(base::BindLambdaForTesting([&] {
+          capture_started_.Signal();
+          // Simulate capturing that takes a long time.
+          PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100));
+          completed_capture_ = true;
+        }));
+
+    hang_watcher_->SetAfterMonitorClosureForTesting(
+        base::BindLambdaForTesting([&]() {
+          // Simulate monitoring that takes a long time.
+          PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100));
+          completed_monitoring_.Signal();
+        }));
+
+    // Make sure no periodic monitoring takes place.
+    hang_watcher_->SetMonitoringPeriodForTesting(base::TimeDelta::Max());
+
+    // Register the test main thread for hang watching.
+    unregister_thread_closure_ = hang_watcher_->RegisterThread();
+  }
+
+  void VerifyScopesDontBlock() {
+    // Start a hang watch scope that cannot possibly cause a hang to be
+    // detected.
+    {
+      HangWatchScope long_scope(base::TimeDelta::Max());
+
+      // Manually trigger a monitoring.
+      hang_watcher_->SignalMonitorEventForTesting();
+
+      // Execution has to continue freely here as no capture is in progress.
+    }
+
+    // Monitoring should not be over yet because the test code should execute
+    // faster when not blocked.
+    EXPECT_FALSE(completed_monitoring_.IsSignaled());
+
+    // Wait for the full monitoring process to be complete. This is to prove
+    // that monitoring truly executed and that we raced the signaling.
+    completed_monitoring_.Wait();
+
+    // No hang means no capture.
+    EXPECT_FALSE(completed_capture_);
+  }
+
+ protected:
+  base::WaitableEvent capture_started_;
+  base::WaitableEvent completed_monitoring_;
+
+  // No need for this to be atomic because in tests with no capture the variable
+  // is not even written to by the HangWatcher thread and in tests with a
+  // capture the accesses are serialized by the blocking in ~HangWatchScope().
+  bool completed_capture_ = false;
+
+  std::unique_ptr<HangWatcher> hang_watcher_;
+  base::ScopedClosureRunner unregister_thread_closure_;
+};
+
+// Tests that execution is unimpeded by ~HangWatchScope() when no capture ever
+// takes place.
+TEST_F(HangWatchScopeBlockingTest, ScopeDoesNotBlocksWithoutCapture) {
+  VerifyScopesDontBlock();
+}
+
+// Test that execution blocks in ~HangWatchScope() for a thread under watch
+// during the capturing of a hang.
+TEST_F(HangWatchScopeBlockingTest, ScopeBlocksDuringCapture) {
+  // Start a hang watch scope that expires in the past already. Ensures that the
+  // first monitor will detect a hang.
+  {
+    HangWatchScope already_over(base::TimeDelta::FromDays(-1));
+
+    // Manually trigger a monitoring.
+    hang_watcher_->SignalMonitorEventForTesting();
+
+    // Ensure that the hang capturing started.
+    capture_started_.Wait();
+
+    // Execution will get stuck in this scope because execution does not escape
+    // ~HangWatchScope() if a hang capture is under way.
+  }
+
+  // A hang was in progress so execution should have been blocked in
+  // BlockWhileCaptureInProgress() until capture finishes.
+  EXPECT_TRUE(completed_capture_);
+
+  // Reset expectations
+  completed_monitoring_.Reset();
+  capture_started_.Reset();
+  completed_capture_ = false;
+
+  // Verify that scopes don't block just because a capture happened in the past.
+  VerifyScopesDontBlock();
+}
+
 }  // namespace base