Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1356383002:
win: Implement CRASHPAD_SIMULATE_CRASH() (Closed)
Descriptionwin: Implement CRASHPAD_SIMULATE_CRASH()
Windows requires the connection to the handler to do anything, so it
can't really be implemented or tested without CrashpadClient and the
connection machinery.
R=mark@chromium.org
BUG=crashpad:53
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/475ac81cce06e0ee0c9b327c8b4ee12aff17f788
Patch Set 1 #Patch Set 2 : no-find-copies #Patch Set 3 : . #Patch Set 4 : . #
Total comments: 25
Patch Set 5 : different approach #Patch Set 6 : . #Patch Set 7 : mac #
Total comments: 6
Patch Set 8 : don't include exception if Exception() == 0 #Patch Set 9 : . #Patch Set 10 : . #Patch Set 11 : . #Patch Set 12 : . #
Total comments: 8
Patch Set 13 : fixes #
Messages
Total messages: 29 (5 generated)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
scottmg@chromium.org changed reviewers: + mark@chromium.org
Patchset #2 (id:30016) has been deleted
https://codereview.chromium.org/1356383002/diff/110001/client/crashpad_client... File client/crashpad_client_win.cc (right): https://codereview.chromium.org/1356383002/diff/110001/client/crashpad_client... client/crashpad_client_win.cc:54: if (base::subtle::Barrier_AtomicIncrement(&g_capturing_dump, 1) > 1) { 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. https://codereview.chromium.org/1356383002/diff/110001/client/crashpad_client... client/crashpad_client_win.cc:55: SleepEx(INFINITE, false); 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. https://codereview.chromium.org/1356383002/diff/110001/client/crashpad_client... client/crashpad_client_win.cc:151: // Someone else is either writing or has crashed, simply abort here. Decrement to undo what you just did, since there’s an early return. https://codereview.chromium.org/1356383002/diff/110001/client/crashpad_client... client/crashpad_client_win.cc:156: // Create a fake EXCEPTION_POINTERS (with a null EXCEPTION_RECORD), so that 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. https://codereview.chromium.org/1356383002/diff/110001/client/crashpad_client... client/crashpad_client_win.cc:161: RtlCaptureContext(&context); 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(). https://codereview.chromium.org/1356383002/diff/110001/client/crashpad_client... client/crashpad_client_win.cc:165: g_exception_information.thread_id = GetCurrentThreadId(); This through WaitForSingleObject() looks duplicated. Common helper function? https://codereview.chromium.org/1356383002/diff/110001/client/crashpad_client... client/crashpad_client_win.cc:181: Extra blank line. https://codereview.chromium.org/1356383002/diff/110001/snapshot/win/exception... File snapshot/win/exception_snapshot_win.cc (right): https://codereview.chromium.org/1356383002/diff/110001/snapshot/win/exception... snapshot/win/exception_snapshot_win.cc:147: LOG(INFO) << "null ExceptionRecord, not capturing any exception"; This is fully expected now, I don’t think we need to log anything when this happens. But this should have a comment saying that it happens for simulated crashes. https://codereview.chromium.org/1356383002/diff/110001/util/win/exception_han... File util/win/exception_handler_server.cc (right): https://codereview.chromium.org/1356383002/diff/110001/util/win/exception_han... util/win/exception_handler_server.cc:428: SetEvent(client->dump_completed_event()); PLOG_IF this fails. https://codereview.chromium.org/1356383002/diff/110001/util/win/registration_... File util/win/registration_protocol_win.h (right): https://codereview.chromium.org/1356383002/diff/110001/util/win/registration_... util/win/registration_protocol_win.h:92: //! \brief An event `HANDLE`, valid in the client process, that will be Blank line before this.
(I'll defer the rest of the changes until we figure out what the best approach is.) https://codereview.chromium.org/1356383002/diff/110001/client/crashpad_client... File client/crashpad_client_win.cc (right): https://codereview.chromium.org/1356383002/diff/110001/client/crashpad_client... client/crashpad_client_win.cc:54: if (base::subtle::Barrier_AtomicIncrement(&g_capturing_dump, 1) > 1) { On 2015/09/23 20:00:52, Mark Mentovai - August is over wrote: > 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. 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.
Three events sounds OK. Not amazing, but OK. This definitely shouldn’t move in-process. We’ve already got a perfectly good handler process that’s an expert at writing and uploading dumps. We shouldn’t have to drag all of that code into the app itself to enable this feature. And anyway, sandboxing ought to make that a non-starter. I don’t know how much scarier some other kind of IPC operation would be on Windows. The plan for non-Mac Unixish systems is to do a pipe write, and assuming that you’ve got the stack space to make that call, it’s just a system call into the kernel with no real dependency on user space (except that you hope that nobody else messed with your pipe, which is an unfortunate possibility, but that’s the case with the current Windows event design too). I don’t really care about supporting multiple simultaneous crashes, it’s enough in my mind to just report the first one. But multiple simultaneous-ish “dump without crashing” dumps probably ought to work, and a real crash should produce a dump even with a “dump without crashing” one in progress. I think we can arrive at this with the three events and not more complex IPC. If we want multiple simultaneous dump-without-crashings, I guess we’d want a CRITICAL_SECTION for that instead of the atomic. Speaking of the stack, does the unhandled exception filter function run on some sort of alternate stack, or are things hosed if stack space is exhausted?
https://codereview.chromium.org/1356383002/diff/110001/client/crashpad_client... File client/crashpad_client_win.cc (right): https://codereview.chromium.org/1356383002/diff/110001/client/crashpad_client... client/crashpad_client_win.cc:162: EXCEPTION_POINTERS exception_pointers = {0}; 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.
On 2015/09/23 22:18:12, Mark Mentovai - August is over wrote: > Three events sounds OK. Not amazing, but OK. Agreed. > > This definitely shouldn’t move in-process. We’ve already got a perfectly good > handler process that’s an expert at writing and uploading dumps. We shouldn’t > have to drag all of that code into the app itself to enable this feature. And > anyway, sandboxing ought to make that a non-starter. Good point. > > I don’t know how much scarier some other kind of IPC operation would be on > Windows. The plan for non-Mac Unixish systems is to do a pipe write, and > assuming that you’ve got the stack space to make that call, it’s just a system > call into the kernel with no real dependency on user space (except that you hope > that nobody else messed with your pipe, which is an unfortunate possibility, but > that’s the case with the current Windows event design too). > > I don’t really care about supporting multiple simultaneous crashes, it’s enough > in my mind to just report the first one. But multiple simultaneous-ish “dump > without crashing” dumps probably ought to work, and a real crash should produce > a dump even with a “dump without crashing” one in progress. I think we can > arrive at this with the three events and not more complex IPC. If we want > multiple simultaneous dump-without-crashings, I guess we’d want a > CRITICAL_SECTION for that instead of the atomic. Agreed on all that. The atomic is a bit iffy/complex once we get into multiple simultaneous, so I think if I de-share, then I'd have the atomic used as-is for the crashing case, and then have a simple CRITICAL_SECTION for the without-crashing case, where we aren't worried about those extra calls. > > Speaking of the stack, does the unhandled exception filter function run on some > sort of alternate stack, or are things hosed if stack space is exhausted? It runs in the context of the thread that faulted, so I think the best we can hope for is that there's a few words left after a stack probe in the case of a stack overflow exception (and that's part of why we want to do as little as possible). I wonder if there's any upside to writing an (almost) "stackless" version of UnhandledExceptionFilter(). The caller still needs to be able to push one word to pass us the EXCEPTION_POINTERS*. But we know we're ending in TerminateProcess() anyway, so we could have the callback save the current ESP (to get the argument), then rehome the stack to a small global buffer, and then call the real handler, which would give us a bit of breathing room. I'm still not sure that we want to call more Win32 functions even with bonus stack space, because once we start doing things like IO, we might be heading back into the code that was causing the crashes in the first place.
cpu@chromium.org changed reviewers: + cpu@chromium.org
The current version is near stackless :) Ok, a better answer is that you need to add some extra magic. I think I was supposed to do this but forgot https://msdn.microsoft.com/en-us/library/windows/desktop/ms686283(v=vs.85).aspx
On 2015/09/23 23:22:54, cpu wrote: > The current version is near stackless :) Yeah, it's only about 20 bytes in a release build, but it's still something. > > Ok, a better answer is that you need to add some extra magic. I think I was > supposed to do this but forgot > > https://msdn.microsoft.com/en-us/library/windows/desktop/ms686283(v=vs.85).aspx Aha, magic, thanks! That's even more useful since the user mode stuff before we get called probably takes a chunk of stack too. Sadly (weirdly) only XP x64 and Vista+ for x86, but well, XP.
Sounds like we’ve got a way forward, then. SetThreadStackGuarantee() also provides an answer to my side question, and it looks like we can just call it when it’s present and deal with possibly missing stack overflows on XP.
https://codereview.chromium.org/1356383002/diff/110001/client/crashpad_client... File client/crashpad_client_win.cc (right): https://codereview.chromium.org/1356383002/diff/110001/client/crashpad_client... client/crashpad_client_win.cc:54: if (base::subtle::Barrier_AtomicIncrement(&g_capturing_dump, 1) > 1) { Restructured. https://codereview.chromium.org/1356383002/diff/110001/client/crashpad_client... client/crashpad_client_win.cc:55: SleepEx(INFINITE, false); On 2015/09/23 20:00:52, Mark Mentovai - August is over wrote: > 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. This problem is gone now, only the crashing path uses the atomic. https://codereview.chromium.org/1356383002/diff/110001/client/crashpad_client... client/crashpad_client_win.cc:156: // Create a fake EXCEPTION_POINTERS (with a null EXCEPTION_RECORD), so that On 2015/09/23 20:00:52, Mark Mentovai - August is over wrote: > 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. Done. https://codereview.chromium.org/1356383002/diff/110001/client/crashpad_client... client/crashpad_client_win.cc:161: RtlCaptureContext(&context); On 2015/09/23 20:00:52, Mark Mentovai - August is over wrote: > 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(). 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? https://codereview.chromium.org/1356383002/diff/110001/client/crashpad_client... client/crashpad_client_win.cc:162: EXCEPTION_POINTERS exception_pointers = {0}; On 2015/09/23 22:20:38, Mark Mentovai - August is over wrote: > 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. 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. https://codereview.chromium.org/1356383002/diff/110001/client/crashpad_client... client/crashpad_client_win.cc:181: On 2015/09/23 20:00:52, Mark Mentovai - August is over wrote: > Extra blank line. Done. https://codereview.chromium.org/1356383002/diff/110001/snapshot/win/exception... File snapshot/win/exception_snapshot_win.cc (right): https://codereview.chromium.org/1356383002/diff/110001/snapshot/win/exception... snapshot/win/exception_snapshot_win.cc:147: LOG(INFO) << "null ExceptionRecord, not capturing any exception"; On 2015/09/23 20:00:52, Mark Mentovai - August is over wrote: > This is fully expected now, I don’t think we need to log anything when this > happens. But this should have a comment saying that it happens for simulated > crashes. Done. https://codereview.chromium.org/1356383002/diff/110001/util/win/exception_han... File util/win/exception_handler_server.cc (right): https://codereview.chromium.org/1356383002/diff/110001/util/win/exception_han... util/win/exception_handler_server.cc:428: SetEvent(client->dump_completed_event()); On 2015/09/23 20:00:52, Mark Mentovai - August is over wrote: > PLOG_IF this fails. Done. https://codereview.chromium.org/1356383002/diff/110001/util/win/registration_... File util/win/registration_protocol_win.h (right): https://codereview.chromium.org/1356383002/diff/110001/util/win/registration_... util/win/registration_protocol_win.h:92: //! \brief An event `HANDLE`, valid in the client process, that will be On 2015/09/23 20:00:52, Mark Mentovai - August is over wrote: > Blank line before this. Done.
Not yet re-reviewed, just responding to comments. https://codereview.chromium.org/1356383002/diff/110001/client/crashpad_client... File client/crashpad_client_win.cc (right): https://codereview.chromium.org/1356383002/diff/110001/client/crashpad_client... client/crashpad_client_win.cc:161: RtlCaptureContext(&context); On 2015/09/24 19:16:52, scottmg wrote: > On 2015/09/23 20:00:52, Mark Mentovai - August is over wrote: > > 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(). > > 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? 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. https://codereview.chromium.org/1356383002/diff/110001/client/crashpad_client... client/crashpad_client_win.cc:162: EXCEPTION_POINTERS exception_pointers = {0}; On 2015/09/24 19:16:52, scottmg wrote: > On 2015/09/23 22:20:38, Mark Mentovai - August is over wrote: > > 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. > > 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. 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.
https://codereview.chromium.org/1356383002/diff/110001/client/crashpad_client... File client/crashpad_client_win.cc (right): https://codereview.chromium.org/1356383002/diff/110001/client/crashpad_client... client/crashpad_client_win.cc:161: RtlCaptureContext(&context); On 2015/09/24 19:26:05, Mark Mentovai - August is over wrote: > On 2015/09/24 19:16:52, scottmg wrote: > > On 2015/09/23 20:00:52, Mark Mentovai - August is over wrote: > > > 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(). > > > > 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? > > 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. OK, I can just land it slightly borked for now. https://codereview.chromium.org/1356383002/diff/110001/client/crashpad_client... client/crashpad_client_win.cc:162: EXCEPTION_POINTERS exception_pointers = {0}; On 2015/09/24 19:26:05, Mark Mentovai - August is over wrote: > On 2015/09/24 19:16:52, scottmg wrote: > > On 2015/09/23 22:20:38, Mark Mentovai - August is over wrote: > > > 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. > > > > 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. > > 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. 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.)
On 2015/09/24 20:15:22, scottmg wrote: > 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.) Not really. We still need the ThreadId and ThreadContext members of MINIDUMP_EXCEPTION_STREAM.
https://codereview.chromium.org/1356383002/diff/170001/client/crashpad_client.h File client/crashpad_client.h (right): https://codereview.chromium.org/1356383002/diff/170001/client/crashpad_client... client/crashpad_client.h:99: //! \return `true` on success, `false` on failure with a message logged. Nobody checks the return value, so you can just make it return void. But leave the log message on failure, that’s useful. https://codereview.chromium.org/1356383002/diff/170001/client/crashpad_client... File client/crashpad_client_win.cc (right): https://codereview.chromium.org/1356383002/diff/170001/client/crashpad_client... client/crashpad_client_win.cc:45: base::Lock g_non_crash_dump_lock; This carries a static constructor. This should be a Lock* that you can initialize in SetHandler() when you initialize the other things. https://codereview.chromium.org/1356383002/diff/170001/client/crashpad_client... client/crashpad_client_win.cc:181: // This is logically const, but EXCEPTION_POINTERS does not declare it as Blank line before this, so that EXCEPTION_POINTERS don’t get lost in the sea of comments.
On 2015/09/24 20:37:10, Mark Mentovai - August is over wrote: > On 2015/09/24 20:15:22, scottmg wrote: > > 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.) > > Not really. We still need the ThreadId and ThreadContext members of > MINIDUMP_EXCEPTION_STREAM. (Isn't that in the main context though anyway when we're simulating a crash?) Switched to emit an fake exception anyway. I tagged it with a different value than Mac as some of the real ones have a top nibble of 4 (as 'C' does) so 'CPsx' as 0x43507378 isn't particularly notable when it's displayed as hex in the debugger.
https://codereview.chromium.org/1356383002/diff/170001/client/crashpad_client.h File client/crashpad_client.h (right): https://codereview.chromium.org/1356383002/diff/170001/client/crashpad_client... client/crashpad_client.h:99: //! \return `true` on success, `false` on failure with a message logged. On 2015/09/24 20:37:29, Mark Mentovai - August is over wrote: > Nobody checks the return value, so you can just make it return void. But leave > the log message on failure, that’s useful. Done. https://codereview.chromium.org/1356383002/diff/170001/client/crashpad_client... File client/crashpad_client_win.cc (right): https://codereview.chromium.org/1356383002/diff/170001/client/crashpad_client... client/crashpad_client_win.cc:45: base::Lock g_non_crash_dump_lock; On 2015/09/24 20:37:29, Mark Mentovai - August is over wrote: > This carries a static constructor. This should be a Lock* that you can > initialize in SetHandler() when you initialize the other things. Done. https://codereview.chromium.org/1356383002/diff/170001/client/crashpad_client... client/crashpad_client_win.cc:181: // This is logically const, but EXCEPTION_POINTERS does not declare it as On 2015/09/24 20:37:29, Mark Mentovai - August is over wrote: > Blank line before this, so that EXCEPTION_POINTERS don’t get lost in the sea of > comments. Done.
scottmg wrote: > On 2015/09/24 20:37:10, Mark Mentovai - August is over wrote: > > On 2015/09/24 20:15:22, scottmg wrote: > > > 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.) > > > > Not really. We still need the ThreadId and ThreadContext members of > > MINIDUMP_EXCEPTION_STREAM. > > (Isn't that in the main context though anyway when we're simulating a crash?) The ThreadId is totally lost. The context won’t be the same as what was captured, it’ll be the context of the thread blocked in WaitForSingleObject(). All of the hard work we did to get a precise “here, this file, this line, registers just like so” will be lost.
LGTM https://codereview.chromium.org/1356383002/diff/260001/client/client.gyp File client/client.gyp (right): https://codereview.chromium.org/1356383002/diff/260001/client/client.gyp#newc... client/client.gyp:1: # Copyright 2014 The Crashpad Authors. All rights reserved. > The name is a slight misnomer on Windows, as we just capture a dump, > rather than simulating a crash, but usage is the same as on OS X. I think we have different perspectives on what “simulate crash” means. I was thinking of it from the perspective of crashpad_handler, which processes it the same way that it processes a real crash (except now on Windows, the signaling machinery is a little different). It sounds like you’re thinking of it from the perspective of the OS machinery. On OS X, we just capture a dump too. https://codereview.chromium.org/1356383002/diff/260001/client/crashpad_client... File client/crashpad_client_win.cc (right): https://codereview.chromium.org/1356383002/diff/260001/client/crashpad_client... client/crashpad_client_win.cc:87: // TODO(scottmg): Re-add the "reply" event here, for implementing This comment is obsolete. https://codereview.chromium.org/1356383002/diff/260001/util/win/exception_han... File util/win/exception_handler_server.cc (right): https://codereview.chromium.org/1356383002/diff/260001/util/win/exception_han... util/win/exception_handler_server.cc:121: non_crash_dump_completed_event_( Duplicate initialization?
https://codereview.chromium.org/1356383002/diff/260001/client/client.gyp File client/client.gyp (right): https://codereview.chromium.org/1356383002/diff/260001/client/client.gyp#newc... client/client.gyp:1: # Copyright 2014 The Crashpad Authors. All rights reserved. On 2015/09/25 17:21:01, Mark Mentovai - August is over wrote: > > The name is a slight misnomer on Windows, as we just capture a dump, > > rather than simulating a crash, but usage is the same as on OS X. > > I think we have different perspectives on what “simulate crash” means. I was > thinking of it from the perspective of crashpad_handler, which processes it the > same way that it processes a real crash (except now on Windows, the signaling > machinery is a little different). It sounds like you’re thinking of it from the > perspective of the OS machinery. I see. Yes, that's what I was thinking. Removed that part of the CL description. > > On OS X, we just capture a dump too. https://codereview.chromium.org/1356383002/diff/260001/client/crashpad_client... File client/crashpad_client_win.cc (right): https://codereview.chromium.org/1356383002/diff/260001/client/crashpad_client... client/crashpad_client_win.cc:87: // TODO(scottmg): Re-add the "reply" event here, for implementing On 2015/09/25 17:21:01, Mark Mentovai - August is over wrote: > This comment is obsolete. TODONE. https://codereview.chromium.org/1356383002/diff/260001/util/win/exception_han... File util/win/exception_handler_server.cc (right): https://codereview.chromium.org/1356383002/diff/260001/util/win/exception_han... util/win/exception_handler_server.cc:121: non_crash_dump_completed_event_( On 2015/09/25 17:21:01, Mark Mentovai - August is over wrote: > Duplicate initialization? Sorry? Do you mean making a helper function that does CreateEvent() with these arguments? There's 3 events, crash_dump_requested_event_ non_crash_dump_requested_event_, and non_crash_dump_completed_event_, I think I'm doing each one once here, but this code isn't the prettiest so maybe I'm misreading something.
LGTM https://codereview.chromium.org/1356383002/diff/260001/util/win/exception_han... File util/win/exception_handler_server.cc (right): https://codereview.chromium.org/1356383002/diff/260001/util/win/exception_han... util/win/exception_handler_server.cc:121: non_crash_dump_completed_event_( scottmg wrote: > Sorry? Do you mean making a helper function that does CreateEvent() with these > arguments? > > There's 3 events, crash_dump_requested_event_ non_crash_dump_requested_event_, > and non_crash_dump_completed_event_, I think I'm doing each one once here, but > this code isn't the prettiest so maybe I'm misreading something. My mistake. I misread, requested and completed are the same length, and I think I was just seeing the beginnings and ends of the variable names.
Message was sent while issue was closed.
Committed patchset #13 (id:280001) manually as 475ac81cce06e0ee0c9b327c8b4ee12aff17f788 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/1356383002/diff/260001/util/win/exception_han... File util/win/exception_handler_server.cc (right): https://codereview.chromium.org/1356383002/diff/260001/util/win/exception_han... util/win/exception_handler_server.cc:121: non_crash_dump_completed_event_( On 2015/09/25 20:32:05, Mark Mentovai - August is over wrote: > scottmg wrote: > > Sorry? Do you mean making a helper function that does CreateEvent() with these > > arguments? > > > > There's 3 events, crash_dump_requested_event_ non_crash_dump_requested_event_, > > and non_crash_dump_completed_event_, I think I'm doing each one once here, but > > this code isn't the prettiest so maybe I'm misreading something. > > My mistake. I misread, requested and completed are the same length, and I think > I was just seeing the beginnings and ends of the variable names. I did the same thing a couple times. Maybe I should come up with better names for them.
Message was sent while issue was closed.
As expected, the bots went red on the x86 dump-without-crashing. So we need to add our custom RtlCaptureContext() at least for x86 to green those up.
Message was sent while issue was closed.
I should be able to get it landed, at least for x86, on Monday. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
