Reland of [tracing] Simplify logic of MemoryDumpManager
Reason for revert:
Original CL: crrev.com/1536533004
Revert: crrev.com/1540283003
Reason for reland:
The CL was reverted because caused a LayoutTest failure
(pending-version-change-stuck-works-with-terminate.html).
The failure was due to a race condition in an unrelated part of the
codebase (see crbug.com/571432). The original CL managed to just
unveil reliably the race. The race condition has been fixed in
crrev.com/1544663002. I verified that the LayoutTest doesn't crash
locally anymore.
Original issue's description:
> Revert of [tracing] Simplify logic of MemoryDumpManager (patchset #9 id:160001 of https://codereview.chromium.org/1536533004/ )
>
> Reason for revert:
> Caused crash in pending-version-change-stuck-works-with-terminate.html layout test.
>
> See:
> https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.10/builds/9506/steps/webkit_tests/logs/stdio
>
> Original issue's description:
> > [tracing] Simplify logic of MemoryDumpManager
> >
> > This CL simplifies the unregistration logic of MemoryDumpManager.
> > This is to make the overall code more readable and simplify upcoming
> > changes.
> > Prior to this CL, the MemoryDumpManager was keeping only one list to
> > keep track of the registered dump providers. This caused the
> > unregistration logic to be tricky because:
> > - We couldn't remove the MDPInfo straight away, as that might
> > cause invalidation of the |next_dump_provider| iterator.
> > - We had to flag the MDPInfo as unregistered and postpone the deletion
> > on the next dump.
> > This has a major drawback: if we keep registering and unregistering
> > dump providers when no tracing is happening at all, the dump_providers_
> > list grows without bounds. This is bad.
> >
> > This CL changes the logic as follows:
> > - MDPInfo becomes refcounted. At any time it can be referenced by:
> > - The MDM's |dump_providers_| list.
> > - The |ProcessMemoryDumpAsyncState.pending_dump_providers| list.
> > - Upon each dump, the dump provider list is snapshotted in the
> > |ProcessMemoryDumpAsyncState.pending_dump_providers|
> > - Upon unregistration of a dump provider we just remove the MDPInfo
> > from the MDM's |dump_providers_|. If a dump is ongoing, the
> > ProcessMemoryDumpAsyncState will keep it refcounted and delete it
> > during the dump.
> >
> > This CL does not add or change any behavior of the MemoryDumpManager,
> > with the exception of:
> > - Fixing a corner case when dumping with no dump providers registered
> > (See changes in the unittest).
> > - Making the fail-safe logic more strict: if a dumper fails once, it
> > will stay disabled forever.
> >
> > BUG=461788
> >
> > Committed: https://crrev.com/9734733909e7cb41ef5c153f3c2d0927e8209133
> > Cr-Commit-Position: refs/heads/master@{#366374}
>
> TBR=ruuda@google.com,ssid@chromium.org
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=461788
>
> Committed: https://crrev.com/6a7365c4c5ad3e3083ce3d9a269c3e1d8fdb35bc
> Cr-Commit-Position: refs/heads/master@{#366386}
TBR=ruuda@google.com,ssid@chromium.org
BUG=461788
Review URL: https://codereview.chromium.org/1546583002
Cr-Commit-Position: refs/heads/master@{#366588}
CrOS-Libchrome-Original-Commit: 6dfc15bc20d12fdcd401011b00a521c4e557386e
diff --git a/base/trace_event/memory_dump_manager.cc b/base/trace_event/memory_dump_manager.cc
index cbce210..f5c3c4f 100644
--- a/base/trace_event/memory_dump_manager.cc
+++ b/base/trace_event/memory_dump_manager.cc
@@ -193,17 +193,16 @@
if (dumper_registrations_ignored_for_testing_)
return;
- MemoryDumpProviderInfo mdp_info(mdp, name, task_runner, options);
- AutoLock lock(lock_);
- auto iter_new = dump_providers_.insert(mdp_info);
+ scoped_refptr<MemoryDumpProviderInfo> mdpinfo =
+ new MemoryDumpProviderInfo(mdp, name, task_runner, options);
- // If there was a previous entry, replace it with the new one. This is to deal
- // with the case where a dump provider unregisters itself and then re-
- // registers before a memory dump happens, so its entry was still in the
- // collection but flagged |unregistered|.
- if (!iter_new.second) {
- dump_providers_.erase(iter_new.first);
- dump_providers_.insert(mdp_info);
+ {
+ AutoLock lock(lock_);
+ bool already_registered = !dump_providers_.insert(mdpinfo).second;
+ // This actually happen in some tests which don't have a clean tear-down
+ // path for RenderThreadImpl::Init().
+ if (already_registered)
+ return;
}
if (heap_profiling_enabled_)
@@ -222,7 +221,7 @@
auto mdp_iter = dump_providers_.begin();
for (; mdp_iter != dump_providers_.end(); ++mdp_iter) {
- if (mdp_iter->dump_provider == mdp)
+ if ((*mdp_iter)->dump_provider == mdp)
break;
}
@@ -236,12 +235,18 @@
// Otherwise, it is not possible to guarantee that its unregistration is
// race-free. If you hit this DCHECK, your MDP has a bug.
DCHECK(!subtle::NoBarrier_Load(&memory_tracing_enabled_) ||
- (mdp_iter->task_runner &&
- mdp_iter->task_runner->BelongsToCurrentThread()))
- << "MemoryDumpProvider \"" << mdp_iter->name << "\" attempted to "
+ ((*mdp_iter)->task_runner &&
+ (*mdp_iter)->task_runner->BelongsToCurrentThread()))
+ << "MemoryDumpProvider \"" << (*mdp_iter)->name << "\" attempted to "
<< "unregister itself in a racy way. Please file a crbug.";
- mdp_iter->unregistered = true;
+ // The MDPInfo instance can still be referenced by the
+ // |ProcessMemoryDumpAsyncState.pending_dump_providers|. For this reason
+ // the MDPInfo is flagged as disabled. It will cause ContinueAsyncProcessDump
+ // to just skip it, without actually invoking the |mdp|, which might be
+ // destroyed by the caller soon after this method returns.
+ (*mdp_iter)->disabled = true;
+ dump_providers_.erase(mdp_iter);
}
void MemoryDumpManager::RequestGlobalDump(
@@ -296,9 +301,9 @@
scoped_ptr<ProcessMemoryDumpAsyncState> pmd_async_state;
{
AutoLock lock(lock_);
- pmd_async_state.reset(new ProcessMemoryDumpAsyncState(
- args, dump_providers_.begin(), session_state_, callback,
- dump_thread_->task_runner()));
+ pmd_async_state.reset(
+ new ProcessMemoryDumpAsyncState(args, dump_providers_, session_state_,
+ callback, dump_thread_->task_runner()));
}
TRACE_EVENT_WITH_FLOW0(kTraceCategory, "MemoryDumpManager::CreateProcessDump",
@@ -308,7 +313,7 @@
// Start the thread hop. |dump_providers_| are kept sorted by thread, so
// ContinueAsyncProcessDump will hop at most once per thread (w.r.t. thread
// affinity specified by the MemoryDumpProvider(s) in RegisterDumpProvider()).
- ContinueAsyncProcessDump(std::move(pmd_async_state));
+ ContinueAsyncProcessDump(pmd_async_state.release());
}
// At most one ContinueAsyncProcessDump() can be active at any time for a given
@@ -318,128 +323,103 @@
// means of subsequent PostTask(s).
//
// 1) Prologue:
-// - Check if the dump provider is disabled, if so skip the dump.
+// - If this was the last hop, create a trace event, add it to the trace
+// and finalize (invoke callback).
// - Check if we are on the right thread. If not hop and continue there.
+// - Check if the dump provider is disabled, if so skip the dump.
// 2) Invoke the dump provider's OnMemoryDump() (unless skipped).
// 3) Epilogue:
-// - Unregister the dump provider if it failed too many times consecutively.
-// - Advance the |next_dump_provider| iterator to the next dump provider.
-// - If this was the last hop, create a trace event, add it to the trace
-// and finalize (invoke callback).
-
+// - Unregister the dump provider if it failed too many times consecutively.
+// - Pop() the MDP from the |pending_dump_providers| list, eventually
+// destroying the MDPInfo if that was unregistered in the meantime.
void MemoryDumpManager::ContinueAsyncProcessDump(
- scoped_ptr<ProcessMemoryDumpAsyncState> pmd_async_state) {
+ ProcessMemoryDumpAsyncState* owned_pmd_async_state) {
// Initalizes the ThreadLocalEventBuffer to guarantee that the TRACE_EVENTs
// in the PostTask below don't end up registering their own dump providers
// (for discounting trace memory overhead) while holding the |lock_|.
TraceLog::GetInstance()->InitializeThreadLocalEventBufferIfSupported();
- const uint64_t dump_guid = pmd_async_state->req_args.dump_guid;
- const char* dump_provider_name = nullptr;
+ // In theory |owned_pmd_async_state| should be a scoped_ptr. The only reason
+ // why it isn't is because of the corner case logic of |did_post_task| below,
+ // which needs to take back the ownership of the |pmd_async_state| when a
+ // thread goes away and consequently the PostTask() fails.
+ // Unfortunately, PostTask() destroys the scoped_ptr arguments upon failure
+ // to prevent accidental leaks. Using a scoped_ptr would prevent us to to
+ // skip the hop and move on. Hence the manual naked -> scoped ptr juggling.
+ auto pmd_async_state = make_scoped_ptr(owned_pmd_async_state);
+ owned_pmd_async_state = nullptr;
- // Pid of the target process being dumped. Often kNullProcessId (= current
- // process), non-zero when the coordinator process creates dumps on behalf
- // of child processes (see crbug.com/461788).
- ProcessId pid;
-
- // DO NOT put any LOG() statement in the locked sections, as in some contexts
- // (GPU process) LOG() ends up performing PostTask/IPCs.
- MemoryDumpProvider* mdp;
- bool skip_dump = false;
- {
- AutoLock lock(lock_);
-
- auto mdp_info = pmd_async_state->next_dump_provider;
- mdp = mdp_info->dump_provider;
- dump_provider_name = mdp_info->name;
- pid = mdp_info->options.target_pid;
-
- // If the dump provider did not specify a thread affinity, dump on
- // |dump_thread_|.
- SingleThreadTaskRunner* task_runner = mdp_info->task_runner.get();
- if (!task_runner)
- task_runner = pmd_async_state->dump_thread_task_runner.get();
-
- // |dump_thread_| might have been Stop()-ed at this point (if tracing was
- // disabled in the meanwhile). In such case the PostTask() below will fail.
- // |task_runner|, however, should always be non-null.
- DCHECK(task_runner);
-
- if (mdp_info->disabled || mdp_info->unregistered) {
- skip_dump = true;
- } else if (!task_runner->BelongsToCurrentThread()) {
- // It's time to hop onto another thread.
-
- // Copy the callback + arguments just for the unlikley case in which
- // PostTask fails. In such case the Bind helper will destroy the
- // pmd_async_state and we must keep a copy of the fields to notify the
- // abort.
- MemoryDumpCallback callback = pmd_async_state->callback;
- scoped_refptr<SingleThreadTaskRunner> callback_task_runner =
- pmd_async_state->callback_task_runner;
-
- const bool did_post_task = task_runner->PostTask(
- FROM_HERE, Bind(&MemoryDumpManager::ContinueAsyncProcessDump,
- Unretained(this), Passed(&pmd_async_state)));
- if (did_post_task)
- return;
-
- // The thread is gone. At this point the best thing we can do is to
- // disable the dump provider and abort this dump.
- mdp_info->disabled = true;
- return AbortDumpLocked(callback, callback_task_runner, dump_guid);
- }
- } // AutoLock(lock_)
-
- // Invoke the dump provider without holding the |lock_|.
- bool finalize = false;
- bool dump_successful = false;
-
- if (!skip_dump) {
- TRACE_EVENT_WITH_FLOW1(kTraceCategory,
- "MemoryDumpManager::ContinueAsyncProcessDump",
- TRACE_ID_MANGLE(dump_guid),
- TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT,
- "dump_provider.name", dump_provider_name);
- MemoryDumpArgs args = {pmd_async_state->req_args.level_of_detail};
- ProcessMemoryDump* process_memory_dump =
- pmd_async_state->GetOrCreateMemoryDumpContainerForProcess(pid);
- dump_successful = mdp->OnMemoryDump(args, process_memory_dump);
- }
-
- {
- AutoLock lock(lock_);
- auto mdp_info = pmd_async_state->next_dump_provider;
- if (dump_successful) {
- mdp_info->consecutive_failures = 0;
- } else if (!skip_dump) {
- ++mdp_info->consecutive_failures;
- if (mdp_info->consecutive_failures >= kMaxConsecutiveFailuresCount) {
- mdp_info->disabled = true;
- }
- }
- ++pmd_async_state->next_dump_provider;
- finalize = pmd_async_state->next_dump_provider == dump_providers_.end();
-
- if (mdp_info->unregistered)
- dump_providers_.erase(mdp_info);
- }
-
- if (!skip_dump && !dump_successful) {
- LOG(ERROR) << "MemoryDumpProvider \"" << dump_provider_name << "\" failed, "
- << "possibly due to sandboxing (crbug.com/461788)."
- << "Disabling dumper for current process. Try --no-sandbox.";
- }
-
- if (finalize)
+ if (pmd_async_state->pending_dump_providers.empty())
return FinalizeDumpAndAddToTrace(std::move(pmd_async_state));
- ContinueAsyncProcessDump(std::move(pmd_async_state));
+ // Read MemoryDumpProviderInfo thread safety considerations in
+ // memory_dump_manager.h when accessing |mdpinfo| fields.
+ MemoryDumpProviderInfo* mdpinfo =
+ pmd_async_state->pending_dump_providers.back().get();
+
+ // If the dump provider did not specify a thread affinity, dump on
+ // |dump_thread_|. Note that |dump_thread_| might have been Stop()-ed at this
+ // point (if tracing was disabled in the meanwhile). In such case the
+ // PostTask() below will fail, but |task_runner| should always be non-null.
+ SingleThreadTaskRunner* task_runner = mdpinfo->task_runner.get();
+ if (!task_runner)
+ task_runner = pmd_async_state->dump_thread_task_runner.get();
+
+ if (!task_runner->BelongsToCurrentThread()) {
+ // It's time to hop onto another thread.
+ const bool did_post_task = task_runner->PostTask(
+ FROM_HERE, Bind(&MemoryDumpManager::ContinueAsyncProcessDump,
+ Unretained(this), Unretained(pmd_async_state.get())));
+ if (did_post_task) {
+ // Ownership is tranferred to the next ContinueAsyncProcessDump().
+ ignore_result(pmd_async_state.release());
+ return;
+ }
+ // The thread is gone. Skip the dump provider and keep going.
+ mdpinfo->disabled = true;
+ }
+
+ // At this point wither we are on the right thread (|mdpinfo.task_runner|)
+ // to access mdp fields, or the right thread is gone (and |disabled| == true).
+
+ if (!mdpinfo->disabled) {
+ // Invoke the dump provider.
+ TRACE_EVENT_WITH_FLOW1(kTraceCategory,
+ "MemoryDumpManager::ContinueAsyncProcessDump",
+ TRACE_ID_MANGLE(pmd_async_state->req_args.dump_guid),
+ TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT,
+ "dump_provider.name", mdpinfo->name);
+
+ // Pid of the target process being dumped. Often kNullProcessId (= current
+ // process), non-zero when the coordinator process creates dumps on behalf
+ // of child processes (see crbug.com/461788).
+ ProcessId target_pid = mdpinfo->options.target_pid;
+ ProcessMemoryDump* pmd =
+ pmd_async_state->GetOrCreateMemoryDumpContainerForProcess(target_pid);
+ MemoryDumpArgs args = {pmd_async_state->req_args.level_of_detail};
+ bool dump_successful = mdpinfo->dump_provider->OnMemoryDump(args, pmd);
+
+ if (dump_successful) {
+ mdpinfo->consecutive_failures = 0;
+ } else {
+ ++mdpinfo->consecutive_failures;
+ if (mdpinfo->consecutive_failures >= kMaxConsecutiveFailuresCount) {
+ mdpinfo->disabled = true;
+ LOG(ERROR) << "MemoryDumpProvider \"" << mdpinfo->name << "\" failed, "
+ << "possibly due to sandboxing (crbug.com/461788)."
+ << "Disabling dumper for current process. Try --no-sandbox.";
+ }
+ }
+ } // if (!mdpinfo->disabled)
+
+ pmd_async_state->pending_dump_providers.pop_back();
+ ContinueAsyncProcessDump(pmd_async_state.release());
}
// static
void MemoryDumpManager::FinalizeDumpAndAddToTrace(
scoped_ptr<ProcessMemoryDumpAsyncState> pmd_async_state) {
+ DCHECK(pmd_async_state->pending_dump_providers.empty());
const uint64_t dump_guid = pmd_async_state->req_args.dump_guid;
if (!pmd_async_state->callback_task_runner->BelongsToCurrentThread()) {
scoped_refptr<SingleThreadTaskRunner> callback_task_runner =
@@ -483,20 +463,6 @@
TRACE_ID_MANGLE(dump_guid));
}
-// static
-void MemoryDumpManager::AbortDumpLocked(
- MemoryDumpCallback callback,
- scoped_refptr<SingleThreadTaskRunner> task_runner,
- uint64_t dump_guid) {
- if (callback.is_null())
- return; // There is nothing to NACK.
-
- // Post the callback even if we are already on the right thread to avoid
- // invoking the callback while holding the lock_.
- task_runner->PostTask(FROM_HERE,
- Bind(callback, dump_guid, false /* success */));
-}
-
void MemoryDumpManager::OnTraceLogEnabled() {
bool enabled;
TRACE_EVENT_CATEGORY_GROUP_ENABLED(kTraceCategory, &enabled);
@@ -541,11 +507,6 @@
session_state_ = new MemoryDumpSessionState(stack_frame_deduplicator,
type_name_deduplicator);
- for (auto it = dump_providers_.begin(); it != dump_providers_.end(); ++it) {
- it->disabled = false;
- it->consecutive_failures = 0;
- }
-
subtle::NoBarrier_Store(&memory_tracing_enabled_, 1);
// TODO(primiano): This is a temporary hack to disable periodic memory dumps
@@ -617,31 +578,36 @@
task_runner(task_runner),
options(options),
consecutive_failures(0),
- disabled(false),
- unregistered(false) {}
+ disabled(false) {}
MemoryDumpManager::MemoryDumpProviderInfo::~MemoryDumpProviderInfo() {}
-bool MemoryDumpManager::MemoryDumpProviderInfo::operator<(
- const MemoryDumpProviderInfo& other) const {
- if (task_runner == other.task_runner)
- return dump_provider < other.dump_provider;
+bool MemoryDumpManager::MemoryDumpProviderInfo::Comparator::operator()(
+ const scoped_refptr<MemoryDumpManager::MemoryDumpProviderInfo>& a,
+ const scoped_refptr<MemoryDumpManager::MemoryDumpProviderInfo>& b) const {
+ if (!a || !b)
+ return a.get() < b.get();
// Ensure that unbound providers (task_runner == nullptr) always run last.
- return !(task_runner < other.task_runner);
+ // Rationale: some unbound dump providers are known to be slow, keep them last
+ // to avoid skewing timings of the other dump providers.
+ return std::tie(a->task_runner, a->dump_provider) >
+ std::tie(b->task_runner, b->dump_provider);
}
MemoryDumpManager::ProcessMemoryDumpAsyncState::ProcessMemoryDumpAsyncState(
MemoryDumpRequestArgs req_args,
- MemoryDumpProviderInfoSet::iterator next_dump_provider,
+ const MemoryDumpProviderInfo::OrderedSet& dump_providers,
const scoped_refptr<MemoryDumpSessionState>& session_state,
MemoryDumpCallback callback,
const scoped_refptr<SingleThreadTaskRunner>& dump_thread_task_runner)
: req_args(req_args),
- next_dump_provider(next_dump_provider),
session_state(session_state),
callback(callback),
callback_task_runner(MessageLoop::current()->task_runner()),
- dump_thread_task_runner(dump_thread_task_runner) {}
+ dump_thread_task_runner(dump_thread_task_runner) {
+ pending_dump_providers.reserve(dump_providers.size());
+ pending_dump_providers.assign(dump_providers.rbegin(), dump_providers.rend());
+}
MemoryDumpManager::ProcessMemoryDumpAsyncState::~ProcessMemoryDumpAsyncState() {
}
diff --git a/base/trace_event/memory_dump_manager.h b/base/trace_event/memory_dump_manager.h
index a5037cf..5391411 100644
--- a/base/trace_event/memory_dump_manager.h
+++ b/base/trace_event/memory_dump_manager.h
@@ -8,6 +8,7 @@
#include <map>
#include <memory>
#include <set>
+#include <vector>
#include "base/atomicops.h"
#include "base/containers/hash_tables.h"
@@ -130,50 +131,70 @@
friend class MemoryDumpManagerDelegate;
friend class MemoryDumpManagerTest;
- // Descriptor struct used to hold information about registered MDPs. It is
- // deliberately copyable, in order to allow it to be used as std::set value.
- struct MemoryDumpProviderInfo {
+ // Descriptor used to hold information about registered MDPs.
+ // Some important considerations about lifetime of this object:
+ // - In nominal conditions, all the MemoryDumpProviderInfo instances live in
+ // the |dump_providers_| collection (% unregistration while dumping).
+ // - Upon each dump they (actually their scoped_refptr-s) are copied into
+ // the ProcessMemoryDumpAsyncState. This is to allow removal (see below).
+ // - When the MDP.OnMemoryDump() is invoked, the corresponding MDPInfo copy
+ // inside ProcessMemoryDumpAsyncState is removed.
+ // - In nominal conditions, the MDPInfo is destroyed in the
+ // UnregisterDumpProvider() call.
+ // - If UnregisterDumpProvider() is called while a dump is in progress, the
+ // MDPInfo is destroyed in the epilogue of ContinueAsyncProcessDump(), when
+ // the copy inside ProcessMemoryDumpAsyncState is erase()-d.
+ // - The non-const fields of MemoryDumpProviderInfo are safe to access only
+ // in the |task_runner| thread, unless the thread has been destroyed.
+ struct MemoryDumpProviderInfo
+ : public RefCountedThreadSafe<MemoryDumpProviderInfo> {
+ // Define a total order based on the thread (i.e. |task_runner|) affinity,
+ // so that all MDP belonging to the same thread are adjacent in the set.
+ struct Comparator {
+ bool operator()(const scoped_refptr<MemoryDumpProviderInfo>& a,
+ const scoped_refptr<MemoryDumpProviderInfo>& b) const;
+ };
+ using OrderedSet =
+ std::set<scoped_refptr<MemoryDumpProviderInfo>, Comparator>;
+
MemoryDumpProviderInfo(
MemoryDumpProvider* dump_provider,
const char* name,
const scoped_refptr<SingleThreadTaskRunner>& task_runner,
const MemoryDumpProvider::Options& options);
- ~MemoryDumpProviderInfo();
-
- // Define a total order based on the thread (i.e. |task_runner|) affinity,
- // so that all MDP belonging to the same thread are adjacent in the set.
- bool operator<(const MemoryDumpProviderInfo& other) const;
MemoryDumpProvider* const dump_provider;
+
+ // Human readable name, for debugging and testing. Not necessarily unique.
const char* const name;
// The task_runner affinity. Can be nullptr, in which case the dump provider
// will be invoked on |dump_thread_|.
- scoped_refptr<SingleThreadTaskRunner> task_runner;
+ const scoped_refptr<SingleThreadTaskRunner> task_runner;
// The |options| arg passed to RegisterDumpProvider().
const MemoryDumpProvider::Options options;
- // For fail-safe logic (auto-disable failing MDPs). These fields are mutable
- // as can be safely changed without impacting the order within the set.
- mutable int consecutive_failures;
- mutable bool disabled;
+ // For fail-safe logic (auto-disable failing MDPs).
+ int consecutive_failures;
- // When a dump provider unregisters, it is flagged as |unregistered| and it
- // is removed only upon the next memory dump. This is to avoid altering the
- // |dump_providers_| collection while a dump is in progress.
- mutable bool unregistered;
+ // Flagged either by the auto-disable logic or during unregistration.
+ bool disabled;
+
+ private:
+ friend class base::RefCountedThreadSafe<MemoryDumpProviderInfo>;
+ ~MemoryDumpProviderInfo();
+
+ DISALLOW_COPY_AND_ASSIGN(MemoryDumpProviderInfo);
};
- using MemoryDumpProviderInfoSet = std::set<MemoryDumpProviderInfo>;
-
// Holds the state of a process memory dump that needs to be carried over
// across threads in order to fulfil an asynchronous CreateProcessDump()
// request. At any time exactly one thread owns a ProcessMemoryDumpAsyncState.
struct ProcessMemoryDumpAsyncState {
ProcessMemoryDumpAsyncState(
MemoryDumpRequestArgs req_args,
- MemoryDumpProviderInfoSet::iterator next_dump_provider,
+ const MemoryDumpProviderInfo::OrderedSet& dump_providers,
const scoped_refptr<MemoryDumpSessionState>& session_state,
MemoryDumpCallback callback,
const scoped_refptr<SingleThreadTaskRunner>& dump_thread_task_runner);
@@ -191,9 +212,10 @@
// The arguments passed to the initial CreateProcessDump() request.
const MemoryDumpRequestArgs req_args;
- // The |dump_providers_| iterator to the next dump provider that should be
- // invoked (or dump_providers_.end() if at the end of the sequence).
- MemoryDumpProviderInfoSet::iterator next_dump_provider;
+ // An ordered sequence of dump providers that have to be invoked to complete
+ // the dump. This is a copy of |dump_providers_| at the beginning of a dump
+ // and becomes empty at the end, when all dump providers have been invoked.
+ std::vector<scoped_refptr<MemoryDumpProviderInfo>> pending_dump_providers;
// The trace-global session state.
scoped_refptr<MemoryDumpSessionState> session_state;
@@ -226,9 +248,6 @@
static void SetInstanceForTesting(MemoryDumpManager* instance);
static void FinalizeDumpAndAddToTrace(
scoped_ptr<ProcessMemoryDumpAsyncState> pmd_async_state);
- static void AbortDumpLocked(MemoryDumpCallback callback,
- scoped_refptr<SingleThreadTaskRunner> task_runner,
- uint64_t dump_guid);
// Internal, used only by MemoryDumpManagerDelegate.
// Creates a memory dump for the current process and appends it to the trace.
@@ -240,11 +259,11 @@
// Continues the ProcessMemoryDump started by CreateProcessDump(), hopping
// across threads as needed as specified by MDPs in RegisterDumpProvider().
void ContinueAsyncProcessDump(
- scoped_ptr<ProcessMemoryDumpAsyncState> pmd_async_state);
+ ProcessMemoryDumpAsyncState* owned_pmd_async_state);
// An ordererd set of registered MemoryDumpProviderInfo(s), sorted by thread
// affinity (MDPs belonging to the same thread are adjacent).
- MemoryDumpProviderInfoSet dump_providers_;
+ MemoryDumpProviderInfo::OrderedSet dump_providers_;
// Shared among all the PMDs to keep state scoped to the tracing session.
scoped_refptr<MemoryDumpSessionState> session_state_;
diff --git a/base/trace_event/memory_dump_manager_unittest.cc b/base/trace_event/memory_dump_manager_unittest.cc
index 34c98b0..cf0f305 100644
--- a/base/trace_event/memory_dump_manager_unittest.cc
+++ b/base/trace_event/memory_dump_manager_unittest.cc
@@ -4,8 +4,10 @@
#include "base/trace_event/memory_dump_manager.h"
+#include <vector>
+
#include "base/bind_helpers.h"
-#include "base/memory/scoped_vector.h"
+#include "base/memory/scoped_ptr.h"
#include "base/message_loop/message_loop.h"
#include "base/run_loop.h"
#include "base/strings/stringprintf.h"
@@ -202,10 +204,13 @@
// Finally check the unregister logic: the delegate will be invoked but not
// the dump provider, as it has been unregistered.
EnableTracingWithLegacyCategories(MemoryDumpManager::kTraceCategory);
- EXPECT_CALL(*delegate_, RequestGlobalMemoryDump(_, _)).Times(1);
+ EXPECT_CALL(*delegate_, RequestGlobalMemoryDump(_, _)).Times(3);
EXPECT_CALL(mdp, OnMemoryDump(_, _)).Times(0);
- RequestGlobalDumpAndWait(MemoryDumpType::EXPLICITLY_TRIGGERED,
- MemoryDumpLevelOfDetail::DETAILED);
+
+ for (int i = 0; i < 3; ++i) {
+ RequestGlobalDumpAndWait(MemoryDumpType::EXPLICITLY_TRIGGERED,
+ MemoryDumpLevelOfDetail::DETAILED);
+ }
DisableTracing();
}
@@ -531,13 +536,14 @@
// dumping from a different thread than the dumping thread.
TEST_F(MemoryDumpManagerTest, UnregisterDumperFromThreadWhileDumping) {
InitializeMemoryDumpManager(false /* is_coordinator */);
- ScopedVector<TestIOThread> threads;
- ScopedVector<MockMemoryDumpProvider> mdps;
+ std::vector<scoped_ptr<TestIOThread>> threads;
+ std::vector<scoped_ptr<MockMemoryDumpProvider>> mdps;
for (int i = 0; i < 2; i++) {
- threads.push_back(new TestIOThread(TestIOThread::kAutoStart));
- mdps.push_back(new MockMemoryDumpProvider());
- RegisterDumpProvider(mdps.back(), threads.back()->task_runner(),
+ threads.push_back(
+ make_scoped_ptr(new TestIOThread(TestIOThread::kAutoStart)));
+ mdps.push_back(make_scoped_ptr(new MockMemoryDumpProvider()));
+ RegisterDumpProvider(mdps.back().get(), threads.back()->task_runner(),
kDefaultOptions);
}
@@ -545,10 +551,10 @@
// When OnMemoryDump is called on either of the dump providers, it will
// unregister the other one.
- for (MockMemoryDumpProvider* mdp : mdps) {
+ for (const scoped_ptr<MockMemoryDumpProvider>& mdp : mdps) {
int other_idx = (mdps.front() == mdp);
- TestIOThread* other_thread = threads[other_idx];
- MockMemoryDumpProvider* other_mdp = mdps[other_idx];
+ TestIOThread* other_thread = threads[other_idx].get();
+ MockMemoryDumpProvider* other_mdp = mdps[other_idx].get();
auto on_dump = [this, other_thread, other_mdp, &on_memory_dump_call_count](
const MemoryDumpArgs& args, ProcessMemoryDump* pmd) {
other_thread->PostTaskAndWait(
@@ -571,7 +577,54 @@
RequestGlobalDumpAndWait(MemoryDumpType::EXPLICITLY_TRIGGERED,
MemoryDumpLevelOfDetail::DETAILED);
ASSERT_EQ(1, on_memory_dump_call_count);
- ASSERT_EQ(true, last_callback_success_);
+ ASSERT_TRUE(last_callback_success_);
+
+ DisableTracing();
+}
+
+// If a thread (with a dump provider living on it) is torn down during a dump
+// its dump provider should be skipped but the dump itself should succeed.
+TEST_F(MemoryDumpManagerTest, TearDownThreadWhileDumping) {
+ InitializeMemoryDumpManager(false /* is_coordinator */);
+ std::vector<scoped_ptr<TestIOThread>> threads;
+ std::vector<scoped_ptr<MockMemoryDumpProvider>> mdps;
+
+ for (int i = 0; i < 2; i++) {
+ threads.push_back(
+ make_scoped_ptr(new TestIOThread(TestIOThread::kAutoStart)));
+ mdps.push_back(make_scoped_ptr(new MockMemoryDumpProvider()));
+ RegisterDumpProvider(mdps.back().get(), threads.back()->task_runner(),
+ kDefaultOptions);
+ }
+
+ int on_memory_dump_call_count = 0;
+
+ // When OnMemoryDump is called on either of the dump providers, it will
+ // tear down the thread of the other one.
+ for (const scoped_ptr<MockMemoryDumpProvider>& mdp : mdps) {
+ int other_idx = (mdps.front() == mdp);
+ TestIOThread* other_thread = threads[other_idx].get();
+ auto on_dump = [other_thread, &on_memory_dump_call_count](
+ const MemoryDumpArgs& args, ProcessMemoryDump* pmd) {
+ other_thread->Stop();
+ on_memory_dump_call_count++;
+ return true;
+ };
+
+ // OnMemoryDump is called once for the provider that dumps first, and zero
+ // times for the other provider.
+ EXPECT_CALL(*mdp, OnMemoryDump(_, _))
+ .Times(AtMost(1))
+ .WillOnce(Invoke(on_dump));
+ }
+
+ last_callback_success_ = false;
+ EnableTracingWithLegacyCategories(MemoryDumpManager::kTraceCategory);
+ EXPECT_CALL(*delegate_, RequestGlobalMemoryDump(_, _)).Times(1);
+ RequestGlobalDumpAndWait(MemoryDumpType::EXPLICITLY_TRIGGERED,
+ MemoryDumpLevelOfDetail::DETAILED);
+ ASSERT_EQ(1, on_memory_dump_call_count);
+ ASSERT_TRUE(last_callback_success_);
DisableTracing();
}
@@ -761,9 +814,9 @@
tracing_disabled_event.Signal();
run_loop.Run();
- // RequestGlobalMemoryDump() should be NACK-ed because one of the threads
- // threads died before we had a chance to PostTask onto them.
- EXPECT_FALSE(last_callback_success_);
+ // RequestGlobalMemoryDump() should still suceed even if some threads were
+ // torn down during the dump.
+ EXPECT_TRUE(last_callback_success_);
}
TEST_F(MemoryDumpManagerTest, DumpOnBehalfOfOtherProcess) {