WATCHDOG thread and thread_watcher class to monitor the

responsiveness of browser threads. WATCHDOG thread sends
a ping message to watched thread and watched thread responds
back with a pong message. Upload the responsiveness of the
watched thread (ping time minus pong time) as a histogram.

This change list is for readability.

The following is the original code review.

http://codereview.chromium.org/6392018/

BUG=71378
TEST=thread_watcher_unittest

Review URL: http://codereview.chromium.org/6571001

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@78472 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 55 insertions, 70 deletions

diff --git a/chrome/browser/metrics/thread_watcher.cc b/chrome/browser/metrics/thread_watcher.cc
index b557984..b389ae7 100644
--- a/chrome/browser/metrics/thread_watcher.cc
+++ b/chrome/browser/metrics/thread_watcher.cc
@@ -15,10 +15,8 @@
 // static
 const int ThreadWatcher::kPingCount = 3;
 
-//------------------------------------------------------------------------------
 // ThreadWatcher methods and members.
-
-ThreadWatcher::ThreadWatcher(const BrowserThread::ID thread_id,
+ThreadWatcher::ThreadWatcher(const BrowserThread::ID& thread_id,
                              const std::string& thread_name,
                              const base::TimeDelta& sleep_time,
                              const base::TimeDelta& unresponsive_time)
@@ -37,7 +35,7 @@ ThreadWatcher::ThreadWatcher(const BrowserThread::ID thread_id,
 ThreadWatcher::~ThreadWatcher() {}
 
 // static
-void ThreadWatcher::StartWatching(const BrowserThread::ID thread_id,
+void ThreadWatcher::StartWatching(const BrowserThread::ID& thread_id,
                                   const std::string& thread_name,
                                   const base::TimeDelta& sleep_time,
                                   const base::TimeDelta& unresponsive_time) {
@@ -164,31 +162,30 @@ bool ThreadWatcher::OnCheckResponsiveness(uint64 ping_sequence_number) {
   // If the latest ping_sequence_number_ is not same as the ping_sequence_number
   // that is passed in, then we can assume OnPongMessage was called.
   // OnPongMessage increments ping_sequence_number_.
-  if (ping_sequence_number_ != ping_sequence_number)
-    return true;
-  return false;
+  return ping_sequence_number_ != ping_sequence_number;
 }
 
 void ThreadWatcher::Initialize() {
   ThreadWatcherList::Register(this);
+  const std::string histogram_name =
+      "ThreadWatcher.ResponseTime." + thread_name_;
   histogram_ = base::Histogram::FactoryTimeGet(
-      "ThreadWatcher.ResponseTime." + thread_name_,
+      histogram_name,
       base::TimeDelta::FromMilliseconds(1),
       base::TimeDelta::FromSeconds(100), 50,
       base::Histogram::kUmaTargetedHistogramFlag);
 }
 
 // static
-void ThreadWatcher::OnPingMessage(const BrowserThread::ID thread_id,
+void ThreadWatcher::OnPingMessage(const BrowserThread::ID& thread_id,
                                   Task* callback_task) {
   // This method is called on watched thread.
   DCHECK(BrowserThread::CurrentlyOn(thread_id));
   WatchDogThread::PostTask(FROM_HERE, callback_task);
 }
 
-//------------------------------------------------------------------------------
 // ThreadWatcherList methods and members.
-
+//
 // static
 ThreadWatcherList* ThreadWatcherList::global_ = NULL;
 
@@ -328,7 +325,7 @@ void ThreadWatcherList::WakeUpAll() {
 }
 
 // static
-ThreadWatcher* ThreadWatcherList::Find(const BrowserThread::ID thread_id) {
+ThreadWatcher* ThreadWatcherList::Find(const BrowserThread::ID& thread_id) {
   if (!global_)
     return NULL;
   base::AutoLock auto_lock(global_->lock_);
@@ -336,16 +333,15 @@ ThreadWatcher* ThreadWatcherList::Find(const BrowserThread::ID thread_id) {
 }
 
 ThreadWatcher* ThreadWatcherList::PreLockedFind(
-    const BrowserThread::ID thread_id) {
+    const BrowserThread::ID& thread_id) {
   RegistrationList::iterator it = registered_.find(thread_id);
   if (registered_.end() == it)
     return NULL;
   return it->second;
 }
 
-//------------------------------------------------------------------------------
 // WatchDogThread methods and members.
-
+//
 // static
 base::Lock WatchDogThread::lock_;
 // static
diff --git a/chrome/browser/metrics/thread_watcher.h b/chrome/browser/metrics/thread_watcher.h
index ea7db9e..1834ea5 100644
--- a/chrome/browser/metrics/thread_watcher.h
+++ b/chrome/browser/metrics/thread_watcher.h
@@ -2,11 +2,34 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-//------------------------------------------------------------------------------
+// This file defines a WatchDog thread that monitors the responsiveness of other
+// browser threads like UI, IO, DB, FILE and CACHED threads. It also defines
+// ThreadWatcher class which performs health check on threads that would like to
+// be watched. This file also defines ThreadWatcherList class that has list of
+// all active ThreadWatcher objects.
+//
+// ThreadWatcher class sends ping message to the watched thread and the watched
+// thread responds back with a pong message. It uploads response time
+// (difference between ping and pong times) as a histogram.
+//
+// TODO(raman): ThreadWatcher can detect hung threads. If a hung thread is
+// detected, we should probably just crash, and allow the crash system to gather
+// then stack trace.
+//
+// Example Usage:
+//
+//   The following is an example for watching responsiveness of IO thread.
+//   sleep_time specifies how often ping messages have to be sent to IO thread.
+//   unresponsive_time is the wait time after ping message is sent, to check if
+//   we have received pong message or not.
+//
+//   base::TimeDelta sleep_time = base::TimeDelta::FromSeconds(5);
+//   base::TimeDelta unresponsive_time = base::TimeDelta::FromSeconds(10);
+//   ThreadWatcher::StartWatching(BrowserThread::IO, "IO", sleep_time,
+//                                unresponsive_time);
 
 #ifndef CHROME_BROWSER_METRICS_THREAD_WATCHER_H_
 #define CHROME_BROWSER_METRICS_THREAD_WATCHER_H_
-#pragma once
 
 #include <map>
 #include <string>
@@ -29,27 +52,7 @@
 class CustomThreadWatcher;
 class ThreadWatcherList;
 
-//------------------------------------------------------------------------------
-// This class performs health check on threads that would like to be watched. It
-// sends ping message to the watched thread and the watched thread responds back
-// with a pong message. It uploads response time (difference between ping and
-// pong times) as a histogram.
-// TODO(raman:) ThreadWatcher can detect hung threads. If a hung thread is
-// detected, we should probably just crash, and allow the crash system to gather
-// then stack trace.
-//
-// Example Usage:
-//
-//   The following is an example for watching responsiveness of IO thread.
-//   sleep_time specifies how often ping messages have to be sent to IO thread.
-//   unresponsive_time is the wait time after ping message is sent, to check if
-//   we have received pong message or not.
-//
-//   base::TimeDelta sleep_time = base::TimeDelta::FromSeconds(5);
-//   base::TimeDelta unresponsive_time = base::TimeDelta::FromSeconds(10);
-//   ThreadWatcher::StartWatching(BrowserThread::IO, "IO", sleep_time,
-//                                unresponsive_time);
-
+// This class performs health check on threads that would like to be watched.
 class ThreadWatcher {
  public:
   // This method starts performing health check on the given thread_id. It will
@@ -59,7 +62,7 @@ class ThreadWatcher {
   // to check if we have received pong message or not. It will register that
   // ThreadWatcher object and activate the thread watching of the given
   // thread_id.
-  static void StartWatching(const BrowserThread::ID thread_id,
+  static void StartWatching(const BrowserThread::ID& thread_id,
                             const std::string& thread_name,
                             const base::TimeDelta& sleep_time,
                             const base::TimeDelta& unresponsive_time);
@@ -89,7 +92,7 @@ class ThreadWatcher {
   // Construct a ThreadWatcher for the given thread_id. sleep_time_ is the
   // wait time between ping messages. unresponsive_time_ is the wait time after
   // ping message is sent, to check if we have received pong message or not.
-  ThreadWatcher(const BrowserThread::ID thread_id,
+  ThreadWatcher(const BrowserThread::ID& thread_id,
                 const std::string& thread_name,
                 const base::TimeDelta& sleep_time,
                 const base::TimeDelta& unresponsive_time);
@@ -143,7 +146,7 @@ class ThreadWatcher {
 
   // Watched thread does nothing except post callback_task to the WATCHDOG
   // Thread. This method is called on watched thread.
-  static void OnPingMessage(const BrowserThread::ID thread_id,
+  static void OnPingMessage(const BrowserThread::ID& thread_id,
                             Task* callback_task);
 
   // This is the number of ping messages to be sent when the user is idle.
@@ -185,12 +188,14 @@ class ThreadWatcher {
   // Histogram that keeps track of response times for the watched thread.
   scoped_refptr<base::Histogram> histogram_;
 
+  // We use this factory to create callback tasks for ThreadWatcher object. We
+  // use this during ping-pong messaging between WatchDog thread and watched
+  // thread.
   ScopedRunnableMethodFactory<ThreadWatcher> method_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ThreadWatcher);
 };
 
-//------------------------------------------------------------------------------
 // Class with a list of all active thread watchers.  A thread watcher is active
 // if it has been registered, which includes determing the histogram name. This
 // class provides utility functions to start and stop watching all browser
@@ -250,15 +255,17 @@ class ThreadWatcherList : public NotificationObserver {
 
   // The Find() method can be used to test to see if a given ThreadWatcher was
   // already registered, or to retrieve a pointer to it from the global map.
-  static ThreadWatcher* Find(const BrowserThread::ID thread_id);
+  static ThreadWatcher* Find(const BrowserThread::ID& thread_id);
 
   // Helper function should be called only while holding lock_.
-  ThreadWatcher* PreLockedFind(const BrowserThread::ID thread_id);
+  ThreadWatcher* PreLockedFind(const BrowserThread::ID& thread_id);
 
   static ThreadWatcherList* global_;  // The singleton of this class.
 
   // Lock for access to registered_.
   base::Lock lock_;
+
+  // Map of all registered watched threads, from thread_id to ThreadWatcher.
   RegistrationList registered_;
 
   // The registrar that holds NotificationTypes to be observed.
@@ -270,7 +277,6 @@ class ThreadWatcherList : public NotificationObserver {
   DISALLOW_COPY_AND_ASSIGN(ThreadWatcherList);
 };
 
-//------------------------------------------------------------------------------
 // Class for WatchDogThread and in its Init method, we start watching UI, IO,
 // DB, FILE, CACHED threads.
 class WatchDogThread : public base::Thread {
@@ -314,6 +320,8 @@ class WatchDogThread : public base::Thread {
   DISALLOW_COPY_AND_ASSIGN(WatchDogThread);
 };
 
+// DISABLE_RUNNABLE_METHOD_REFCOUNT is a convenience macro for disabling
+// refcounting of ThreadWatcher and ThreadWatcherList classes.
 DISABLE_RUNNABLE_METHOD_REFCOUNT(ThreadWatcher);
 DISABLE_RUNNABLE_METHOD_REFCOUNT(ThreadWatcherList);
 
diff --git a/chrome/browser/metrics/thread_watcher_unittest.cc b/chrome/browser/metrics/thread_watcher_unittest.cc
index d65ccef..5d130b2 100644
--- a/chrome/browser/metrics/thread_watcher_unittest.cc
+++ b/chrome/browser/metrics/thread_watcher_unittest.cc
@@ -40,6 +40,12 @@ enum CheckResponseState {
   FAILED,             // CheckResponse has failed.
 };
 
+// This class helps to track and manipulate thread state during tests. This
+// class also has utility method to simulate hanging of watched thread by making
+// the watched thread wait for a very long time by posting a task on watched
+// thread that keeps it busy. It also has an utility method to block running of
+// tests until ThreadWatcher object's post-condition state changes to an
+// expected state.
 class CustomThreadWatcher : public ThreadWatcher {
  public:
   base::Lock custom_lock_;
@@ -86,8 +92,6 @@ class CustomThreadWatcher : public ThreadWatcher {
       saved_ping_sequence_number_ = ping_sequence_number();
     }
     state_changed_.Broadcast();
-    // LOG(INFO) << "UpdateState: thread_name_: " << thread_name_ <<
-    //     " thread_watcher_state_: " << new_state;
     return old_state;
   }
 
@@ -99,8 +103,6 @@ class CustomThreadWatcher : public ThreadWatcher {
       wait_state_ = new_state;
     }
     state_changed_.Broadcast();
-    // LOG(INFO) << "UpdateWaitState: thread_name_: " << thread_name_ <<
-    //     " wait_state_: " << new_state;
     return old_state;
   }
 
@@ -132,10 +134,6 @@ class CustomThreadWatcher : public ThreadWatcher {
   bool OnCheckResponsiveness(uint64 ping_sequence_number) {
     bool responsive =
         ThreadWatcher::OnCheckResponsiveness(ping_sequence_number);
-    // LOG(INFO) << "CheckResponse: thread_name_: " << thread_name_ <<
-    //     " ping_sequence_number_:" << ping_sequence_number_  <<
-    //     " ping_sequence_number:" << ping_sequence_number <<
-    //     " responsive: " << responsive;
     {
       base::AutoLock auto_lock(custom_lock_);
       if (responsive) {
@@ -182,10 +180,6 @@ class CustomThreadWatcher : public ThreadWatcher {
           while (thread_watcher_state_ != expected_state &&
                  TimeTicks::Now() < end_time) {
             TimeDelta state_change_wait_time = end_time - TimeTicks::Now();
-            // LOG(INFO) << "In WaitForStateChange: thread_name_: " <<
-            //     thread_name_ << " i:" << i <<
-            //     " wait: " << state_change_wait_time.InMilliseconds() <<
-            //     " thread_watcher_state_: " << thread_watcher_state_;
             state_changed_.TimedWait(state_change_wait_time);
           }
           // Capture the thread_watcher_state_ before it changes and return it
@@ -195,9 +189,6 @@ class CustomThreadWatcher : public ThreadWatcher {
             break;
         }
     }
-    // LOG(INFO) << "In WaitForStateChange: thread_name_: " << thread_name_ <<
-    //     " expected_state: " << expected_state <<
-    //     " exit_state:" << exit_state;
     UpdateWaitState(STOPPED_WAITING);
     return exit_state;
   }
@@ -218,10 +209,6 @@ class CustomThreadWatcher : public ThreadWatcher {
           while (check_response_state_ != expected_state &&
                  TimeTicks::Now() < end_time) {
             TimeDelta state_change_wait_time = end_time - TimeTicks::Now();
-            // LOG(INFO) << "In WaitForCheckResponse: " <<
-            //     " thread_name_: " << thread_name_ << " i:" << i <<
-            //     " wait:" << state_change_wait_time.InMilliseconds() <<
-            //     " check_response_state_:" << check_response_state_;
             state_changed_.TimedWait(state_change_wait_time);
           }
           // Capture the check_response_state_ before it changes and return it
@@ -231,9 +218,6 @@ class CustomThreadWatcher : public ThreadWatcher {
             break;
         }
     }
-    // LOG(INFO) << "In WaitForCheckResponse: thread_name_: " << thread_name_ <<
-    //     " expected_state: " << expected_state <<
-    //     " exit_state:" << exit_state;
     UpdateWaitState(STOPPED_WAITING);
     return exit_state;
   }
@@ -276,9 +260,6 @@ class ThreadWatcherTest : public ::testing::Test {
     ThreadWatcherList::StopWatchingAll();
     io_watcher_ = NULL;
     webkit_watcher_ = NULL;
-    // io_thread_->Stop();
-    // webkit_thread_->Stop();
-    // watchdog_thread_->Stop();
     io_thread_.reset();
     webkit_thread_.reset();
     watchdog_thread_.reset();