From 06c856fd6762fc886edd031aad94e6ca4b2064a5 Mon Sep 17 00:00:00 2001 From: "mark@chromium.org" Date: Mon, 28 Nov 2011 19:17:41 +0000 Subject: [PATCH] Speculative back-out of r883, which may have broken Windows crash reporting. git-svn-id: http://google-breakpad.googlecode.com/svn/trunk@891 4c0a9323-5329-0410-9bdc-e9ce6186880e --- src/client/windows/common/ipc_protocol.h | 6 -- .../windows/crash_generation/client_info.cc | 23 ------ .../windows/crash_generation/client_info.h | 15 ---- .../crash_generation_client.cc | 36 --------- .../crash_generation_client.h | 13 ---- .../crash_generation_server.cc | 73 +------------------ .../crash_generation_server.h | 23 ------ .../windows/handler/exception_handler.cc | 6 -- .../windows/handler/exception_handler.h | 6 -- .../crash_generation_app.cc | 1 - .../unittests/crash_generation_server_test.cc | 7 +- .../unittests/exception_handler_death_test.cc | 1 - .../unittests/exception_handler_test.cc | 2 - 13 files changed, 4 insertions(+), 208 deletions(-) diff --git a/src/client/windows/common/ipc_protocol.h b/src/client/windows/common/ipc_protocol.h index 10a84d58..7d101d38 100644 --- a/src/client/windows/common/ipc_protocol.h +++ b/src/client/windows/common/ipc_protocol.h @@ -110,7 +110,6 @@ struct ProtocolMessage { custom_client_info(), dump_request_handle(NULL), dump_generated_handle(NULL), - parent_dump_request_handle(NULL), server_alive_handle(NULL) { } @@ -123,7 +122,6 @@ 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), @@ -134,7 +132,6 @@ 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) { } @@ -164,9 +161,6 @@ 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 12f4bb30..94f9c3cd 100644 --- a/src/client/windows/crash_generation/client_info.cc +++ b/src/client/windows/crash_generation/client_info.cc @@ -50,8 +50,6 @@ 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) { @@ -72,14 +70,6 @@ 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. @@ -98,10 +88,6 @@ 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_); } @@ -110,10 +96,6 @@ ClientInfo::~ClientInfo() { CloseHandle(dump_requested_handle_); } - if (parent_dump_requested_handle_) { - CloseHandle(parent_dump_requested_handle_); - } - if (dump_generated_handle_) { CloseHandle(dump_generated_handle_); } @@ -125,11 +107,6 @@ 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 6a8e0b94..47a5d21f 100644 --- a/src/client/windows/crash_generation/client_info.h +++ b/src/client/windows/crash_generation/client_info.h @@ -65,7 +65,6 @@ 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_; @@ -83,14 +82,6 @@ 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(); @@ -159,18 +150,12 @@ 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 304c6061..5e4e3cb9 100644 --- a/src/client/windows/crash_generation/crash_generation_client.cc +++ b/src/client/windows/crash_generation/crash_generation_client.cc @@ -194,7 +194,6 @@ bool CrashGenerationClient::RegisterClient(HANDLE pipe) { custom_info_, NULL, NULL, - NULL, NULL); ProtocolMessage reply; DWORD bytes_count = 0; @@ -221,10 +220,8 @@ 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; @@ -274,39 +271,6 @@ 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 e026599e..01d13dde 100644 --- a/src/client/windows/crash_generation/crash_generation_client.h +++ b/src/client/windows/crash_generation/crash_generation_client.h @@ -91,15 +91,6 @@ 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. @@ -141,10 +132,6 @@ 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 369768df..61af1b2d 100644 --- a/src/client/windows/crash_generation/crash_generation_server.cc +++ b/src/client/windows/crash_generation/crash_generation_server.cc @@ -119,8 +119,7 @@ CrashGenerationServer::CrashGenerationServer( shutting_down_(false), overlapped_(), client_info_(NULL), - cleanup_item_count_(0), - preferred_parent_thread_id_(0) { + cleanup_item_count_(0) { InitializeCriticalSection(&clients_sync_); if (dump_path) { @@ -601,9 +600,6 @@ 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; } @@ -630,16 +626,6 @@ 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(), @@ -768,20 +754,6 @@ 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_); @@ -813,20 +785,6 @@ 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); @@ -885,34 +843,7 @@ 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, 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); + dump_callback_(dump_context_, &client_info, 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 c31f91cd..31a353bf 100644 --- a/src/client/windows/crash_generation/crash_generation_server.h +++ b/src/client/windows/crash_generation/crash_generation_server.h @@ -49,19 +49,11 @@ 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, @@ -105,11 +97,6 @@ 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. @@ -188,18 +175,12 @@ 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); @@ -297,10 +278,6 @@ 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 fc55ca72..ec5397dc 100644 --- a/src/client/windows/handler/exception_handler.cc +++ b/src/client/windows/handler/exception_handler.cc @@ -704,12 +704,6 @@ 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 7b2da3f3..2c2e7b76 100644 --- a/src/client/windows/handler/exception_handler.h +++ b/src/client/windows/handler/exception_handler.h @@ -190,12 +190,6 @@ 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 f21972dd..1b4bd3c9 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,7 +216,6 @@ 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 01dda951..ce49439c 100644 --- a/src/client/windows/unittests/crash_generation_server_test.cc +++ b/src/client/windows/unittests/crash_generation_server_test.cc @@ -77,9 +77,8 @@ class CrashGenerationServerTest : public ::testing::Test { public: MOCK_METHOD1(OnClientConnected, void(const google_breakpad::ClientInfo* client_info)); - MOCK_METHOD3(OnClientDumpRequested, + MOCK_METHOD2(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)); @@ -177,7 +176,6 @@ class CrashGenerationServerTest : public ::testing::Test { custom_info, NULL, NULL, - NULL, NULL); DWORD bytes_count = 0; @@ -239,10 +237,9 @@ 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, request_type, file_path); + OnClientDumpRequested(client_info, 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 bac96def..adea044f 100644 --- a/src/client/windows/unittests/exception_handler_death_test.cc +++ b/src/client/windows/unittests/exception_handler_death_test.cc @@ -131,7 +131,6 @@ 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 18aeded7..f0341880 100644 --- a/src/client/windows/unittests/exception_handler_test.cc +++ b/src/client/windows/unittests/exception_handler_test.cc @@ -78,7 +78,6 @@ 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, @@ -139,7 +138,6 @@ 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.