Fix 2 bugs in out of process heap profiling implementation.

Bug 1: When an AllocationTracker is destroyed from a client disconnecting,
queued barrier callbacks would not be run. This would cause the heap dump to
never be emitted.

Bug 2: The implementation of MemlogConnectionManager::OnNewConnection called
mojo::SharedBufferHandle::Create, which uses synchronous IPC with the browser
process. This can cause deadlock/timeouts, since the browser process
synchronously waits for the memlog pipe to free up and eventually gives up after
10 seconds, tearing down the allocator shim. The solution is to create a
dedicated thread for calling mojo::SharedBufferHandle::Create, thus avoiding the
deadlock.

Change-Id: Ib3f4143381ce03c353ffe93d45794070e3b007d5
Bug: 811711
Reviewed-on: https://chromium-review.googlesource.com/917243
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: Primiano Tucci <primiano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536729}

chrome/profiling/allocation_tracker.cc

@@ -17,6 +17,14 @@ AllocationTracker::AllocationTracker(CompleteCallback complete_cb,
       backtrace_storage_(backtrace_storage) {}
 
 AllocationTracker::~AllocationTracker() {
+  for (auto& pair : registered_snapshot_callbacks_) {
+    RunnerSnapshotCallbackPair& rsc_pair = pair.second;
+    rsc_pair.first->PostTask(
+        FROM_HERE,
+        base::BindOnce(std::move(rsc_pair.second), false, AllocationCountMap(),
+                       ContextMap(), AddressToStringMap()));
+  }
+
   std::vector<const Backtrace*> to_free;
   to_free.reserve(live_allocs_.size());
   for (const auto& cur : live_allocs_)

chrome/profiling/memlog_connection_manager.cc

@@ -108,7 +108,9 @@ struct MemlogConnectionManager::Connection {
   uint32_t sampling_rate = 1;
 };
 
-MemlogConnectionManager::MemlogConnectionManager() : weak_factory_(this) {
+MemlogConnectionManager::MemlogConnectionManager()
+    : blocking_thread_("Blocking thread"), weak_factory_(this) {
+  blocking_thread_.Start();
   metrics_timer_.Start(FROM_HERE, base::TimeDelta::FromHours(24),
                        base::Bind(&MemlogConnectionManager::ReportMetrics,
                                   base::Unretained(this)));
@@ -267,9 +269,9 @@ void MemlogConnectionManager::DoDumpOneProcessForTracing(
   // All code paths through here must issue the callback when waiting_responses
   // is 0 or the browser will wait forever for the dump.
   DCHECK(tracking->waiting_responses > 0);
-  tracking->waiting_responses--;
 
   if (!success) {
+    tracking->waiting_responses--;
     if (tracking->waiting_responses == 0)
       std::move(tracking->callback).Run(std::move(tracking->results));
     return;
@@ -306,32 +308,57 @@ void MemlogConnectionManager::DoDumpOneProcessForTracing(
   std::ostringstream oss;
   ExportMemoryMapsAndV2StackTraceToJSON(&params, oss);
   std::string reply = oss.str();
+  size_t reply_size = reply.size();
 
   next_id_ = params.next_id;
 
-  mojo::ScopedSharedBufferHandle buffer =
-      mojo::SharedBufferHandle::Create(reply.size());
-  if (!buffer.is_valid()) {
-    DLOG(ERROR) << "Could not create Mojo shared buffer";
-  } else {
-    mojo::ScopedSharedBufferMapping mapping = buffer->Map(reply.size());
-    if (!mapping) {
-      DLOG(ERROR) << "Could not map Mojo shared buffer";
-    } else {
-      memcpy(mapping.get(), reply.c_str(), reply.size());
-
-      profiling::mojom::SharedBufferWithSizePtr result =
-          profiling::mojom::SharedBufferWithSize::New();
-      result->buffer = std::move(buffer);
-      result->size = reply.size();
-      result->pid = pid;
-      tracking->results.push_back(std::move(result));
-    }
-  }
-
-  // When all responses complete, issue done callback.
-  if (tracking->waiting_responses == 0)
-    std::move(tracking->callback).Run(std::move(tracking->results));
+  using FinishedCallback =
+      base::OnceCallback<void(mojo::ScopedSharedBufferHandle)>;
+  FinishedCallback finished_callback = base::BindOnce(
+      [](std::string reply, base::ProcessId pid,
+         scoped_refptr<DumpProcessesForTracingTracking> tracking,
+         mojo::ScopedSharedBufferHandle buffer) {
+        if (!buffer.is_valid()) {
+          DLOG(ERROR) << "Could not create Mojo shared buffer";
+        } else {
+          mojo::ScopedSharedBufferMapping mapping = buffer->Map(reply.size());
+          if (!mapping) {
+            DLOG(ERROR) << "Could not map Mojo shared buffer";
+          } else {
+            memcpy(mapping.get(), reply.c_str(), reply.size());
+
+            profiling::mojom::SharedBufferWithSizePtr result =
+                profiling::mojom::SharedBufferWithSize::New();
+            result->buffer = std::move(buffer);
+            result->size = reply.size();
+            result->pid = pid;
+            tracking->results.push_back(std::move(result));
+          }
+        }
+
+        // When all responses complete, issue done callback.
+        tracking->waiting_responses--;
+        if (tracking->waiting_responses == 0)
+          std::move(tracking->callback).Run(std::move(tracking->results));
+      },
+      std::move(reply), pid, tracking);
+
+  blocking_thread_.task_runner()->PostTask(
+      FROM_HERE, base::BindOnce(
+                     [](size_t size,
+                        scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+                        FinishedCallback callback) {
+                       // This call will send a synchronous IPC to the browser
+                       // process.
+                       mojo::ScopedSharedBufferHandle buffer =
+                           mojo::SharedBufferHandle::Create(size);
+                       task_runner->PostTask(FROM_HERE,
+                                             base::BindOnce(std::move(callback),
+                                                            std::move(buffer)));
+
+                     },
+                     reply_size, base::MessageLoop::current()->task_runner(),
+                     std::move(finished_callback)));
 }
 
 }  // namespace profiling

chrome/profiling/memlog_connection_manager.h

@@ -14,6 +14,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/process/process_handle.h"
 #include "base/synchronization/lock.h"
+#include "base/threading/thread.h"
 #include "base/values.h"
 #include "build/build_config.h"
 #include "chrome/common/profiling/profiling_service.mojom.h"
@@ -124,6 +125,13 @@ class MemlogConnectionManager {
   // Every 24-hours, reports the types of profiled processes.
   base::RepeatingTimer metrics_timer_;
 
+  // To avoid deadlock, synchronous calls to the browser are made on a dedicated
+  // thread that does nothing else. Both the IO thread and connection-specific
+  // threads could potentially be processing messages from the browser process,
+  // which in turn could be blocked on sending more messages over the memlog
+  // pipe.
+  base::Thread blocking_thread_;
+
   // Must be last.
   base::WeakPtrFactory<MemlogConnectionManager> weak_factory_;