snapshot: Add a minimal ModuleSnapshotMinidump and accessor from ProcessSnapshotMinidump

snapshot: Add a minimal ModuleSnapshotMinidump and accessor from
ProcessSnapshotMinidump.

ModuleSnapshotMinidump is currently only capable of reading module
annotations, in both vector and simple-dictionary forms. It does not
read any other module information from minidump files. These annotations
are all that are necessary to be able to upload Crashpad-produced
minidumps to Breakpad crash processor servers, because Breakpad accepts
them as HTTP POST parameters, while Crashpad places them in the minidump
file itself.

TEST=snapshot_test ProcessSnapshotMinidump.Modules
BUG=crashpad:10
R=rsesek@chromium.org

Committed: https://chromium.googlesource.com/crashpad/crashpad/+/445c0eae7ca33491a1d3ba98339bd44035dfb49c

[Mark Mentovai, 2015-03-03 22:43:22]

mark@chromium.org changed reviewers:
+ rsesek@chromium.org

[Mark Mentovai, 2015-03-03 22:44:25]

This depends on https://codereview.chromium.org/978463003/.

[Robert Sesek, 2015-03-04 01:52:26]

Feel free to ignore the "indent +4" comments, but I think we liked that for
breaking in the middle of a condition.

https://codereview.chromium.org/972383002/diff/1/snapshot/mac/process_snapsho...
File snapshot/mac/process_snapshot_mac.cc (right):

