CrashGenerationServer's state machine can be invoked from both the application

thread and thread pool threads. This CL serializes access to the FSM state.
Handling of crash dump and client shutdown requests is still done
asynchronously.

Patch by Alex Pakhunov <alexeypa@chromium.org>

BUG=132164
TEST=remoting_unittests.BreakpadWinDeathTest.*

Review URL: https://breakpad.appspot.com/396002/


git-svn-id: http://google-breakpad.googlecode.com/svn/trunk@970 4c0a9323-5329-0410-9bdc-e9ce6186880e

1 files changed, 51 insertions, 44 deletions

diff --git a/src/client/windows/crash_generation/crash_generation_server.cc b/src/client/windows/crash_generation/crash_generation_server.cc
index 794742db..477973c1 100644
--- a/src/client/windows/crash_generation/crash_generation_server.cc
+++ b/src/client/windows/crash_generation/crash_generation_server.cc
@@ -125,7 +125,7 @@ CrashGenerationServer::CrashGenerationServer(
       overlapped_(),
       client_info_(NULL),
       cleanup_item_count_(0) {
-  InitializeCriticalSection(&clients_sync_);
+  InitializeCriticalSection(&sync_);
 
   if (dump_path) {
     dump_generator_.reset(new MinidumpGenerator(*dump_path));
@@ -133,38 +133,41 @@ CrashGenerationServer::CrashGenerationServer(
 }
 
 CrashGenerationServer::~CrashGenerationServer() {
-  // Indicate to existing threads that server is shutting down.
-  shutting_down_ = true;
-
-  // Even if there are no current worker threads running, it is possible that
-  // an I/O request is pending on the pipe right now but not yet done. In fact,
-  // it's very likely this is the case unless we are in an ERROR state. If we
-  // don't wait for the pending I/O to be done, then when the I/O completes,
-  // it may write to invalid memory. AppVerifier will flag this problem too.
-  // So we disconnect from the pipe and then wait for the server to get into
-  // error state so that the pending I/O will fail and get cleared.
-  DisconnectNamedPipe(pipe_);
-  int num_tries = 100;
-  while (num_tries-- && server_state_ != IPC_SERVER_STATE_ERROR) {
-    Sleep(10);
-  }
-
-  // Unregister wait on the pipe.
-  if (pipe_wait_handle_) {
-    // Wait for already executing callbacks to finish.
-    UnregisterWaitEx(pipe_wait_handle_, INVALID_HANDLE_VALUE);
-  }
+  // New scope to release the lock automatically.
+  {
+    AutoCriticalSection lock(&sync_);
+
+    // Indicate to existing threads that server is shutting down.
+    shutting_down_ = true;
+
+    // Even if there are no current worker threads running, it is possible that
+    // an I/O request is pending on the pipe right now but not yet done.
+    // In fact, it's very likely this is the case unless we are in an ERROR
+    // state. If we don't wait for the pending I/O to be done, then when the I/O
+    // completes, it may write to invalid memory. AppVerifier will flag this
+    // problem too. So we disconnect from the pipe and then wait for the server
+    // to get into error state so that the pending I/O will fail and get
+    // cleared.
+    DisconnectNamedPipe(pipe_);
+    int num_tries = 100;
+    while (num_tries-- && server_state_ != IPC_SERVER_STATE_ERROR) {
+      lock.Release();
+      Sleep(10);
+      lock.Acquire();
+    }
 
-  // Close the pipe to avoid further client connections.
-  if (pipe_) {
-    CloseHandle(pipe_);
-  }
+    // Unregister wait on the pipe.
+    if (pipe_wait_handle_) {
+      // Wait for already executing callbacks to finish.
+      UnregisterWaitEx(pipe_wait_handle_, INVALID_HANDLE_VALUE);
+    }
 
-  // Request all ClientInfo objects to unregister all waits.
-  // New scope to hold the lock for the shortest time.
-  {
-    AutoCriticalSection lock(&clients_sync_);
+    // Close the pipe to avoid further client connections.
+    if (pipe_) {
+      CloseHandle(pipe_);
+    }
 
+    // Request all ClientInfo objects to unregister all waits.
     std::list<ClientInfo*>::iterator iter;
     for (iter = clients_.begin(); iter != clients_.end(); ++iter) {
       ClientInfo* client_info = *iter;
@@ -185,33 +188,35 @@ CrashGenerationServer::~CrashGenerationServer() {
     }
   }
 
-  // Clean up all the ClientInfo objects.
   // New scope to hold the lock for the shortest time.
   {
-    AutoCriticalSection lock(&clients_sync_);
+    AutoCriticalSection lock(&sync_);
 
+    // Clean up all the ClientInfo objects.
     std::list<ClientInfo*>::iterator iter;
     for (iter = clients_.begin(); iter != clients_.end(); ++iter) {
       ClientInfo* client_info = *iter;
       delete client_info;
     }
-  }
 
-  if (server_alive_handle_) {
-    // Release the mutex before closing the handle so that clients requesting
-    // dumps wait for a long time for the server to generate a dump.
-    ReleaseMutex(server_alive_handle_);
-    CloseHandle(server_alive_handle_);
-  }
+    if (server_alive_handle_) {
+      // Release the mutex before closing the handle so that clients requesting
+      // dumps wait for a long time for the server to generate a dump.
+      ReleaseMutex(server_alive_handle_);
+      CloseHandle(server_alive_handle_);
+    }
 
-  if (overlapped_.hEvent) {
-    CloseHandle(overlapped_.hEvent);
+    if (overlapped_.hEvent) {
+      CloseHandle(overlapped_.hEvent);
+    }
   }
 
-  DeleteCriticalSection(&clients_sync_);
+  DeleteCriticalSection(&sync_);
 }
 
 bool CrashGenerationServer::Start() {
+  AutoCriticalSection lock(&sync_);
+
   if (server_state_ != IPC_SERVER_STATE_UNINITIALIZED) {
     return false;
   }
@@ -682,6 +687,8 @@ bool CrashGenerationServer::RespondToClient(ClientInfo* client_info) {
 // implements the state machine described in ReadMe.txt along with the
 // helper methods HandleXXXState.
 void CrashGenerationServer::HandleConnectionRequest() {
+  AutoCriticalSection lock(&sync_);
+
   // If we are shutting doen then get into ERROR state, reset the event so more
   // workers don't run and return immediately.
   if (shutting_down_) {
@@ -768,7 +775,7 @@ bool CrashGenerationServer::AddClient(ClientInfo* client_info) {
 
   // New scope to hold the lock for the shortest time.
   {
-    AutoCriticalSection lock(&clients_sync_);
+    AutoCriticalSection lock(&sync_);
     clients_.push_back(client_info);
   }
 
@@ -836,7 +843,7 @@ void CrashGenerationServer::DoCleanup(ClientInfo* client_info) {
 
   // Start a new scope to release lock automatically.
   {
-    AutoCriticalSection lock(&clients_sync_);
+    AutoCriticalSection lock(&sync_);
     clients_.remove(client_info);
   }