Only HangWatchScopes hung at time of hang detection should block.

If a hang is detected in a thread critical to user responsiveness like
browser UI or IO thread then blocking execution in ~HangWatchScope() on
all threads makes sense because the program was already unresponsive.

It's desirable to keep the blocking to a minimum on the browser UI/IO
threads if they are not the hung thread. This CL achieves that
by only enforcing blocking in HangWatchScopes that were hung at the
moment of hang detection.

Bug: 1034046
Change-Id: I05aace41e272f1daef490be55902225363536396
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2209330
Commit-Queue: Oliver Li <olivierli@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792452}

base/threading/hang_watcher.cc

@@ -99,7 +99,9 @@ HangWatchScope::~HangWatchScope() {
   }
 
   // If a hang is currently being captured we should block here so execution
-  // stops and the relevant stack frames are recorded.
+  // stops and we avoid recording unrelated stack frames in the crash.
+  if (current_hang_watch_state->IsFlagSet(
+          internal::HangWatchDeadline::Flag::kShouldBlockOnHang))
     base::HangWatcher::GetInstance()->BlockIfCaptureInProgress();
 
 #if DCHECK_IS_ON()
@@ -286,9 +288,12 @@ HangWatcher::WatchStateSnapShot::WatchStateSnapShot(
     base::TimeTicks snapshot_time,
     base::TimeTicks deadline_ignore_threshold)
     : snapshot_time_(snapshot_time) {
-  // Initial copy of the values.
+  bool all_threads_marked = true;
+  // Copy hung thread information.
   for (const auto& watch_state : watch_states) {
-    base::TimeTicks deadline = watch_state.get()->GetDeadline();
+    uint64_t flags;
+    base::TimeTicks deadline;
+    std::tie(flags, deadline) = watch_state->GetFlagsAndDeadline();
 
     if (deadline <= deadline_ignore_threshold) {
       hung_watch_state_copies_.clear();
@@ -297,9 +302,31 @@ HangWatcher::WatchStateSnapShot::WatchStateSnapShot(
 
     // Only copy hung threads.
     if (deadline <= snapshot_time) {
+      // Attempt to mark the thread as needing to stay within its current
+      // HangWatchScope until capture is complete.
+      bool thread_marked = watch_state->SetShouldBlockOnHang(flags, deadline);
+
+      // If marking some threads already failed the snapshot won't be kept so
+      // there is no need to keep adding to it. The loop doesn't abort though
+      // to keep marking the other threads. If these threads remain hung until
+      // the next capture then they'll already be marked and will be included
+      // in the capture at that time.
+      if (thread_marked && all_threads_marked) {
         hung_watch_state_copies_.push_back(
             WatchStateCopy{deadline, watch_state.get()->GetThreadID()});
+      } else {
+        all_threads_marked = false;
+      }
+    }
   }
+
+  // If some threads could not be marked for blocking then this snapshot is not
+  // actionable since marked threads could be hung because of unmarked ones.
+  // If only the marked threads were captured the information would be
+  // incomplete.
+  if (!all_threads_marked) {
+    hung_watch_state_copies_.clear();
+    return;
   }
 
   // Sort |hung_watch_state_copies_| by order of decreasing hang severity so the
@@ -317,6 +344,8 @@ HangWatcher::WatchStateSnapShot::~WatchStateSnapShot() = default;
 
 std::string HangWatcher::WatchStateSnapShot::PrepareHungThreadListCrashKey()
     const {
+  DCHECK(IsActionable());
+
   // Build a crash key string that contains the ids of the hung threads.
   constexpr char kSeparator{'|'};
   std::string list_of_hung_thread_ids;
@@ -338,6 +367,10 @@ std::string HangWatcher::WatchStateSnapShot::PrepareHungThreadListCrashKey()
   return list_of_hung_thread_ids;
 }
 
+bool HangWatcher::WatchStateSnapShot::IsActionable() const {
+  return !hung_watch_state_copies_.empty();
+}
+
 HangWatcher::WatchStateSnapShot HangWatcher::GrabWatchStateSnapshotForTesting()
     const {
   WatchStateSnapShot snapshot(watch_states_, base::TimeTicks::Now(),
@@ -374,21 +407,18 @@ void HangWatcher::Monitor() {
 }
 
 void HangWatcher::CaptureHang(base::TimeTicks capture_time) {
-  capture_in_progress.store(true, std::memory_order_relaxed);
+  capture_in_progress_.store(true, std::memory_order_relaxed);
   base::AutoLock scope_lock(capture_lock_);
 
   WatchStateSnapShot watch_state_snapshot(watch_states_, capture_time,
                                           deadline_ignore_threshold_);
-
-  // The hung thread(s) could detected at the start of Monitor() could have
-  // moved on from their scopes. If that happened and there are no more hung
-  // threads then abort capture.
-  std::string list_of_hung_thread_ids =
-      watch_state_snapshot.PrepareHungThreadListCrashKey();
-  if (list_of_hung_thread_ids.empty())
+  if (!watch_state_snapshot.IsActionable()) {
     return;
+  }
 
 #if not defined(OS_NACL)
+  std::string list_of_hung_thread_ids =
+      watch_state_snapshot.PrepareHungThreadListCrashKey();
   static debug::CrashKeyString* crash_key = AllocateCrashKeyString(
       "list-of-hung-threads", debug::CrashKeySize::Size256);
   debug::ScopedCrashKeyString list_of_hung_threads_crash_key_string(
@@ -424,7 +454,7 @@ void HangWatcher::CaptureHang(base::TimeTicks capture_time) {
   // Update after running the actual capture.
   deadline_ignore_threshold_ = latest_expired_deadline;
 
-  capture_in_progress.store(false, std::memory_order_relaxed);
+  capture_in_progress_.store(false, std::memory_order_relaxed);
 }
 
 void HangWatcher::SetAfterMonitorClosureForTesting(
@@ -464,12 +494,11 @@ void HangWatcher::SetTickClockForTesting(const base::TickClock* tick_clock) {
 
 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
+  // 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)) {
+  if (capture_in_progress_.load(std::memory_order_relaxed))
     base::AutoLock hang_lock(capture_lock_);
-  }
 }
 
 void HangWatcher::UnregisterThread() {
@@ -492,9 +521,169 @@ void HangWatcher::UnregisterThread() {
 }
 
 namespace internal {
+namespace {
+
+constexpr uint64_t kOnlyDeadlineMask = 0x00FFFFFFFFFFFFFFu;
+constexpr uint64_t kOnlyFlagsMask = ~kOnlyDeadlineMask;
+constexpr uint64_t kMaximumFlag = 0x8000000000000000u;
+
+// Use as a mask to keep persistent flags and the deadline.
+constexpr uint64_t kPersistentFlagsAndDeadlineMask =
+    kOnlyDeadlineMask |
+    static_cast<uint64_t>(HangWatchDeadline::Flag::kIgnoreHangs);
+}  // namespace
+
+// Flag binary representation assertions.
+static_assert(
+    static_cast<uint64_t>(HangWatchDeadline::Flag::kMinValue) >
+        kOnlyDeadlineMask,
+    "Invalid numerical value for flag. Would interfere with bits of data.");
+static_assert(static_cast<uint64_t>(HangWatchDeadline::Flag::kMaxValue) <=
+                  kMaximumFlag,
+              "A flag can only set a single bit.");
+
+HangWatchDeadline::HangWatchDeadline() = default;
+HangWatchDeadline::~HangWatchDeadline() = default;
+
+std::pair<uint64_t, TimeTicks> HangWatchDeadline::GetFlagsAndDeadline() const {
+  uint64_t bits = bits_.load(std::memory_order_relaxed);
+  return std::make_pair(ExtractFlags(bits),
+                        DeadlineFromBits(ExtractDeadline((bits))));
+}
+
+TimeTicks HangWatchDeadline::GetDeadline() const {
+  return DeadlineFromBits(
+      ExtractDeadline(bits_.load(std::memory_order_relaxed)));
+}
+
+// static
+TimeTicks HangWatchDeadline::Max() {
+  // |kOnlyDeadlineMask| has all the bits reserved for the TimeTicks value
+  // set. This means it also represents the highest representable value.
+  return DeadlineFromBits(kOnlyDeadlineMask);
+}
+
+// static
+bool HangWatchDeadline::IsFlagSet(Flag flag, uint64_t flags) {
+  return static_cast<uint64_t>(flag) & flags;
+}
+
+void HangWatchDeadline::SetDeadline(TimeTicks new_deadline) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK(new_deadline <= Max()) << "Value too high to be represented.";
+  DCHECK(new_deadline >= TimeTicks{}) << "Value cannot be negative.";
+
+  if (switch_bits_callback_for_testing_) {
+    const uint64_t switched_in_bits = SwitchBitsForTesting();
+    // If a concurrent deadline change is tested it cannot have a deadline or
+    // persistent flag change since those always happen on the same thread.
+    DCHECK((switched_in_bits & kPersistentFlagsAndDeadlineMask) == 0u);
+  }
+
+  // Discard all non-persistent flags and apply deadline change.
+  const uint64_t old_bits = bits_.load(std::memory_order_relaxed);
+  const uint64_t new_flags =
+      ExtractFlags(old_bits & kPersistentFlagsAndDeadlineMask);
+  bits_.store(new_flags | ExtractDeadline(new_deadline.ToInternalValue()),
+              std::memory_order_relaxed);
+}
+
+// TODO(crbug.com/1087026): Add flag DCHECKs here.
+bool HangWatchDeadline::SetShouldBlockOnHang(uint64_t old_flags,
+                                             TimeTicks old_deadline) {
+  DCHECK(old_deadline <= Max()) << "Value too high to be represented.";
+  DCHECK(old_deadline >= TimeTicks{}) << "Value cannot be negative.";
+
+  // Set the kShouldBlockOnHang flag only if |bits_| did not change since it was
+  // read. kShouldBlockOnHang is the only non-persistent flag and should never
+  // be set twice. Persistent flags and deadline changes are done from the same
+  // thread so there is no risk of losing concurrently added information.
+  uint64_t old_bits =
+      old_flags | static_cast<uint64_t>(old_deadline.ToInternalValue());
+  const uint64_t desired_bits =
+      old_bits | static_cast<uint64_t>(Flag::kShouldBlockOnHang);
+
+  // If a test needs to simulate |bits_| changing since calling this function
+  // this happens now.
+  if (switch_bits_callback_for_testing_) {
+    const uint64_t switched_in_bits = SwitchBitsForTesting();
+
+    // Injecting the flag being tested is invalid.
+    DCHECK(!IsFlagSet(Flag::kShouldBlockOnHang, switched_in_bits));
+  }
+
+  return bits_.compare_exchange_weak(old_bits, desired_bits,
+                                     std::memory_order_relaxed,
+                                     std::memory_order_relaxed);
+}
+
+void HangWatchDeadline::SetIgnoreHangs() {
+  SetPersistentFlag(Flag::kIgnoreHangs);
+}
+
+void HangWatchDeadline::UnsetIgnoreHangs() {
+  ClearPersistentFlag(Flag::kIgnoreHangs);
+}
+
+void HangWatchDeadline::SetPersistentFlag(Flag flag) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  if (switch_bits_callback_for_testing_)
+    SwitchBitsForTesting();
+  bits_.fetch_or(static_cast<uint64_t>(flag), std::memory_order_relaxed);
+}
+
+void HangWatchDeadline::ClearPersistentFlag(Flag flag) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  if (switch_bits_callback_for_testing_)
+    SwitchBitsForTesting();
+  bits_.fetch_and(~(static_cast<uint64_t>(flag)), std::memory_order_relaxed);
+}
+
+// static
+uint64_t HangWatchDeadline::ExtractFlags(uint64_t bits) {
+  return bits & kOnlyFlagsMask;
+}
+
+// static
+uint64_t HangWatchDeadline::ExtractDeadline(uint64_t bits) {
+  return bits & kOnlyDeadlineMask;
+}
+
+// static
+TimeTicks HangWatchDeadline::DeadlineFromBits(uint64_t bits) {
+  // |kOnlyDeadlineMask| has all the deadline bits set to 1 so is the largest
+  // representable value.
+  DCHECK(bits <= kOnlyDeadlineMask)
+      << "Flags bits are set. Remove them before returning deadline.";
+  return TimeTicks::FromInternalValue(bits);
+}
+
+bool HangWatchDeadline::IsFlagSet(Flag flag) const {
+  return bits_.load(std::memory_order_relaxed) & static_cast<uint64_t>(flag);
+}
+
+void HangWatchDeadline::SetSwitchBitsClosureForTesting(
+    RepeatingCallback<uint64_t(void)> closure) {
+  switch_bits_callback_for_testing_ = closure;
+}
+
+void HangWatchDeadline::ResetSwitchBitsClosureForTesting() {
+  DCHECK(switch_bits_callback_for_testing_);
+  switch_bits_callback_for_testing_.Reset();
+}
+
+uint64_t HangWatchDeadline::SwitchBitsForTesting() {
+  DCHECK(switch_bits_callback_for_testing_);
+
+  const uint64_t old_bits = bits_.load(std::memory_order_relaxed);
+  const uint64_t new_bits = switch_bits_callback_for_testing_.Run();
+  const uint64_t old_flags = ExtractFlags(old_bits);
+
+  const uint64_t switched_in_bits = old_flags | new_bits;
+  bits_.store(switched_in_bits, std::memory_order_relaxed);
+  return switched_in_bits;
+}
 
-// |deadline_| starts at Max() to avoid validation problems
-// when setting the first legitimate value.
 HangWatchState::HangWatchState() : thread_id_(PlatformThread::CurrentId()) {
   // There should not exist a state object for this thread already.
   DCHECK(!GetHangWatchStateForCurrentThread()->Get());
@@ -532,16 +721,29 @@ HangWatchState::CreateHangWatchStateForCurrentThread() {
 }
 
 TimeTicks HangWatchState::GetDeadline() const {
-  return deadline_.load(std::memory_order_relaxed);
+  return deadline_.GetDeadline();
+}
+
+std::pair<uint64_t, TimeTicks> HangWatchState::GetFlagsAndDeadline() const {
+  return deadline_.GetFlagsAndDeadline();
 }
 
 void HangWatchState::SetDeadline(TimeTicks deadline) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
-  deadline_.store(deadline, std::memory_order_relaxed);
+  deadline_.SetDeadline(deadline);
 }
 
 bool HangWatchState::IsOverDeadline() const {
-  return TimeTicks::Now() > deadline_.load(std::memory_order_relaxed);
+  return TimeTicks::Now() > deadline_.GetDeadline();
+}
+
+bool HangWatchState::SetShouldBlockOnHang(uint64_t old_flags,
+                                          TimeTicks old_deadline) {
+  return deadline_.SetShouldBlockOnHang(old_flags, old_deadline);
+}
+
+bool HangWatchState::IsFlagSet(HangWatchDeadline::Flag flag) {
+  return deadline_.IsFlagSet(flag);
 }
 
 #if DCHECK_IS_ON()
@@ -556,6 +758,10 @@ HangWatchScope* HangWatchState::GetCurrentHangWatchScope() {
 }
 #endif
 
+HangWatchDeadline* HangWatchState::GetHangWatchDeadlineForTesting() {
+  return &deadline_;
+}
+
 // static
 ThreadLocalPointer<HangWatchState>*
 HangWatchState::GetHangWatchStateForCurrentThread() {

base/threading/hang_watcher.h

@@ -6,10 +6,13 @@
 #define BASE_THREADING_HANG_WATCHER_H_
 
 #include <atomic>
+#include <cstdint>
 #include <memory>
+#include <type_traits>
 #include <vector>
 
 #include "base/atomicops.h"
+#include "base/bits.h"
 #include "base/callback.h"
 #include "base/callback_forward.h"
 #include "base/callback_helpers.h"
@@ -196,12 +199,17 @@ class BASE_EXPORT HangWatcher : public DelegateSimpleThread::Delegate {
 
     // Returns a string that contains the ids of the hung threads separated by a
     // '|'. The size of the string is capped at debug::CrashKeySize::Size256. If
-    // no threads are hung returns an empty string.
+    // no threads are hung returns an empty string. Can only be invoked if
+    // IsActionable().
     std::string PrepareHungThreadListCrashKey() const;
 
     // Return the highest deadline included in this snapshot.
     base::TimeTicks GetHighestDeadline() const;
 
+    // Returns true if the snapshot can be used to record an actionable hang
+    // report and false if not.
+    bool IsActionable() const;
+
    private:
     base::TimeTicks snapshot_time_;
     std::vector<WatchStateCopy> hung_watch_state_copies_;
@@ -259,7 +267,7 @@ class BASE_EXPORT HangWatcher : public DelegateSimpleThread::Delegate {
   RepeatingCallback<void(TimeTicks)> after_wait_callback_;
 
   base::Lock capture_lock_ ACQUIRED_AFTER(watch_state_lock_);
-  std::atomic<bool> capture_in_progress{false};
+  std::atomic<bool> capture_in_progress_{false};
 
   const base::TickClock* tick_clock_;
 
@@ -269,12 +277,157 @@ class BASE_EXPORT HangWatcher : public DelegateSimpleThread::Delegate {
 
   FRIEND_TEST_ALL_PREFIXES(HangWatcherTest, NestedScopes);
   FRIEND_TEST_ALL_PREFIXES(HangWatcherSnapshotTest, HungThreadIDs);
+  FRIEND_TEST_ALL_PREFIXES(HangWatcherSnapshotTest, NonActionableReport);
 };
 
 // Classes here are exposed in the header only for testing. They are not
 // intended to be used outside of base.
 namespace internal {
 
+// Threadsafe class that manages a deadline of type TimeTicks alongside hang
+// watching specific flags. The flags are stored in the higher bits of the
+// underlying TimeTicks deadline. This enables setting the flags on thread T1 in
+// a way that's resilient to concurrent deadline or flag changes from thread T2.
+// Flags can be queried separately from the deadline and users of this class
+// should not have to care about them when doing so.
+class BASE_EXPORT HangWatchDeadline {
+ public:
+  // Masks to set flags by flipping a single bit in the TimeTicks value. There
+  // are two types of flags. Persistent flags remain set through a deadline
+  // change and non-persistent flags are cleared when the deadline changes.
+  enum class Flag : uint64_t {
+    // Minimum value for validation purposes. Not currently used.
+    kMinValue = bits::LeftmostBit<uint64_t>() >> 7,
+    // Persistent because if hang detection is disabled on a thread it should
+    // be re-enabled manually.
+    kIgnoreHangs = bits::LeftmostBit<uint64_t>() >> 1,
+    // Non-persistent because a new value means a new hang watch scope started
+    // after the beginning of capture. It can't be implicated in the hang so we
+    // don't want it to block.
+    kShouldBlockOnHang = bits::LeftmostBit<uint64_t>() >> 0,
+    kMaxValue = kShouldBlockOnHang
+  };
+
+  HangWatchDeadline();
+  ~HangWatchDeadline();
+
+  // HangWatchDeadline should never be copied. To keep a copy of the deadline or
+  // flags use the appropriate accessors.
+  HangWatchDeadline(const HangWatchDeadline&) = delete;
+  HangWatchDeadline& operator=(const HangWatchDeadline&) = delete;
+
+  // Returns the underlying TimeTicks deadline. WARNING: The deadline and flags
+  // can change concurrently. To inspect both, use GetFlagsAndDeadline() to get
+  // a coherent race-free view of the state.
+  TimeTicks GetDeadline() const;
+
+  // Returns a mask containing the flags and the deadline as a pair. Use to
+  // inspect the flags and deadline and then optionally call
+  // SetShouldBlockOnHang() .
+  std::pair<uint64_t, TimeTicks> GetFlagsAndDeadline() const;
+
+  // Returns true if the flag is set and false if not. WARNING: The deadline and
+  // flags can change concurrently. To inspect both, use GetFlagsAndDeadline()
+  // to get a coherent race-free view of the state.
+  bool IsFlagSet(Flag flag) const;
+
+  // Returns true if a flag is set in |flags| and false if not. Use to inspect
+  // the flags mask returned by GetFlagsAndDeadline(). WARNING: The deadline and
+  // flags can change concurrently. If you need to inspect both you need to use
+  // GetFlagsAndDeadline() to get a coherent race-free view of the state.
+  static bool IsFlagSet(Flag flag, uint64_t flags);
+
+  // Replace the deadline value. |new_value| needs to be within [0,
+  // Max()]. This function can never fail.
+  void SetDeadline(TimeTicks new_value);
+
+  // Sets the kShouldBlockOnHang flag and returns true if current flags and
+  // deadline are still equal to |old_flags| and  |old_deadline|. Otherwise does
+  // not set the flag and returns false.
+  bool SetShouldBlockOnHang(uint64_t old_flags, TimeTicks old_deadline);
+
+  // Sets the kIgnoreHangs flag.
+  void SetIgnoreHangs();
+
+  // Clears the kIgnoreHangs flag.
+  void UnsetIgnoreHangs();
+
+  // Use to simulate the value of |bits_| changing between the calling a
+  // Set* function and the moment of atomically switching the values. The
+  // callback should return a value containing the desired flags and deadline
+  // bits. The flags that are already set will be preserved upon applying. Use
+  // only for testing.
+  void SetSwitchBitsClosureForTesting(
+      RepeatingCallback<uint64_t(void)> closure);
+
+  // Remove the deadline modification callback for when testing is done. Use
+  // only for testing.
+  void ResetSwitchBitsClosureForTesting();
+
+ private:
+  using TimeTicksInternalRepresentation =
+      std::result_of<decltype (&TimeTicks::ToInternalValue)(TimeTicks)>::type;
+  static_assert(std::is_same<TimeTicksInternalRepresentation, int64_t>::value,
+                "Bit manipulations made by HangWatchDeadline need to be"
+                "adapted if internal representation of TimeTicks changes.");
+
+  // Replace the bits with the ones provided through the callback. Preserves the
+  // flags that were already set. Returns the switched in bits. Only call if
+  // |switch_bits_callback_for_testing_| is installed.
+  uint64_t SwitchBitsForTesting();
+
+  // Atomically sets persitent flag |flag|. Cannot fail.
+  void SetPersistentFlag(Flag flag);
+
+  // Atomically clears persitent flag |flag|. Cannot fail.
+  void ClearPersistentFlag(Flag flag);
+
+  // Converts bits to TimeTicks with some sanity checks. Use to return the
+  // deadline outside of this class.
+  static TimeTicks DeadlineFromBits(uint64_t bits);
+
+  // Returns the largest representable deadline.
+  static TimeTicks Max();
+
+  // Extract the flag bits from |bits|.
+  static uint64_t ExtractFlags(uint64_t bits);
+
+  // Extract the deadline bits from |bits|.
+  static uint64_t ExtractDeadline(uint64_t bits);
+
+  // BitsType is uint64_t. This type is chosen for having
+  // std::atomic<BitsType>{}.is_lock_free() true on many platforms and having no
+  // undefined behaviors with regards to bit shift operations. Throughout this
+  // class this is the only type that is used to store, retrieve and manipulate
+  // the bits. When returning a TimeTicks value outside this class it's
+  // necessary to run the proper checks to insure correctness of the conversion
+  // that has to go through int_64t. (See DeadlineFromBits()).
+  using BitsType = uint64_t;
+  static_assert(std::is_same<std::underlying_type<Flag>::type, BitsType>::value,
+                "Flag should have the same underlying type as bits_ to "
+                "simplify thinking about bit operations");
+
+  // Holds the bits of both the flags and the TimeTicks deadline.
+  // TimeTicks values represent a count of microseconds since boot which may or
+  // may not include suspend time depending on the platform. Using the seven
+  // highest order bits and the sign bit to store flags still enables the
+  // storing of TimeTicks values that can represent up to ~1142 years of uptime
+  // in the remaining bits. Should never be directly accessed from outside the
+  // class. Starts out at Max() to provide a base-line deadline that will not be
+  // reached during normal execution.
+  //
+  // Binary format: 0xFFDDDDDDDDDDDDDDDD
+  // F = Flags
+  // D = Deadline
+  std::atomic<BitsType> bits_{static_cast<uint64_t>(Max().ToInternalValue())};
+
+  RepeatingCallback<uint64_t(void)> switch_bits_callback_for_testing_;
+
+  THREAD_CHECKER(thread_checker_);
+
+  FRIEND_TEST_ALL_PREFIXES(HangWatchDeadlineTest, BitsPreservedThroughExtract);
+};
+
 // Contains the information necessary for hang watching a specific
 // thread. Instances of this class are accessed concurrently by the associated
 // thread and the HangWatcher. The HangWatcher owns instances of this
@@ -298,13 +451,30 @@ class BASE_EXPORT HangWatchState {
   static ThreadLocalPointer<HangWatchState>*
   GetHangWatchStateForCurrentThread();
 
-  // Returns the value of the current deadline. Use this function if you need to
+  // Returns the current deadline. Use this function if you need to
   // store the value. To test if the deadline has expired use IsOverDeadline().
+  // WARNING: The deadline and flags can change concurrently. If you need to
+  // inspect both you need to use GetFlagsAndDeadline() to get a coherent
+  // race-free view of the state.
   TimeTicks GetDeadline() const;
 
-  // Atomically sets the deadline to a new value.
+  // Returns a mask containing the hang watching flags and the value as a pair.
+  // Use to inspect the flags and deadline and optionally call
+  // SetShouldBlockOnHang(flags, deadline).
+  std::pair<uint64_t, TimeTicks> GetFlagsAndDeadline() const;
+
+  // Sets the deadline to a new value.
   void SetDeadline(TimeTicks deadline);
 
+  // Mark the current state as having to block in its destruction until hang
+  // capture completes.
+  bool SetShouldBlockOnHang(uint64_t old_flags, TimeTicks old_deadline);
+
+  // Returns true if |flag| is set and false if not. WARNING: The deadline and
+  // flags can change concurrently. If you need to inspect both you need to use
+  // GetFlagsAndDeadline() to get a coherent race-free view of the state.
+  bool IsFlagSet(HangWatchDeadline::Flag flag);
+
   // Tests whether the associated thread's execution has gone over the deadline.
   bool IsOverDeadline() const;
 
@@ -318,6 +488,9 @@ class BASE_EXPORT HangWatchState {
 
   PlatformThreadId GetThreadID() const;
 
+  // Retrieve the current hang watch deadline directly. For testing only.
+  HangWatchDeadline* GetHangWatchDeadlineForTesting();
+
  private:
   // The thread that creates the instance should be the class that updates
   // the deadline.
@@ -325,7 +498,7 @@ class BASE_EXPORT HangWatchState {
 
   // If the deadline fails to be updated before TimeTicks::Now() ever
   // reaches the value contained in it this constistutes a hang.
-  std::atomic<TimeTicks> deadline_{base::TimeTicks::Max()};
+  HangWatchDeadline deadline_;
 
   const PlatformThreadId thread_id_;
 

base/threading/hang_watcher_unittest.cc

@@ -28,6 +28,17 @@
 namespace base {
 namespace {
 
+// Use this value to mark things very far off in the future. Adding this
+// to TimeTicks::Now() gives a point that will never be reached during the
+// normal execution of a test.
+constexpr TimeDelta kVeryLongDelta{base::TimeDelta::FromDays(365)};
+
+constexpr uint64_t kArbitraryDeadline = 0x0000C0FFEEC0FFEEu;
+constexpr uint64_t kAllOnes = 0xFFFFFFFFFFFFFFFFu;
+constexpr uint64_t kAllZeros = 0x0000000000000000u;
+constexpr uint64_t kOnesThenZeroes = 0xAAAAAAAAAAAAAAAAu;
+constexpr uint64_t kZeroesThenOnes = 0x5555555555555555u;
+
 // Waits on provided WaitableEvent before executing and signals when done.
 class BlockingThread : public DelegateSimpleThread::Delegate {
  public:
@@ -94,7 +105,7 @@ class HangWatcherTest : public testing::Test {
 
     // We're not testing the monitoring loop behavior in this test so we want to
     // trigger monitoring manually.
-    hang_watcher_.SetMonitoringPeriodForTesting(base::TimeDelta::Max());
+    hang_watcher_.SetMonitoringPeriodForTesting(kVeryLongDelta);
 
     // Start the monitoring loop.
     hang_watcher_.Start();
@@ -233,6 +244,12 @@ TEST_F(HangWatcherBlockingThreadTest, NoHang) {
 namespace {
 class HangWatcherSnapshotTest : public testing::Test {
  public:
+  void SetUp() override {
+    // The monitoring loop behavior is not verified in this test so we want to
+    // trigger monitoring manually.
+    hang_watcher_.SetMonitoringPeriodForTesting(kVeryLongDelta);
+  }
+
   HangWatcherSnapshotTest() = default;
   HangWatcherSnapshotTest(const HangWatcherSnapshotTest& other) = delete;
   HangWatcherSnapshotTest& operator=(const HangWatcherSnapshotTest& other) =
@@ -296,7 +313,47 @@ class HangWatcherSnapshotTest : public testing::Test {
 };
 }  // namespace
 
-// TODO(crbug.com/2193655): Test flaky on iPad.
+// Verify that the hang capture fails when marking a thread for blocking fails.
+// This simulates a HangWatchScope completing between the time the hang was
+// dected and the time it is recorded which would create a non-actionable
+// report.
+TEST_F(HangWatcherSnapshotTest, NonActionableReport) {
+  hang_watcher_.SetOnHangClosureForTesting(
+      base::BindLambdaForTesting([this]() { ++hang_capture_count_; }));
+  hang_watcher_.SetAfterMonitorClosureForTesting(
+      base::BindLambdaForTesting([this]() { monitor_event_.Signal(); }));
+
+  hang_watcher_.Start();
+
+  // Register the main test thread for hang watching.
+  auto unregister_thread_closure_ = hang_watcher_.RegisterThread();
+  {
+    // Start a hang watch scope that expires right away. Ensures that
+    // the first monitor will detect a hang.
+    HangWatchScope expires_instantly(base::TimeDelta{});
+
+    internal::HangWatchState* current_hang_watch_state =
+        internal::HangWatchState::GetHangWatchStateForCurrentThread()->Get();
+
+    // Simulate the deadline changing concurrently during the capture. This
+    // makes the capture fail since marking of the deadline fails.
+    ASSERT_NE(current_hang_watch_state->GetDeadline(),
+              base::TimeTicks::FromInternalValue(kArbitraryDeadline));
+    current_hang_watch_state->GetHangWatchDeadlineForTesting()
+        ->SetSwitchBitsClosureForTesting(
+            base::BindLambdaForTesting([]() { return kArbitraryDeadline; }));
+
+    ExpectNoCapture();
+
+    // Marking failed.
+    ASSERT_FALSE(current_hang_watch_state->IsFlagSet(
+        internal::HangWatchDeadline::Flag::kShouldBlockOnHang));
+
+    current_hang_watch_state->GetHangWatchDeadlineForTesting()
+        ->ResetSwitchBitsClosureForTesting();
+  }
+}
+
 TEST_F(HangWatcherSnapshotTest, HungThreadIDs) {
   // During hang capture the list of hung threads should be populated.
   hang_watcher_.SetOnHangClosureForTesting(base::BindLambdaForTesting([this]() {
@@ -309,9 +366,6 @@ TEST_F(HangWatcherSnapshotTest, HungThreadIDs) {
   // When hang capture is over the list should be empty.
   hang_watcher_.SetAfterMonitorClosureForTesting(
       base::BindLambdaForTesting([this]() {
-        EXPECT_EQ(hang_watcher_.GrabWatchStateSnapshotForTesting()
-                      .PrepareHungThreadListCrashKey(),
-                  "");
         monitor_event_.Signal();
       }));
 
@@ -482,7 +536,6 @@ TEST_F(HangWatcherPeriodicMonitoringTest, PeriodicCallsTakePlace) {
 
 // If the HangWatcher detects it slept for longer than expected it will not
 // monitor.
-// TODO(crbug.com/1081654): Test flaky on ChromeOS.
 TEST_F(HangWatcherPeriodicMonitoringTest, NoMonitorOnOverSleep) {
   RunLoop run_loop;
 
@@ -522,19 +575,21 @@ class HangWatchScopeBlockingTest : public testing::Test {
     hang_watcher_.SetOnHangClosureForTesting(base::BindLambdaForTesting([&] {
       capture_started_.Signal();
       // Simulate capturing that takes a long time.
-      PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100));
+      PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(500));
+
+      continue_capture_.Wait();
       completed_capture_ = true;
     }));
 
     hang_watcher_.SetAfterMonitorClosureForTesting(
-        base::BindLambdaForTesting([&]() {
+        base::BindLambdaForTesting([&] {
           // Simulate monitoring that takes a long time.
-          PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100));
+          PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(500));
           completed_monitoring_.Signal();
         }));
 
     // Make sure no periodic monitoring takes place.
-    hang_watcher_.SetMonitoringPeriodForTesting(base::TimeDelta::Max());
+    hang_watcher_.SetMonitoringPeriodForTesting(kVeryLongDelta);
 
     hang_watcher_.Start();
 
@@ -550,7 +605,7 @@ class HangWatchScopeBlockingTest : public testing::Test {
     // Start a hang watch scope that cannot possibly cause a hang to be
     // detected.
     {
-      HangWatchScope long_scope(base::TimeDelta::Max());
+      HangWatchScope long_scope(kVeryLongDelta);
 
       // Manually trigger a monitoring.
       hang_watcher_.SignalMonitorEventForTesting();
@@ -574,10 +629,10 @@ class HangWatchScopeBlockingTest : public testing::Test {
   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;
+  // The HangWatcher waits on this event via the "on hang" closure when a hang
+  // is detected.
+  base::WaitableEvent continue_capture_;
+  bool completed_capture_{false};
 
   HangWatcher hang_watcher_;
   base::ScopedClosureRunner unregister_thread_closure_;
@@ -587,20 +642,24 @@ class HangWatchScopeBlockingTest : public testing::Test {
 // Tests that execution is unimpeded by ~HangWatchScope() when no capture ever
 // takes place.
 TEST_F(HangWatchScopeBlockingTest, ScopeDoesNotBlocksWithoutCapture) {
+  // No capture should take place so |continue_capture_| is not signaled to
+  // create a test hang if one ever does.
   VerifyScopesDontBlock();
 }
 
 // Test that execution blocks in ~HangWatchScope() for a thread under watch
 // during the capturing of a hang.
 TEST_F(HangWatchScopeBlockingTest, ScopeBlocksDuringCapture) {
+  // The capture completing is not dependent on any test event. Signal to make
+  // sure the test is not blocked.
+  continue_capture_.Signal();
+
   // Start a hang watch scope that expires in the past already. Ensures that the
   // first monitor will detect a hang.
   {
-    // Start a hang watch scope that expires immediately . Ensures that
-    // the first monitor will detect a hang.
-    BlockingThread blocking_thread(&capture_started_,
-                                   base::TimeDelta::FromMilliseconds(0));
-    blocking_thread.StartAndWaitForScopeEntered();
+    // Start a hang watch scope that expires right away. Ensures that the
+    // first monitor will detect a hang.
+    HangWatchScope expires_right_away(base::TimeDelta{});
 
     // Manually trigger a monitoring.
     hang_watcher_.SignalMonitorEventForTesting();
@@ -608,10 +667,8 @@ TEST_F(HangWatchScopeBlockingTest, ScopeBlocksDuringCapture) {
     // Ensure that the hang capturing started.
     capture_started_.Wait();
 
-    // Execution will get stuck in this scope because execution does not escape
+    // Execution will get stuck in the outer scope because it can't escape
     // ~HangWatchScope() if a hang capture is under way.
-
-    blocking_thread.Join();
   }
 
   // A hang was in progress so execution should have been blocked in
@@ -628,4 +685,232 @@ TEST_F(HangWatchScopeBlockingTest, ScopeBlocksDuringCapture) {
   VerifyScopesDontBlock();
 }
 
+// Test that execution does not block in ~HangWatchScope() when the scope was
+// created after the start of a capture.
+TEST_F(HangWatchScopeBlockingTest, NewScopeDoesNotBlockDuringCapture) {
+  // Start a hang watch scope that expires right away. Ensures that the
+  // first monitor will detect a hang.
+  HangWatchScope expires_right_away(base::TimeDelta{});
+
+  // Manually trigger a monitoring.
+  hang_watcher_.SignalMonitorEventForTesting();
+
+  // Ensure that the hang capturing started.
+  capture_started_.Wait();
+
+  // A scope started once a capture is already under way should not block
+  // execution.
+  { HangWatchScope also_expires_right_away(base::TimeDelta{}); }
+
+  // Wait for the new HangWatchScope to be destroyed to let the capture finish.
+  // If the new scope block waiting for the capture to finish this would create
+  // a deadlock and the test would hang.
+  continue_capture_.Signal();
+}
+
+namespace internal {
+namespace {
+
+constexpr std::array<HangWatchDeadline::Flag, 3> kAllFlags{
+    {HangWatchDeadline::Flag::kMinValue, HangWatchDeadline::Flag::kIgnoreHangs,
+     HangWatchDeadline::Flag::kShouldBlockOnHang}};
+}  // namespace
+
+class HangWatchDeadlineTest : public testing::Test {
+ protected:
+  void AssertNoFlagsSet() const {
+    for (HangWatchDeadline::Flag flag : kAllFlags) {
+      ASSERT_FALSE(deadline_.IsFlagSet(flag));
+    }
+  }
+
+  // Return a flag mask without one of the flags for test purposes. Use to
+  // ignore that effect of setting a flag that was just set.
+  uint64_t FlagsMinus(uint64_t flags, HangWatchDeadline::Flag flag) {
+    return flags & ~(static_cast<uint64_t>(flag));
+  }
+
+  HangWatchDeadline deadline_;
+};
+
+// Verify that the extract functions don't mangle any bits.
+TEST_F(HangWatchDeadlineTest, BitsPreservedThroughExtract) {
+  for (auto bits : {kAllOnes, kAllZeros, kOnesThenZeroes, kZeroesThenOnes}) {
+    ASSERT_TRUE((HangWatchDeadline::ExtractFlags(bits) |
+                 HangWatchDeadline::ExtractDeadline(bits)) == bits);
+  }
+}
+
+// Verify that setting and clearing a persistent flag works and has no unwanted
+// side-effects. Neither the flags nor the deadline change concurrently in this
+// test.
+TEST_F(HangWatchDeadlineTest, SetAndClearPersistentFlag) {
+  AssertNoFlagsSet();
+
+  // Grab the original values for flags and deadline.
+  uint64_t old_flags;
+  base::TimeTicks old_deadline;
+  std::tie(old_flags, old_deadline) = deadline_.GetFlagsAndDeadline();
+
+  // Set the flag. Operation cannot fail.
+  deadline_.SetIgnoreHangs();
+
+  // Get new flags and deadline.
+  uint64_t new_flags;
+  base::TimeTicks new_deadline;
+  std::tie(new_flags, new_deadline) = deadline_.GetFlagsAndDeadline();
+
+  // Flag was set properly.
+  ASSERT_TRUE(HangWatchDeadline::IsFlagSet(
+      HangWatchDeadline::Flag::kIgnoreHangs, new_flags));
+  // No side-effect on deadline.
+  ASSERT_EQ(new_deadline, old_deadline);
+
+  // No side-effect on other flags.
+  ASSERT_EQ(FlagsMinus(new_flags, HangWatchDeadline::Flag::kIgnoreHangs),
+            old_flags);
+
+  // Clear the flag, operation cannot fail.
+  deadline_.UnsetIgnoreHangs();
+
+  // Update new values.
+  std::tie(new_flags, new_deadline) = deadline_.GetFlagsAndDeadline();
+
+  // All flags back to original state.
+  ASSERT_EQ(new_flags, old_flags);
+
+  // Deadline still unnafected.
+  ASSERT_EQ(new_deadline, old_deadline);
+}
+
+// Verify setting the TimeTicks value works and has no unwanted side-effects.
+TEST_F(HangWatchDeadlineTest, SetDeadline) {
+  TimeTicks ticks;
+
+  AssertNoFlagsSet();
+  ASSERT_NE(deadline_.GetDeadline(), ticks);
+
+  // Set the deadline and verify it stuck.
+  deadline_.SetDeadline(ticks);
+  ASSERT_EQ(deadline_.GetDeadline(), ticks);
+
+  // Only the value was modified, no flags should be set.
+  AssertNoFlagsSet();
+}
+
+// Verify that setting a non-persistent flag (kShouldBlockOnHang)
+// when the TimeTicks value changed since calling the flag setting
+// function fails and has no side-effects.
+TEST_F(HangWatchDeadlineTest, SetShouldBlockOnHangDeadlineChanged) {
+  AssertNoFlagsSet();
+
+  uint64_t flags;
+  base::TimeTicks deadline;
+  std::tie(flags, deadline) = deadline_.GetFlagsAndDeadline();
+
+  // Simulate value change. Flags are constant.
+  const base::TimeTicks new_deadline =
+      base::TimeTicks::FromInternalValue(kArbitraryDeadline);
+  ASSERT_NE(deadline, new_deadline);
+  deadline_.SetSwitchBitsClosureForTesting(
+      base::BindLambdaForTesting([]() { return kArbitraryDeadline; }));
+
+  // kShouldBlockOnHangs does not persist through value change.
+  ASSERT_FALSE(deadline_.SetShouldBlockOnHang(flags, deadline));
+
+  // Flag was not applied.
+  ASSERT_FALSE(
+      deadline_.IsFlagSet(HangWatchDeadline::Flag::kShouldBlockOnHang));
+
+  // New value that was changed concurrently is preserved.
+  ASSERT_EQ(deadline_.GetDeadline(), new_deadline);
+}
+
+// Verify that clearing a persistent (kIgnoreHangs) when the value changed
+// succeeds and has non side-effects.
+TEST_F(HangWatchDeadlineTest, ClearIgnoreHangsDeadlineChanged) {
+  AssertNoFlagsSet();
+
+  uint64_t flags;
+  base::TimeTicks deadline;
+  std::tie(flags, deadline) = deadline_.GetFlagsAndDeadline();
+
+  deadline_.SetIgnoreHangs();
+  std::tie(flags, deadline) = deadline_.GetFlagsAndDeadline();
+  ASSERT_TRUE(HangWatchDeadline::IsFlagSet(
+      HangWatchDeadline::Flag::kIgnoreHangs, flags));
+
+  // Simulate deadline change. Flags are constant.
+  const base::TimeTicks new_deadline =
+      base::TimeTicks::FromInternalValue(kArbitraryDeadline);
+  ASSERT_NE(deadline, new_deadline);
+  deadline_.SetSwitchBitsClosureForTesting(base::BindLambdaForTesting([]() {
+    return static_cast<uint64_t>(HangWatchDeadline::Flag::kShouldBlockOnHang) |
+           kArbitraryDeadline;
+  }));
+
+  // Clearing kIgnoreHang is unafected by deadline or flags change.
+  deadline_.UnsetIgnoreHangs();
+  ASSERT_FALSE(deadline_.IsFlagSet(HangWatchDeadline::Flag::kIgnoreHangs));
+
+  // New deadline that was changed concurrently is preserved.
+  ASSERT_TRUE(deadline_.IsFlagSet(HangWatchDeadline::Flag::kShouldBlockOnHang));
+  ASSERT_EQ(deadline_.GetDeadline(), new_deadline);
+}
+
+// Verify that setting a persistent (kIgnoreHangs) when the deadline or flags
+// changed succeeds and has non side-effects.
+TEST_F(HangWatchDeadlineTest, SetIgnoreHangsDeadlineChanged) {
+  AssertNoFlagsSet();
+
+  uint64_t flags;
+  base::TimeTicks deadline;
+  std::tie(flags, deadline) = deadline_.GetFlagsAndDeadline();
+
+  // Simulate deadline change. Flags are constant.
+  const base::TimeTicks new_deadline =
+      base::TimeTicks::FromInternalValue(kArbitraryDeadline);
+
+  ASSERT_NE(deadline, new_deadline);
+  deadline_.SetSwitchBitsClosureForTesting(base::BindLambdaForTesting([]() {
+    return static_cast<uint64_t>(HangWatchDeadline::Flag::kShouldBlockOnHang) |
+           kArbitraryDeadline;
+  }));
+
+  // kIgnoreHang persists through value change.
+  deadline_.SetIgnoreHangs();
+  ASSERT_TRUE(deadline_.IsFlagSet(HangWatchDeadline::Flag::kIgnoreHangs));
+
+  // New deadline and flags that changed concurrently are preserved.
+  ASSERT_TRUE(deadline_.IsFlagSet(HangWatchDeadline::Flag::kShouldBlockOnHang));
+  ASSERT_EQ(deadline_.GetDeadline(), new_deadline);
+}
+
+// Setting a new deadline should wipe flags that a not persistent.
+// Persistent flags should not be disturbed.
+TEST_F(HangWatchDeadlineTest, SetDeadlineWipesFlags) {
+  uint64_t flags;
+  base::TimeTicks deadline;
+  std::tie(flags, deadline) = deadline_.GetFlagsAndDeadline();
+
+  ASSERT_TRUE(deadline_.SetShouldBlockOnHang(flags, deadline));
+  ASSERT_TRUE(deadline_.IsFlagSet(HangWatchDeadline::Flag::kShouldBlockOnHang));
+
+  std::tie(flags, deadline) = deadline_.GetFlagsAndDeadline();
+
+  deadline_.SetIgnoreHangs();
+  ASSERT_TRUE(deadline_.IsFlagSet(HangWatchDeadline::Flag::kIgnoreHangs));
+
+  // Change the deadline.
+  deadline_.SetDeadline(TimeTicks{});
+  ASSERT_EQ(deadline_.GetDeadline(), TimeTicks{});
+
+  // Verify the persistent flag stuck and the non-persistent one was unset.
+  ASSERT_FALSE(
+      deadline_.IsFlagSet(HangWatchDeadline::Flag::kShouldBlockOnHang));
+  ASSERT_TRUE(deadline_.IsFlagSet(HangWatchDeadline::Flag::kIgnoreHangs));
+}
+
+}  // namespace internal
+
 }  // namespace base