memory-infra: Move dump level check to observer and rename session 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 <utility>
@@ skipping 145 matching lines @@
   MOCK_METHOD0(SuspendFastMemoryPolling, void());
 
   MockMemoryDumpProvider() : enable_mock_destructor(false) {
     ON_CALL(*this, OnMemoryDump(_, _))
         .WillByDefault(Invoke([](const MemoryDumpArgs&,
                                  ProcessMemoryDump* pmd) -> bool {
           // |session_state| should not be null under any circumstances when
           // invoking a memory dump. The problem might arise in race conditions
           // like crbug.com/600570 .
           EXPECT_TRUE(pmd->session_state().get() != nullptr);
+          pmd->CreateAllocatorDump("example")->AddScalar(

[Primiano Tucci (use gerrit), 2017/05/05 13:38:25]

do you need this or is this the leftover from some local debugging?

[hjd, 2017/05/05 14:50:53]

Yeah, removed. Thanks!

+              MemoryAllocatorDump::kNameSize, MemoryAllocatorDump::kUnitsBytes,
+              1024);
           return true;
         }));
 
     ON_CALL(*this, PollFastMemoryTotal(_))
         .WillByDefault(
             Invoke([](uint64_t* memory_total) -> void { NOTREACHED(); }));
   }
   ~MockMemoryDumpProvider() override {
     if (enable_mock_destructor)
       Destructor();
@@ skipping 224 matching lines @@
   RegisterDumpProvider(&mdp, ThreadTaskRunnerHandle::Get());
   EnableTracingWithLegacyCategories(MemoryDumpManager::kTraceCategory);
   EXPECT_CALL(global_dump_handler_, RequestGlobalMemoryDump(_, _)).Times(1);
   EXPECT_CALL(mdp, OnMemoryDump(IsLightDump(), _)).WillOnce(Return(true));
   RequestGlobalDumpAndWait(MemoryDumpType::EXPLICITLY_TRIGGERED,
                            MemoryDumpLevelOfDetail::LIGHT);
   DisableTracing();
   mdm_->UnregisterDumpProvider(&mdp);
 }
 
 // Checks that the SharedSessionState object is acqually shared over time.

[Primiano Tucci (use gerrit), 2017/05/05 13:38:25]

update the comment and the name of the TEST_F

[hjd, 2017/05/05 14:50:53]

Done.

 TEST_F(MemoryDumpManagerTest, SharedSessionState) {
   InitializeMemoryDumpManager(false /* is_coordinator */);
   MockMemoryDumpProvider mdp1;
   MockMemoryDumpProvider mdp2;
   RegisterDumpProvider(&mdp1, nullptr);
   RegisterDumpProvider(&mdp2, nullptr);
 
   EnableTracingWithLegacyCategories(MemoryDumpManager::kTraceCategory);
-  const MemoryDumpSessionState* session_state =
-      mdm_->session_state_for_testing().get();
+  const MemoryDumpSessionState* heap_profiler_serialization_state =
+      mdm_->heap_profiler_serialization_state_for_testing().get();
   EXPECT_CALL(global_dump_handler_, RequestGlobalMemoryDump(_, _)).Times(2);
   EXPECT_CALL(mdp1, OnMemoryDump(_, _))
       .Times(2)
-      .WillRepeatedly(Invoke([session_state](const MemoryDumpArgs&,
-                                             ProcessMemoryDump* pmd) -> bool {
-        EXPECT_EQ(session_state, pmd->session_state().get());
-        return true;
-      }));
+      .WillRepeatedly(
+          Invoke([heap_profiler_serialization_state](
+                     const MemoryDumpArgs&, ProcessMemoryDump* pmd) -> bool {
+            EXPECT_EQ(heap_profiler_serialization_state,
+                      pmd->session_state().get());
+            return true;
+          }));
   EXPECT_CALL(mdp2, OnMemoryDump(_, _))
       .Times(2)
-      .WillRepeatedly(Invoke([session_state](const MemoryDumpArgs&,
-                                             ProcessMemoryDump* pmd) -> bool {
-        EXPECT_EQ(session_state, pmd->session_state().get());
-        return true;
-      }));
+      .WillRepeatedly(
+          Invoke([heap_profiler_serialization_state](
+                     const MemoryDumpArgs&, ProcessMemoryDump* pmd) -> bool {
+            EXPECT_EQ(heap_profiler_serialization_state,
+                      pmd->session_state().get());
+            return true;
+          }));
 
   for (int i = 0; i < 2; ++i) {
     RequestGlobalDumpAndWait(MemoryDumpType::EXPLICITLY_TRIGGERED,
                              MemoryDumpLevelOfDetail::DETAILED);
   }
 
   DisableTracing();
 }
 
 // Checks that the (Un)RegisterDumpProvider logic behaves sanely.
