Allow processing dumps with missing stack memory for some threads (issue 413002)

[ted.mie...@gmail.com]

Reviewers: ,

Description:
I'm not sure if this patch is really something we want to take, but we
started seeing some minidumps with missing stack memory for a few
threads (we're writing minidumps of the Flash player's sandboxed
subprocess without its cooperation, so there's probably some issues
there). This patch lets us get stacks for the threads that work. In
general I think getting some data out of a dump is better than nothing,
but as-implemented this kind of sucks because it just drops thraeds
without stack memory on the floor.

Please review this at http://breakpad.appspot.com/413002/

Affected files:
M src/processor/minidump.cc
M src/processor/minidump_processor.cc


Index: src/processor/minidump.cc
===================================================================
--- a/src/processor/minidump.cc
+++ b/src/processor/minidump.cc
@@ -1317,22 +1317,21 @@ bool MinidumpThread::Read() {

// Check for base + size overflow or undersize.
if (thread_.stack.memory.data_size == 0 ||
thread_.stack.memory.data_size > numeric_limits<u_int64_t>::max() -

thread_.stack.start_of_memory_range) {
BPLOG(ERROR) << "MinidumpThread has a memory region problem, " <<
HexString(thread_.stack.start_of_memory_range) << "+"
<<
HexString(thread_.stack.memory.data_size);
- return false;
+ } else {
+ memory_ = new MinidumpMemoryRegion(minidump_);
+ memory_->SetDescriptor(&thread_.stack);
}

- memory_ = new MinidumpMemoryRegion(minidump_);
- memory_->SetDescriptor(&thread_.stack);
-
valid_ = true;
return true;
}


MinidumpMemoryRegion* MinidumpThread::GetMemory() {
if (!valid_) {
BPLOG(ERROR) << "Invalid MinidumpThread for GetMemory";
Index: src/processor/minidump_processor.cc
===================================================================
--- a/src/processor/minidump_processor.cc
+++ b/src/processor/minidump_processor.cc
@@ -187,17 +187,17 @@ ProcessResult MinidumpProcessor::Process
MinidumpContext *ctx = exception->GetContext();
context = ctx ? ctx : thread->GetContext();
}
}

MinidumpMemoryRegion *thread_memory = thread->GetMemory();
if (!thread_memory) {
BPLOG(ERROR) << "No memory region for " << thread_string;
- return PROCESS_ERROR_NO_MEMORY_FOR_THREAD;
+ continue;
}

// Use process_state->modules_ instead of module_list, because the
// |modules| argument will be used to populate the |module| fields in
// the returned StackFrame objects, which will be placed into the
// returned ProcessState object. module_list's lifetime is only as
// long as the Minidump object: it will be deleted when this function
// returns. process_state->modules_ is owned by the ProcessState
object

[ted.mie...@gmail.com]

Mark, what do you think about this?

http://breakpad.appspot.com/413002/

[ma...@chromium.org]

http://breakpad.appspot.com/413002/diff/1/src/processor/minidump.cc
File src/processor/minidump.cc (right):

http://breakpad.appspot.com/413002/diff/1/src/processor/minidump.cc#newcode1330
src/processor/minidump.cc:1330: valid_ = true;
To me, valid_ implies that everything (including memory_) is set.

Can the checks elsewhere be shifted so that they check validity, or
something?

http://breakpad.appspot.com/413002/

[ted.mie...@gmail.com]

On 2012/07/10 18:18:32, Mark Mentovai wrote:

http://breakpad.appspot.com/413002/diff/1/src/processor/minidump.cc#newcode1330
> src/processor/minidump.cc:1330: valid_ = true;
> To me, valid_ implies that everything (including memory_) is set.

> Can the checks elsewhere be shifted so that they check validity, or
something?

I'm inclined to agree with you on this, but not setting valid_ means
that GetThreadID will fail, which means that we can't really do the
right thing in MinidumpThreadList::Read:
http://code.google.com/p/google-breakpad/source/browse/trunk/src/processor/minidump.cc#1514

The line in minidump_processor.cc is literally the only caller of
MinidumpThread::GetMemory in the tree, FWIW.

http://breakpad.appspot.com/413002/

[ma...@chromium.org]

Well, right, you’d need a way to signal “invalid by reason of no stack
memory” and handle appropriately in MinidumpThreadList::Read.

To do it the way you’re proposing, at the very least, I think you’d need
to add some comments explaining that a thread might not have memory and
we still treat it as valid.

When you do a stackwalk on such a dump, you still get an entry for such
a thread, right? It just doesn’t have anything other than the context
frame. Right?

http://breakpad.appspot.com/413002/

[Ted Mielczarek]

On Tue, Jul 10, 2012 at 3:51 PM, <ma...@chromium.org> wrote:
> Well, right, you’d need a way to signal “invalid by reason of no stack
> memory” and handle appropriately in MinidumpThreadList::Read.

Yeah, that seems a lot more fiddly than what my patch already does.

> When you do a stackwalk on such a dump, you still get an entry for such
> a thread, right? It just doesn’t have anything other than the context
> frame. Right?

With this patch it gets skipped entirely, which is not great. Ideally
you'd get the context frame, but the Stackwalker ::GetContextFrame
implementations bail out if memory is not present, which is kind of
weird.

[Mark Mentovai]

I’d like to get at least the context frame, or at the very worst, a thread with no stack.

I don’t consider no thread at all showing up in the stackwalk output to be acceptable, if that’s what you mean by “skipped entirely.”

[Ted Mielczarek]

On Tue, Jul 10, 2012 at 4:00 PM, Mark Mentovai <ma...@chromium.org> wrote:
> I’d like to get at least the context frame, or at the very worst, a thread
> with no stack.
>
> I don’t consider no thread at all showing up in the stackwalk output to be
> acceptable, if that’s what you mean by “skipped entirely.”

That is what I mean. Okay, I'll look to rework this to get to that state.

Are you okay with the "memory == NULL but still valid" state for the
MinidumpThread, or should I work something else up there also? If we
don't present it as valid we won't be able to even get a context
frame.

[Mark Mentovai]

It’s more palatable if it’s documented.