Implement more of CrashpadClient on Windows

Implement more of CrashpadClient on Windows

SetHandler connects to RegistrationServer as a handler,
and catching an exception writes EXCEPTION_POINTERS to
CrashpadInfo.

R=cpu@chromium.org, mark@chromium.org
BUG=crashpad:1
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/1a770c8237733e2f68bd506a99d05d82fe25fc6b

[scottmg, 2015-08-13 04:43:18]

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

[Mark Mentovai, 2015-08-13 13:38:53]

https://codereview.chromium.org/1287073002/diff/80001/client/crashpad_client_...
File client/crashpad_client_win.cc (right):

https://codereview.chromium.org/1287073002/diff/80001/client/crashpad_client_...
client/crashpad_client_win.cc:42:
crashpad::CrashpadInfo::GetCrashpadInfo()->set_exception_pointers(
Is there a guarantee that this will only be called on one thread at a time?

https://codereview.chromium.org/1287073002/diff/80001/client/crashpad_client_...
client/crashpad_client_win.cc:64: const int kMaxRetries = 5;
Since the first try doesn’t count as a REtry, name these const int kMaxTries and
int try?

https://codereview.chromium.org/1287073002/diff/80001/client/crashpad_client_...
client/crashpad_client_win.cc:71: NULL,
nullptr, line 74 too.

https://codereview.chromium.org/1287073002/diff/80001/client/crashpad_client_...
client/crashpad_client_win.cc:86: LOG(ERROR) << "Connect";
There are three things that can fail above. Is it useful to know which one
failed and to get the reason for failure included with PLOG?

https://codereview.chromium.org/1287073002/diff/80001/client/crashpad_info.h
File client/crashpad_info.h (right):

https://codereview.chromium.org/1287073002/diff/80001/client/crashpad_info.h#...
client/crashpad_info.h:131: WinVMAddress exception_pointers_;
Since this always points to the local address space, it should be a native
pointer type. You shouldn’t need address_types.h in here. simple_annotations_ is
referenced by a SimpleStringDictionary* here, for example.

[scottmg, 2015-08-13 22:40:29]

Patchset #6 (id:90001) has been deleted

[scottmg, 2015-08-13 22:41:46]

https://codereview.chromium.org/1287073002/diff/80001/client/crashpad_client_...
File client/crashpad_client_win.cc (right):

https://codereview.chromium.org/1287073002/diff/80001/client/crashpad_client_...
client/crashpad_client_win.cc:42:
crashpad::CrashpadInfo::GetCrashpadInfo()->set_exception_pointers(
On 2015/08/13 13:38:53, Mark Mentovai - out til August wrote:
> Is there a guarantee that this will only be called on one thread at a time?

Good call. I went back and forth on this for a while, but I think what's here
now makes sense for multiple simultaneous crashes.

https://codereview.chromium.org/1287073002/diff/80001/client/crashpad_client_...
client/crashpad_client_win.cc:64: const int kMaxRetries = 5;
On 2015/08/13 13:38:53, Mark Mentovai - out til August wrote:
> Since the first try doesn’t count as a REtry, name these const int kMaxTries
and
> int try?

Done. ("try" not so good in C++ :)

https://codereview.chromium.org/1287073002/diff/80001/client/crashpad_client_...
client/crashpad_client_win.cc:71: NULL,
On 2015/08/13 13:38:53, Mark Mentovai - out til August wrote:
> nullptr, line 74 too.

Done.

https://codereview.chromium.org/1287073002/diff/80001/client/crashpad_client_...
client/crashpad_client_win.cc:86: LOG(ERROR) << "Connect";
On 2015/08/13 13:38:53, Mark Mentovai - out til August wrote:
> There are three things that can fail above. Is it useful to know which one
> failed and to get the reason for failure included with PLOG?

Done.

[scottmg, 2015-08-13 22:43:23]

Oops, forgot to note this one also.

https://codereview.chromium.org/1287073002/diff/80001/client/crashpad_info.h
File client/crashpad_info.h (right):

https://codereview.chromium.org/1287073002/diff/80001/client/crashpad_info.h#...
client/crashpad_info.h:131: WinVMAddress exception_pointers_;
On 2015/08/13 13:38:53, Mark Mentovai - out til August wrote:
> Since this always points to the local address space, it should be a native
> pointer type. You shouldn’t need address_types.h in here. simple_annotations_
is
> referenced by a SimpleStringDictionary* here, for example.

Done.

[cpu_(ooo_6.6-7.5), 2015-08-14 00:47:13]

windows side lgtm

https://codereview.chromium.org/1287073002/diff/110001/client/crashpad_client...
File client/crashpad_client_win.cc (right):

https://codereview.chromium.org/1287073002/diff/110001/client/crashpad_client...
client/crashpad_client_win.cc:57: // location.
yeah, it would be nice to know the count of threads that tried to enter after
the first one but blocking is the right call imho.

https://codereview.chromium.org/1287073002/diff/110001/client/crashpad_client...
client/crashpad_client_win.cc:89: PLOG(ERROR) << "WaitNamedPipe";
WaitNamedPipe internally tries to open the NPFS device, so it is a bit slow and
beholden to AV crazies. The recommended way is to try to Open first and if that
fails then wait on it.

I am not sure if infinite timeout is the right amount, sort of makes hosedness
of the crash service manifest into hosedness of chrome but we can start there. I
am guessing at some point we want to error out after some time, maybe in the
minutes. I mean as long we can get telemetry back to us of this case. If we
don't have telemetry back to us then infinite is better.

https://codereview.chromium.org/1287073002/diff/110001/client/crashpad_client...
client/crashpad_client_win.cc:147: LoggingReadFile(pipe.get(), &response,
sizeof(response));
TransactNamedPipe is available if you want to, but I am fine with write + read. 
Just sayin'

[Mark Mentovai, 2015-08-14 14:20:55]

LGTM

https://codereview.chromium.org/1287073002/diff/110001/client/crashpad_client...
File client/crashpad_client_win.cc (right):

https://codereview.chromium.org/1287073002/diff/110001/client/crashpad_client...
client/crashpad_client_win.cc:55: // the exception pointers from the second and
further crashes, but contention
Missing a word.

https://codereview.chromium.org/1287073002/diff/110001/client/crashpad_client...
client/crashpad_client_win.cc:91: pipe.reset(CreateFile(pipe_name.c_str(),
So regardless of whether WaitNamedPipe() succeeds, you proceed to the
CreateFile()?

[scottmg, 2015-08-14 21:13:21]

https://codereview.chromium.org/1287073002/diff/110001/client/crashpad_client...
File client/crashpad_client_win.cc (right):

https://codereview.chromium.org/1287073002/diff/110001/client/crashpad_client...
client/crashpad_client_win.cc:55: // the exception pointers from the second and
further crashes, but contention
On 2015/08/14 14:20:54, Mark Mentovai - out til August wrote:
> Missing a word.

Done.

https://codereview.chromium.org/1287073002/diff/110001/client/crashpad_client...
client/crashpad_client_win.cc:57: // location.
On 2015/08/14 00:47:13, cpu (OOO until 8-15) wrote:
> yeah, it would be nice to know the count of threads that tried to enter after
> the first one but blocking is the right call imho.

We could alias the count to the stack, but since it should be very uncommon
anyway, I think I won't bother adding the clutter for now.

https://codereview.chromium.org/1287073002/diff/110001/client/crashpad_client...
client/crashpad_client_win.cc:89: PLOG(ERROR) << "WaitNamedPipe";
On 2015/08/14 00:47:13, cpu (OOO until 8-15) wrote:
> WaitNamedPipe internally tries to open the NPFS device, so it is a bit slow
and
> beholden to AV crazies. The recommended way is to try to Open first and if
that
> fails then wait on it.
> 
> I am not sure if infinite timeout is the right amount, sort of makes hosedness
> of the crash service manifest into hosedness of chrome but we can start there.
I
> am guessing at some point we want to error out after some time, maybe in the
> minutes. I mean as long we can get telemetry back to us of this case. If we
> don't have telemetry back to us then infinite is better.

Thanks, switched to a 60s timeout and tried opening before waiting.
Telemetry-wise, we don't have anything in Crashpad, but we plan to gather the
LOG()s eventually so that would let us discover this case.

https://codereview.chromium.org/1287073002/diff/110001/client/crashpad_client...
client/crashpad_client_win.cc:91: pipe.reset(CreateFile(pipe_name.c_str(),
On 2015/08/14 14:20:54, Mark Mentovai - out til August wrote:
> So regardless of whether WaitNamedPipe() succeeds, you proceed to the
> CreateFile()?

Done.

https://codereview.chromium.org/1287073002/diff/110001/client/crashpad_client...
client/crashpad_client_win.cc:147: LoggingReadFile(pipe.get(), &response,
sizeof(response));
On 2015/08/14 00:47:13, cpu (OOO until 8-15) wrote:
> TransactNamedPipe is available if you want to, but I am fine with write +
read. 
> Just sayin'
> 

I think I'll stick with Write+Read since it has logging already.

[Mark Mentovai, 2015-08-14 22:19:49]

Still LGTM

https://codereview.chromium.org/1287073002/diff/130001/client/crashpad_client...
File client/crashpad_client_win.cc (right):

https://codereview.chromium.org/1287073002/diff/130001/client/crashpad_client...
client/crashpad_client_win.cc:102: } else {
else if

[scottmg, 2015-08-14 22:21:14]

https://codereview.chromium.org/1287073002/diff/130001/client/crashpad_client...
File client/crashpad_client_win.cc (right):

https://codereview.chromium.org/1287073002/diff/130001/client/crashpad_client...
client/crashpad_client_win.cc:102: } else {
On 2015/08/14 22:19:49, Mark Mentovai - out til August wrote:
> else if

Done.

[scottmg, 2015-08-14 22:22:14]

Committed patchset #8 (id:150001) manually as
1a770c8237733e2f68bd506a99d05d82fe25fc6b (presubmit successful).