@@ skipping 797 matching lines @@
   EnableTracingWithLegacyCategories(MemoryDumpManager::kTraceCategory);
   EXPECT_FALSE(IsPeriodicDumpingEnabled());
   RequestGlobalDumpAndWait(MemoryDumpType::EXPLICITLY_TRIGGERED,
                            MemoryDumpLevelOfDetail::BACKGROUND);
   DisableTracing();
 }
 
 TEST_F(MemoryDumpManagerTest, TestBackgroundTracingSetup) {
   InitializeMemoryDumpManager(true /* is_coordinator */);
 
+  MockMemoryDumpProvider mdp;
+  RegisterDumpProvider(&mdp, ThreadTaskRunnerHandle::Get());

[hjd, 2017/05/05 11:42:27]

We now need an mdp to hit the code path where the dump will be rejected now
(since rejection happens just before serialization.

[Primiano Tucci (use gerrit), 2017/05/05 13:38:25]

Acknowledged. Maybe add a comment to explain this in the code?

+
   RunLoop run_loop;
   auto test_task_runner = ThreadTaskRunnerHandle::Get();
   auto quit_closure = run_loop.QuitClosure();
 
   testing::InSequence sequence;
   EXPECT_CALL(global_dump_handler_,
               RequestGlobalMemoryDump(IsBackgroundDump(), _))
       .Times(5);
   EXPECT_CALL(global_dump_handler_,
               RequestGlobalMemoryDump(IsBackgroundDump(), _))
       .WillOnce(Invoke([test_task_runner, quit_closure](
                            const MemoryDumpRequestArgs& args,
                            const GlobalMemoryDumpCallback& callback) {
         test_task_runner->PostTask(FROM_HERE, quit_closure);
       }));
   EXPECT_CALL(global_dump_handler_, RequestGlobalMemoryDump(_, _))
       .Times(AnyNumber());
 
   EnableTracingWithTraceConfig(
       TraceConfigMemoryTestUtil::GetTraceConfig_BackgroundTrigger(
           1 /* period_ms */));
 
+  run_loop.Run();

[hjd, 2017/05/05 11:42:27]

Previously this was racy, we relied on the 6 background dumps executing before
the light and detailed dumps.

[Primiano Tucci (use gerrit), 2017/05/05 13:38:25]

Well I think that became just racy now. Before we wouldn't have started the
dumps, so we would have seen only the background dumps. Instead now they would
be interleaved (because we nack only after reaching the serialization point), so
this is now required.

[hjd, 2017/05/05 14:50:53]

On master if I run the test on its own it hangs forever about 50% of the time.
(Although after your explanation I'm not sure why.)

+
   // Only background mode dumps should be allowed with the trace config.
   last_callback_success_ = false;
   RequestGlobalDumpAndWait(MemoryDumpType::EXPLICITLY_TRIGGERED,
                            MemoryDumpLevelOfDetail::LIGHT);
   EXPECT_FALSE(last_callback_success_);
   last_callback_success_ = false;
   RequestGlobalDumpAndWait(MemoryDumpType::EXPLICITLY_TRIGGERED,
                            MemoryDumpLevelOfDetail::DETAILED);
   EXPECT_FALSE(last_callback_success_);
 
   ASSERT_TRUE(IsPeriodicDumpingEnabled());
-  run_loop.Run();
   DisableTracing();
 }
 
 // Tests that we can manually take a dump without enabling tracing.
 TEST_F(MemoryDumpManagerTest, DumpWithTracingDisabled) {
   InitializeMemoryDumpManager(false /* is_coordinator */);
   MockMemoryDumpProvider mdp;
   RegisterDumpProvider(&mdp, ThreadTaskRunnerHandle::Get());
 
   DisableTracing();
@@ skipping 95 matching lines @@
   EXPECT_EQ(3u, result.chrome_dump.v8_total_kb);
   // partition_alloc has partition_alloc/allocated_objects/* which is a subset
   // of partition_alloc/partitions/* so we only count the latter.
   EXPECT_EQ(4u, result.chrome_dump.partition_alloc_total_kb);
   // resident_set_kb should read from process_totals.
   EXPECT_EQ(5u, result.os_dump.resident_set_kb);
 };
 
 }  // namespace trace_event
 }  // namespace base