win: Implement CrashpadClient::StartHandler()

win: Implement CrashpadClient::StartHandler()

BUG=crashpad:69
R=scottmg@chromium.org

Committed: https://chromium.googlesource.com/crashpad/crashpad/+/740c668e872456d969aa990a0316b1b8fdaba944

[Mark Mentovai, 2015-10-30 19:31:11]

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

[Mark Mentovai, 2015-10-30 19:31:12]

There’s no reason you have to use this in Chrome, but there’s also no reason it
shouldn’t work in Crashpad.

Initially, I thought I’d make this work by having this mode use an inheritable
anonymous pipe for registration, but that fell apart when I learned that
anonymous pipes are half-duplex and stream-oriented.

[scottmg, 2015-10-30 23:16:31]

Cool! I started on this, but didn't get this far.

LGTM

https://codereview.chromium.org/1428803006/diff/1/client/crashpad_client_win.cc
File client/crashpad_client_win.cc (right):

https://codereview.chromium.org/1428803006/diff/1/client/crashpad_client_win....
client/crashpad_client_win.cc:166: startup_info.hStdInput =
GetStdHandle(STD_INPUT_HANDLE);
This was one reason I hadn't done this yet. In order to use crashpad_handler
directly in Chrome we'd need to make it a windowed app, so then I wasn't sure
what to do with output.

https://codereview.chromium.org/1428803006/diff/1/client/crashpad_client_win....
client/crashpad_client_win.cc:171: const_cast<wchar_t*>(command_line.c_str()),
Does this actually make it mutable? (In practice I'm sure it doesn't matter, but
I thought it should be &command_line[0]). (CreateProcessW can write to the
argument.)

https://codereview.chromium.org/1428803006/diff/1/handler/crashpad_handler.ad
File handler/crashpad_handler.ad (right):

https://codereview.chromium.org/1428803006/diff/1/handler/crashpad_handler.ad...
handler/crashpad_handler.ad:49: to exit cleanly without crashing. When the
server loses all clients, it exits
, and --persistent isn't specified

https://codereview.chromium.org/1428803006/diff/1/handler/win/crashy_test_pro...
File handler/win/crashy_test_program.cc (right):

