[memory-infra] Add support for queueing memory dump requests in the browser process

content/browser/tracing/tracing_controller_impl.cc

Index: content/browser/tracing/tracing_controller_impl.cc
diff --git a/content/browser/tracing/tracing_controller_impl.cc b/content/browser/tracing/tracing_controller_impl.cc
index 8380c45c44cd2924301ebd3b7e34f803daf23480..f3e43cc71a7019ab3f626d34077db3625673f428 100644
--- a/content/browser/tracing/tracing_controller_impl.cc
+++ b/content/browser/tracing/tracing_controller_impl.cc
@@ -561,6 +561,7 @@ void TracingControllerImpl::RemoveTraceMessageFilter(
     }
   }
   if (pending_memory_dump_ack_count_ > 0) {
+    DCHECK(!queued_memory_dump_requests_.empty());
     TraceMessageFilterSet::const_iterator it =
         pending_memory_dump_filters_.find(trace_message_filter);
     if (it != pending_memory_dump_filters_.end()) {
@@ -569,7 +570,8 @@ void TracingControllerImpl::RemoveTraceMessageFilter(
           base::Bind(&TracingControllerImpl::OnProcessMemoryDumpResponse,
                      base::Unretained(this),
                      base::RetainedRef(trace_message_filter),
-                     pending_memory_dump_guid_, false /* success */));
+                     queued_memory_dump_requests_.front().args.dump_guid,
+                     false /* success */));
     }
   }
   trace_message_filters_.erase(trace_message_filter);
@@ -920,22 +922,53 @@ void TracingControllerImpl::RequestGlobalMemoryDump(
                    base::Unretained(this), args, callback));
     return;
   }
-  // Abort if another dump is already in progress.
-  if (pending_memory_dump_guid_) {
-    VLOG(1) << base::trace_event::MemoryDumpManager::kLogPrefix
-            << " (" << args.dump_guid << ") aborted because another dump ("
-            << pending_memory_dump_guid_ << ") is in progress";
-    if (!callback.is_null())
-      callback.Run(args.dump_guid, false /* success */);
-    return;
+
+  bool another_dump_already_in_progress = !queued_memory_dump_requests_.empty();

[Primiano Tucci (use gerrit), 2016/06/23 11:09:16]

I really love this perfectly touching the 80th col without wrapping :)

[petrcermak, 2016/06/24 10:49:57]

Ack :-)

+
+  // If this is a periodic memory dump request without a callback and there

[Primiano Tucci (use gerrit), 2016/06/23 11:09:16]

remove the "without a callback" part. It is not true and if it was true would be
wrong. I think realistically most of them have a callback

[petrcermak, 2016/06/24 10:49:57]

Done.

+  // already is another request in the queue with the same level of detail,
+  // there's no point in enqueuing this request.
+  if (another_dump_already_in_progress) {

[Primiano Tucci (use gerrit), 2016/06/23 11:09:16]

why not if (another... && dump_type == ). Would reduce the indentation level of
the code below and increase its readability.

[petrcermak, 2016/06/24 10:49:57]

Done.

+    if (args.dump_type ==
+        base::trace_event::MemoryDumpType::PERIODIC_INTERVAL) {
+      const auto requested_level_of_detail = args.level_of_detail;

[Primiano Tucci (use gerrit), 2016/06/23 11:09:16]

not sure this variable buys you anything really

[petrcermak, 2016/06/24 10:49:57]

I removed it.

+      for (const auto& request : queued_memory_dump_requests_) {
+        if (request.args.level_of_detail == requested_level_of_detail) {
+          VLOG(1) << base::trace_event::MemoryDumpManager::kLogPrefix << " ("
+                  << base::trace_event::MemoryDumpTypeToString(args.dump_type)
+                  << ") skipped because another dump request with the same "
+                     "level of detail ("
+                  << base::trace_event::MemoryDumpLevelOfDetailToString(
+                         requested_level_of_detail)
+                  << ") is already in the queue";
+          if (!callback.is_null())
+            callback.Run(args.dump_guid, false /* success */);
+          return;
+        }
+      }
+    }
   }
 
