Clarify HangWatcher code and comments

Document some intent of atomic operations with memory order
directives and make some comments more correct.

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

base/threading/hang_watcher.cc

@@ -100,13 +100,14 @@ HangWatchScope::~HangWatchScope() {
 
 HangWatcher::HangWatcher(RepeatingClosure on_hang_closure)
     : monitor_period_(kMonitoringPeriod),
-      monitor_event_(WaitableEvent::ResetPolicy::AUTOMATIC,
-                     WaitableEvent::InitialState::NOT_SIGNALED),
+      should_monitor_(WaitableEvent::ResetPolicy::AUTOMATIC),
       on_hang_closure_(std::move(on_hang_closure)),
       thread_(this, kThreadName) {
   // |thread_checker_| should not be bound to the constructing thread.
   DETACH_FROM_THREAD(thread_checker_);
 
+  should_monitor_.declare_only_used_while_idle();
+
   DCHECK(!g_instance);
   g_instance = this;
   Start();
@@ -124,8 +125,8 @@ void HangWatcher::Start() {
 }
 
 void HangWatcher::Stop() {
-  keep_monitoring_.store(false);
-  monitor_event_.Signal();
+  keep_monitoring_.store(false, std::memory_order_relaxed);
+  should_monitor_.Signal();
   thread_.Join();
 }
 
@@ -139,17 +140,17 @@ void HangWatcher::Run() {
   // sure of that.
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
 
-  while (keep_monitoring_) {
+  while (keep_monitoring_.load(std::memory_order_relaxed)) {
     // If there is nothing to watch sleep until there is.
     if (IsWatchListEmpty()) {
-      monitor_event_.Wait();
+      should_monitor_.Wait();
     } else {
       Monitor();
     }
 
-    if (keep_monitoring_) {
+    if (keep_monitoring_.load(std::memory_order_relaxed)) {
       // Sleep until next scheduled monitoring.
-      monitor_event_.TimedWait(monitor_period_);
+      should_monitor_.TimedWait(monitor_period_);
     }
   }
 }
@@ -178,7 +179,7 @@ ScopedClosureRunner HangWatcher::RegisterThread() {
 
   // Now that there is a thread to monitor we wake the HangWatcher thread.
   if (watch_states_.size() == 1) {
-    monitor_event_.Signal();
+    should_monitor_.Signal();
   }
 
   return ScopedClosureRunner(BindOnce(&HangWatcher::UnregisterThread,
@@ -225,7 +226,7 @@ void HangWatcher::SetMonitoringPeriodForTesting(base::TimeDelta period) {
 }
 
 void HangWatcher::SignalMonitorEventForTesting() {
-  monitor_event_.Signal();
+  should_monitor_.Signal();
 }
 
 void HangWatcher::BlockIfCaptureInProgress() {
@@ -261,7 +262,7 @@ namespace internal {
 
 // |deadline_| starts at Max() to avoid validation problems
 // when setting the first legitimate value.
-HangWatchState::HangWatchState() : deadline_(TimeTicks::Max()) {
+HangWatchState::HangWatchState() {
   // There should not exist a state object for this thread already.
   DCHECK(!GetHangWatchStateForCurrentThread()->Get());
 
@@ -298,16 +299,16 @@ HangWatchState::CreateHangWatchStateForCurrentThread() {
 }
 
 TimeTicks HangWatchState::GetDeadline() const {
-  return deadline_.load();
+  return deadline_.load(std::memory_order_relaxed);
 }
 
-TimeTicks HangWatchState::SetDeadline(TimeTicks deadline) {
+void HangWatchState::SetDeadline(TimeTicks deadline) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
-  return deadline_.exchange(deadline);
+  deadline_.store(deadline, std::memory_order_relaxed);
 }
 
 bool HangWatchState::IsOverDeadline() const {
-  return TimeTicks::Now() > deadline_.load();
+  return TimeTicks::Now() > deadline_.load(std::memory_order_relaxed);
 }
 
 #if DCHECK_IS_ON()

base/threading/hang_watcher.h

@@ -40,10 +40,12 @@ namespace base {
 //
 // If DoSomeWork() takes more than 5s to run and the HangWatcher
 // inspects the thread state before Foobar returns a hang will be
-// reported. Instances of this object should live on the stack only as they are
-// intrinsically linked to the execution scopes that contain them.
-// Keeping a HangWatchScope alive after the scope in which it was created has
-// exited would lead to non-actionable hang reports.
+// reported.
+//
+// HangWatchScopes are typically meant to live on the stack. In some cases it's
+// necessary to keep a HangWatchScope instance as a class member but special
+// care is required when doing so as a HangWatchScope that stays alive longer
+// than intended will generate non-actionable hang reports.
 class BASE_EXPORT HangWatchScope {
  public:
   // A good default value needs to be large enough to represent a significant
@@ -148,7 +150,7 @@ class BASE_EXPORT HangWatcher : public DelegateSimpleThread::Delegate {
 
   // Use to make the HangWatcher thread wake or sleep to schedule the
   // appropriate monitoring frequency.
-  WaitableEvent monitor_event_;
+  WaitableEvent should_monitor_;
 
   bool IsWatchListEmpty() LOCKS_EXCLUDED(watch_state_lock_);
 
@@ -204,7 +206,7 @@ class BASE_EXPORT HangWatchState {
   TimeTicks GetDeadline() const;
 
   // Atomically sets the deadline to a new value and return the previous value.
-  TimeTicks SetDeadline(TimeTicks deadline);
+  void SetDeadline(TimeTicks deadline);
 
   // Tests whether the associated thread's execution has gone over the deadline.
   bool IsOverDeadline() const;
@@ -224,7 +226,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_;
+  std::atomic<TimeTicks> deadline_{base::TimeTicks::Max()};
 
 #if DCHECK_IS_ON()
   // Used to keep track of the current HangWatchScope and detect improper usage.