[tracing] Allow asynchronous unregistration of unbound dump providers

base/trace_event/memory_dump_manager_unittest.cc

Index: base/trace_event/memory_dump_manager_unittest.cc
diff --git a/base/trace_event/memory_dump_manager_unittest.cc b/base/trace_event/memory_dump_manager_unittest.cc
index cf0f30574d2544a3919e0c26727b366ac5de129c..998235e0416b904e2f53165100d5f78415c384c0 100644
--- a/base/trace_event/memory_dump_manager_unittest.cc
+++ b/base/trace_event/memory_dump_manager_unittest.cc
@@ -15,6 +15,7 @@
 #include "base/test/test_io_thread.h"
 #include "base/test/trace_event_analyzer.h"
 #include "base/thread_task_runner_handle.h"
+#include "base/threading/platform_thread.h"
 #include "base/threading/thread.h"
 #include "base/trace_event/memory_dump_provider.h"
 #include "base/trace_event/process_memory_dump.h"
@@ -91,8 +92,17 @@ class MemoryDumpManagerDelegateForTesting : public MemoryDumpManagerDelegate {
 
 class MockMemoryDumpProvider : public MemoryDumpProvider {
  public:
+  MOCK_METHOD0(Destructor, void());
   MOCK_METHOD2(OnMemoryDump,
                bool(const MemoryDumpArgs& args, ProcessMemoryDump* pmd));
+
+  MockMemoryDumpProvider() : enable_mock_destructor(false) {}
+  ~MockMemoryDumpProvider() override {
+    if (enable_mock_destructor)
+      Destructor();
+  }
+
+  bool enable_mock_destructor;
 };
 
 class MemoryDumpManagerTest : public testing::Test {
@@ -875,5 +885,76 @@ TEST_F(MemoryDumpManagerTest, DumpOnBehalfOfOtherProcess) {
   ASSERT_EQ(events[0]->id, events[2]->id);
 }
 
+// Tests the basics of the UnregisterAndDeleteDumpProviderAsync(): the
+// unregistration should actually delete the providers and not leak them.
+TEST_F(MemoryDumpManagerTest, UnregisterAndDeleteDumpProviderAsync) {
+  InitializeMemoryDumpManager(false /* is_coordinator */);
+  static const int kNumProviders = 3;
+  int dtor_count = 0;
+  std::vector<scoped_ptr<MemoryDumpProvider>> mdps;
+  for (int i = 0; i < kNumProviders; ++i) {
+    scoped_ptr<MockMemoryDumpProvider> mdp(new MockMemoryDumpProvider);
+    mdp->enable_mock_destructor = true;
+    EXPECT_CALL(*mdp, Destructor())
+        .WillOnce(Invoke([&dtor_count]() -> void { dtor_count++; }));

[Ruud van Asseldonk, 2015/12/29 11:32:46]

The |-> void| part can be omitted.

[Primiano Tucci (use gerrit), 2016/01/04 14:44:38]

Did I already say  this week that I hate C++11 lambda syntax?
Done

+    RegisterDumpProvider(mdp.get());
+    mdps.push_back(std::move(mdp));
+  }
+
+  while (!mdps.empty()) {
+    mdm_->UnregisterAndDeleteDumpProviderAsync(std::move(mdps.back()));

[Ruud van Asseldonk, 2015/12/29 11:32:46]

So actually deletion is happening synchronously here?

[Primiano Tucci (use gerrit), 2016/01/04 14:44:38]

Ok, Ok. I see what you mean. I renamed the method to
UnregisterAndDeleteSoonDumpProvider

+    mdps.pop_back();
+  }
+
+  ASSERT_EQ(kNumProviders, dtor_count);
+}
+
+// This test checks against races when unregistering an unbound dump provider
+// from another thread while dumping. It register one MDP and when

[ssid, 2015/12/22 16:14:56]

s/register/registers/

[Primiano Tucci (use gerrit), 2016/01/04 14:44:38]

Done.

+// OnMemoryDump() is called, it invokes UnregisterAndDeleteDumpProviderAsync()
+// from another thread. The OnMemoryDump() and the dtor call are expected to
+// happen on the same thread (the MemoryDumpManager utility thread).
+TEST_F(MemoryDumpManagerTest, UnregisterAndDeleteDumpProviderAsyncDuringDump) {
+  InitializeMemoryDumpManager(false /* is_coordinator */);
+  scoped_ptr<MockMemoryDumpProvider> mdp(new MockMemoryDumpProvider);
+  mdp->enable_mock_destructor = true;
+  RegisterDumpProvider(mdp.get());
+
+  base::PlatformThreadRef thread_ref;
+  auto self_unregister_from_another_thread = [&mdp, &thread_ref](
+      const MemoryDumpArgs&, ProcessMemoryDump*) -> bool {
+    thread_ref = PlatformThread::CurrentRef();
+    TestIOThread thread_for_unregistration(TestIOThread::kAutoStart);
+    thread_for_unregistration.PostTaskAndWait(
+        FROM_HERE,
+        base::Bind(
+            &MemoryDumpManager::UnregisterAndDeleteDumpProviderAsync,
+            base::Unretained(MemoryDumpManager::GetInstance()),
+            base::Passed(scoped_ptr<MemoryDumpProvider>(std::move(mdp)))));

[Ruud van Asseldonk, 2015/12/29 11:32:46]

Why is the extra |scoped_ptr| temporary required? |mdp| is already a
|scoped_ptr|.

[Primiano Tucci (use gerrit), 2016/01/04 14:44:38]

I know but this is for upcasting from scoped_ptr<MockMDP> to scoped_ptr<MDP>.
The template magic doesn't align properly in this case with base::Passed and
doesn't hit automatically the cast operator.
Not sure if there is a better way to achieve this.

[Ruud van Asseldonk, 2016/01/04 15:43:01]

Ah, I see.

+    thread_for_unregistration.Stop();
+    return true;
+  };
+  EXPECT_CALL(*mdp, OnMemoryDump(_, _))
+      .Times(1)
+      .WillOnce(Invoke(self_unregister_from_another_thread));
+  EXPECT_CALL(*mdp, Destructor())
+      .Times(1)
+      .WillOnce(Invoke([&thread_ref]() -> void {

[Ruud van Asseldonk, 2015/12/29 11:32:46]

The |-> void| part can be omitted.

[Primiano Tucci (use gerrit), 2016/01/04 14:44:38]

Done.

+        EXPECT_EQ(thread_ref, PlatformThread::CurrentRef());
+      }));
+
+  EnableTracingWithLegacyCategories(MemoryDumpManager::kTraceCategory);
+  EXPECT_CALL(*delegate_, RequestGlobalMemoryDump(_, _)).Times(2);
+  for (int i = 0; i < 2; ++i) {
+    RequestGlobalDumpAndWait(MemoryDumpType::EXPLICITLY_TRIGGERED,
+                             MemoryDumpLevelOfDetail::DETAILED);
+  }
+  DisableTracing();
+
+  // Ownership of |mdp| should have been transferred to the MemoryDumpManager at
+  // the time UnregisterAndDeleteDumpProviderAsync() was called.
+  ASSERT_FALSE(mdp);

[Ruud van Asseldonk, 2015/12/29 11:32:46]

I would say |ASSERT_NE(nullptr, mdp.get())|, |mdp| is not a boolean.

Worse though, this is accessing a moved-from object. A moved-from object is
generally in an unspecified state. In this case it works, but this relies on an
implementation detail of |scoped_ptr| (it zeroing its pointer when it is moved
from).

I think you can assume |scoped_ptr| to work properly so this line can just be
removed, there already is an |EXPECT_CALL| for the destructor.

[Primiano Tucci (use gerrit), 2016/01/04 14:44:38]

I think you are right. What I am really doing here in this line is testing
Passed(scoped_ptr). but I shouldn't be doing this (At least not in this
unittest)

+}
+
 }  // namespace trace_event
 }  // namespace base