Fix CrashGenerationServer to recover from protocol errors and a test for same.

[Erik Wright]

Hi Siggi,

Feel like doing another review? The CF boys tell me that you are relatively
familiar with this code and that there is no natural owner (willing to admit it,
anyway).

Thanks,

Erik

http://breakpad.appspot.com/196001/diff/1/7
File src/client/windows/crash_generation/crash_generation_server.cc (right):

http://breakpad.appspot.com/196001/diff/1/7#newcode37
Line 37: #include "client/windows/crash_generation/client_info.h"
Changed to a forward-decl in crash_generation_server.h.

http://breakpad.appspot.com/196001/diff/1/7#newcode209
Line 209: bool CrashGenerationServer::Start() {
A non-negligible change in this function's behaviour, but probably meets what
clients previously expected. When it returns success, the server is started and
listening (clients may connect). Previously there was a theoretical gap, and a
possibility that it would fail after reporting success.

The actual work that is done in the worker thread is trivial (and only there,
IMO, for convenience/style), so in practice the return is probably not really
delayed and the client probably expected these things to be done before Start
returns.

Also, this way the tests don't need a sleep/poll thing for checking when the
server is started.

http://breakpad.appspot.com/196001/diff/1/7#newcode259
Line 259: if (WaitForSingleObject(server_start_complete_event_,
kMaxStartDelayMs) !=
I don't think it is possible for the worker thread to not complete, but using
anything less than INFINITE could result in hard-to-repro failures under stress.
Hence kMaxStartDelayMs is INFINITE...

http://breakpad.appspot.com/196001/diff/1/7#newcode570
Line 570: void CrashGenerationServer::EnterErrorState() {
May these functions aid in readability, and prevent the accidental forgetting of
SetEvent where it is needed for immediate transition to next state.

NB that in some cases, it is true, the event is already signalled by the
overlapped IO stuff. But it doesn't hurt to signal it redundantly and it was
clearly easy for previous authors to not remember where it was required.

http://breakpad.appspot.com/196001/diff/1/7#newcode647
Line 647: bool CrashGenerationServer::RespondToClient(ClientInfo* client_info) {
There was a bug in this function where it could take ownership of the pointer
but then return failure. The client function then freed the pointer.

[Siggi]

Very nice!

http://breakpad.appspot.com/196001/diff/10001/11006
File src/client/windows/crash_generation/crash_generation_server.cc (right):

http://breakpad.appspot.com/196001/diff/10001/11006#newcode212
Line 212: server_start_complete_event_ = CreateEvent(NULL, TRUE, FALSE, NULL);
This handle needs to be closed at some point, am I missing that code? Would be
logical to do this in the destructor?

http://breakpad.appspot.com/196001/diff/10001/11006#newcode223
Line 223: overlapped_.hEvent = CreateEvent(NULL,   // Security descriptor.
I don't see this handle closed anywhere either. Am I missing that too?

http://breakpad.appspot.com/196001/diff/10001/11006#newcode501
Line 501: } else if (GetLastError() == ERROR_IO_PENDING) {
since GLE reads transient thread-local state, it's generally more maintainable
to read the error into a local variable immediately after invoking the
potentially erring call, then using the local variable. 
Using GLE in arbitrary expressions tends to be fragile against future changes...

http://breakpad.appspot.com/196001/diff/10001/11006#newcode529
Line 529: assert(GetLastError() != ERROR_IO_INCOMPLETE);
ditto.

http://breakpad.appspot.com/196001/diff/10001/11006#newcode660
Line 660: if (!success && GetLastError() != ERROR_IO_PENDING) {
ditto...

http://breakpad.appspot.com/196001/diff/10001/11007
File src/client/windows/crash_generation/crash_generation_server.h (right):

http://breakpad.appspot.com/196001/diff/10001/11007#newcode97
Line 97: bool Start();
Maybe add a comment on the behavior of this method. Note also that since you've
added a wait on the completion of an event on another thread (and potentially
the start of that new thread), you've added a new constraint to this function,
namely that it can't be called from within the loader's lock. This may or may
not be an issue for current users of this class, I don't know, but it's probably
not a problem  for Chrome nor Omaha. 
As a courtesy to other users, it'd be nice to comment in the CL description and
perhaps in a note here too.

http://breakpad.appspot.com/196001/diff/10001/11007#newcode195
Line 195: void EnterErrorState();
One-line comment on the purpose of each event.

http://breakpad.appspot.com/196001/diff/10001/11002
File src/client/windows/unittests/crash_generation_server_test.cc (right):

http://breakpad.appspot.com/196001/diff/10001/11002#newcode71
Line 71: void FaultyClient(ClientFault fault_type) {
suggest making this a member of the test fixture, adding these locals as
members, and using TearDown to cleanup.

http://breakpad.appspot.com/196001/diff/10001/11002#newcode75
Line 75: DWORD thread_id;
initialize all variables at point of declaration.

http://breakpad.appspot.com/196001/diff/10001/11002#newcode275
Line 275: 
nit: extra line.

[Erik Wright]

Responded to Siggi's feedback.

http://breakpad.appspot.com/196001/diff/10001/11006
File src/client/windows/crash_generation/crash_generation_server.cc (right):

http://breakpad.appspot.com/196001/diff/10001/11006#newcode212
Line 212: server_start_complete_event_ = CreateEvent(NULL, TRUE, FALSE, NULL);
On 2010/09/17 14:33:16, Siggi wrote:
> This handle needs to be closed at some point, am I missing that code? Would be
> logical to do this in the destructor?

*blush*

Anyway, removed this in favor of completing initialization within the original
thread.

http://breakpad.appspot.com/196001/diff/10001/11006#newcode223
Line 223: overlapped_.hEvent = CreateEvent(NULL,   // Security descriptor.
On 2010/09/17 14:33:16, Siggi wrote:
> I don't see this handle closed anywhere either. Am I missing that too?

It was done in HandleErrorState but not in the case of normal shutdown. I added
it to the destructor.

http://breakpad.appspot.com/196001/diff/10001/11006#newcode501
Line 501: } else if (GetLastError() == ERROR_IO_PENDING) {
On 2010/09/17 14:33:16, Siggi wrote:
> since GLE reads transient thread-local state, it's generally more maintainable
> to read the error into a local variable immediately after invoking the
> potentially erring call, then using the local variable. 
> Using GLE in arbitrary expressions tends to be fragile against future
changes...

Done.

http://breakpad.appspot.com/196001/diff/10001/11006#newcode529
Line 529: assert(GetLastError() != ERROR_IO_INCOMPLETE);
On 2010/09/17 14:33:16, Siggi wrote:
> ditto.

done.

http://breakpad.appspot.com/196001/diff/10001/11006#newcode660
Line 660: if (!success && GetLastError() != ERROR_IO_PENDING) {
On 2010/09/17 14:33:16, Siggi wrote:
> ditto...
Done.

http://breakpad.appspot.com/196001/diff/10001/11007
File src/client/windows/crash_generation/crash_generation_server.h (right):

http://breakpad.appspot.com/196001/diff/10001/11007#newcode97
Line 97: bool Start();
On 2010/09/17 14:33:16, Siggi wrote:
> Maybe add a comment on the behavior of this method. Note also that since
you've
> added a wait on the completion of an event on another thread (and potentially
> the start of that new thread), you've added a new constraint to this function,
> namely that it can't be called from within the loader's lock. This may or may
> not be an issue for current users of this class, I don't know, but it's
probably
> not a problem  for Chrome nor Omaha. 
> As a courtesy to other users, it'd be nice to comment in the CL description
and
> perhaps in a note here too.

Based on discussion, modified the behaviour to complete all initialization
within the invoking thread.

http://breakpad.appspot.com/196001/diff/10001/11007#newcode195
Line 195: void EnterErrorState();
On 2010/09/17 14:33:16, Siggi wrote:
> One-line comment on the purpose of each event.

Done.

http://breakpad.appspot.com/196001/diff/10001/11002
File src/client/windows/unittests/crash_generation_server_test.cc (right):

http://breakpad.appspot.com/196001/diff/10001/11002#newcode71
Line 71: void FaultyClient(ClientFault fault_type) {
On 2010/09/17 14:33:16, Siggi wrote:
> suggest making this a member of the test fixture, adding these locals as
> members, and using TearDown to cleanup. 

Yes, very reasonable.

The only cleanup needed is to close the pipe HANDLE. I fixed that up by putting
all the code that uses the pipe in 'DoFaultyClient'. 'FaultyClient' then opens
the pipe, calls 'DoFaultyClient', and unconditionally closes the pipe
afterwards. This simplified much of the conditional logic in what is now
'DoFaultyClient'.

http://breakpad.appspot.com/196001/diff/10001/11002#newcode75
Line 75: DWORD thread_id;
On 2010/09/17 14:33:16, Siggi wrote:
> initialize all variables at point of declaration.

Done now in member initialization list / constructor.

http://breakpad.appspot.com/196001/diff/10001/11002#newcode275
Line 275: 
On 2010/09/17 14:33:16, Siggi wrote:
> nit: extra line.

Fixed.

[Erik Wright]

NB I did not yet run with the app verifier. I will do that before commit and fix
any newly introduced leaks (if any). If there are pre-existing leaks I will at
least log a bug.

On 2010/09/17 17:51:11, Erik Wright wrote:
> Responded to Siggi's feedback.
> 
> http://breakpad.appspot.com/196001/diff/10001/11006
> File src/client/windows/crash_generation/crash_generation_server.cc (right):
> 
> http://breakpad.appspot.com/196001/diff/10001/11006#newcode212
> Line 212: server_start_complete_event_ = CreateEvent(NULL, TRUE, FALSE, NULL);
> On 2010/09/17 14:33:16, Siggi wrote:
> > This handle needs to be closed at some point, am I missing that code? Would
be
> > logical to do this in the destructor?
> 
> *blush*
> 
> Anyway, removed this in favor of completing initialization within the original
> thread.
> 
> http://breakpad.appspot.com/196001/diff/10001/11006#newcode223
> Line 223: overlapped_.hEvent = CreateEvent(NULL,   // Security descriptor.
> On 2010/09/17 14:33:16, Siggi wrote:
> > I don't see this handle closed anywhere either. Am I missing that too?
> 
> It was done in HandleErrorState but not in the case of normal shutdown. I
added
> it to the destructor.
> 
> http://breakpad.appspot.com/196001/diff/10001/11006#newcode501
> Line 501: } else if (GetLastError() == ERROR_IO_PENDING) {
> On 2010/09/17 14:33:16, Siggi wrote:
> > since GLE reads transient thread-local state, it's generally more
maintainable
> > to read the error into a local variable immediately after invoking the
> > potentially erring call, then using the local variable. 
> > Using GLE in arbitrary expressions tends to be fragile against future
> changes...
> 
> Done.
> 
> http://breakpad.appspot.com/196001/diff/10001/11006#newcode529
> Line 529: assert(GetLastError() != ERROR_IO_INCOMPLETE);
> On 2010/09/17 14:33:16, Siggi wrote:
> > ditto.
> 
> done.
> 
> http://breakpad.appspot.com/196001/diff/10001/11006#newcode660
> Line 660: if (!success && GetLastError() != ERROR_IO_PENDING) {
> On 2010/09/17 14:33:16, Siggi wrote:
> > ditto...
> Done.
> 
> http://breakpad.appspot.com/196001/diff/10001/11007
> File src/client/windows/crash_generation/crash_generation_server.h (right):
> 
> http://breakpad.appspot.com/196001/diff/10001/11007#newcode97
> Line 97: bool Start();
> On 2010/09/17 14:33:16, Siggi wrote:
> > Maybe add a comment on the behavior of this method. Note also that since
> you've
> > added a wait on the completion of an event on another thread (and
potentially
> > the start of that new thread), you've added a new constraint to this
function,
> > namely that it can't be called from within the loader's lock. This may or
may
> > not be an issue for current users of this class, I don't know, but it's
> probably
> > not a problem  for Chrome nor Omaha. 
> > As a courtesy to other users, it'd be nice to comment in the CL description
> and
> > perhaps in a note here too.
> 
> Based on discussion, modified the behaviour to complete all initialization
> within the invoking thread.
> 
> http://breakpad.appspot.com/196001/diff/10001/11007#newcode195
> Line 195: void EnterErrorState();
> On 2010/09/17 14:33:16, Siggi wrote:
> > One-line comment on the purpose of each event.
> 
> Done.
> 
> http://breakpad.appspot.com/196001/diff/10001/11002
> File src/client/windows/unittests/crash_generation_server_test.cc (right):
> 
> http://breakpad.appspot.com/196001/diff/10001/11002#newcode71
> Line 71: void FaultyClient(ClientFault fault_type) {
> On 2010/09/17 14:33:16, Siggi wrote:
> > suggest making this a member of the test fixture, adding these locals as
> > members, and using TearDown to cleanup. 
> 
> Yes, very reasonable.
> 
> The only cleanup needed is to close the pipe HANDLE. I fixed that up by
putting
> all the code that uses the pipe in 'DoFaultyClient'. 'FaultyClient' then opens
> the pipe, calls 'DoFaultyClient', and unconditionally closes the pipe
> afterwards. This simplified much of the conditional logic in what is now
> 'DoFaultyClient'.
> 
> http://breakpad.appspot.com/196001/diff/10001/11002#newcode75
> Line 75: DWORD thread_id;
> On 2010/09/17 14:33:16, Siggi wrote:
> > initialize all variables at point of declaration.
> 
> Done now in member initialization list / constructor.
> 
> http://breakpad.appspot.com/196001/diff/10001/11002#newcode275
> Line 275: 
> On 2010/09/17 14:33:16, Siggi wrote:
> > nit: extra line.
> 
> Fixed.

[Siggi]

lgtm++

http://breakpad.appspot.com/196001/diff/16001/17006
File src/client/windows/crash_generation/crash_generation_server.cc (right):

http://breakpad.appspot.com/196001/diff/16001/17006#newcode490
Line 490: DWORD error_code = GetLastError();
I'd prefer this on the code line immediately following the ReadFile statement.
This has much the same dangers wrgt maintainability as an inline call in the
expression.

http://breakpad.appspot.com/196001/diff/16001/17006#newcode518
Line 518: DWORD error_code = GetLastError();
ditto ...

http://breakpad.appspot.com/196001/diff/16001/17006#newcode654
Line 654: DWORD error_code = GetLastError();
and ditto.

http://breakpad.appspot.com/196001/diff/16001/17003
File src/client/windows/unittests/testing.gyp (right):

http://breakpad.appspot.com/196001/diff/16001/17003#newcode56
Line 56: 'target_name': 'gmock',
out of curiosity, how does this work in the original breakpad   repo - do they
use SVN externals to pull these in?

[Erik Wright]

Another take on usage of GetLastError...

http://breakpad.appspot.com/196001/diff/16001/17006
File src/client/windows/crash_generation/crash_generation_server.cc (right):

http://breakpad.appspot.com/196001/diff/16001/17006#newcode490
Line 490: DWORD error_code = GetLastError();
On 2010/09/17 18:07:46, Siggi wrote:
> I'd prefer this on the code line immediately following the ReadFile statement.
> This has much the same dangers wrgt maintainability as an inline call in the
> expression.

How about this? It doesn't send off flags for me...

http://breakpad.appspot.com/196001/diff/16001/17006#newcode518
Line 518: DWORD error_code = GetLastError();
On 2010/09/17 18:07:46, Siggi wrote:
> ditto ...

ditto ditto.

http://breakpad.appspot.com/196001/diff/16001/17006#newcode654
Line 654: DWORD error_code = GetLastError();
On 2010/09/17 18:07:46, Siggi wrote:
> and ditto.
ditto ditto.

http://breakpad.appspot.com/196001/diff/16001/17003
File src/client/windows/unittests/testing.gyp (right):

http://breakpad.appspot.com/196001/diff/16001/17003#newcode56
Line 56: 'target_name': 'gmock',
On 2010/09/17 18:07:46, Siggi wrote:
> out of curiosity, how does this work in the original breakpad   repo - do they
> use SVN externals to pull these in? 

Yes.

[Siggi]

lgtm+++