Allow processing dumps with missing stack memory for some threads

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
threads without stack memory on the floor.

[Ted Mielczarek]

Mark, what do you think about this?

[Mark Mentovai]

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?

[Ted Mielczarek]

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/mi...

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

[Mark Mentovai]

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?

[Ted Mielczarek]

On Tue, Jul 10, 2012 at 3:51 PM,  <mark@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.”


On Tue, Jul 10, 2012 at 3:56 PM, Ted Mielczarek
<ted.mielczarek@gmail.com>wrote:

> On Tue, Jul 10, 2012 at 3:51 PM,  <mark@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.
>

[Ted Mielczarek]

On Tue, Jul 10, 2012 at 4:00 PM, Mark Mentovai <mark@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.


On Tue, Jul 10, 2012 at 8:16 PM, Ted Mielczarek
<ted.mielczarek@gmail.com>wrote:

> On Tue, Jul 10, 2012 at 4:00 PM, Mark Mentovai <mark@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.
>

[Ted Mielczarek]

Okay, I made the requested changes. The rest of this patch is mostly bloat from
writing a bunch of unit tests for it. Specifically, in minidump.h, the only
changes of substance are the comment changes in MinidumpThread, the rest is just
s/private/protected/ and virtual shuffling to make things more easily
unit-testable with mocks.

With this patch MinidumpProcessor can now cope with threads that are missing
stack memory, and also threads that are missing a CPU context. You'll get just
the context frame for the former, and an empty CallStack for the latter. I added
unit tests for both cases. I also made minidump_stackwalk print out a "<no
frames>" line for threads with zero frames, so the output is a little more
readable in those cases.

After my reworking of MinidumpProcessor::Process I realized that the error
results PROCESS_ERROR_NO_MEMORY_FOR_THREAD and
PROCESS_ERROR_NO_STACKWALKER_FOR_THREAD were no longer used, so I removed them.
This does expose one interesting quirk: if we process a minidump but fail to get
any stack frames for any threads, should we return an error? We won't currently,
but my feeling is that we probably should, since we're not providing any useful
information at that point. (You'd hit this case if all your threads were missing
CPU contexts, say.)

[Michael Krebs]

BTW, I just applied this patch in order to stackwalk a minidump gotten due to my
change in CL https://breakpad.appspot.com/478002/ -- and this worked.  So,
thanks!

I think it would be great to get this committed, so I don't have to run my local
builds by hand. :)

I haven't looked through the code itself, but I would say it might be worth at
least updating the commit message, because I don't think "it just drops threads
without stack memory on the floor" is accurate anymore.

[Michael Krebs]

LGTM.

So I got a chance to take a look at this, and it seems fine to me.  And, as I
mentioned, I successfully tried this on an actual minidump where a thread was
missing its stack memory.  See issue crosbug.com/35846.