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.
Relands crrev.com/255322, fixing race condition in the test.

BUG=347887,349987

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

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

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

chrome/browser/metrics/thread_watcher.h

@@ -406,12 +406,14 @@ class ThreadWatcherList {
  private:
   // Allow tests to access our innards for testing purposes.
   friend class CustomThreadWatcher;
+  friend class ThreadWatcherListTest;
   friend class ThreadWatcherTest;
+  FRIEND_TEST_ALL_PREFIXES(ThreadWatcherAndroidTest,
+                           ApplicationStatusNotification);
+  FRIEND_TEST_ALL_PREFIXES(ThreadWatcherListTest, Restart);
   FRIEND_TEST_ALL_PREFIXES(ThreadWatcherTest, ThreadNamesOnlyArgs);
   FRIEND_TEST_ALL_PREFIXES(ThreadWatcherTest, ThreadNamesAndLiveThresholdArgs);
   FRIEND_TEST_ALL_PREFIXES(ThreadWatcherTest, CrashOnHangThreadsAllArgs);
-  FRIEND_TEST_ALL_PREFIXES(ThreadWatcherAndroidTest,
-                           ApplicationStatusNotification);
 
   // This singleton holds the global list of registered ThreadWatchers.
   ThreadWatcherList();
@@ -464,10 +466,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;
 

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,109 @@ TEST_F(ThreadWatcherTest, MultipleThreadsNotResponding) {
   // Wait for the io_watcher_'s VeryLongMethod to finish.
   io_watcher_->WaitForWaitStateChange(kUnresponsiveTime * 10, ALL_DONE);
 }
+
+class ThreadWatcherListTest : public ::testing::Test {
+ protected:
+  ThreadWatcherListTest()
+      : done_(&lock_),
+        state_available_(false),
+        has_thread_watcher_list_(false),
+        stopped_(false) {
+  }
+
+  void ReadStateOnWatchDogThread() {
+    CHECK(WatchDogThread::CurrentlyOnWatchDogThread());
+    {
+      base::AutoLock auto_lock(lock_);
+      has_thread_watcher_list_ =
+          ThreadWatcherList::g_thread_watcher_list_ != NULL;
+      stopped_ = ThreadWatcherList::g_stopped_;
+      state_available_ = true;
+    }
+    done_.Signal();
+  }
+
+  void CheckState(bool has_thread_watcher_list,
+                  bool stopped,
+                  const char* const msg) {
+    CHECK(!WatchDogThread::CurrentlyOnWatchDogThread());
+    {
+      base::AutoLock auto_lock(lock_);
+      state_available_ = false;
+    }
+
+    WatchDogThread::PostTask(
+        FROM_HERE,
+        base::Bind(&ThreadWatcherListTest::ReadStateOnWatchDogThread,
+                   base::Unretained(this)));
+    {
+      base::AutoLock auto_lock(lock_);
+      while (!state_available_)
+        done_.Wait();
+
+      EXPECT_EQ(has_thread_watcher_list, has_thread_watcher_list_) << msg;
+      EXPECT_EQ(stopped, stopped_) << msg;
+    }
+  }
+
+  base::Lock lock_;
+  base::ConditionVariable done_;
+
+  bool state_available_;
+  bool has_thread_watcher_list_;
+  bool stopped_;
+};
+
+TEST_F(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();
+
+  // 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();
+
+  CheckState(false /* has_thread_watcher_list */,
+             true /* stopped */,
+             "Start / 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();
+
+  CheckState(true /* has_thread_watcher_list */,
+             false /* stopped */,
+             "Started");
+
+  // 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();
+
+  CheckState(false /* has_thread_watcher_list */,
+             true /* stopped */,
+             "Stopped");
+}