From 06cede988b4d15b4d6b7e759fda676bfe3455682 Mon Sep 17 00:00:00 2001 From: "ted.mielczarek" Date: Fri, 11 Nov 2011 19:05:51 +0000 Subject: Allow CrashGenerationClient to request that a dump of the parent process be written. A=Jim Mathies R=ted at https://bugzilla.mozilla.org/show_bug.cgi?id=679238 git-svn-id: http://google-breakpad.googlecode.com/svn/trunk@883 4c0a9323-5329-0410-9bdc-e9ce6186880e --- src/client/windows/common/ipc_protocol.h | 6 ++ src/client/windows/crash_generation/client_info.cc | 23 +++++++ src/client/windows/crash_generation/client_info.h | 15 +++++ .../crash_generation/crash_generation_client.cc | 36 +++++++++++ .../crash_generation/crash_generation_client.h | 13 ++++ .../crash_generation/crash_generation_server.cc | 73 +++++++++++++++++++++- .../crash_generation/crash_generation_server.h | 23 +++++++ src/client/windows/handler/exception_handler.cc | 6 ++ src/client/windows/handler/exception_handler.h | 6 ++ .../crash_generation_app/crash_generation_app.cc | 1 + .../unittests/crash_generation_server_test.cc | 7 ++- .../unittests/exception_handler_death_test.cc | 1 + .../windows/unittests/exception_handler_test.cc | 2 + 13 files changed, 208 insertions(+), 4 deletions(-) diff --git a/src/client/windows/common/ipc_protocol.h b/src/client/windows/common/ipc_protocol.h index 7d101d38..10a84d58 100644 --- a/src/client/windows/common/ipc_protocol.h +++ b/src/client/windows/common/ipc_protocol.h @@ -110,6 +110,7 @@ struct ProtocolMessage { custom_client_info(), dump_request_handle(NULL), dump_generated_handle(NULL), + parent_dump_request_handle(NULL), server_alive_handle(NULL) { } @@ -122,6 +123,7 @@ struct ProtocolMessage { MDRawAssertionInfo* arg_assert_info, const CustomClientInfo& custom_info, HANDLE arg_dump_request_handle, + HANDLE arg_parent_dump_request_handle, HANDLE arg_dump_generated_handle, HANDLE arg_server_alive) : tag(arg_tag), @@ -132,6 +134,7 @@ struct ProtocolMessage { assert_info(arg_assert_info), custom_client_info(custom_info), dump_request_handle(arg_dump_request_handle), + parent_dump_request_handle(arg_parent_dump_request_handle), dump_generated_handle(arg_dump_generated_handle), server_alive_handle(arg_server_alive) { } @@ -161,6 +164,9 @@ struct ProtocolMessage { // Handle to signal the crash event. HANDLE dump_request_handle; + // Handle to signal a request for a server side minidump. + HANDLE parent_dump_request_handle; + // Handle to check if server is done generating crash. HANDLE dump_generated_handle; diff --git a/src/client/windows/crash_generation/client_info.cc b/src/client/windows/crash_generation/client_info.cc index 94f9c3cd..12f4bb30 100644 --- a/src/client/windows/crash_generation/client_info.cc +++ b/src/client/windows/crash_generation/client_info.cc @@ -50,6 +50,8 @@ ClientInfo::ClientInfo(CrashGenerationServer* crash_server, thread_id_(thread_id), process_handle_(NULL), dump_requested_handle_(NULL), + parent_dump_requested_handle_(NULL), + parent_dump_request_wait_handle_(NULL), dump_generated_handle_(NULL), dump_request_wait_handle_(NULL), process_exit_wait_handle_(NULL) { @@ -70,6 +72,14 @@ bool ClientInfo::Initialize() { return false; } + parent_dump_requested_handle_ = CreateEvent(NULL, // Security attributes. + TRUE, // Manual reset. + FALSE, // Initial state. + NULL); // Name. + if (!parent_dump_requested_handle_) { + return false; + } + dump_generated_handle_ = CreateEvent(NULL, // Security attributes. TRUE, // Manual reset. FALSE, // Initial state. @@ -88,6 +98,10 @@ ClientInfo::~ClientInfo() { UnregisterWaitEx(process_exit_wait_handle_, INVALID_HANDLE_VALUE); } + if (parent_dump_request_wait_handle_) { + UnregisterWaitEx(parent_dump_request_wait_handle_, INVALID_HANDLE_VALUE); + } + if (process_handle_) { CloseHandle(process_handle_); } @@ -96,6 +110,10 @@ ClientInfo::~ClientInfo() { CloseHandle(dump_requested_handle_); } + if (parent_dump_requested_handle_) { + CloseHandle(parent_dump_requested_handle_); + } + if (dump_generated_handle_) { CloseHandle(dump_generated_handle_); } @@ -107,6 +125,11 @@ void ClientInfo::UnregisterWaits() { dump_request_wait_handle_ = NULL; } + if (parent_dump_request_wait_handle_) { + UnregisterWait(parent_dump_request_wait_handle_); + parent_dump_request_wait_handle_ = NULL; + } + if (process_exit_wait_handle_) { UnregisterWait(process_exit_wait_handle_); process_exit_wait_handle_ = NULL; diff --git a/src/client/windows/crash_generation/client_info.h b/src/client/windows/crash_generation/client_info.h index 47a5d21f..6a8e0b94 100644 --- a/src/client/windows/crash_generation/client_info.h +++ b/src/client/windows/crash_generation/client_info.h @@ -65,6 +65,7 @@ class ClientInfo { HANDLE process_handle() const { return process_handle_; } HANDLE dump_requested_handle() const { return dump_requested_handle_; } HANDLE dump_generated_handle() const { return dump_generated_handle_; } + HANDLE parent_dump_requested_handle() const { return parent_dump_requested_handle_; } HANDLE dump_request_wait_handle() const { return dump_request_wait_handle_; @@ -82,6 +83,14 @@ class ClientInfo { process_exit_wait_handle_ = value; } + HANDLE parent_dump_request_wait_handle() const { + return parent_dump_request_wait_handle_; + } + + void set_parent_dump_request_wait_handle(HANDLE value) { + parent_dump_request_wait_handle_ = value; + } + // Unregister all waits for the client. void UnregisterWaits(); @@ -150,12 +159,18 @@ class ClientInfo { // Dump generated event handle. HANDLE dump_generated_handle_; + // Parent dump request event handle. + HANDLE parent_dump_requested_handle_; + // Wait handle for dump request event. HANDLE dump_request_wait_handle_; // Wait handle for process exit event. HANDLE process_exit_wait_handle_; + // Wait handle for parent dump request event. + HANDLE parent_dump_request_wait_handle_; + // Time when the client process started. It is used to determine the uptime // for the client process when it signals a crash. FILETIME start_time_; diff --git a/src/client/windows/crash_generation/crash_generation_client.cc b/src/client/windows/crash_generation/crash_generation_client.cc index 5e4e3cb9..304c6061 100644 --- a/src/client/windows/crash_generation/crash_generation_client.cc +++ b/src/client/windows/crash_generation/crash_generation_client.cc @@ -194,6 +194,7 @@ bool CrashGenerationClient::RegisterClient(HANDLE pipe) { custom_info_, NULL, NULL, + NULL, NULL); ProtocolMessage reply; DWORD bytes_count = 0; @@ -220,8 +221,10 @@ bool CrashGenerationClient::RegisterClient(HANDLE pipe) { if (!WriteFile(pipe, &ack_msg, sizeof(ack_msg), &bytes_count, NULL)) { return false; } + crash_event_ = reply.dump_request_handle; crash_generated_ = reply.dump_generated_handle; + parent_dump_request_event_ = reply.parent_dump_request_handle; server_alive_ = reply.server_alive_handle; server_process_id_ = reply.pid; @@ -271,6 +274,39 @@ bool CrashGenerationClient::IsRegistered() const { return crash_event_ != NULL; } +bool CrashGenerationClient::RequestParentDump() { + if (!IsRegistered()) { + return false; + } + + assert(parent_dump_request_event_); + assert(server_alive_); + + // Reset the dump generated event before signaling the crash + // event so that the server can set the dump generated event + // once it is done generating the event. + if (!ResetEvent(crash_generated_)) { + return false; + } + + // Signal we want a server side crash dump generated + if (!SetEvent(parent_dump_request_event_)) { + return false; + } + + // Wait for the crash dump process to complete + HANDLE wait_handles[kWaitEventCount] = {crash_generated_, server_alive_}; + + DWORD result = WaitForMultipleObjects(kWaitEventCount, + wait_handles, + FALSE, + kWaitForServerTimeoutMs); + + // Crash dump was successfully generated only if the server + // signaled the crash generated event. + return result == WAIT_OBJECT_0; +} + bool CrashGenerationClient::RequestDump(EXCEPTION_POINTERS* ex_info, MDRawAssertionInfo* assert_info) { if (!IsRegistered()) { diff --git a/src/client/windows/crash_generation/crash_generation_client.h b/src/client/windows/crash_generation/crash_generation_client.h index 01d13dde..e026599e 100644 --- a/src/client/windows/crash_generation/crash_generation_client.h +++ b/src/client/windows/crash_generation/crash_generation_client.h @@ -91,6 +91,15 @@ class CrashGenerationClient { // if the registration step was not performed or it was not successful, // false will be returned. bool RequestDump(MDRawAssertionInfo* assert_info); + + // Requests the crash server generate a minidump on the parent side + // of the connection. Blocks until the dump has been completed or + // the request times out. + // + // Returns true if the dump was successful; false otherwise. Note that + // if the registration step was not performed or it was not successful, + // false will be returned. + bool RequestParentDump(); private: // Connects to the appropriate pipe and sets the pipe handle state. @@ -132,6 +141,10 @@ class CrashGenerationClient { // Event to signal in case of a crash. HANDLE crash_event_; + // Event signaling a server side minidump is being requested + // by the client. + HANDLE parent_dump_request_event_; + // Handle to wait on after signaling a crash for the server // to finish generating crash dump. HANDLE crash_generated_; diff --git a/src/client/windows/crash_generation/crash_generation_server.cc b/src/client/windows/crash_generation/crash_generation_server.cc index 61af1b2d..369768df 100644 --- a/src/client/windows/crash_generation/crash_generation_server.cc +++ b/src/client/windows/crash_generation/crash_generation_server.cc @@ -119,7 +119,8 @@ CrashGenerationServer::CrashGenerationServer( shutting_down_(false), overlapped_(), client_info_(NULL), - cleanup_item_count_(0) { + cleanup_item_count_(0), + preferred_parent_thread_id_(0) { InitializeCriticalSection(&clients_sync_); if (dump_path) { @@ -600,6 +601,9 @@ bool CrashGenerationServer::PrepareReply(const ClientInfo& client_info, CloseHandle(reply->server_alive_handle); } + if (reply->parent_dump_request_handle) { + CloseHandle(reply->parent_dump_request_handle); + } return false; } @@ -626,6 +630,16 @@ bool CrashGenerationServer::CreateClientHandles(const ClientInfo& client_info, return false; } + if (!DuplicateHandle(current_process, + client_info.parent_dump_requested_handle(), + client_info.process_handle(), + &reply->parent_dump_request_handle, + kDumpRequestEventAccess, + FALSE, + 0)) { + return false; + } + if (!DuplicateHandle(current_process, server_alive_handle_, client_info.process_handle(), @@ -754,6 +768,20 @@ bool CrashGenerationServer::AddClient(ClientInfo* client_info) { client_info->set_process_exit_wait_handle(process_wait_handle); + // OnParentDumpRequest will be called if the client requests + // a server side minidump be generated. + HANDLE parent_request_wait_handle = NULL; + if (!RegisterWaitForSingleObject(&parent_request_wait_handle, + client_info->parent_dump_requested_handle(), + OnParentDumpRequest, + client_info, + INFINITE, + kDumpRequestThreadFlags)) { + return false; + } + + client_info->set_parent_dump_request_wait_handle(parent_request_wait_handle); + // New scope to hold the lock for the shortest time. { AutoCriticalSection lock(&clients_sync_); @@ -785,6 +813,20 @@ void CALLBACK CrashGenerationServer::OnDumpRequest(void* context, BOOLEAN) { ResetEvent(client_info->dump_requested_handle()); } +// static +void CALLBACK CrashGenerationServer::OnParentDumpRequest(void* context, BOOLEAN) { + assert(context); + ClientInfo* client_info = reinterpret_cast(context); + client_info->PopulateCustomInfo(); + + CrashGenerationServer* crash_server = client_info->crash_server(); + assert(crash_server); + + crash_server->HandleParentDumpRequest(*client_info); + + ResetEvent(client_info->parent_dump_requested_handle()); +} + // static void CALLBACK CrashGenerationServer::OnClientEnd(void* context, BOOLEAN) { assert(context); @@ -843,7 +885,34 @@ void CrashGenerationServer::HandleDumpRequest(const ClientInfo& client_info) { if (dump_callback_) { std::wstring* ptr_dump_path = (dump_path == L"") ? NULL : &dump_path; - dump_callback_(dump_context_, &client_info, ptr_dump_path); + dump_callback_(dump_context_, &client_info, DUMP_REQ_CHILD, ptr_dump_path); + } + + SetEvent(client_info.dump_generated_handle()); +} + +void CrashGenerationServer::HandleParentDumpRequest(const ClientInfo& client_info) { + std::wstring dump_path; + if (generate_dumps_) { + DWORD preferred_thread_id = !preferred_parent_thread_id_ ? + GetCurrentThreadId() : preferred_parent_thread_id_; + bool success = + dump_generator_->WriteMinidump(GetCurrentProcess(), + GetCurrentProcessId(), + preferred_thread_id, + GetCurrentThreadId(), + NULL, // no exception + NULL, // no assert info + MiniDumpNormal, + true, + &dump_path); + if (!success) + return; + } + + if (dump_callback_) { + std::wstring* ptr_dump_path = (dump_path == L"") ? NULL : &dump_path; + dump_callback_(dump_context_, &client_info, DUMP_REQ_PARENT, ptr_dump_path); } SetEvent(client_info.dump_generated_handle()); diff --git a/src/client/windows/crash_generation/crash_generation_server.h b/src/client/windows/crash_generation/crash_generation_server.h index 31a353bf..c31f91cd 100644 --- a/src/client/windows/crash_generation/crash_generation_server.h +++ b/src/client/windows/crash_generation/crash_generation_server.h @@ -49,11 +49,19 @@ class ClientInfo; // generation in this way, the server generates Windows minidump files. class CrashGenerationServer { public: + // For client minidump request callbacks indicates the type of minidump + // the request generated. + enum ClientDumpRequestType { + DUMP_REQ_PARENT, + DUMP_REQ_CHILD, + }; + typedef void (*OnClientConnectedCallback)(void* context, const ClientInfo* client_info); typedef void (*OnClientDumpRequestCallback)(void* context, const ClientInfo* client_info, + const ClientDumpRequestType request_type, const std::wstring* file_path); typedef void (*OnClientExitedCallback)(void* context, @@ -97,6 +105,11 @@ class CrashGenerationServer { // Returns true if initialization is successful; false otherwise. bool Start(); + // Sets the preferred parent side minidump thread id passed + // to breakpad when a server side minidump is requested by + // a client. + void SetParentDumpThreadId(DWORD thread_id) { preferred_parent_thread_id_ = thread_id; } + private: // Various states the client can be in during the handshake with // the server. @@ -175,12 +188,18 @@ class CrashGenerationServer { // Handles a dump request from the client. void HandleDumpRequest(const ClientInfo& client_info); + // Handles a server side minidump request from the client. + void HandleParentDumpRequest(const ClientInfo& client_info); + // Callback for pipe connected event. static void CALLBACK OnPipeConnected(void* context, BOOLEAN timer_or_wait); // Callback for a dump request. static void CALLBACK OnDumpRequest(void* context, BOOLEAN timer_or_wait); + // Callback for a server minidump request. + static void CALLBACK OnParentDumpRequest(void* context, BOOLEAN timer_or_wait); + // Callback for client process exit event. static void CALLBACK OnClientEnd(void* context, BOOLEAN timer_or_wait); @@ -278,6 +297,10 @@ class CrashGenerationServer { // already queued to run. volatile LONG cleanup_item_count_; + // For client requested server side minidumps, the preferred + // minidump thread id passed to breakpad. + DWORD preferred_parent_thread_id_; + // Disable copy ctor and operator=. CrashGenerationServer(const CrashGenerationServer& crash_server); CrashGenerationServer& operator=(const CrashGenerationServer& crash_server); diff --git a/src/client/windows/handler/exception_handler.cc b/src/client/windows/handler/exception_handler.cc index ec5397dc..fc55ca72 100644 --- a/src/client/windows/handler/exception_handler.cc +++ b/src/client/windows/handler/exception_handler.cc @@ -704,6 +704,12 @@ bool ExceptionHandler::WriteMinidump(const wstring &dump_path, return handler.WriteMinidump(); } +bool ExceptionHandler::RequestMinidumpForParent() { + if (!IsOutOfProcess()) + return false; + return crash_generation_client_->RequestParentDump(); +} + bool ExceptionHandler::WriteMinidumpWithException( DWORD requesting_thread_id, EXCEPTION_POINTERS* exinfo, diff --git a/src/client/windows/handler/exception_handler.h b/src/client/windows/handler/exception_handler.h index 2c2e7b76..7b2da3f3 100644 --- a/src/client/windows/handler/exception_handler.h +++ b/src/client/windows/handler/exception_handler.h @@ -190,6 +190,12 @@ class ExceptionHandler { static bool WriteMinidump(const wstring &dump_path, MinidumpCallback callback, void* callback_context); + // Requests a minidump of the server be generated and waits for completion. + // This can be used to capture the execution state of parent process + // independent of a crash on either side of the connection. Only valid for + // out-of-process clients. + bool RequestMinidumpForParent(); + // Get the thread ID of the thread requesting the dump (either the exception // thread or any other thread that called WriteMinidump directly). This // may be useful if you want to include additional thread state in your diff --git a/src/client/windows/tests/crash_generation_app/crash_generation_app.cc b/src/client/windows/tests/crash_generation_app/crash_generation_app.cc index 1b4bd3c9..f21972dd 100644 --- a/src/client/windows/tests/crash_generation_app/crash_generation_app.cc +++ b/src/client/windows/tests/crash_generation_app/crash_generation_app.cc @@ -216,6 +216,7 @@ static void _cdecl ShowClientConnected(void* context, static void _cdecl ShowClientCrashed(void* context, const ClientInfo* client_info, + const CrashGenerationServer::ClientDumpRequestType request_type, const wstring* dump_path) { TCHAR* line = new TCHAR[kMaximumLineLength]; line[0] = _T('\0'); diff --git a/src/client/windows/unittests/crash_generation_server_test.cc b/src/client/windows/unittests/crash_generation_server_test.cc index ce49439c..01dda951 100644 --- a/src/client/windows/unittests/crash_generation_server_test.cc +++ b/src/client/windows/unittests/crash_generation_server_test.cc @@ -77,8 +77,9 @@ class CrashGenerationServerTest : public ::testing::Test { public: MOCK_METHOD1(OnClientConnected, void(const google_breakpad::ClientInfo* client_info)); - MOCK_METHOD2(OnClientDumpRequested, + MOCK_METHOD3(OnClientDumpRequested, void(const google_breakpad::ClientInfo* client_info, + const google_breakpad::CrashGenerationServer::ClientDumpRequestType request_type, const std::wstring* file_path)); MOCK_METHOD1(OnClientExited, void(const google_breakpad::ClientInfo* client_info)); @@ -176,6 +177,7 @@ class CrashGenerationServerTest : public ::testing::Test { custom_info, NULL, NULL, + NULL, NULL); DWORD bytes_count = 0; @@ -237,9 +239,10 @@ class CrashGenerationServerTest : public ::testing::Test { static void CallOnClientDumpRequested( void* context, const google_breakpad::ClientInfo* client_info, + const google_breakpad::CrashGenerationServer::ClientDumpRequestType request_type, const std::wstring* file_path) { static_cast(context)-> - OnClientDumpRequested(client_info, file_path); + OnClientDumpRequested(client_info, request_type, file_path); } static void CallOnClientExited( diff --git a/src/client/windows/unittests/exception_handler_death_test.cc b/src/client/windows/unittests/exception_handler_death_test.cc index adea044f..bac96def 100644 --- a/src/client/windows/unittests/exception_handler_death_test.cc +++ b/src/client/windows/unittests/exception_handler_death_test.cc @@ -131,6 +131,7 @@ static bool gDumpCallbackCalled = false; void clientDumpCallback(void *dump_context, const google_breakpad::ClientInfo *client_info, + const google_breakpad::CrashGenerationServer::ClientDumpRequestType request_type, const std::wstring *dump_path) { gDumpCallbackCalled = true; } diff --git a/src/client/windows/unittests/exception_handler_test.cc b/src/client/windows/unittests/exception_handler_test.cc index f0341880..18aeded7 100644 --- a/src/client/windows/unittests/exception_handler_test.cc +++ b/src/client/windows/unittests/exception_handler_test.cc @@ -78,6 +78,7 @@ class ExceptionHandlerTest : public ::testing::Test { static void ClientDumpCallback( void *dump_context, const google_breakpad::ClientInfo *client_info, + const google_breakpad::CrashGenerationServer::ClientDumpRequestType request_type, const std::wstring *dump_path); static bool DumpCallback(const wchar_t* dump_path, @@ -138,6 +139,7 @@ BOOL ExceptionHandlerTest::DoesPathExist(const TCHAR *path_name) { void ExceptionHandlerTest::ClientDumpCallback( void *dump_context, const google_breakpad::ClientInfo *client_info, + const google_breakpad::CrashGenerationServer::ClientDumpRequestType request_type, const wstring *dump_path) { dump_file = *dump_path; // Create the full dump file name from the dump path. -- cgit v1.2.1