Support generating minidumps from live processes on Windows

This patch adds an ExceptionHandler::WriteMinidumpForChild method to write
minidumps for non-crashed child processes.

The patch is by Chris Jones, originally written in:
https://bugzilla.mozilla.org/show_bug.cgi?id=544936
https://bugzilla.mozilla.org/show_bug.cgi?id=555309

[Mark Mentovai]

http://breakpad.appspot.com/115002/diff/1/2
File src/client/windows/handler/exception_handler.cc (right):

http://breakpad.appspot.com/115002/diff/1/2#newcode44
Line 44: // Helper for GetProcId()
OK, but when would we expect this one to work? When would we expect the other to
work? The answer is buried in the implementation of GetProcId, but I’d think it
should be at the top of each “helper.”

For that matter, why is GetProcessId better than NtQueryInformationProcess? Why
doesn’t the latter work on all systems? I found the answer buried in its
documentation, but should I have had to look that up?

http://breakpad.appspot.com/115002/diff/1/2#newcode49
Line 49: static bool initialize_get_process_id = true;
I prefer for these to track the “initialized?” state and not the “need to
initialize?” state, for them to start out at false and then be brought to true
when initializing.

http://breakpad.appspot.com/115002/diff/1/2#newcode54
Line 54: return false;
Rather than having multiple “return false” points, invert the !-ness of the
conditional and stick the GetProcAddress inside it.

http://breakpad.appspot.com/115002/diff/1/2#newcode63
Line 63: return true;
If *id is zero, maybe return false? Or do you not want to do that because you
want to use GetProcessId in preference to NtQueryInformationProcess if it’s
available regardless of whether the call itself fails? That’d be comment-worthy.

http://breakpad.appspot.com/115002/diff/1/2#newcode72
Line 72: static bool initialize_query_information_process = true;
The same “initialized?” and multiple-return-false-if-lookup-failed things in the
other function apply to this one.

http://breakpad.appspot.com/115002/diff/1/2#newcode92
Line 92: &info, sizeof info,
I rarely see sizeof written without parentheses. Same on line 94.

http://breakpad.appspot.com/115002/diff/1/2#newcode94
Line 94: if (!SUCCEEDED(status) || (bytes_returned != (sizeof info)))
Is it possible for info to be larger than NtQueryInformationProcess needs?
Should this fail when bytes_returned > sizeof(info)?

http://breakpad.appspot.com/115002/diff/1/2#newcode799
Line 799: if (0 == childId)
childId == 0 is normal.

http://breakpad.appspot.com/115002/diff/1/2#newcode811
Line 811: // this thread may have died already, so not opening the handle is a
Full sentences start with capital letters and end with periods.

