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::AtMost;
 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();
 }
 
+// Verify that whether OnMemoryDump is called depends only on the current
+// registration state and not on previous registrations and dumps.
+TEST_F(MemoryDumpManagerTest, RegistrationConsistency) {
+  MockDumpProvider mdp;
+  EnableTracing(kTraceCategory);
+
+  mdm_->RegisterDumpProvider(&mdp);
+
+  EXPECT_CALL(mdp, OnMemoryDump(_, _)).Times(1);

[Primiano Tucci (use gerrit), 2015/08/24 09:31:19]

I wonder if you should put all these macro-steps into braces, in order to limit
the scope of EXPECT_CALL and give it the semantic you intend.
In other words, I'm not sure that

EXPECT_CALL(Foo).Times(1)
do_something()

EXPECT_CALL(Foo).Times(0)
do_something()

is equivalent to 
{
  EXPECT_CALL(Foo).Times(1)
  do_something()
}
{
  EXPECT_CALL(Foo).Times(0)
  do_something()
}

[Ruud van Asseldonk, 2015/08/24 10:08:37]

As I understand from the documentation, in this case a call to OnMemoryDump
should trigger the most recently set expectation. Still, it won’t hurt to be
more explicit so I added the scopes.

+  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

[Primiano Tucci (use gerrit), 2015/08/24 09:31:19]

s/during dump/while dumping/

[Ruud van Asseldonk, 2015/08/24 10:08:37]

Done.

+// 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_call_count = 0;
+  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/24 09:31:19]

According to our C++11 codestyle [1], "Don't use default captures ([=], [&] "

[1] https://chromium-cpp.appspot.com/

[Ruud van Asseldonk, 2015/08/24 10:08:37]

Done.

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

[Primiano Tucci (use gerrit), 2015/08/24 09:31:19]

shouldn't you set this to false to make sure you actually get a callback that
sets it to true?

[Ruud van Asseldonk, 2015/08/24 10:08:37]

Definitely, good catch.

+  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_call_count);
+  ASSERT_EQ(true, last_callback_success_);
+
+  DisableTracing();
+}
+
 // 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