+  queued_memory_dump_requests_.emplace_back(args, callback);
+
+  // If another dump is already in progress, this dump will automatically be
+  // scheduled when the other dump finishes.
+  if (another_dump_already_in_progress)
+    return;
+
+  PerformGlobalMemoryDump();
+}
+
+void TracingControllerImpl::PerformGlobalMemoryDump() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK(!queued_memory_dump_requests_.empty());
+  const base::trace_event::MemoryDumpRequestArgs& args =
+      queued_memory_dump_requests_.front().args;
+
   // Count myself (local trace) in pending_memory_dump_ack_count_, acked by
   // OnBrowserProcessMemoryDumpDone().
   pending_memory_dump_ack_count_ = trace_message_filters_.size() + 1;
   pending_memory_dump_filters_.clear();
-  pending_memory_dump_guid_ = args.dump_guid;
-  pending_memory_dump_callback_ = callback;
   failed_memory_dump_count_ = 0;
 
   MemoryDumpManagerDelegate::CreateProcessDump(
@@ -952,6 +985,13 @@ void TracingControllerImpl::RequestGlobalMemoryDump(
     tmf->SendProcessMemoryDumpRequest(args);
 }
 
+TracingControllerImpl::QueuedMemoryDumpRequest::QueuedMemoryDumpRequest(
+    const base::trace_event::MemoryDumpRequestArgs& args,
+    const base::trace_event::MemoryDumpCallback& callback)
+    : args(args), callback(callback) {}
+
+TracingControllerImpl::QueuedMemoryDumpRequest::~QueuedMemoryDumpRequest() {}
+
 uint64_t TracingControllerImpl::GetTracingProcessId() const {
   return ChildProcessHost::kBrowserTracingProcessId;
 }
@@ -991,7 +1031,8 @@ void TracingControllerImpl::OnProcessMemoryDumpResponse(
   TraceMessageFilterSet::iterator it =
       pending_memory_dump_filters_.find(trace_message_filter);
 
-  if (pending_memory_dump_guid_ != dump_guid ||
+  DCHECK(!queued_memory_dump_requests_.empty());
+  if (queued_memory_dump_requests_.front().args.dump_guid != dump_guid ||
       it == pending_memory_dump_filters_.end()) {
     DLOG(WARNING) << "Received unexpected memory dump response: " << dump_guid;
     return;
@@ -1025,14 +1066,24 @@ void TracingControllerImpl::FinalizeGlobalMemoryDumpIfAllProcessesReplied() {
   if (pending_memory_dump_ack_count_ > 0)
     return;
 
-  DCHECK_NE(0u, pending_memory_dump_guid_);
-  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();
+  DCHECK(!queued_memory_dump_requests_.empty());
+  {

[Primiano Tucci (use gerrit), 2016/06/23 11:09:16]

why this extra scope, seems unnecessary?

[petrcermak, 2016/06/24 10:49:57]

It's because |callback| is a reference to the front of the queue, which is
popped on line 1076. If there's no scope, someone could incorrectly use
|callback| after it's popped.

[Primiano Tucci (use gerrit), 2016/06/24 11:23:05]

ahh right. smart.

+    const auto& callback = queued_memory_dump_requests_.front().callback;
+    if (!callback.is_null()) {
+      const bool global_success = failed_memory_dump_count_ == 0;
+      callback.Run(queued_memory_dump_requests_.front().args.dump_guid,
+                   global_success);
+    }
+  }
+  queued_memory_dump_requests_.pop_front();
+
+  // Schedule the next queued dump (if applicable).
+  if (!queued_memory_dump_requests_.empty()) {
+    BrowserThread::PostTask(
+        BrowserThread::UI, FROM_HERE,
+        base::Bind(&TracingControllerImpl::PerformGlobalMemoryDump,
+                   base::Unretained(this)));
   }
-  pending_memory_dump_guid_ = 0;
 }
 
 }  // namespace content