https://codereview.chromium.org/1428803006/diff/1/handler/win/crashy_test_pro...
handler/win/crashy_test_program.cc:101: if (!client.SetHandler(argv[1])) {
We could probably just get rid of the 2 arg version, but I guess good to have
some way to exercise that path if we use it in Chrome.

https://codereview.chromium.org/1428803006/diff/1/snapshot/win/end_to_end_tes...
File snapshot/win/end_to_end_test.py (right):

https://codereview.chromium.org/1428803006/diff/1/snapshot/win/end_to_end_tes...
snapshot/win/end_to_end_test.py:259: start_handler_dump_path =
GetDumpFromCrashyProgram(args[0], '')
I think None rather than '' would be better.

https://codereview.chromium.org/1428803006/diff/1/test/win/child_launcher.cc
File test/win/child_launcher.cc (right):

https://codereview.chromium.org/1428803006/diff/1/test/win/child_launcher.cc#...
test/win/child_launcher.cc:73: command_line += L" ";
Hmm, this is unfortunate. Oh well.

https://codereview.chromium.org/1428803006/diff/1/util/win/command_line.h
File util/win/command_line.h (right):

https://codereview.chromium.org/1428803006/diff/1/util/win/command_line.h#new...
util/win/command_line.h:23: //!
Note that it adds a space to separate arguments.

[Mark Mentovai, 2015-10-31 01:22:24]

https://codereview.chromium.org/1428803006/diff/1/client/crashpad_client_win.cc
File client/crashpad_client_win.cc (right):

https://codereview.chromium.org/1428803006/diff/1/client/crashpad_client_win....
client/crashpad_client_win.cc:166: startup_info.hStdInput =
GetStdHandle(STD_INPUT_HANDLE);
scottmg wrote:
> This was one reason I hadn't done this yet. In order to use crashpad_handler
> directly in Chrome we'd need to make it a windowed app, so then I wasn't sure
> what to do with output.

Can you explain this a little more?

These handles are one of the warts here, I think. I don’t really want to give
the new process a stdin at all, but I wasn’t sure if I could pass
INVALID_HANDLE_VALUE here or if that would do the right thing.

I was also planning on changing this in a follow-up to use
PROC_THREAD_ATTRIBUTE_LIST to limit the inherited handles, when the runtime OS
supports that.

https://codereview.chromium.org/1428803006/diff/1/client/crashpad_client_win....
client/crashpad_client_win.cc:171: const_cast<wchar_t*>(command_line.c_str()),
scottmg wrote:
> (CreateProcessW can write to the argument.)

I didn’t realize that! Should have read the documentation more closely. I’ll fix
it.

[Mark Mentovai, 2015-10-31 18:34:29]

https://codereview.chromium.org/1428803006/diff/1/client/crashpad_client_win.cc
File client/crashpad_client_win.cc (right):

https://codereview.chromium.org/1428803006/diff/1/client/crashpad_client_win....
client/crashpad_client_win.cc:166: startup_info.hStdInput =
GetStdHandle(STD_INPUT_HANDLE);
I wrote:
> scottmg wrote:
> > This was one reason I hadn't done this yet. In order to use crashpad_handler
> > directly in Chrome we'd need to make it a windowed app, so then I wasn't
sure
> > what to do with output.
> 
> Can you explain this a little more?

I guess that question is kinda vague…

What I mean is: was this going to be a problem if we were to run
crashpad_handler (in its current console program form) as a service without
being started by Chrome too?

Would it help to put STARTF_USESHOWWINDOW into startup_info.dwFlags and set
startup_info.wShowWindow to SW_HIDE?

As for the output, is there a handle that we can get that would go to a system
log or something? Chrome uses the same LOG() family and it’s a GUI program, how
does it deal with this?

[scottmg, 2015-10-31 18:34:59]

https://codereview.chromium.org/1428803006/diff/1/client/crashpad_client_win.cc
File client/crashpad_client_win.cc (right):

https://codereview.chromium.org/1428803006/diff/1/client/crashpad_client_win....
client/crashpad_client_win.cc:166: startup_info.hStdInput =
GetStdHandle(STD_INPUT_HANDLE);
On 2015/10/31 01:22:24, Mark Mentovai wrote:
> scottmg wrote:
> > This was one reason I hadn't done this yet. In order to use crashpad_handler
> > directly in Chrome we'd need to make it a windowed app, so then I wasn't
sure
> > what to do with output.
> 
> Can you explain this a little more?

Not strictly related, but this is where I stopped because I wasn't sure what to
do. In general, we want to have the logs from crashpad_handler go somewhere, but
in order to use crashpad_handler in Chrome it needs to be switched to a
/subsystem:windows. So then I wasn't sure about creating a log file, or creating
a console sometimes, or attaching to an existing console, or some other option.

> 
> These handles are one of the warts here, I think. I don’t really want to give
> the new process a stdin at all, but I wasn’t sure if I could pass
> INVALID_HANDLE_VALUE here or if that would do the right thing.'

I believe INVALID_HANDLE_VALUE is fine as long as we don't actually do a read
from it. That is, it stays truly invalid, it's not a null handle. For that, we
have to open "NUL" and pass that through.

> 
> I was also planning on changing this in a follow-up to use
> PROC_THREAD_ATTRIBUTE_LIST to limit the inherited handles, when the runtime OS
> supports that.

[Mark Mentovai, 2015-11-02 16:30:16]

Thanks. I updated this to address your comments, and I added a test for
AppendCommandLineArgument().

This is a good starting point now, and if we want to make further improvements,
we can iterate on it.

[scottmg, 2015-11-02 17:51:41]

On 2015/10/31 18:34:29, Mark Mentovai wrote:
>
https://codereview.chromium.org/1428803006/diff/1/client/crashpad_client_win.cc
> File client/crashpad_client_win.cc (right):
> 
>
https://codereview.chromium.org/1428803006/diff/1/client/crashpad_client_win....
> client/crashpad_client_win.cc:166: startup_info.hStdInput =
> GetStdHandle(STD_INPUT_HANDLE);
> I wrote:
> > scottmg wrote:
> > > This was one reason I hadn't done this yet. In order to use
crashpad_handler
> > > directly in Chrome we'd need to make it a windowed app, so then I wasn't
> sure
> > > what to do with output.
> > 
> > Can you explain this a little more?
> 
> I guess that question is kinda vague…
> 
> What I mean is: was this going to be a problem if we were to run
> crashpad_handler (in its current console program form) as a service without
> being started by Chrome too?

Yes, I think so.

> 
> Would it help to put STARTF_USESHOWWINDOW into startup_info.dwFlags and set
> startup_info.wShowWindow to SW_HIDE?

That seems "hokey" to me, but it might work. I'd be worried about some weird
case where the console window ends up showing up again, or flashing visible.

> 
> As for the output, is there a handle that we can get that would go to a system
> log or something? Chrome uses the same LOG() family and it’s a GUI program,
how
> does it deal with this?

It doesn't do a very good job. We've tried various things, for example having it
attach to the parent console if there is one, but it doesn't work that well. The
best thing to do at the moment for logs from chrome is to run it as `chrome
>stdout.txt 2>stderr.txt` which I guess we can do the equivalent of here.

[scottmg, 2015-11-02 17:55:58]

On 2015/11/02 16:30:16, Mark Mentovai wrote:
> Thanks. I updated this to address your comments, and I added a test for
> AppendCommandLineArgument().

Nice, LGTM.

> 
> This is a good starting point now, and if we want to make further
improvements,
> we can iterate on it.

[Mark Mentovai, 2015-11-02 18:59:42]

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