Fix MemoryDumpManagerTest's thread unsafe usage of the base::TestIOThread API

Also move TestIOThread::PostTaskAndWait to those tests as they were the
only user.

BUG=629139

Review-Url: https://codereview.chromium.org/2170953002
Cr-Commit-Position: refs/heads/master@{#407577}

base/test/test_io_thread.cc

@@ -4,19 +4,7 @@
 
 #include "base/test/test_io_thread.h"
 
-#include "base/bind.h"
-#include "base/callback.h"
-#include "base/synchronization/waitable_event.h"
-
-namespace {
-
-void PostTaskAndWaitHelper(base::WaitableEvent* event,
-                           const base::Closure& task) {
-  task.Run();
-  event->Signal();
-}
-
-}  // namespace
+#include "base/logging.h"
 
 namespace base {
 
@@ -54,13 +42,4 @@ void TestIOThread::PostTask(const tracked_objects::Location& from_here,
   task_runner()->PostTask(from_here, task);
 }
 
-void TestIOThread::PostTaskAndWait(const tracked_objects::Location& from_here,
-                                   const base::Closure& task) {
-  base::WaitableEvent event(WaitableEvent::ResetPolicy::AUTOMATIC,
-                            WaitableEvent::InitialState::NOT_SIGNALED);
-  task_runner()->PostTask(from_here,
-                          base::Bind(&PostTaskAndWaitHelper, &event, task));
-  event.Wait();
-}
-
 }  // namespace base

base/test/test_io_thread.h

@@ -18,6 +18,13 @@ namespace base {
 // Create and run an IO thread with a MessageLoop, and
 // making the MessageLoop accessible from its client.
 // It also provides some ideomatic API like PostTaskAndWait().
+//
+// This API is not thread-safe:
+//   - Start()/Stop() should only be called from the main (creation) thread.
+//   - PostTask()/message_loop()/task_runner() are also safe to call from the
+//     underlying thread itself (to post tasks from other threads: get the
+//     task_runner() from the main thread first, it is then safe to pass _it_
+//     around).
 class TestIOThread {
  public:
   enum Mode { kAutoStart, kManualStart };
@@ -25,19 +32,14 @@ class TestIOThread {
   // Stops the I/O thread if necessary.
   ~TestIOThread();
 
-  // |Start()|/|Stop()| should only be called from the main (creation) thread.
-  // After |Stop()|, |Start()| may be called again to start a new I/O thread.
-  // |Stop()| may be called even when the I/O thread is not started.
+  // After Stop(), Start() may be called again to start a new I/O thread.
+  // Stop() may be called even when the I/O thread is not started.
   void Start();
   void Stop();
 
   // Post |task| to the IO thread.
   void PostTask(const tracked_objects::Location& from_here,
                 const base::Closure& task);
-  // Posts |task| to the IO-thread with an WaitableEvent associated blocks on
-  // it until the posted |task| is executed, then returns.
-  void PostTaskAndWait(const tracked_objects::Location& from_here,
-                       const base::Closure& task);
 
   base::MessageLoopForIO* message_loop() {
     return static_cast<base::MessageLoopForIO*>(io_thread_.message_loop());

base/trace_event/memory_dump_manager_unittest.cc

@@ -19,6 +19,7 @@
 #include "base/test/test_io_thread.h"
 #include "base/test/trace_event_analyzer.h"
 #include "base/threading/platform_thread.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ -94,6 +95,20 @@ void OnTraceDataCollected(Closure quit_closure,
     quit_closure.Run();
 }
 
+// Posts |task| to |task_runner| and blocks until it is executed.
+void PostTaskAndWait(const tracked_objects::Location& from_here,
+                     SequencedTaskRunner* task_runner,
+                     const base::Closure& task) {
+  base::WaitableEvent event(WaitableEvent::ResetPolicy::MANUAL,
+                            WaitableEvent::InitialState::NOT_SIGNALED);
+  task_runner->PostTask(from_here, task);
+  task_runner->PostTask(
+      FROM_HERE, base::Bind(&WaitableEvent::Signal, base::Unretained(&event)));
+  // The SequencedTaskRunner guarantees that |event| will only be signaled after
+  // |task| is executed.
+  event.Wait();
+}
+
 }  // namespace
 
 // Testing MemoryDumpManagerDelegate which, by default, short-circuits dump
@@ -688,13 +703,16 @@ TEST_F(MemoryDumpManagerTest, UnregisterDumperFromThreadWhileDumping) {
   // unregister the other one.
   for (const std::unique_ptr<MockMemoryDumpProvider>& mdp : mdps) {
     int other_idx = (mdps.front() == mdp);
-    TestIOThread* other_thread = threads[other_idx].get();
+    // TestIOThread's task runner must be obtained from the main thread but can
+    // then be used from other threads.
+    scoped_refptr<SingleThreadTaskRunner> other_runner =
+        threads[other_idx]->task_runner();
     MockMemoryDumpProvider* other_mdp = mdps[other_idx].get();
-    auto on_dump = [this, other_thread, other_mdp, &on_memory_dump_call_count](
+    auto on_dump = [this, other_runner, other_mdp, &on_memory_dump_call_count](
         const MemoryDumpArgs& args, ProcessMemoryDump* pmd) {
-      other_thread->PostTaskAndWait(
-          FROM_HERE, base::Bind(&MemoryDumpManager::UnregisterDumpProvider,
-                                base::Unretained(&*mdm_), other_mdp));
+      PostTaskAndWait(FROM_HERE, other_runner.get(),
+                      base::Bind(&MemoryDumpManager::UnregisterDumpProvider,
+                                 base::Unretained(&*mdm_), other_mdp));
       on_memory_dump_call_count++;
       return true;
     };
@@ -739,9 +757,14 @@ TEST_F(MemoryDumpManagerTest, TearDownThreadWhileDumping) {
   for (const std::unique_ptr<MockMemoryDumpProvider>& mdp : mdps) {
     int other_idx = (mdps.front() == mdp);
     TestIOThread* other_thread = threads[other_idx].get();
-    auto on_dump = [other_thread, &on_memory_dump_call_count](
+    // TestIOThread isn't thread-safe and must be stopped on the |main_runner|.
+    scoped_refptr<SequencedTaskRunner> main_runner =
+        SequencedTaskRunnerHandle::Get();
+    auto on_dump = [other_thread, main_runner, &on_memory_dump_call_count](
         const MemoryDumpArgs& args, ProcessMemoryDump* pmd) {
-      other_thread->Stop();
+      PostTaskAndWait(
+          FROM_HERE, main_runner.get(),
+          base::Bind(&TestIOThread::Stop, base::Unretained(other_thread)));
       on_memory_dump_call_count++;
       return true;
     };
@@ -1086,8 +1109,8 @@ TEST_F(MemoryDumpManagerTest, UnregisterAndDeleteDumpProviderSoonDuringDump) {
       const MemoryDumpArgs&, ProcessMemoryDump*) -> bool {
     thread_ref = PlatformThread::CurrentRef();
     TestIOThread thread_for_unregistration(TestIOThread::kAutoStart);
-    thread_for_unregistration.PostTaskAndWait(
-        FROM_HERE,
+    PostTaskAndWait(
+        FROM_HERE, thread_for_unregistration.task_runner().get(),
         base::Bind(
             &MemoryDumpManager::UnregisterAndDeleteDumpProviderSoon,
             base::Unretained(MemoryDumpManager::GetInstance()),