Add most of ProcessReader and its test

Add most of ProcessReader and its test.

TEST=util_test ProcessReader.*
R=rsesek@chromium.org

Committed: https://chromium.googlesource.com/crashpad/crashpad/+/8256f9fc23ab

[Mark Mentovai, 2014-08-20 19:07:28]

The Modules() portion of ProcessReader depends on process_types, but
process_types depends on ProcessReader and has not yet landed. The circular
relationship isn’t wonderful. The problem is that process_types objects need
both the target process’ TaskMemory and its bitness. ProcessReader objects have
both of those, so they seem like really good things to pass to process_types.
Unfortunately, ProcessReader also needs to be able to do things like figure out
what modules are loaded, and doing so requires some a few process_types structs.

[Robert Sesek, 2014-08-21 15:26:55]

https://codereview.chromium.org/491963002/diff/1/util/mac/process_reader.cc
File util/mac/process_reader.cc (right):

https://codereview.chromium.org/491963002/diff/1/util/mac/process_reader.cc#n...
util/mac/process_reader.cc:101: for (ProcessReaderThread& thread : threads_) {
const& ?

https://codereview.chromium.org/491963002/diff/1/util/mac/process_reader.cc#n...
util/mac/process_reader.cc:227: base::mac::ScopedMachSendRight
thread_port_owner(thread.port);
Maybe leave a comment as to why you scope it here, since this function is so
long the .release() at the end of the loop body is off screen.

https://codereview.chromium.org/491963002/diff/1/util/mac/process_reader.cc#n...
util/mac/process_reader.cc:229: #if defined(ARCH_CPU_X86_FAMILY)
When Apple produces a ARM-based Mac, this could get messy…

https://codereview.chromium.org/491963002/diff/1/util/mac/process_reader.cc#n...
util/mac/process_reader.cc:398: if (Is64Bit()) {
You could pull this into a LocateRedZone(mach_vm_address_t start_addr,
mach_vm_address_t region_base, mach_vm_size_t region_size) helper.

https://codereview.chromium.org/491963002/diff/1/util/mac/process_reader.cc#n...
util/mac/process_reader.cc:402: const mach_vm_size_t kRedZoneSize = 128;
Optional: Since this size is mandated by the ABI, and if Crashpad is ultimately
going to be cross platform, this could be moved into a non-Mac-specific header.

https://codereview.chromium.org/491963002/diff/1/util/mac/process_reader.cc#n...
util/mac/process_reader.cc:430: // already found, it’s readable (not the guard
region), and they’ve
nit: Pronoun agreement in this sentence.

https://codereview.chromium.org/491963002/diff/1/util/mac/process_reader.cc#n...
util/mac/process_reader.cc:453: // region (or regions) already found.
nit: Missing a verb.

https://codereview.chromium.org/491963002/diff/1/util/mac/process_reader.h
File util/mac/process_reader.h (right):

https://codereview.chromium.org/491963002/diff/1/util/mac/process_reader.h#ne...
util/mac/process_reader.h:120: //! \return The threads that are in the task
(process).
It's not abundantly clear that the .port will be invalidated if this vector is
copied and ProcessReader is destroyed.

https://codereview.chromium.org/491963002/diff/1/util/mac/process_reader_test.cc
File util/mac/process_reader_test.cc (right):

https://codereview.chromium.org/491963002/diff/1/util/mac/process_reader_test...
util/mac/process_reader_test.cc:77: ssize_t rv = ReadFD(fd, &pid, sizeof(pid));
Couldn't you rely on MachMultiprocess::ChildPid()?

https://codereview.chromium.org/491963002/diff/1/util/mac/process_reader_test...
util/mac/process_reader_test.cc:99: const char kTestMemory[] = "Read me from
another process";
Make this a static member that can be used by both Parent() and Child()?

https://codereview.chromium.org/491963002/diff/1/util/mac/process_reader_test...
util/mac/process_reader_test.cc:114: }
No test for threads of another process?

[Mark Mentovai, 2014-08-21 21:40:52]

This now depends on https://codereview.chromium.org/491363002/.

https://codereview.chromium.org/491963002/diff/1/util/mac/process_reader.cc
File util/mac/process_reader.cc (right):

https://codereview.chromium.org/491963002/diff/1/util/mac/process_reader.cc#n...
util/mac/process_reader.cc:227: base::mac::ScopedMachSendRight
thread_port_owner(thread.port);
rsesek wrote:
> Maybe leave a comment as to why you scope it here, since this function is so
> long the .release() at the end of the loop body is off screen.

I think I’ll get rid of thread_port_owner and replace it with a comment. It’d be
one thing to have a small loop to set up send right owners for every thread in
the list, but as it is, one of the things that this loop does is set up the send
right owners. An early return in the loop would still leak send rights for all
of the threads that hadn’t been processed yet.

The new comment explains what’s going on, but I’m also thinking that maybe we
should invent a ScopedMustNotReturn object that will abort when destroyed unless
disarmed—we’d arm it right after the task_threads call, and disarm it after the
loop. What do you think?

https://codereview.chromium.org/491963002/diff/1/util/mac/process_reader.cc#n...
util/mac/process_reader.cc:402: const mach_vm_size_t kRedZoneSize = 128;
rsesek wrote:
> Optional: Since this size is mandated by the ABI, and if Crashpad is
ultimately
> going to be cross platform, this could be moved into a non-Mac-specific
header.

I’d like to do that, but I don’t have a good place for ABI-specific things yet.

Also, adherence to a specific ABI is normally an OS+CPU-specific thing, so in
many cases, I’m not sure there’d be too much value. Mac and Linux happen to use
the exact same ABI for x86_64 (at least for most things), but their ABIs are
more divergent for 32-bit x86. The Windows x86_64 ABI, to take another example,
is totally different from the Mac and Linux one.

https://codereview.chromium.org/491963002/diff/1/util/mac/process_reader_test.cc
File util/mac/process_reader_test.cc (right):

https://codereview.chromium.org/491963002/diff/1/util/mac/process_reader_test...
util/mac/process_reader_test.cc:77: ssize_t rv = ReadFD(fd, &pid, sizeof(pid));
rsesek wrote:
> Couldn't you rely on MachMultiprocess::ChildPid()?

Yes, but this was also testing that ChildPID() worked correctly. I guess that
should happen in mach_multiprocess_test.

[Robert Sesek, 2014-08-25 21:31:42]

LGTM. That test took forever to review.

https://codereview.chromium.org/491963002/diff/40001/util/mac/process_reader.h
File util/mac/process_reader.h (right):

https://codereview.chromium.org/491963002/diff/40001/util/mac/process_reader....
util/mac/process_reader.h:176: //! \param[inout] region_side The size of the
region that contains stack
sp: region_size

[Mark Mentovai, 2014-08-25 21:51:13]

Committed patchset #4 manually as 8256f9fc23ab (presubmit successful).

[Mark Mentovai, 2014-08-25 21:51:59]

I’m happy to have this huge chunk checked in. Iterating on it will be much
easier.

[Mark Mentovai, 2014-08-25 22:05:00]

53% as of this being checked in.