Allow unregistering MemoryDumpProviders during dump

base/trace_event/memory_dump_manager_unittest.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/trace_event/memory_dump_manager.h"
 
 #include "base/bind_helpers.h"
 #include "base/memory/scoped_vector.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
+#include "base/test/test_io_thread.h"
 #include "base/thread_task_runner_handle.h"
 #include "base/threading/thread.h"
 #include "base/trace_event/memory_dump_provider.h"
 #include "base/trace_event/process_memory_dump.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using testing::_;
+using testing::AtLeast;
 using testing::Between;
 using testing::Invoke;
 using testing::Return;
 
 namespace base {
 namespace trace_event {
 namespace {
 MemoryDumpArgs high_detail_args = {MemoryDumpArgs::LevelOfDetail::HIGH};
 MemoryDumpArgs low_detail_args = {MemoryDumpArgs::LevelOfDetail::LOW};
 }
@@ skipping 246 matching lines @@
   // Enable both mdp1 and mdp2.
   mdm_->RegisterDumpProvider(&mdp1);
   EnableTracing(kTraceCategory);
   EXPECT_CALL(mdp1, OnMemoryDump(_, _)).Times(1).WillRepeatedly(Return(true));
   EXPECT_CALL(mdp2, OnMemoryDump(_, _)).Times(1).WillRepeatedly(Return(true));
   mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
                           high_detail_args);
   DisableTracing();
 }
 
+// Even though internally, after unregistration a dump provider is retained in

[Primiano Tucci (use gerrit), 2015/08/21 09:00:31]

This comment seems to describe the internal implementation of MDM. Instead it
should reflect what this test is doing and what is protecting from.
How about something like:
"Checks that Unregistration of a MemoryDumpProvider has always effect regardless
of the state of the trace and ongoing dumps"

And I'd probably name the test "RegistrationConsistency" as you aren't really
checking any ordering here, right? (unless I'm misreading this)

[Ruud van Asseldonk, 2015/08/21 10:51:40]

Done.

+// the set until the next dump, this should not affect dump behavior.
+TEST_F(MemoryDumpManagerTest, RegistrationOrder) {
+  MockDumpProvider mdp;
+  EnableTracing(kTraceCategory);
+
+  mdm_->RegisterDumpProvider(&mdp);
+
+  EXPECT_CALL(mdp, OnMemoryDump(_, _)).Times(1);
+  EnableTracing(kTraceCategory);
+  mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
+                          high_detail_args);
+  DisableTracing();
+
+  mdm_->UnregisterDumpProvider(&mdp);
+
+  EXPECT_CALL(mdp, OnMemoryDump(_, _)).Times(0);
+  EnableTracing(kTraceCategory);
+  mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
+                          high_detail_args);
+  DisableTracing();
+
+  mdm_->RegisterDumpProvider(&mdp);
+  mdm_->UnregisterDumpProvider(&mdp);
+
+  EXPECT_CALL(mdp, OnMemoryDump(_, _)).Times(0);
+  EnableTracing(kTraceCategory);
+  mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
+                          high_detail_args);
+  DisableTracing();
+
+  mdm_->RegisterDumpProvider(&mdp);
+  mdm_->UnregisterDumpProvider(&mdp);
+  mdm_->RegisterDumpProvider(&mdp);
+
+  EXPECT_CALL(mdp, OnMemoryDump(_, _)).Times(1);
+  EnableTracing(kTraceCategory);
+  mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
+                          high_detail_args);
+  DisableTracing();
+}
+
 // Checks that the MemoryDumpManager respects the thread affinity when a
 // MemoryDumpProvider specifies a task_runner(). The test starts creating 8
 // threads and registering a MemoryDumpProvider on each of them. At each
 // iteration, one thread is removed, to check the live unregistration logic.
 TEST_F(MemoryDumpManagerTest, RespectTaskRunnerAffinity) {
   const uint32 kNumInitialThreads = 8;
 
   ScopedVector<Thread> threads;
   ScopedVector<MockDumpProvider> mdps;
 
@@ skipping 131 matching lines @@
       .WillRepeatedly(Return(true));
 
   for (int i = 0; i < 4; i++) {
     mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
                             high_detail_args);
   }
 
   DisableTracing();
 }
 
