Fix race condition when dump providers invoked within destroyed state

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 <stdint.h>
 
 #include <memory>
 #include <vector>
@@ skipping 96 matching lines @@
     return MemoryDumpManager::kInvalidTracingProcessId;
   }
 
   // Promote the CreateProcessDump to public so it can be used by test fixtures.
   using MemoryDumpManagerDelegate::CreateProcessDump;
 };
 
 class MockMemoryDumpProvider : public MemoryDumpProvider {
  public:
   MOCK_METHOD0(Destructor, void());
   MOCK_METHOD2(OnMemoryDump,

[Primiano Tucci (use gerrit), 2016/04/13 17:08:41]

Nothing in this test is checking that the session_state is never null, which is
the thing you are fixing in this bug.
Can you add a default mock implementation to this OnMemoryDump which does the
assert.
See the WillByDefault() above for the pattern to use.

[kraynov, 2016/04/13 17:17:41]

Acknowledged.

                bool(const MemoryDumpArgs& args, ProcessMemoryDump* pmd));
 
   MockMemoryDumpProvider() : enable_mock_destructor(false) {}
   ~MockMemoryDumpProvider() override {
     if (enable_mock_destructor)
       Destructor();
   }
 
   bool enable_mock_destructor;
 };
@@ skipping 803 matching lines @@
 
   EXPECT_FALSE(last_callback_success_);
 }
 
 // Tests against race conditions that can happen if tracing is disabled before
 // the CreateProcessDump() call. Real-world regression: crbug.com/580295 .
 TEST_F(MemoryDumpManagerTest, DisableTracingRightBeforeStartOfDump) {
   base::WaitableEvent tracing_disabled_event(false, false);
   InitializeMemoryDumpManager(false /* is_coordinator */);
 
+  auto thread = WrapUnique(new Thread("test thread"));

[Primiano Tucci (use gerrit), 2016/04/13 17:08:41]

I think we don't allow yet auto with WrapUnique. 
Also, there should be no need of WrapUnique here, just:
std::unique_ptr<Thread> thread(new Thread(...)) right?

[kraynov, 2016/04/13 17:17:41]

Acknowledged.

+  thread->Start();
+
   MockMemoryDumpProvider mdp;
-  RegisterDumpProvider(&mdp);
+  RegisterDumpProvider(&mdp, thread->task_runner(), kDefaultOptions);

[Primiano Tucci (use gerrit), 2016/04/13 17:08:41]

hmm here you are overriding the previous behavior. 
I would much rather add two mdp, one not bound to any thread and one bound to
|thread| here.

[kraynov, 2016/04/13 17:17:41]

Acknowledged.

   EnableTracingWithLegacyCategories(MemoryDumpManager::kTraceCategory);
 
   EXPECT_CALL(*delegate_, RequestGlobalMemoryDump(_, _))
       .WillOnce(Invoke([this](const MemoryDumpRequestArgs& args,
                               const MemoryDumpCallback& callback) {
         DisableTracing();
         delegate_->CreateProcessDump(args, callback);
       }));
 
+  // If tracing is disabled for current session CreateProcessDump() should NOT
+  // request dumps from providers. Real-world regression: crbug.com/600570 .
+  EXPECT_CALL(mdp, OnMemoryDump(_, _)).Times(0);
+
   last_callback_success_ = true;
   RequestGlobalDumpAndWait(MemoryDumpType::EXPLICITLY_TRIGGERED,
                            MemoryDumpLevelOfDetail::DETAILED);
   EXPECT_FALSE(last_callback_success_);
 }
 
 TEST_F(MemoryDumpManagerTest, DumpOnBehalfOfOtherProcess) {
   using trace_analyzer::Query;
 
   InitializeMemoryDumpManager(false /* is_coordinator */);
@@ skipping 111 matching lines @@
   EXPECT_CALL(*delegate_, RequestGlobalMemoryDump(_, _)).Times(2);
   for (int i = 0; i < 2; ++i) {
     RequestGlobalDumpAndWait(MemoryDumpType::EXPLICITLY_TRIGGERED,
                              MemoryDumpLevelOfDetail::DETAILED);
   }
   DisableTracing();
 }
 
 }  // namespace trace_event
 }  // namespace base