win: Don't try to inherit console handles in StartHandler()

win: Don't try to inherit console handles in StartHandler()

FILE_TYPE_CHAR handles can't be inherited via
PROC_THREAD_ATTRIBUTE_HANDLE_LIST, or CreateProcess() fails with
GetLastError() == 1450 on Windows 7.

I confirmed that an fprintf(stderr, ...) in HandlerMain() does make it
to the console when running tests even after this.

See bug for more discussion.

R=mark@chromium.org
BUG=crashpad:77
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/9b92d2fb7101bae2af9bb5447227df296b37b56a

[scottmg, 2015-11-25 00:03:06]

Description was changed from

==========
win: Don't try to inherit console handles in StartHandler()

FILE_TYPE_CHAR handles can't be inherited via
PROC_THREAD_ATTRIBUTE_HANDLE_LIST, or CreateProcess() fails with
GetLastError() == 1450 on Windows 7.

I confirmed that an fprintf(stderr, ...) in HandlerMain() does make it
to the console when running tests even after this.

See bug for more discussion.

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

to

==========
win: Don't try to inherit console handles in StartHandler()

FILE_TYPE_CHAR handles can't be inherited via
PROC_THREAD_ATTRIBUTE_HANDLE_LIST, or CreateProcess() fails with
GetLastError() == 1450 on Windows 7.

I confirmed that an fprintf(stderr, ...) in HandlerMain() does make it
to the console when running tests even after this.

See bug for more discussion.

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

[scottmg, 2015-11-25 00:03:52]

I'm not sure if this will fix the 0x57 error on the Chrome waterfall, but I
guess it's getting closer anyway.

[Mark Mentovai, 2015-11-25 00:33:16]

LGTM. Let’s try it at least.

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

https://codereview.chromium.org/1473793002/diff/60001/client/crashpad_client_...
client/crashpad_client_win.cc:134: // File handles (FILE_TYPE_DISK) and pipe
handles (FILE_TYPE_PIPE) are known
Blank line before.

https://codereview.chromium.org/1473793002/diff/60001/client/crashpad_client_...
client/crashpad_client_win.cc:137: //
https://bugs.chromium.org/p/crashpad/issues/detail?id=77.
https://crashpad.chromium.org/bug/77

https://codereview.chromium.org/1473793002/diff/60001/client/crashpad_client_...
client/crashpad_client_win.cc:164: using ScopedProcThreadAttributeList =
Can you keep this with the traits it belongs with? Either move this up or move
the traits down.

[scottmg, 2015-11-25 00:36:14]

Thanks. I confirmed it does help the Vista VM too, so I'm hopeful.

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

https://codereview.chromium.org/1473793002/diff/60001/client/crashpad_client_...
client/crashpad_client_win.cc:134: // File handles (FILE_TYPE_DISK) and pipe
handles (FILE_TYPE_PIPE) are known
On 2015/11/25 00:33:16, Mark Mentovai wrote:
> Blank line before.

Done.

https://codereview.chromium.org/1473793002/diff/60001/client/crashpad_client_...
client/crashpad_client_win.cc:137: //
https://bugs.chromium.org/p/crashpad/issues/detail?id=77.
On 2015/11/25 00:33:16, Mark Mentovai wrote:
> https://crashpad.chromium.org/bug/77

