[memory-infra] Add support for queueing memory dump requests in the browser process

content/browser/tracing/memory_tracing_browsertest.cc

Index: content/browser/tracing/memory_tracing_browsertest.cc
diff --git a/content/browser/tracing/memory_tracing_browsertest.cc b/content/browser/tracing/memory_tracing_browsertest.cc
index c2cafd80d0de503e8bee43c5bbbe711878b6685f..9150376977bd4ea445f068616aa26d69f70767d8 100644
--- a/content/browser/tracing/memory_tracing_browsertest.cc
+++ b/content/browser/tracing/memory_tracing_browsertest.cc
@@ -7,6 +7,8 @@
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/run_loop.h"
+#include "base/synchronization/waitable_event.h"
+#include "base/threading/thread.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/trace_event/memory_dump_manager.h"
 #include "base/trace_event/memory_dump_provider.h"
@@ -25,10 +27,15 @@ using base::trace_event::MemoryDumpManager;
 using base::trace_event::MemoryDumpType;
 using base::trace_event::ProcessMemoryDump;
 using testing::_;
+using testing::DoAll;
 using testing::Return;
 
 namespace content {
 
+ACTION_P(WaitEvent, event) {
+  event->Wait();
+}
+
 // A mock dump provider, used to check that dump requests actually end up
 // creating memory dumps.
 class MockDumpProvider : public base::trace_event::MemoryDumpProvider {
@@ -72,9 +79,13 @@ class MemoryTracingTest : public ContentBrowserTest {
     last_callback_dump_guid_ = 0;
     last_callback_success_ = false;
 
+    mock_dump_provider_thread_.reset(
+        new base::Thread("MockDumpProvider thread"));
+    EXPECT_TRUE(mock_dump_provider_thread_->Start());
     mock_dump_provider_.reset(new MockDumpProvider());
     MemoryDumpManager::GetInstance()->RegisterDumpProvider(
-        mock_dump_provider_.get(), "MockDumpProvider", nullptr);
+        mock_dump_provider_.get(), "MockDumpProvider",
+        mock_dump_provider_thread_->task_runner());
     MemoryDumpManager::GetInstance()
         ->set_dumper_registrations_ignored_for_testing(false);
     ContentBrowserTest::SetUp();
@@ -84,6 +95,7 @@ class MemoryTracingTest : public ContentBrowserTest {
     MemoryDumpManager::GetInstance()->UnregisterDumpProvider(
         mock_dump_provider_.get());
     mock_dump_provider_.reset();
+    mock_dump_provider_thread_.reset();
     ContentBrowserTest::TearDown();
   }
 
@@ -106,11 +118,11 @@ class MemoryTracingTest : public ContentBrowserTest {
     base::RunLoop().RunUntilIdle();
   }
 
-  void RequestGlobalDumpAndWait(bool from_renderer_thread) {
-    base::RunLoop run_loop;
+  void RequestGlobalDumpWhichQuitsLoop(bool from_renderer_thread,
+                                       base::RunLoop* run_loop) {
     base::trace_event::MemoryDumpCallback callback = base::Bind(
         &MemoryTracingTest::OnGlobalMemoryDumpDone, base::Unretained(this),
-        base::ThreadTaskRunnerHandle::Get(), run_loop.QuitClosure());
+        base::ThreadTaskRunnerHandle::Get(), run_loop->QuitClosure());
     if (from_renderer_thread) {
       PostTaskToInProcessRendererAndWait(
           base::Bind(&MemoryTracingTest::DoRequestGlobalDump,
@@ -118,6 +130,11 @@ class MemoryTracingTest : public ContentBrowserTest {
     } else {
       DoRequestGlobalDump(callback);
     }
+  }
+
+  void RequestGlobalDumpAndWait(bool from_renderer_thread) {
+    base::RunLoop run_loop;
+    RequestGlobalDumpWhichQuitsLoop(from_renderer_thread, &run_loop);
     run_loop.Run();
   }
 
@@ -126,6 +143,7 @@ class MemoryTracingTest : public ContentBrowserTest {
   }
 
   base::Closure on_memory_dump_complete_closure_;
+  std::unique_ptr<base::Thread> mock_dump_provider_thread_;
   std::unique_ptr<MockDumpProvider> mock_dump_provider_;
   uint32_t callback_call_count_;
   uint64_t last_callback_dump_guid_;
@@ -206,6 +224,57 @@ IN_PROC_BROWSER_TEST_F(SingleProcessMemoryTracingTest, ManyInterleavedDumps) {
   DisableTracing();
 }
 
+IN_PROC_BROWSER_TEST_F(SingleProcessMemoryTracingTest, EnqueuedDumps) {
+  Navigate(shell());
+
+  EnableMemoryTracing();
+
+  base::RunLoop loop1;

[Primiano Tucci (use gerrit), 2016/06/16 08:37:23]

never seen this pattern, why do you need 4 run loops?.
I think you want four scopes { RunLoop loop; .... loop.run() }

Also I think you don't need all this cmplexity. As long as you have one MDP 
bound to the same thread, that will necessarily prevent the dump to complete
before you finish the current task (because it depends on task posted next)

[petrcermak, 2016/06/16 12:08:29]

The problem is that I need to do the following:

 REQUEST1 ---------------------------> FINISHED1
             REQUEST2 -----------------------------> FINISHED2
                                 ^
                                 :
Because I'm interested in this ..:

Therefore, the run loops need to overlap.

+  base::RunLoop loop2;
+  base::RunLoop loop3;
+  base::RunLoop loop4;
+
+  // We force all dumps to wait on a waitable event to make sure that they are
+  // indeed queued.
+  base::WaitableEvent event(base::WaitableEvent::ResetPolicy::AUTOMATIC,

[Primiano Tucci (use gerrit), 2016/06/16 08:37:23]

I think you don't need this waitable event if you just have one MDP bound to the
same thread of this (i.e. ThreadTaskRunnerHandle::Get()

[petrcermak, 2016/06/16 12:08:29]

OK. Just to check: Am I guaranteed that a dump doesn't finish (from
TraceController's point of view) before RequestGlobalMemoryDump exits?

+                            base::WaitableEvent::InitialState::NOT_SIGNALED);
+  EXPECT_CALL(*mock_dump_provider_, OnMemoryDump(_, _))
+      .Times(4)
+      .WillRepeatedly(DoAll(WaitEvent(&event), Return(true)));
+
+  RequestGlobalDumpWhichQuitsLoop(false /* from_renderer_thread */, &loop1);
+  RequestGlobalDumpWhichQuitsLoop(true /* from_renderer_thread */, &loop2);
+  RequestGlobalDumpWhichQuitsLoop(false /* from_renderer_thread */, &loop3);
+  RequestGlobalDumpWhichQuitsLoop(true /* from_renderer_thread */, &loop4);
+  EXPECT_EQ(0u, callback_call_count_);
+
+  event.Signal();
+  loop1.Run();
+  EXPECT_EQ(1u, callback_call_count_);
+  EXPECT_NE(0u, last_callback_dump_guid_);
+  EXPECT_TRUE(last_callback_success_);
+
+  event.Signal();
+  loop2.Run();
+  EXPECT_EQ(2u, callback_call_count_);
+  EXPECT_NE(0u, last_callback_dump_guid_);
+  EXPECT_TRUE(last_callback_success_);
+
+  event.Signal();
+  loop3.Run();
+  EXPECT_EQ(3u, callback_call_count_);
+  EXPECT_NE(0u, last_callback_dump_guid_);
+  EXPECT_TRUE(last_callback_success_);
+
+  event.Signal();
+  loop4.Run();
+  EXPECT_EQ(4u, callback_call_count_);
+  EXPECT_NE(0u, last_callback_dump_guid_);
+  EXPECT_TRUE(last_callback_success_);
+
+  DisableTracing();
+}
+
 #endif  // !defined(GOOGLE_CHROME_BUILD)
 
 // Non-deterministic races under TSan. crbug.com/529678