ThreadWatcher: fixes Start/StopWatchingAll.

Start is deferred in relation to Stop, and when they're both called
within the Start interval, the end result was started.

BUG=347887
TEST=unit_tests --gtest_filter=ThreadWatcherList*

Review URL: https://codereview.chromium.org/183063007

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

3 files changed, 82 insertions, 0 deletions

diff --git a/chrome/browser/metrics/thread_watcher.cc b/chrome/browser/metrics/thread_watcher.cc
index 1737593..8246b3e 100644
--- a/chrome/browser/metrics/thread_watcher.cc
+++ b/chrome/browser/metrics/thread_watcher.cc
@@ -409,6 +409,8 @@ bool ThreadWatcher::IsVeryUnresponsive() {
 // static
 ThreadWatcherList* ThreadWatcherList::g_thread_watcher_list_ = NULL;
 // static
+bool ThreadWatcherList::g_stopped_ = false;
+// static
 const int ThreadWatcherList::kSleepSeconds = 1;
 // static
 const int ThreadWatcherList::kUnresponsiveSeconds = 2;
@@ -443,6 +445,10 @@ void ThreadWatcherList::StartWatchingAll(const CommandLine& command_line) {
   ThreadWatcherObserver::SetupNotifications(
       base::TimeDelta::FromSeconds(kSleepSeconds * unresponsive_threshold));
 
+  WatchDogThread::PostTask(
+      FROM_HERE,
+      base::Bind(&ThreadWatcherList::SetStopped, false));
+
   WatchDogThread::PostDelayedTask(
       FROM_HERE,
       base::Bind(&ThreadWatcherList::InitializeAndStartWatching,
@@ -635,6 +641,12 @@ void ThreadWatcherList::InitializeAndStartWatching(
     const CrashOnHangThreadMap& crash_on_hang_threads) {
   DCHECK(WatchDogThread::CurrentlyOnWatchDogThread());
 
+  // This method is deferred in relationship to its StopWatchingAll()
+  // counterpart. If a previous initialization has already happened, or if
+  // stop has been called, there's nothing left to do here.
+  if (g_thread_watcher_list_ || g_stopped_)
+    return;
+
   ThreadWatcherList* thread_watcher_list = new ThreadWatcherList();
   CHECK(thread_watcher_list);
 
@@ -700,6 +712,9 @@ void ThreadWatcherList::DeleteAll() {
   }
 
   DCHECK(WatchDogThread::CurrentlyOnWatchDogThread());
+
+  SetStopped(true);
+
   if (!g_thread_watcher_list_)
     return;
 
@@ -725,6 +740,12 @@ ThreadWatcher* ThreadWatcherList::Find(const BrowserThread::ID& thread_id) {
   return it->second;
 }
 
+// static
+void ThreadWatcherList::SetStopped(bool stopped) {
+  DCHECK(WatchDogThread::CurrentlyOnWatchDogThread());
+  g_stopped_ = stopped;
+}
+
 // ThreadWatcherObserver methods and members.
 //
 // static
diff --git a/chrome/browser/metrics/thread_watcher.h b/chrome/browser/metrics/thread_watcher.h
index 2c4b027..939375a 100644
--- a/chrome/browser/metrics/thread_watcher.h
+++ b/chrome/browser/metrics/thread_watcher.h
@@ -410,6 +410,7 @@ class ThreadWatcherList {
   FRIEND_TEST_ALL_PREFIXES(ThreadWatcherTest, ThreadNamesOnlyArgs);
   FRIEND_TEST_ALL_PREFIXES(ThreadWatcherTest, ThreadNamesAndLiveThresholdArgs);
   FRIEND_TEST_ALL_PREFIXES(ThreadWatcherTest, CrashOnHangThreadsAllArgs);
+  FRIEND_TEST_ALL_PREFIXES(ThreadWatcherListTest, Restart);
   FRIEND_TEST_ALL_PREFIXES(ThreadWatcherAndroidTest,
                            ApplicationStatusNotification);
 
@@ -464,10 +465,19 @@ class ThreadWatcherList {
   // already registered, or to retrieve a pointer to it from the global map.
   static ThreadWatcher* Find(const content::BrowserThread::ID& thread_id);
 
+  // Sets |g_stopped_| on the WatchDogThread. This is necessary to reflect the
+  // state between the delayed |StartWatchingAll| and the immediate
+  // |StopWatchingAll|.
+  static void SetStopped(bool stopped);
+
   // The singleton of this class and is used to keep track of information about
   // threads that are being watched.
   static ThreadWatcherList* g_thread_watcher_list_;
 
+  // StartWatchingAll() is delayed in relation to StopWatchingAll(), so if
+  // a Stop comes first, prevent further initialization.
+  static bool g_stopped_;
+
   // This is the wait time between ping messages.
   static const int kSleepSeconds;
 
diff --git a/chrome/browser/metrics/thread_watcher_unittest.cc b/chrome/browser/metrics/thread_watcher_unittest.cc
index 4b799e9..7efb041 100644
--- a/chrome/browser/metrics/thread_watcher_unittest.cc
+++ b/chrome/browser/metrics/thread_watcher_unittest.cc
@@ -10,6 +10,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/message_loop/message_loop_proxy.h"
+#include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_tokenizer.h"
@@ -629,3 +630,53 @@ TEST_F(ThreadWatcherTest, MultipleThreadsNotResponding) {
   // Wait for the io_watcher_'s VeryLongMethod to finish.
   io_watcher_->WaitForWaitStateChange(kUnresponsiveTime * 10, ALL_DONE);
 }
+
+TEST(ThreadWatcherListTest, Restart) {
+  ThreadWatcherList::g_initialize_delay_seconds = 1;
+
+  base::MessageLoopForUI message_loop_for_ui;
+  content::TestBrowserThread ui_thread(BrowserThread::UI, &message_loop_for_ui);
+
+  scoped_ptr<WatchDogThread> watchdog_thread_(new WatchDogThread());
+  watchdog_thread_->Start();
+
+  EXPECT_FALSE(ThreadWatcherList::g_thread_watcher_list_);
+
+  // See http://crbug.com/347887.
+  // StartWatchingAll() will PostDelayedTask to create g_thread_watcher_list_,
+  // whilst StopWatchingAll() will just PostTask to destroy it.
+  // Ensure that when Stop is called, Start will NOT create
+  // g_thread_watcher_list_ later on.
+  ThreadWatcherList::StartWatchingAll(*CommandLine::ForCurrentProcess());
+  ThreadWatcherList::StopWatchingAll();
+  message_loop_for_ui.PostDelayedTask(
+      FROM_HERE,
+      message_loop_for_ui.QuitClosure(),
+      base::TimeDelta::FromSeconds(
+          ThreadWatcherList::g_initialize_delay_seconds));
+  message_loop_for_ui.Run();
+  EXPECT_FALSE(ThreadWatcherList::g_thread_watcher_list_);
+  EXPECT_TRUE(ThreadWatcherList::g_stopped_);
+
+  // Proceed with just |StartWatchingAll| and ensure it'll be started.
+  ThreadWatcherList::StartWatchingAll(*CommandLine::ForCurrentProcess());
+  message_loop_for_ui.PostDelayedTask(
+      FROM_HERE,
+      message_loop_for_ui.QuitClosure(),
+      base::TimeDelta::FromSeconds(
+          ThreadWatcherList::g_initialize_delay_seconds + 1));
+  message_loop_for_ui.Run();
+  EXPECT_TRUE(ThreadWatcherList::g_thread_watcher_list_);
+  EXPECT_FALSE(ThreadWatcherList::g_stopped_);
+
+  // Finally, StopWatchingAll() must stop.
+  ThreadWatcherList::StopWatchingAll();
+  message_loop_for_ui.PostDelayedTask(
+      FROM_HERE,
+      message_loop_for_ui.QuitClosure(),
+      base::TimeDelta::FromSeconds(
+          ThreadWatcherList::g_initialize_delay_seconds));
+  message_loop_for_ui.Run();
+  EXPECT_FALSE(ThreadWatcherList::g_thread_watcher_list_);
+  EXPECT_TRUE(ThreadWatcherList::g_stopped_);
+}