Refactor handler/main for Windows, implement CrashHandlerExceptionServer

Refactor handler/main for Windows, implement CrashHandlerExceptionServer

BUG=crashpad:1
R=mark@chromium.org

Committed: https://chromium.googlesource.com/crashpad/crashpad/+/5de461e8c8d9cef9da09e2525918682635d562ea

[scottmg, 2015-08-26 21:27:59]

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

[scottmg, 2015-09-03 18:15:10]

This makes handler/main.cc a bit noisier due to #ifs, but enough seemed common
that it was better than not sharing the code.

[Mark Mentovai, 2015-09-03 18:33:37]

This is so cool!

https://codereview.chromium.org/1314093002/diff/120001/handler/handler.gyp
File handler/handler.gyp (right):

https://codereview.chromium.org/1314093002/diff/120001/handler/handler.gyp#ne...
handler/handler.gyp:75: 'target_name': 'crashpad_handler',
Can we share this target between mac and win now?

https://codereview.chromium.org/1314093002/diff/120001/handler/main.cc
File handler/main.cc (right):

https://codereview.chromium.org/1314093002/diff/120001/handler/main.cc#newcode59
handler/main.cc:59: "      --pipe-name=PIPE        communicate with the client
over PIPE\n"
Update crashpad_handler.ad (and move it up a level).

