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.
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.
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.
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 ...
Issue 196001: Fix CrashGenerationServer to recover from protocol errors and a test for same.
(Closed)
Created 13 years, 12 months ago by Erik Wright
Modified 13 years, 11 months ago
Reviewers: Siggi
Base URL: http://google-breakpad.googlecode.com/svn/trunk/
Comments: 33