https://codereview.chromium.org/972383002/diff/1/snapshot/mac/process_snapsho...
snapshot/mac/process_snapshot_mac.cc:149: if
(!thread->Initialize(&process_reader_, process_reader_thread)) {
Could you restructure this as:

auto thread = make_scoped_pointer(new internal::ThreadSnapshotMac());
if (thread->Initialize())
  threads_.push_back(thread.release());

?

https://codereview.chromium.org/972383002/diff/1/snapshot/minidump/module_sna...
File snapshot/minidump/module_snapshot_minidump.cc (right):

https://codereview.chromium.org/972383002/diff/1/snapshot/minidump/module_sna...
snapshot/minidump/module_snapshot_minidump.cc:1: // Copyright 2014 The Crashpad
Authors. All rights reserved.
2015

https://codereview.chromium.org/972383002/diff/1/snapshot/minidump/module_sna...
snapshot/minidump/module_snapshot_minidump.cc:59: NOTREACHED();
Are these going to be implemented eventually? If so, maybe NOTIMPLEMENETED()
would be better (does mini_chromium have that?).

https://codereview.chromium.org/972383002/diff/1/snapshot/minidump/module_sna...
snapshot/minidump/module_snapshot_minidump.cc:139:
sizeof(minidump_module_crashpad_info)) {
nit: indent +4

https://codereview.chromium.org/972383002/diff/1/snapshot/minidump/module_sna...
snapshot/minidump/module_snapshot_minidump.cc:159:
ReadMinidumpStringList(file_reader,
What if these functions return false? There's no way to tell if this object is
in a valid state or not beyond initialization.

This function should probably return bool and report that from ::Initialize()
proper.

https://codereview.chromium.org/972383002/diff/1/snapshot/minidump/module_sna...
File snapshot/minidump/module_snapshot_minidump.h (right):

https://codereview.chromium.org/972383002/diff/1/snapshot/minidump/module_sna...
snapshot/minidump/module_snapshot_minidump.h:1: // Copyright 2014 The Crashpad
Authors. All rights reserved.
2015

https://codereview.chromium.org/972383002/diff/1/snapshot/minidump/module_sna...
snapshot/minidump/module_snapshot_minidump.h:44: //!     The file reader must
support seeking.
Optional and out of scope: Go does a nice thing where it composes Reader and
Seeker into a ReaderSeeker. The same could be done here to have a
FileReaderSeeker pure virtual interface.

https://codereview.chromium.org/972383002/diff/1/snapshot/minidump/process_sn...
File snapshot/minidump/process_snapshot_minidump.cc (right):

https://codereview.chromium.org/972383002/diff/1/snapshot/minidump/process_sn...
snapshot/minidump/process_snapshot_minidump.cc:227:
internal::ModuleSnapshotMinidump* module =
Similar to the earlier comment, it'll be easier to catch leaks from early
returns if this were in a scoper and then released into the vector after a
successful Initialize().

https://codereview.chromium.org/972383002/diff/1/snapshot/minidump/process_sn...
snapshot/minidump/process_snapshot_minidump.cc:266:
sizeof(MinidumpModuleCrashpadInfoList)) {
nit: indent +4

https://codereview.chromium.org/972383002/diff/1/snapshot/minidump/process_sn...
snapshot/minidump/process_snapshot_minidump.cc:278: return;
Similar comment as to returning false under failure conditions, unless we're
going to tolerate them because this data is non-critical. If that's the case,
then maybe mention it in the method comment?

https://codereview.chromium.org/972383002/diff/1/snapshot/minidump/process_sn...
File snapshot/minidump/process_snapshot_minidump_test.cc (right):

https://codereview.chromium.org/972383002/diff/1/snapshot/minidump/process_sn...
snapshot/minidump/process_snapshot_minidump_test.cc:68:
EXPECT_TRUE(writer->Write(string.c_str(), string.size() + 1));  // include NUL
nit: punctuation and capitalization (yes I know this will make it 81 cols ;-)

[Mark Mentovai, 2015-03-04 16:23:36]

Patchset #3 (id:40001) has been deleted

[Mark Mentovai, 2015-03-04 16:24:24]

Updated. Thanks for the review!

https://codereview.chromium.org/972383002/diff/1/snapshot/minidump/module_sna...
File snapshot/minidump/module_snapshot_minidump.cc (right):

https://codereview.chromium.org/972383002/diff/1/snapshot/minidump/module_sna...
snapshot/minidump/module_snapshot_minidump.cc:59: NOTREACHED();
Robert Sesek wrote:
> Are these going to be implemented eventually? If so, maybe NOTIMPLEMENETED()
> would be better (does mini_chromium have that?).

mini_chromium doesn’t have it. I don’t like that NOTIMPLEMENTED() in Chrome is
LOG(ERROR), where what I really want is more like DCHECK(false). NOTREACHED() is
DCHECK(false).

https://codereview.chromium.org/972383002/diff/1/snapshot/minidump/module_sna...
snapshot/minidump/module_snapshot_minidump.cc:159:
ReadMinidumpStringList(file_reader,
Robert Sesek wrote:
> What if these functions return false? There's no way to tell if this object is
> in a valid state or not beyond initialization.
> 
> This function should probably return bool and report that from ::Initialize()
> proper.

I’ve treated “crashpad info” as optional. This isn’t so much because it needs to
be optional, because if the file says the data should be there, it really should
be legible. It’s more because when I initially wrote this stuff, I wanted to
load stuff as lazily as possible, and there wasn’t an opportunity to return
false from Initialize() because these functions wouldn’t be called until well
after Initialize() returned.

I’ll change it so that they report success/failure and bubble it back up to
Initialize(), but I’d still like to make the lazy-loading change once the const
interface bug is fixed, and at that point, we’re back to Initialize() not
knowing about problems with these extension structures.

https://codereview.chromium.org/972383002/diff/1/snapshot/minidump/process_sn...
File snapshot/minidump/process_snapshot_minidump_test.cc (right):

https://codereview.chromium.org/972383002/diff/1/snapshot/minidump/process_sn...
snapshot/minidump/process_snapshot_minidump_test.cc:68:
EXPECT_TRUE(writer->Write(string.c_str(), string.size() + 1));  // include NUL
Robert Sesek wrote:
> nit: punctuation and capitalization (yes I know this will make it 81 cols ;-)

:)

[Robert Sesek, 2015-03-04 17:06:25]

https://codereview.chromium.org/972383002/diff/1/snapshot/minidump/module_sna...
File snapshot/minidump/module_snapshot_minidump.cc (right):

https://codereview.chromium.org/972383002/diff/1/snapshot/minidump/module_sna...
snapshot/minidump/module_snapshot_minidump.cc:59: NOTREACHED();
On 2015/03/04 16:24:24, Mark Mentovai wrote:
> Robert Sesek wrote:
> > Are these going to be implemented eventually? If so, maybe NOTIMPLEMENETED()
> > would be better (does mini_chromium have that?).
> 
> mini_chromium doesn’t have it. I don’t like that NOTIMPLEMENTED() in Chrome is
> LOG(ERROR), where what I really want is more like DCHECK(false). NOTREACHED()
is
> DCHECK(false).

OK. Are these methods going to be implemented at some point, though?

[Mark Mentovai, 2015-03-04 17:10:21]

https://codereview.chromium.org/972383002/diff/1/snapshot/minidump/module_sna...
File snapshot/minidump/module_snapshot_minidump.cc (right):

https://codereview.chromium.org/972383002/diff/1/snapshot/minidump/module_sna...
snapshot/minidump/module_snapshot_minidump.cc:59: NOTREACHED();
Robert Sesek wrote:
> On 2015/03/04 16:24:24, Mark Mentovai wrote:
> > Robert Sesek wrote:
> > > Are these going to be implemented eventually? If so, maybe
NOTIMPLEMENETED()
> > > would be better (does mini_chromium have that?).
> > 
> > mini_chromium doesn’t have it. I don’t like that NOTIMPLEMENTED() in Chrome
is
> > LOG(ERROR), where what I really want is more like DCHECK(false).
NOTREACHED()
> is
> > DCHECK(false).
> 
> OK. Are these methods going to be implemented at some point, though?

Yes, of course. Do you want to see comments?

[Robert Sesek, 2015-03-04 17:14:00]

https://codereview.chromium.org/972383002/diff/1/snapshot/minidump/module_sna...
File snapshot/minidump/module_snapshot_minidump.cc (right):

https://codereview.chromium.org/972383002/diff/1/snapshot/minidump/module_sna...
snapshot/minidump/module_snapshot_minidump.cc:59: NOTREACHED();
On 2015/03/04 17:10:21, Mark Mentovai wrote:
> Robert Sesek wrote:
> > On 2015/03/04 16:24:24, Mark Mentovai wrote:
> > > Robert Sesek wrote:
> > > > Are these going to be implemented eventually? If so, maybe
> NOTIMPLEMENETED()
> > > > would be better (does mini_chromium have that?).
> > > 
> > > mini_chromium doesn’t have it. I don’t like that NOTIMPLEMENTED() in
Chrome
> is
> > > LOG(ERROR), where what I really want is more like DCHECK(false).
> NOTREACHED()
> > is
> > > DCHECK(false).
> > 
> > OK. Are these methods going to be implemented at some point, though?
> 
> Yes, of course. Do you want to see comments?

Yeah TODO would probably be good (w/ associated bug). NOTIMPLEMENTED has the
nice property of being self-documenting, which is why I raised it.

[Mark Mentovai, 2015-03-04 17:24:58]

Filed a bug and pointed to it via comments on the NOTREACHED()s.

[Robert Sesek, 2015-03-04 17:26:38]

LGTM

[Mark Mentovai, 2015-03-04 17:29:03]

Committed patchset #4 (id:80001) manually as
445c0eae7ca33491a1d3ba98339bd44035dfb49c (presubmit successful).