[tracing] Introduce "allowed_dump_modes" for memory dump config

base/trace_event/memory_dump_manager.h

Index: base/trace_event/memory_dump_manager.h
diff --git a/base/trace_event/memory_dump_manager.h b/base/trace_event/memory_dump_manager.h
index 2b1bb01f7677b22dfcd56ed31c16e50a48d10b7a..c4a296c1c3581aff43c439ab858a45f4e406415f 100644
--- a/base/trace_event/memory_dump_manager.h
+++ b/base/trace_event/memory_dump_manager.h
@@ -115,6 +115,11 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
   void OnTraceLogEnabled() override;
   void OnTraceLogDisabled() override;
 
+  // Returns true if the dump mode is allowed for current tracing session.
+  bool IsDumpModeAllowed(MemoryDumpLevelOfDetail dump_mode) {
+    return allowed_dump_modes_.find(dump_mode) != allowed_dump_modes_.end();

[Primiano Tucci (use gerrit), 2016/06/08 15:48:30]

- Let's move this to the .cc file, there is no need to inline this.
- Can we make this such as: if the set if empty, then everything is allowed,
otherwise only what's in the set?
- I think .count(dump_mode) != 0 is a bit more legible and probably even more
performant, as it doesn't have to return an iterator.

[ssid, 2016/06/08 18:04:17]

Fixed.
This is already done in trace_config file where this kind of default behavior is
set for periodic dumps. Please see comment in trace_config.cc#545.
Done.

+  }
+
   // Returns the MemoryDumpSessionState object, which is shared by all the
   // ProcessMemoryDump and MemoryAllocatorDump instances through all the tracing
   // session lifetime.
@@ -275,7 +280,7 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
   // the dump triggers from trace config.
   class BASE_EXPORT PeriodicGlobalDumpTimer {
    public:
-    PeriodicGlobalDumpTimer();
+    explicit PeriodicGlobalDumpTimer(MemoryDumpManager* mdm);

[Primiano Tucci (use gerrit), 2016/06/08 15:48:30]

uh why cannot we rely on MDM being a singleton?

[ssid, 2016/06/08 18:04:17]

I did not want to do MDM::GetInstance() multiple times at each timer call also
makes code longer . If you prefer keeping Get calls here, i will remove this
change.

[Primiano Tucci (use gerrit), 2016/06/09 18:46:39]

GetInstance is pretty fast and generally nothing compared to the cost of all the
other things we do to create our dumps. I think it just makes it the code easier
to read.

     ~PeriodicGlobalDumpTimer();
 
     void Start(const std::vector<TraceConfig::MemoryDumpConfig::Trigger>&
@@ -288,6 +293,8 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
     // Periodically called by the timer.
     void RequestPeriodicGlobalDump();
 
+    MemoryDumpManager* mdm_;
+
     RepeatingTimer timer_;
     uint32_t periodic_dumps_count_;
     uint32_t light_dump_rate_;
@@ -350,6 +357,10 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
   // When true, this instance is in charge of coordinating periodic dumps.
   bool is_coordinator_;
 
+  // Set of dump modes allowed for the current tracing session by the trace
+  // config.
+  std::set<MemoryDumpLevelOfDetail> allowed_dump_modes_;

[Primiano Tucci (use gerrit), 2016/06/08 15:48:30]

Can me put ths into the SessionState. Every time in the past we tried to add
state to MDM we ended up with some bug on corner cases when stopping/starting
trace. With SessionState we know that everything is inside there, so either all
the session state logic is buggy or not.
(And when I say "we ended up in bug" I mean mostly code that I wrote myself :) )

Actually wait, it's already there: the sessionstate contains a copy of the
traceconfig. The traceconfig contains the allowed dump modes.

[ssid, 2016/06/08 18:04:17]

Sorry, fixed this! I should be reading session state here.

+
   // Protects from concurrent accesses to the |dump_providers_*| and |delegate_|
   // to guard against disabling logging while dumping on another thread.
   Lock lock_;