win: Don't duplicate handles in handle restriction list

client/crashpad_client_win.cc

Index: client/crashpad_client_win.cc
diff --git a/client/crashpad_client_win.cc b/client/crashpad_client_win.cc
index 8faebbf2fbe507f316044fd7d227a38a3b680f88..2848f464258ae8bb64df37c2460bc6def9fdd396 100644
--- a/client/crashpad_client_win.cc
+++ b/client/crashpad_client_win.cc
@@ -132,7 +132,8 @@ using ScopedProcThreadAttributeList =
     base::ScopedGeneric<PPROC_THREAD_ATTRIBUTE_LIST,
                         ScopedProcThreadAttributeListTraits>;
 
-// Adds |handle| to |handle_list| if it appears valid.
+// Adds |handle| to |handle_list| if it appears valid, and is not already in
+// |handle_list|.
 //
 // Invalid handles (including INVALID_HANDLE_VALUE and null handles) cannot be
 // added to a PPROC_THREAD_ATTRIBUTE_LIST’s PROC_THREAD_ATTRIBUTE_HANDLE_LIST.
@@ -143,7 +144,13 @@ using ScopedProcThreadAttributeList =
 // Use this function to add handles with uncertain validities.
 void AddHandleToListIfValid(std::vector<HANDLE>* handle_list, HANDLE handle) {
   if (handle && handle != INVALID_HANDLE_VALUE) {
-    handle_list->push_back(handle);
+    // There doesn't seem to be any documentation of this, but if there's a
+    // handle duplicated in this list, CreateProcess() fails with
+    // ERROR_INVALID_PARAMETER.
+    if (std::find(handle_list->begin(), handle_list->end(), handle) ==
+        handle_list->end()) {
+      handle_list->push_back(handle);
+    }
   }
 }
 
@@ -156,6 +163,13 @@ CrashpadClient::CrashpadClient()
 }
 
 CrashpadClient::~CrashpadClient() {
+  g_signal_exception = INVALID_HANDLE_VALUE;

[Mark Mentovai, 2015/11/19 19:00:19]

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.

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

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]

scottmg wrote:
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.

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

Sounds good.

+  g_signal_non_crash_dump = INVALID_HANDLE_VALUE;
+  g_non_crash_dump_done = INVALID_HANDLE_VALUE;
+  g_critical_section_with_debug_info.DebugInfo = nullptr;
+
+  delete g_non_crash_dump_lock;
+  g_non_crash_dump_lock = nullptr;
 }
 
 bool CrashpadClient::StartHandler(
@@ -337,6 +351,7 @@ bool CrashpadClient::UseHandler() {
   DCHECK_EQ(g_signal_non_crash_dump, INVALID_HANDLE_VALUE);
   DCHECK_EQ(g_non_crash_dump_done, INVALID_HANDLE_VALUE);
   DCHECK(!g_critical_section_with_debug_info.DebugInfo);
+  DCHECK(!g_non_crash_dump_lock);
 
   ClientToServerMessage message;
   memset(&message, 0, sizeof(message));