Done. (Doesn't quite fit with trailing '.' still.)

https://codereview.chromium.org/1473793002/diff/60001/client/crashpad_client_...
client/crashpad_client_win.cc:164: using ScopedProcThreadAttributeList =
On 2015/11/25 00:33:16, Mark Mentovai wrote:
> Can you keep this with the traits it belongs with? Either move this up or move
> the traits down.

Oops, I'll put it back.

[scottmg, 2015-11-25 00:36:34]

Committed patchset #5 (id:80001) manually as
9b92d2fb7101bae2af9bb5447227df296b37b56a (presubmit successful).

[Mark Mentovai, 2015-11-25 04:58:05]

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

https://codereview.chromium.org/1473793002/diff/80001/client/crashpad_client_...
client/crashpad_client_win.cc:234: startup_info.StartupInfo.hStdInput =
GetStdHandle(STD_INPUT_HANDLE);
We probably shouldn’t even be setting hStdInput/Output/Error for these
FILE_TYPE_CHAR handles. We’re not seeing errors, but they don’t seem
duplicatable or inheritable at all, so setting them here can’t actually be
working for FILE_TYPE_CHAR.

[scottmg, 2015-11-25 18:13:58]

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

https://codereview.chromium.org/1473793002/diff/80001/client/crashpad_client_...
client/crashpad_client_win.cc:234: startup_info.StartupInfo.hStdInput =
GetStdHandle(STD_INPUT_HANDLE);
On 2015/11/25 04:58:05, Mark Mentovai wrote:
> We probably shouldn’t even be setting hStdInput/Output/Error for these
> FILE_TYPE_CHAR handles. We’re not seeing errors, but they don’t seem
> duplicatable or inheritable at all, so setting them here can’t actually be
> working for FILE_TYPE_CHAR.

They are duplicatable I think. I'm not clear on whether they not inheritable, or
whether they're just not inherit-restrictable.

Did you have something in mind? We need to set a value for all of the handle if
we set if for any when setting STARTF_USESTDHANDLES.

[Mark Mentovai, 2015-11-25 18:17:45]

If it’s not inheritable, the best we can do in those fields is
INVALID_HANDLE_VALUE. The OS has already indicated to us that it doesn’t like
those handles in a related context (the PROC_THREAD_ATTRIBUTE_LIST), so we
should probably stop giving them to it in this context too.

If that’s what all three are, I suppose we could clear STARTF_USESTDHANDLES too.
But that might wind up being treated differently, so maybe not.

[scottmg, 2015-11-25 18:28:31]

On 2015/11/25 18:17:45, Mark Mentovai wrote:
> If it’s not inheritable, the best we can do in those fields is
> INVALID_HANDLE_VALUE. The OS has already indicated to us that it doesn’t like
> those handles in a related context (the PROC_THREAD_ATTRIBUTE_LIST), so we
> should probably stop giving them to it in this context too.

I was imagining that it _could_ be checking that those were the parent console,
and then attaching the child to the console if it was (unrelated to
inheritance).

I don't see any documentation of whether INVALID_HANDLE_VALUE (or maybe 0)
indicates "go back to default behaviour of attaching to console".
https://msdn.microsoft.com/en-ca/library/windows/desktop/ms686331(v=vs.85).aspx
only discusses valid values when STARTF_USESTDHANDLES is set.

But that's all speculation. I haven't found any helpful specific documentation
either way, so would need to do some exhaustive testing to confirm what actually
happens in practice.

> 
> If that’s what all three are, I suppose we could clear STARTF_USESTDHANDLES
too.
> But that might wind up being treated differently, so maybe not.

[Mark Mentovai, 2015-11-25 21:35:18]

Ugh, I just had this test failure locally (32-bit Debug):

[ RUN      ] CrashpadClient.StartWithInvalidHandles
c:\chrome\crashpad\crashpad\test\scoped_temp_dir_win.cc(95): error: Value of:
DeleteFile(entry_path.value().c_str())
  Actual: false
Expected: true
c:\chrome\crashpad\crashpad\test\scoped_temp_dir_win.cc(100): error: Value of:
RemoveDirectory(path.value().c_str())
  Actual: false
Expected: true
c:\chrome\crashpad\crashpad\test\win\win_multiprocess.h(64): error: Value of:
exit_code
  Actual: 255
Expected: parent_process.exit_code_
Which is: 0
[  FAILED  ] CrashpadClient.StartWithInvalidHandles (32 ms)

The failure may be flake having nothing to do with invalid handles, but I can’t
help but feel like it’s this bug giving me the finger.

[scottmg, 2015-11-25 21:40:11]

On 2015/11/25 21:35:18, Mark Mentovai wrote:
> Ugh, I just had this test failure locally (32-bit Debug):
> 
> [ RUN      ] CrashpadClient.StartWithInvalidHandles
> c:\chrome\crashpad\crashpad\test\scoped_temp_dir_win.cc(95): error: Value of:
> DeleteFile(entry_path.value().c_str())
>   Actual: false
> Expected: true
> c:\chrome\crashpad\crashpad\test\scoped_temp_dir_win.cc(100): error: Value of:
> RemoveDirectory(path.value().c_str())
>   Actual: false
> Expected: true
> c:\chrome\crashpad\crashpad\test\win\win_multiprocess.h(64): error: Value of:
> exit_code
>   Actual: 255
> Expected: parent_process.exit_code_
> Which is: 0
> [  FAILED  ] CrashpadClient.StartWithInvalidHandles (32 ms)
> 
> The failure may be flake having nothing to do with invalid handles, but I
can’t
> help but feel like it’s this bug giving me the finger.

Yeah, I've had that too, but I'm not sure what to do about it. I meant to file a
bug.

It's because shutdown deleting the temp dir is racing with crashpad_handler's
shutdown (which has the database dir open).

We know crashpad_handler will shut down after UseHandler(), but we don't know
when it's finished doing so.