http://breakpad.appspot.com/115002/diff/1/2#newcode813
Line 813: if (NULL != child_thread_handle) {
Invert operands here and on the next line too. Humans are used to thinking of
things in the other order.

http://breakpad.appspot.com/115002/diff/1/2#newcode814
Line 814: if (0 <= (last_suspend_cnt = SuspendThread(child_thread_handle))) {
(This one is especially egregious.)

http://breakpad.appspot.com/115002/diff/1/2#newcode833
Line 833: child_blamed_thread,
Usually continuation lines get a 4-space indent, not 2.

http://breakpad.appspot.com/115002/diff/1/2#newcode837
Line 837: if (0 <= last_suspend_cnt) {
…

http://breakpad.appspot.com/115002/diff/1/2#newcode934
Line 934: user_stream_array[idx].Type = MD_BREAKPAD_INFO_STREAM;
OK, good.

http://breakpad.appspot.com/115002/diff/1/3
File src/client/windows/handler/exception_handler.h (right):

http://breakpad.appspot.com/115002/diff/1/3#newcode310
Line 310: DWORD processId,
I’m wondering why processId needs to be supplied separately here when process is
already given. Would it work to use the GetProcId thing inside this method? Does
that do the right thing even when dumping the current process?

[cjones]

Hello Mark,

Thanks for the review.

On 2010/06/29 20:31:29, Mark Mentovai wrote:
> http://breakpad.appspot.com/115002/diff/1/2
> File src/client/windows/handler/exception_handler.cc (right):
> 
> http://breakpad.appspot.com/115002/diff/1/2#newcode44
> Line 44: // Helper for GetProcId()
> OK, but when would we expect this one to work? When would we expect the other
to
> work? The answer is buried in the implementation of GetProcId, but I’d think
it
> should be at the top of each “helper.”
> 
> For that matter, why is GetProcessId better than NtQueryInformationProcess?
Why
> doesn’t the latter work on all systems? I found the answer buried in its
> documentation, but should I have had to look that up?
> 
> http://breakpad.appspot.com/115002/diff/1/2#newcode49
> Line 49: static bool initialize_get_process_id = true;
> I prefer for these to track the “initialized?” state and not the “need to
> initialize?” state, for them to start out at false and then be brought to true
> when initializing.
> 
> http://breakpad.appspot.com/115002/diff/1/2#newcode54
> Line 54: return false;
> Rather than having multiple “return false” points, invert the !-ness of the
> conditional and stick the GetProcAddress inside it.
> 
> http://breakpad.appspot.com/115002/diff/1/2#newcode63
> Line 63: return true;
> If *id is zero, maybe return false? Or do you not want to do that because you
> want to use GetProcessId in preference to NtQueryInformationProcess if it’s
> available regardless of whether the call itself fails? That’d be
comment-worthy.
> 
> http://breakpad.appspot.com/115002/diff/1/2#newcode72
> Line 72: static bool initialize_query_information_process = true;
> The same “initialized?” and multiple-return-false-if-lookup-failed things
in the
> other function apply to this one.
> 
> http://breakpad.appspot.com/115002/diff/1/2#newcode92
> Line 92: &info, sizeof info,
> I rarely see sizeof written without parentheses. Same on line 94.
> 
> http://breakpad.appspot.com/115002/diff/1/2#newcode94
> Line 94: if (!SUCCEEDED(status) || (bytes_returned != (sizeof info)))
> Is it possible for info to be larger than NtQueryInformationProcess needs?
> Should this fail when bytes_returned > sizeof(info)?
> 

Re: all preceding comments:

I can't speak for this code personally; I copied and pasted from Mozilla's
import of some chromium code.  But I see that with
http://src.chromium.org/viewvc/chrome/trunk/src/base/process_util_win.cc?r1=3...,
all this complexity (and a bug therein) went away at the expense of dropping
support for win2k.  I don't know what Mozilla's policy wrt win2k is --- maybe
Ted knows?  If we still intend to support it, we can chase down the issues you
pointed out.

> http://breakpad.appspot.com/115002/diff/1/2#newcode799
> Line 799: if (0 == childId)
> childId == 0 is normal.
> 

0 is returned in the failure case of GetProcId() above; is there a canonical
"null process ID" value it can use instead?

> http://breakpad.appspot.com/115002/diff/1/2#newcode811
> Line 811: // this thread may have died already, so not opening the handle is a
> Full sentences start with capital letters and end with periods.
> 

OK.

> http://breakpad.appspot.com/115002/diff/1/2#newcode813
> Line 813: if (NULL != child_thread_handle) {
> Invert operands here and on the next line too. Humans are used to thinking of
> things in the other order.
> 

OK.

> http://breakpad.appspot.com/115002/diff/1/2#newcode814
> Line 814: if (0 <= (last_suspend_cnt = SuspendThread(child_thread_handle))) {
> (This one is especially egregious.)
> 

Just to be sure, are you referring to the "0 <= ..." operand order?

> http://breakpad.appspot.com/115002/diff/1/2#newcode833
> Line 833: child_blamed_thread,
> Usually continuation lines get a 4-space indent, not 2.
> 

OK

> http://breakpad.appspot.com/115002/diff/1/3
> File src/client/windows/handler/exception_handler.h (right):
> 
> http://breakpad.appspot.com/115002/diff/1/3#newcode310
> Line 310: DWORD processId,
> I’m wondering why processId needs to be supplied separately here when process
is
> already given. Would it work to use the GetProcId thing inside this method?
Does
> that do the right thing even when dumping the current process?

To be honest, I no longer remember the motivation for this interface :).  I
think the idea may have been to early-return from WriteMinidumpForChild() if
GetProcId() failed; the other client uses GetCurrentProcessId() which MSDN
implies is infallible.  However, I don't see anything in the code or on MSDN to
indicate anything wrong with using GetProcId() instead of carrying around the
processId param.  Suits me.

[Mark Mentovai]

http://breakpad.appspot.com/115002/diff/1/2
File src/client/windows/handler/exception_handler.cc (right):

http://breakpad.appspot.com/115002/diff/1/2#newcode799
Line 799: if (0 == childId)
I meant to flip the order of the operands. Same thing on 813 and 814.

[Ted Mielczarek]

...and we're back. This patch now works with the latest Breakpad sources. The
largest changes are the removal of all the GetProcessId code and the extra
WriteMinidump method that manufactured an exception stream.

https://breakpad.appspot.com/115002/diff/1/src/client/windows/handler/excepti...
File src/client/windows/handler/exception_handler.cc (right):

https://breakpad.appspot.com/115002/diff/1/src/client/windows/handler/excepti...
src/client/windows/handler/exception_handler.cc:42: namespace {
I dropped all this code because we're no longer concerned with supporting
Windows 2000.

https://breakpad.appspot.com/115002/diff/1/src/client/windows/handler/excepti...
src/client/windows/handler/exception_handler.cc:765: void* callback_context) {
This method became redundant now that the existing ::WriteMinidump method adds
an exception context. Unfortunately we used a different ExceptionCode, but I
think we can live with this. Note that the WriteMinidumpForChild case still uses
EXCEPTION_BREAKPOINT.

[Mark Mentovai]

https://breakpad.appspot.com/115002/diff/5002/src/client/windows/handler/exce...
File src/client/windows/handler/exception_handler.cc (right):

https://breakpad.appspot.com/115002/diff/5002/src/client/windows/handler/exce...
src/client/windows/handler/exception_handler.cc:1: // Copyright (c) 2006, Google
Inc.
Again, write the change description as a description.

[Ted Mielczarek]

On 2012/09/12 19:07:05, Mark Mentovai wrote:
> Again, write the change description as a description.

Fixed. How does the updated patch look?

[Mark Mentovai]

LGTM