https://codereview.chromium.org/1314093002/diff/120001/handler/main.cc#newcod...
handler/main.cc:231: );
Nit: align with the (.

https://codereview.chromium.org/1314093002/diff/120001/handler/win/crash_repo...
File handler/win/crash_report_exception_handler.cc (right):

https://codereview.chromium.org/1314093002/diff/120001/handler/win/crash_repo...
handler/win/crash_report_exception_handler.cc:63: if
(!process_snapshot.Initialize(process)) {
ProcessSnapshotWin is going to make a ProcessReader, but you just made one to
read the ExceptionInformation struct, and now that original ProcessReader is
going to go to waste. There are a few ways to fix this, do you want to clean it
up?

https://codereview.chromium.org/1314093002/diff/120001/handler/win/crash_repo...
handler/win/crash_report_exception_handler.cc:64: LOG(WARNING) <<
"ProcessSnapshotWin Initialize failed";
You can use :: here, and you can say ProcessSnapshotWin::InitializeException on
line 71.

https://codereview.chromium.org/1314093002/diff/120001/handler/win/crash_repo...
handler/win/crash_report_exception_handler.cc:77: unsigned int termination_code
= process_snapshot.Exception()->Exception();
OK, but not required, to be const. (I’m trying to use const more frequently for
more than just traditional kConstants to prevent accidental modification.)

[scottmg, 2015-09-03 19:54:31]

Yeah, I was happy the top-level ended up being so similar in the end. :)

https://codereview.chromium.org/1314093002/diff/120001/handler/handler.gyp
File handler/handler.gyp (right):

https://codereview.chromium.org/1314093002/diff/120001/handler/handler.gyp#ne...
handler/handler.gyp:75: 'target_name': 'crashpad_handler',
On 2015/09/03 18:33:37, Mark Mentovai - August is over wrote:
> Can we share this target between mac and win now?

Indeed, and that fixes some missing dependencies.

https://codereview.chromium.org/1314093002/diff/120001/handler/main.cc
File handler/main.cc (right):

https://codereview.chromium.org/1314093002/diff/120001/handler/main.cc#newcode59
handler/main.cc:59: "      --pipe-name=PIPE        communicate with the client
over PIPE\n"
On 2015/09/03 18:33:37, Mark Mentovai - August is over wrote:
> Update crashpad_handler.ad (and move it up a level).

Done.

https://codereview.chromium.org/1314093002/diff/120001/handler/main.cc#newcod...
handler/main.cc:231: );
On 2015/09/03 18:33:37, Mark Mentovai - August is over wrote:
> Nit: align with the (.

clang-format wanted it aligned with the options, not sure. Maybe separate lines
are better in this case, at the cost of some minor duplication.

https://codereview.chromium.org/1314093002/diff/120001/handler/win/crash_repo...
File handler/win/crash_report_exception_handler.cc (right):

https://codereview.chromium.org/1314093002/diff/120001/handler/win/crash_repo...
handler/win/crash_report_exception_handler.cc:63: if
(!process_snapshot.Initialize(process)) {
On 2015/09/03 18:33:37, Mark Mentovai - August is over wrote:
> ProcessSnapshotWin is going to make a ProcessReader, but you just made one to
> read the ExceptionInformation struct, and now that original ProcessReader is
> going to go to waste. There are a few ways to fix this, do you want to clean
it
> up?

Done.

https://codereview.chromium.org/1314093002/diff/120001/handler/win/crash_repo...
handler/win/crash_report_exception_handler.cc:64: LOG(WARNING) <<
"ProcessSnapshotWin Initialize failed";
On 2015/09/03 18:33:37, Mark Mentovai - August is over wrote:
> You can use :: here, and you can say ProcessSnapshotWin::InitializeException
on
> line 71.

Done.

https://codereview.chromium.org/1314093002/diff/120001/handler/win/crash_repo...
handler/win/crash_report_exception_handler.cc:77: unsigned int termination_code
= process_snapshot.Exception()->Exception();
On 2015/09/03 18:33:37, Mark Mentovai - August is over wrote:
> OK, but not required, to be const. (I’m trying to use const more frequently
for
> more than just traditional kConstants to prevent accidental modification.)

Done.

[Mark Mentovai, 2015-09-03 20:07:29]

LGTM

https://codereview.chromium.org/1314093002/diff/140001/handler/crashpad_handl...
File handler/crashpad_handler.ad (right):

https://codereview.chromium.org/1314093002/diff/140001/handler/crashpad_handl...
handler/crashpad_handler.ad:1: // Copyright 2014 The Crashpad Authors. All
rights reserved.
Also update doc/support/generate_asciidoc.sh.

https://codereview.chromium.org/1314093002/diff/140001/handler/crashpad_handl...
handler/crashpad_handler.ad:78: This option is required. This option is only
valid on Mac.
Mac OS X in formal documentation, or just OS X (which is what Apple says the
proper name is now).

https://codereview.chromium.org/1314093002/diff/140001/handler/crashpad_handl...
handler/crashpad_handler.ad:82: the form \\.\pipe\<somename>. This option is
required. This option is only valid
Put + signs around this like +\\.\pipe\<somename>+ to render it in monospaced
text.

https://codereview.chromium.org/1314093002/diff/140001/handler/win/crash_repo...
File handler/win/crash_report_exception_handler.cc (right):

https://codereview.chromium.org/1314093002/diff/140001/handler/win/crash_repo...
handler/win/crash_report_exception_handler.cc:21: #include
"snapshot/win/process_reader_win.h"
Unused now.

https://codereview.chromium.org/1314093002/diff/140001/handler/win/crash_repo...
handler/win/crash_report_exception_handler.cc:68: if
(client_options.crashpad_handler_behavior != TriState::kDisabled) {
For a follow-up if at all: is there anything we can do with the “forwarding”
option, or is it better to declare lack of support for that on Windows?

https://codereview.chromium.org/1314093002/diff/140001/snapshot/win/exception...
File snapshot/win/exception_snapshot_win_test.cc (right):

https://codereview.chromium.org/1314093002/diff/140001/snapshot/win/exception...
snapshot/win/exception_snapshot_win_test.cc:23: #include
"snapshot/win/process_reader_win.h"
No longer necessary.

[Mark Mentovai, 2015-09-03 20:08:56]

scottmg wrote:
> Yeah, I was happy the top-level ended up being so similar in the end. :)

Yeah, pretty impressive!

I wouldn’t expect it basd on how cleanly this one came together, but if anything
about the design feels forced on Windows, please say something.

[scottmg, 2015-09-03 20:30:01]

Thanks

https://codereview.chromium.org/1314093002/diff/140001/handler/crashpad_handl...
File handler/crashpad_handler.ad (right):

https://codereview.chromium.org/1314093002/diff/140001/handler/crashpad_handl...
handler/crashpad_handler.ad:1: // Copyright 2014 The Crashpad Authors. All
rights reserved.
On 2015/09/03 20:07:29, Mark Mentovai - August is over wrote:
> Also update doc/support/generate_asciidoc.sh.

Done.

https://codereview.chromium.org/1314093002/diff/140001/handler/crashpad_handl...
handler/crashpad_handler.ad:78: This option is required. This option is only
valid on Mac.
On 2015/09/03 20:07:29, Mark Mentovai - August is over wrote:
> Mac OS X in formal documentation, or just OS X (which is what Apple says the
> proper name is now).

Done.

https://codereview.chromium.org/1314093002/diff/140001/handler/crashpad_handl...
handler/crashpad_handler.ad:82: the form \\.\pipe\<somename>. This option is
required. This option is only valid
On 2015/09/03 20:07:29, Mark Mentovai - August is over wrote:
> Put + signs around this like +\\.\pipe\<somename>+ to render it in monospaced
> text.

Done.

https://codereview.chromium.org/1314093002/diff/140001/handler/win/crash_repo...
File handler/win/crash_report_exception_handler.cc (right):

https://codereview.chromium.org/1314093002/diff/140001/handler/win/crash_repo...
handler/win/crash_report_exception_handler.cc:21: #include
"snapshot/win/process_reader_win.h"
On 2015/09/03 20:07:29, Mark Mentovai - August is over wrote:
> Unused now.

Done.

https://codereview.chromium.org/1314093002/diff/140001/handler/win/crash_repo...
handler/win/crash_report_exception_handler.cc:68: if
(client_options.crashpad_handler_behavior != TriState::kDisabled) {
On 2015/09/03 20:07:29, Mark Mentovai - August is over wrote:
> For a follow-up if at all: is there anything we can do with the “forwarding”
> option, or is it better to declare lack of support for that on Windows?

I'm not sure. The only obvious way to pass it on to the system is to have the
client return EXCEPTION_CONTINUE_SEARCH from the unhandled exception filter. So
we'd have to have the server communicate back to the client that it wasn't going
to handle the exception, but that seems a bit tortured since the client could
just not signal the event in that case. (Assuming I understand what the
forwarding is doing.)

https://codereview.chromium.org/1314093002/diff/140001/snapshot/win/exception...
File snapshot/win/exception_snapshot_win_test.cc (right):

https://codereview.chromium.org/1314093002/diff/140001/snapshot/win/exception...
snapshot/win/exception_snapshot_win_test.cc:23: #include
"snapshot/win/process_reader_win.h"
On 2015/09/03 20:07:29, Mark Mentovai - August is over wrote:
> No longer necessary.

Done.

[scottmg, 2015-09-03 20:31:23]

Committed patchset #9 (id:160001) manually as
5de461e8c8d9cef9da09e2525918682635d562ea (presubmit successful).