[tracing] Introduce "allowed_dump_modes" for memory dump config

content/browser/tracing/tracing_controller_impl.cc

 // Copyright (c) 2013 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 "content/browser/tracing/tracing_controller_impl.h"
 
 #include "base/bind.h"
 #include "base/cpu.h"
 #include "base/files/file_util.h"
 #include "base/guid.h"
 #include "base/json/string_escape.h"
@@ skipping 895 matching lines @@
   // Stop tracing only if all agents report back.
   if(--pending_clock_sync_ack_count_ == 0) {
     clock_sync_timer_.Stop();
     StopTracingAfterClockSync();
   }
 }
 
 void TracingControllerImpl::RequestGlobalMemoryDump(
     const base::trace_event::MemoryDumpRequestArgs& args,
     const base::trace_event::MemoryDumpCallback& callback) {
+  if (!base::trace_event::MemoryDumpManager::GetInstance()->IsDumpModeAllowed(

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

why do we need this extra check here? Can we let it fail only in one place?
Is this only a perf optimization?

[ssid, 2016/06/09 21:34:15]

It is a public method of TracingControllerImpl and anyone outside codebase can
get instance and trigger dump. I was also paranoid about some child process
sending the dump request ipc message. Essentially this method could be called
without MDM.

[Primiano Tucci (use gerrit), 2016/06/10 12:31:57]

Yes what I am saying is that this checks seems superfluos. Even if you pass this
you are going to hit the MDM that will NACK the dump few moments later. Also you
should never just return without invoking the callback.

Just remove these two lines. in the worst case if a child process send the IPC
and we get here, they will receive a nack. (unless I'm not thinking to something
here)

IIRC even in the case where a child process doesn't receive the traceconfig by
mistake, upon a call to RequestGlobalDump, the child process will not create the
local dump, but will wait for the browser to ask it do dump. In other word, even
for dumps initiated by the child process, the flow is:
ReqGLobalDUmp->(child)->RequestGlobalDump->(browser)->CreateLocalDump->(all
childs)

[ssid, 2016/06/10 18:23:47]

MDM can't nack the dump if someone calls
TracingControllerImpl::RequestGlobalMemoryDump. It assumes MDM has accepted the
dump at this point.
If I take skia and use:
TracingControllerImpl::GetInstance()->RequestGlobalMemoryDump(DETAILED)
This would still go through without checking MDM.
Yes, but this flow does not include browser MDM::RequestGlobalDump which
actually checks for allowed modes. This flow bypasses the MDM::Request of the
browser

[Primiano Tucci (use gerrit), 2016/06/13 19:56:54]

Ok fine. But let's instead  make this a CHECK in MDM::CreateProcessDump (on top
of the more graceful NACK in MDM::RequestGlobalDump).
Nobody should use TracingControllerImpl directly. But I agree with you that it
is technically possible, in which case we should just crash the world.

[ssid, 2016/06/14 01:26:11]

Done.

+          args.level_of_detail))
+    return;
+
   if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
     BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE,
         base::Bind(&TracingControllerImpl::RequestGlobalMemoryDump,
                    base::Unretained(this), args, callback));
     return;
   }
   // Abort if another dump is already in progress.
   if (pending_memory_dump_guid_) {
     DVLOG(1) << "Requested memory dump " << args.dump_guid
@@ skipping 100 matching lines @@
   const bool global_success = failed_memory_dump_count_ == 0;
   if (!pending_memory_dump_callback_.is_null()) {
     pending_memory_dump_callback_.Run(pending_memory_dump_guid_,
                                       global_success);
     pending_memory_dump_callback_.Reset();
   }
   pending_memory_dump_guid_ = 0;
 }
 
 }  // namespace content