+// Verify that the dump does not abort when unregistering a provider during dump
+// from a different thread than the dumping thread.
+TEST_F(MemoryDumpManagerTest, UnregisterDumperFromThreadWhileDumping) {
+  ScopedVector<TestIOThread> threads;
+  ScopedVector<MockDumpProvider> mdps;
+
+  for (int i = 0; i < 2; i++) {
+    threads.push_back(new TestIOThread(TestIOThread::kAutoStart));
+    mdps.push_back(new MockDumpProvider(threads.back()->task_runner()));
+    mdm_->RegisterDumpProvider(mdps.back(), threads.back()->task_runner());
+  }
+
+  int on_memory_dump_calls = 0;

[Primiano Tucci (use gerrit), 2015/08/21 09:00:31]

nit: +count (i.e. on_memory_dump_call_count)

[Ruud van Asseldonk, 2015/08/21 10:51:40]

Done.

+  RunLoop run_loop;
+
+  // When OnMemoryDump is called on either of the dump providers, it will
+  // unregister the other one.
+  for (MockDumpProvider* mdp : mdps) {
+    int other_idx = (mdps.front() == mdp);
+    auto on_dump = [&, other_idx](const MemoryDumpArgs& args,

[Primiano Tucci (use gerrit), 2015/08/21 09:00:31]

nit: on_memory_dump

Also, make sure you run git cl format on this, the formatting of this line looks
a bit strange (but might be correct)

[Ruud van Asseldonk, 2015/08/21 10:51:40]

This is how git cl format formats it.

+                                  ProcessMemoryDump* pmd) {
+
+      threads[other_idx]->PostTaskAndWait(
+          FROM_HERE, base::Bind(&MemoryDumpManager::UnregisterDumpProvider,
+                                base::Unretained(&*mdm_), mdps[other_idx]));
+      on_memory_dump_calls++;
+      return true;
+    };
+
+    // OnMemoryDump is called once for the provider that dumps first, and zero
+    // times for the other provider.
+    EXPECT_CALL(*mdp, OnMemoryDump(_, _))
+        .Times(AtLeast(0))

[Primiano Tucci (use gerrit), 2015/08/21 09:00:31]

I'd probably make this AtMost(1)

[Ruud van Asseldonk, 2015/08/21 10:51:40]

Done.

+        .WillOnce(Invoke(on_dump));
+  }
+
+  last_callback_success_ = true;
+  MemoryDumpCallback callback =
+      Bind(&MemoryDumpManagerTest::DumpCallbackAdapter, Unretained(this),
+           MessageLoop::current()->task_runner(), run_loop.QuitClosure());
+
+  EnableTracing(kTraceCategory);
+  MemoryDumpRequestArgs request_args = {0, MemoryDumpType::EXPLICITLY_TRIGGERED,
+                                        high_detail_args};
+  mdm_->CreateProcessDump(request_args, callback);
+
+  run_loop.Run();
+
+  ASSERT_EQ(1, on_memory_dump_calls);
+  ASSERT_EQ(true, last_callback_success_);
+
+  DisableTracing();
+
+  // Thread destructors will stop the threads.

[Primiano Tucci (use gerrit), 2015/08/21 09:00:31]

I'd probably remove this comment. Should be clear from the "Scoped" name

[Ruud van Asseldonk, 2015/08/21 10:51:40]

Done.

+}
+
 // Ensures that a NACK callback is invoked if RequestGlobalDump is called when
 // tracing is not enabled.
 TEST_F(MemoryDumpManagerTest, CallbackCalledOnFailure) {
   MockDumpProvider mdp1;
 
   mdm_->RegisterDumpProvider(&mdp1);
   EXPECT_CALL(mdp1, OnMemoryDump(_, _)).Times(0);
 
   last_callback_success_ = true;
   {
     RunLoop run_loop;
     MemoryDumpCallback callback =
         Bind(&MemoryDumpManagerTest::DumpCallbackAdapter, Unretained(this),
              MessageLoop::current()->task_runner(), run_loop.QuitClosure());
     mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
                             high_detail_args, callback);
     run_loop.Run();
   }
   EXPECT_FALSE(last_callback_success_);
 }
 
 }  // namespace trace_event
 }  // namespace base