win: Don't duplicate handles in handle restriction list

win: Don't duplicate handles in handle restriction list

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

[Mark Mentovai, 2015-11-19 19:00:20]

This would be a good use for a std::set<>, except there’s no &set[0] or
set.data(), which we’d need.

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

https://codereview.chromium.org/1464473003/diff/40001/client/crashpad_client_...
client/crashpad_client_win.cc:166: g_signal_exception = INVALID_HANDLE_VALUE;
Not so sure about this. The intention was that you could actually destroy
CrashpadClient and have everything keep working. I think it’s bad for users to
easily be able to tear down CrashpadClient and have that effectively unhook
Crashpad.

https://codereview.chromium.org/1464473003/diff/40001/client/crashpad_client_...
File client/crashpad_client_win_test.cc (right):

https://codereview.chromium.org/1464473003/diff/40001/client/crashpad_client_...
client/crashpad_client_win_test.cc:26: TEST(CrashpadClient,
StartWithInvalidHandles) {
…that argues for these being out-of-process tests.

[scottmg, 2015-11-19 19:15:49]

Yeah, I considered using a set, but since N <= 4, it didn't seem worth the
additional complexity.

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

https://codereview.chromium.org/1464473003/diff/40001/client/crashpad_client_...
client/crashpad_client_win.cc:166: g_signal_exception = INVALID_HANDLE_VALUE;
On 2015/11/19 19:00:19, Mark Mentovai wrote:
> Not so sure about this. The intention was that you could actually destroy
> CrashpadClient and have everything keep working. I think it’s bad for users to
> easily be able to tear down CrashpadClient and have that effectively unhook
> Crashpad.

OK. We do have one soft request for being able to re-init Crashpad at least (if
not unhook see go/windowszygote). But that doesn't have to be here.

(I was also hoping to delete test/win/win_multiprocess because it's kind of icky
and we only use it for two tests now...)

But moved OOP anyway.

[Mark Mentovai, 2015-11-19 19:20:57]

LGTM

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

https://codereview.chromium.org/1464473003/diff/40001/client/crashpad_client_...
client/crashpad_client_win.cc:166: g_signal_exception = INVALID_HANDLE_VALUE;
scottmg wrote:
> OK. We do have one soft request for being able to re-init Crashpad at least
(if
> not unhook see go/windowszygote). But that doesn't have to be here.

We can still do something like that later (like how we have the discouraged but
occasionally necessary UseSystemDefaultHandler() on Mac), I just don’t want for
it to be something that’s easy to do accidentally.

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

https://codereview.chromium.org/1464473003/diff/60001/client/crashpad_client_...
client/crashpad_client_win.cc:150: if (std::find(handle_list->begin(),
handle_list->end(), handle) ==
Just && this into the if that’s currently wrapping it.

[scottmg, 2015-11-19 19:25:33]

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

https://codereview.chromium.org/1464473003/diff/40001/client/crashpad_client_...
client/crashpad_client_win.cc:166: g_signal_exception = INVALID_HANDLE_VALUE;
On 2015/11/19 19:20:57, Mark Mentovai wrote:
> scottmg wrote:
> > OK. We do have one soft request for being able to re-init Crashpad at least
> (if
> > not unhook see go/windowszygote). But that doesn't have to be here.
> 
> We can still do something like that later (like how we have the discouraged
but
> occasionally necessary UseSystemDefaultHandler() on Mac), I just don’t want
for
> it to be something that’s easy to do accidentally.

Sounds good.

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

https://codereview.chromium.org/1464473003/diff/60001/client/crashpad_client_...
client/crashpad_client_win.cc:150: if (std::find(handle_list->begin(),
handle_list->end(), handle) ==
On 2015/11/19 19:20:57, Mark Mentovai wrote:
> Just && this into the if that’s currently wrapping it.

Done.

[scottmg, 2015-11-19 19:26:01]

Committed patchset #5 (id:80001) manually as
3562fe4ccdd59891d7d8ca9f620327f928b3c90f (presubmit successful).

[Mark Mentovai, 2015-11-19 19:26:33]

LGTM