handler: Add report upload

handler: Add report upload.

The handler is now capable of uploading crash reports from the database.
At present, only one upload attempt is made, and the report will be
moved to “completed” in the database after the attempt, regardless of
whether it succeeded or failed.

The handler also has support to push annotations from its command line
into the process annotations map of each crash report it writes. This is
intended to set basic information about each crash report, such as the
product ID and version. Each potentially crashy process can’t be relied
on to maintain this information on their own.

With this change, Crashpad is now 100% capable of running a handler that
maintains a database and uploads crash reports to a Breakpad-type server
such that Breakpad properly interprets the reports. This is all possible
from the command line.

TEST=run_with_crashpad --handler crashpad_handler \
         -a --database=/tmp/crashpad_db \
         -a --url=https://clients2.google.com/cr/staging_report \
         -a --annotation=prod=crashpad \
         -a --annotation=ver=0.6.0 \
         crashy_program

R=rsesek@chromium.org

Committed: https://chromium.googlesource.com/crashpad/crashpad/+/ae1ccf621b00a2d753e5a0e5341e2c63b9248b49

[Mark Mentovai, 2015-03-04 22:10:15]

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

[Mark Mentovai, 2015-03-04 22:10:36]

Here are some test crashes that I generated:

go/crash/1d2c98446ec51edb: This is a client of the Crashpad client interface
that set a module annotation for the key-value pair “crash”: “expected” before
calling __builtin_trap().

go/crash/33e948ae0ca8850c: This is not a client of the Crashpad client
interface. It called abort(), and the CrashReporter annotation “abort() called”
set by Libc shows up in the “list_annotations” key.

go/crash/839a0da2bc3ba0fb: This just called __builtin_trap(). No annotations and
no HTTP form parameters were provided other than the “prod” and “ver” values
inserted by the handler as process annotations. These are the minimum required
to get the server to accept the report.

go/crash/0c989ad87ae66721: This is a client of the Crashpad client interface
that set a module annotation for the key-value pair “crash”: “simulated” before
calling CRASHPAD_SIMULATE_CRASH() and then exiting cleanly.

There are no stacks because the symbol server doesn’t have symbol files for
these test programs.

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

Crash stacks weren’t showing up because I wasn’t adding a
MINIDUMP_EXCEPTION_STREAM to the minidumps. I’ve fixed that in the latest patch
set. (It was just missing a single call.)

The new test crashes, which do show exception stacks (but they’re still mostly
not symbolized) are:

go/crash/ce01aea646e79583: just __builtin_trap()
go/crash/e02a31818aed6872: set a custom annotation (“crash key”) and then call
__builtin_trap()
go/crash/126b6919d545b9ff: call abort()
go/crash/7b524a6197c95cd8: set a custom annotation, call
CRASHPAD_SIMULATE_CRASH(), and exit cleanly

Even though most symbols weren’t available for these tests, the abort() test
shows that abort() from libsystem_c.dylib is symbolized correctly.

[Robert Sesek, 2015-03-05 00:14:42]

Very exciting! FYI there's also a staging crash server that's probably a better
choice to use for uploading test reports.

https://codereview.chromium.org/982613002/diff/1/handler/mac/crash_report_exc...
File handler/mac/crash_report_exception_handler.h (right):

https://codereview.chromium.org/982613002/diff/1/handler/mac/crash_report_exc...
handler/mac/crash_report_exception_handler.h:39: //! \param[in]
process_annotations A map of annotations to insert into each
Chrome's crash keys will not be set here, correct? Chrome's will be from the
Framework's module of annotations. If so, what are some example process
annotations?

https://codereview.chromium.org/982613002/diff/1/handler/mac/crash_report_exc...
handler/mac/crash_report_exception_handler.h:44: const std::map<std::string,
std::string>& process_annotations);
Since this parameter is being stored by reference, it may be better to use a
pointer here to convey to the caller the lifetime requirements (i.e. this is not
copied).

https://codereview.chromium.org/982613002/diff/1/handler/mac/crash_report_upl...
File handler/mac/crash_report_upload_thread.cc (right):

https://codereview.chromium.org/982613002/diff/1/handler/mac/crash_report_upl...
handler/mac/crash_report_upload_thread.cc:237: UploadResult upload_result =
UploadReport(*upload_report, &response_body);
Why switch from const* to const& ?

https://codereview.chromium.org/982613002/diff/1/handler/mac/crash_report_upl...
handler/mac/crash_report_upload_thread.cc:246: // TODO(mark): Deal with retries
properly.
Intention here will be RecordUploadAttempt(…, false, …) right?

[Mark Mentovai, 2015-03-05 19:41:31]

Feedback addressed. Thanks for the review!

I’m in the (admittedly bad) habit of not using the staging server because it
used to accept uploads at a not-memorable address, and it was easier to just
grab the upload URL out of Chrome. Now that it’s basically the same address,
there’s really no excuse.

But after I made the requested changes, I re-ran the tests against the staging
server, this time with symbols uploaded too.

go/crash-staging/c8cbe5a7705b3cca: builtin_trap
go/crash-staging/c8cbe5a7705b3756: abort
go/crash-staging/c8cbe5a7705b3829: client_builtin_trap
go/crash-staging/c8cbe5a7705b3958: simulate_crash

I did upload symbols for libdyld and libsystem_kernel, so I’m not sure why it
didn’t process with those, but all of the symbols from the main executables are
present, along with libsystem_c for the abort test.

https://codereview.chromium.org/982613002/diff/1/handler/mac/crash_report_exc...
File handler/mac/crash_report_exception_handler.h (right):

https://codereview.chromium.org/982613002/diff/1/handler/mac/crash_report_exc...
handler/mac/crash_report_exception_handler.h:44: const std::map<std::string,
std::string>& process_annotations);
Robert Sesek wrote:
> Since this parameter is being stored by reference, it may be better to use a
> pointer here to convey to the caller the lifetime requirements (i.e. this is
not
> copied).

I used a reference because that’s what the callee,
ProcessSnapshotMac::SetAnnotationsSimpleMap(), wanted. But I can store a pointer
instead, I’ve just got to do the *dereference where it’s used now.

https://codereview.chromium.org/982613002/diff/1/handler/mac/crash_report_upl...
File handler/mac/crash_report_upload_thread.cc (right):

https://codereview.chromium.org/982613002/diff/1/handler/mac/crash_report_upl...
handler/mac/crash_report_upload_thread.cc:237: UploadResult upload_result =
UploadReport(*upload_report, &response_body);
Robert Sesek wrote:
> Why switch from const* to const& ?

At first I was using |report| instead of |upload_report| here, so it was a
reference to begin with. Later, I changed it to use |upload_report|, which is
more correct since it conceptually represents the report locked for upload. I’ll
fix the type to match.

https://codereview.chromium.org/982613002/diff/1/handler/mac/crash_report_upl...
handler/mac/crash_report_upload_thread.cc:246: // TODO(mark): Deal with retries
properly.
Robert Sesek wrote:
> Intention here will be RecordUploadAttempt(…, false, …) right?

Yeah, but the Fire() on the line above already does that. For a retry, we’ll
just want to avoid calling SkipReportUpload(). Do you think that having the
scoper make the call is unclear? I get rid of Fire(), call Disarm(), and then
SkipReportUpload() right here.

[Robert Sesek, 2015-03-05 20:36:15]

LGTM

[Mark Mentovai, 2015-03-05 20:40:48]

Committed patchset #3 (id:40001) manually as
ae1ccf621b00a2d753e5a0e5341e2c63b9248b49 (presubmit successful).