win: Implement CRASHPAD_SIMULATE_CRASH()

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 e19aa7469d81a8015f5bd9bc70a52f97761ab999..e65f526b65170586aa17930747700525872f095b 100644
--- a/client/crashpad_client_win.cc
+++ b/client/crashpad_client_win.cc
@@ -27,16 +27,18 @@
 
 namespace {
 
-// This handle is never closed.
+// These handles are never closed.
 HANDLE g_signal_exception = INVALID_HANDLE_VALUE;
+HANDLE g_dump_completed = INVALID_HANDLE_VALUE;
 
 // Where we store the exception information that the crash handler reads.
 crashpad::ExceptionInformation g_exception_information;
 
-LONG WINAPI UnhandledExceptionHandler(EXCEPTION_POINTERS* exception_pointers) {
-  // Tracks whether a thread has already entered UnhandledExceptionHandler.
-  static base::subtle::AtomicWord have_crashed;
+// Tracks whether a thread has already entered UnhandledExceptionHandler() or
+// DumpWithoutCrash().
+base::subtle::AtomicWord g_capturing_dump;
 
+LONG WINAPI UnhandledExceptionHandler(EXCEPTION_POINTERS* exception_pointers) {
   // This is a per-process handler. While this handler is being invoked, other
   // threads are still executing as usual, so multiple threads could enter at
   // the same time. Because we're in a crashing state, we shouldn't be doing
@@ -49,7 +51,7 @@ LONG WINAPI UnhandledExceptionHandler(EXCEPTION_POINTERS* exception_pointers) {
   // that we won't save the exception pointers from the second and further
   // crashes, but contention here is very unlikely, and we'll still have a stack
   // that's blocked at this location.
-  if (base::subtle::Barrier_AtomicIncrement(&have_crashed, 1) > 1) {
+  if (base::subtle::Barrier_AtomicIncrement(&g_capturing_dump, 1) > 1) {

[Mark Mentovai, 2015/09/23 20:00:52]

Maybe it’d be nice to not have an in-process non-crash dump block a crash from
dumping, but I don’t see an easy way to do that with the code’s current
structure.

[scottmg, 2015/09/23 22:03:40]

Yeah, that would be better. Ideally it might be just in-process instead in the
non-crashing case, but that's pulling in a lot of code for debug-only use, so
probably not worthwhile.

Another way I think it could work would be by duplicating some of the globals in
this file. We'd need to register a separate ExceptionInformation structure, and
then two events for the non-crashing case (and I probably wouldn't bother adding
a "resume" for the crashing case then, so 3 events total).

It'd be bit of complexity in the handler, and two extra events per process,
again primarily for debug use. It would make them not contend though (there's
nothing particularly problematic about writing two dumps simultaneously for the
same process, only that we only have one g_exception_information). Does that
sound better to you?

Another possible approach would be to use something other than events so we
could pass data at the time of the crash, but I think we don't want to go that
way because we'd be increasing the amount of work done in-process at the time of
the crash.

[scottmg, 2015/09/24 19:16:52]

Restructured.

     SleepEx(INFINITE, false);

[Mark Mentovai, 2015/09/23 20:00:52]

But this line is a bigger problem. g_capturing_dump >= 1 no longer implies that
anything else will TerminateProcess() this process. If a crash happens while a
non-crash dump is in process, the process will get stuck here.

[scottmg, 2015/09/24 19:16:52]

This problem is gone now, only the crashing path uses the atomic.

   }
 
@@ -59,24 +61,25 @@ LONG WINAPI UnhandledExceptionHandler(EXCEPTION_POINTERS* exception_pointers) {
   g_exception_information.exception_pointers =
       reinterpret_cast<crashpad::WinVMAddress>(exception_pointers);
 
-  // Now signal the crash server, which will take a dump and then terminate us
-  // when it's complete.
+  // Now signal the crash server, which will take a dump and then signal back on
+  // the other event when it's done.
   SetEvent(g_signal_exception);
 
   // Time to wait for the handler to create a dump.
   const DWORD kMillisecondsUntilTerminate = 60 * 1000;
 
-  // Sleep for a while to allow it to process us. Eventually, we terminate
-  // ourselves in case the crash server is gone, so that we don't leave zombies
-  // around. This would ideally never happen.
-  // TODO(scottmg): Re-add the "reply" event here, for implementing
-  // DumpWithoutCrashing.
-  Sleep(kMillisecondsUntilTerminate);
-
-  LOG(ERROR) << "crash server did not respond, self-terminating";
+  // Once the server has told us it's finished, terminate. If
+  // WaitForSingleObject() times out or fails in some other way, our only
+  // recourse is still to kill ourselves, but we use a different exit code than
+  // if we terminated via a normal path.
+  DWORD result =
+      WaitForSingleObject(g_dump_completed, kMillisecondsUntilTerminate);
 
-  const UINT kCrashExitCodeNoDump = 0xffff7001;
-  TerminateProcess(GetCurrentProcess(), kCrashExitCodeNoDump);
+  const unsigned int kCrashExitCodeNoDump = 0xffff7001;
+  TerminateProcess(GetCurrentProcess(),
+                   result == WAIT_OBJECT_0
+                       ? exception_pointers->ExceptionRecord->ExceptionCode
+                       : kCrashExitCodeNoDump);
 
   return EXCEPTION_CONTINUE_SEARCH;
 }
@@ -120,16 +123,60 @@ bool CrashpadClient::SetHandler(const std::string& ipc_port) {
   // The server returns these already duplicated to be valid in this process.
   g_signal_exception = reinterpret_cast<HANDLE>(
       static_cast<uintptr_t>(response.registration.request_report_event));
+  g_dump_completed = reinterpret_cast<HANDLE>(
+      static_cast<uintptr_t>(response.registration.report_completed_event));
   return true;
 }
 
 bool CrashpadClient::UseHandler() {
-  if (g_signal_exception == INVALID_HANDLE_VALUE)
+  if (g_signal_exception == INVALID_HANDLE_VALUE ||
+      g_dump_completed == INVALID_HANDLE_VALUE) {
     return false;
+  }
   // In theory we could store the previous handler but it is not clear what
   // use we have for it.
   SetUnhandledExceptionFilter(&UnhandledExceptionHandler);
   return true;
 }
 
+// static
+bool CrashpadClient::DumpWithoutCrash() {
+  if (g_signal_exception == INVALID_HANDLE_VALUE ||
+      g_dump_completed == INVALID_HANDLE_VALUE) {
+    LOG(ERROR) << "not registered with a server";
+    return false;
+  }
+
+  if (base::subtle::Barrier_AtomicIncrement(&g_capturing_dump, 1) > 1) {
+    // Someone else is either writing or has crashed, simply abort here.

[Mark Mentovai, 2015/09/23 20:00:52]

Decrement to undo what you just did, since there’s an early return.

+    LOG(WARNING) << "already writing";
+    return false;
+  }
+
+  // Create a fake EXCEPTION_POINTERS (with a null EXCEPTION_RECORD), so that

[Mark Mentovai, 2015/09/23 20:00:52]

Expand on the thing about the “null EXCEPTION_RECORD” a bit by saying that the
handler understands this to mean that it’s a simulated crash. Since this is kind
of an informal protocol thing, there’s no great place to write it down other
than here and in the handler.

[scottmg, 2015/09/24 19:16:52]

Done.

+  // the handler has a context to start from.
+  CONTEXT context;
+  // TODO(scottmg): Replace with our custom version. See:
+  // https://code.google.com/p/crashpad/issues/detail?id=53.
+  RtlCaptureContext(&context);

[Mark Mentovai, 2015/09/23 20:00:52]

We’ll actually want to capture the context from this thing’s caller, which is
why CRASHPAD_SIMULATE_CRASH() is a macro. But we can deal with that when we
replace RtlCaptureContext().

[scottmg, 2015/09/24 19:16:52]

I guess as we discovered, this happens to be what this function is doing now,
right?

...

ah, so I moved it outside. If the RtlCaptureContext() is *inside* the function,
then it works on x86 but fails on x64. If the RtlCaptureContext() is in the
caller (i.e. in the macro), then it works on x64 but fails on x86, which I think
corresponds with the weird behaviour we discussed.

So, I'll need to sort out a non-crazy capture function for the test to work. Did
you have a CL with that started already?

[Mark Mentovai, 2015/09/24 19:26:05]

Yeah, I already have an x86 CaptureContext for Windows, and I’m going to add an
x86_64 one now that we talked about how to run the assembler for that. We can go
with the sorta-broken RtlCaptureContext() for now and I can swoop in later and
switch it to ours. Or if you’d prefer, you can wait for me to land
CaptureContext() first.

[scottmg, 2015/09/24 20:15:22]

OK, I can just land it slightly borked for now.

+  EXCEPTION_POINTERS exception_pointers = {0};

[Mark Mentovai, 2015/09/23 22:20:38]

P.S.

On Mac, we have our own made-up exception ID for simulated exceptions,
kMachExceptionSimulated = 'CPsx'. That means that we can identify from the
exception code carried in the minidump that it was simulated and not just
broken. We may want to do something similar on Windows, although exception codes
are always OS-specific, so if that number is likely to conflict, we can pick
something else.

[scottmg, 2015/09/24 19:16:52]

My thinking was that we don't really want it to have an exception record.
Normally in windbg you start by doing `.ecxr` to see if it's actually got one.
By not putting one of those then it's clear that it's just a minidump, not an
crash/exception.

I guess it's a bit of a mismatch between SIMULATE_CRASH() which makes more sense
on Mac, and DumpWithoutCrash(), which makes more sense on Windows since we don't
get any help with the "crash" part.

[Mark Mentovai, 2015/09/24 19:26:05]

Do we at least wind up with a minidump with a MINIDUMP_EXCEPTION_STREAM, though?
That’s where the captured context and ID of the requesting thread go.

If we have a MINIDUMP_EXCEPTION_STREAM, I assume .ecxr will work, even if it
just reports zeroes for the things in ExceptionRecord.

[scottmg, 2015/09/24 20:15:22]

Oh, you're right. I had only run the (in-memory) test, not actually tested it.
:) windbg says it has a stored exception with code == 0.

Does not including the MINIDUMP_EXCEPTION_STREAM if Exception() == 0 seem
reasonable? (Updated to do that.)

+  exception_pointers.ContextRecord = &context;
+
+  g_exception_information.thread_id = GetCurrentThreadId();

[Mark Mentovai, 2015/09/23 20:00:52]

This through WaitForSingleObject() looks duplicated. Common helper function?

+  g_exception_information.exception_pointers =
+      reinterpret_cast<crashpad::WinVMAddress>(&exception_pointers);
+
+  // Now signal the crash server, which will take a dump and then signal back on
+  // the other event when it's done.
+  SetEvent(g_signal_exception);
+
+  // Wait until the server tells us it's finished.
+  DWORD result = WaitForSingleObject(g_dump_completed, INFINITE);
+  PLOG_IF(ERROR, result != WAIT_OBJECT_0) << "WaitForSingleObject";
+
+  base::subtle::Barrier_AtomicIncrement(&g_capturing_dump, -1);
+  return true;
+}
+
+

[Mark Mentovai, 2015/09/23 20:00:52]

Extra blank line.

[scottmg, 2015/09/24 19:16:52]

Done.

 }  // namespace crashpad