From 9a3de4160b4808b89c367126027533323690915e Mon Sep 17 00:00:00 2001 From: "cdn@chromium.org" Date: Fri, 13 Apr 2012 22:20:30 +0000 Subject: [PATCH] Expose a callback to allow crash server implementations to defer the uploading of crash dumps to a later time. The client can provide a crash_id when the dump is performed and then at a later time connect again and request that the crash id be uploaded triggering an implementation defined callback. BUG=473 TEST=CrashGenerationServerTest.* Review URL: https://breakpad.appspot.com/379001 git-svn-id: http://google-breakpad.googlecode.com/svn/trunk@952 4c0a9323-5329-0410-9bdc-e9ce6186880e --- src/client/windows/common/ipc_protocol.h | 14 ++++++----- .../windows/crash_generation/client_info.cc | 9 ++++++- .../windows/crash_generation/client_info.h | 6 +++++ .../crash_generation_client.cc | 4 +-- .../crash_generation_server.cc | 25 +++++++++++++------ .../crash_generation_server.h | 11 ++++++++ .../crash_generation_app.cc | 2 ++ .../unittests/crash_generation_server_test.cc | 8 ++++++ .../unittests/exception_handler_death_test.cc | 4 +-- .../unittests/exception_handler_test.cc | 8 +++--- 10 files changed, 69 insertions(+), 22 deletions(-) diff --git a/src/client/windows/common/ipc_protocol.h b/src/client/windows/common/ipc_protocol.h index 7d101d38..b03c032b 100644 --- a/src/client/windows/common/ipc_protocol.h +++ b/src/client/windows/common/ipc_protocol.h @@ -90,7 +90,8 @@ enum MessageTag { MESSAGE_TAG_NONE = 0, MESSAGE_TAG_REGISTRATION_REQUEST = 1, MESSAGE_TAG_REGISTRATION_RESPONSE = 2, - MESSAGE_TAG_REGISTRATION_ACK = 3 + MESSAGE_TAG_REGISTRATION_ACK = 3, + MESSAGE_TAG_UPLOAD_REQUEST = 4 }; struct CustomClientInfo { @@ -102,7 +103,7 @@ struct CustomClientInfo { struct ProtocolMessage { ProtocolMessage() : tag(MESSAGE_TAG_NONE), - pid(0), + id(0), dump_type(MiniDumpNormal), thread_id(0), exception_pointers(NULL), @@ -115,7 +116,7 @@ struct ProtocolMessage { // Creates an instance with the given parameters. ProtocolMessage(MessageTag arg_tag, - DWORD arg_pid, + DWORD arg_id, MINIDUMP_TYPE arg_dump_type, DWORD* arg_thread_id, EXCEPTION_POINTERS** arg_exception_pointers, @@ -125,7 +126,7 @@ struct ProtocolMessage { HANDLE arg_dump_generated_handle, HANDLE arg_server_alive) : tag(arg_tag), - pid(arg_pid), + id(arg_id), dump_type(arg_dump_type), thread_id(arg_thread_id), exception_pointers(arg_exception_pointers), @@ -139,8 +140,9 @@ struct ProtocolMessage { // Tag in the message. MessageTag tag; - // Process id. - DWORD pid; + // The id for this message. This may be either a process id or a crash id + // depending on the type of message. + DWORD id; // Dump type requested. MINIDUMP_TYPE dump_type; diff --git a/src/client/windows/crash_generation/client_info.cc b/src/client/windows/crash_generation/client_info.cc index 94f9c3cd..9c51cda3 100644 --- a/src/client/windows/crash_generation/client_info.cc +++ b/src/client/windows/crash_generation/client_info.cc @@ -52,7 +52,8 @@ ClientInfo::ClientInfo(CrashGenerationServer* crash_server, dump_requested_handle_(NULL), dump_generated_handle_(NULL), dump_request_wait_handle_(NULL), - process_exit_wait_handle_(NULL) { + process_exit_wait_handle_(NULL), + crash_id_(NULL) { GetSystemTimeAsFileTime(&start_time_); } @@ -62,6 +63,12 @@ bool ClientInfo::Initialize() { return false; } + // The crash_id will be the low order word of the process creation time. + FILETIME creation_time, exit_time, kernel_time, user_time; + if (GetProcessTimes(process_handle_, &creation_time, &exit_time, + &kernel_time, &user_time)) + crash_id_ = creation_time.dwLowDateTime; + dump_requested_handle_ = CreateEvent(NULL, // Security attributes. TRUE, // Manual reset. FALSE, // Initial state. diff --git a/src/client/windows/crash_generation/client_info.h b/src/client/windows/crash_generation/client_info.h index 47a5d21f..999e6678 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_; } + DWORD crash_id() const { return crash_id_; } HANDLE dump_request_wait_handle() const { return dump_request_wait_handle_; @@ -160,6 +161,11 @@ class ClientInfo { // for the client process when it signals a crash. FILETIME start_time_; + // The crash id which can be used to request an upload. This will be the + // value of the low order dword of the process creation time for the process + // being dumped. + DWORD crash_id_; + // Disallow copy ctor and operator=. ClientInfo(const ClientInfo& client_info); ClientInfo& operator=(const ClientInfo& client_info); diff --git a/src/client/windows/crash_generation/crash_generation_client.cc b/src/client/windows/crash_generation/crash_generation_client.cc index 5e4e3cb9..749223f1 100644 --- a/src/client/windows/crash_generation/crash_generation_client.cc +++ b/src/client/windows/crash_generation/crash_generation_client.cc @@ -223,7 +223,7 @@ bool CrashGenerationClient::RegisterClient(HANDLE pipe) { crash_event_ = reply.dump_request_handle; crash_generated_ = reply.dump_generated_handle; server_alive_ = reply.server_alive_handle; - server_process_id_ = reply.pid; + server_process_id_ = reply.id; return true; } @@ -261,7 +261,7 @@ HANDLE CrashGenerationClient::ConnectToPipe(const wchar_t* pipe_name, bool CrashGenerationClient::ValidateResponse( const ProtocolMessage& msg) const { return (msg.tag == MESSAGE_TAG_REGISTRATION_RESPONSE) && - (msg.pid != 0) && + (msg.id != 0) && (msg.dump_request_handle != NULL) && (msg.dump_generated_handle != NULL) && (msg.server_alive_handle != NULL); diff --git a/src/client/windows/crash_generation/crash_generation_server.cc b/src/client/windows/crash_generation/crash_generation_server.cc index b180b2c2..d9769ae8 100644 --- a/src/client/windows/crash_generation/crash_generation_server.cc +++ b/src/client/windows/crash_generation/crash_generation_server.cc @@ -84,11 +84,12 @@ static const int kShutdownDelayMs = 10000; static const int kShutdownSleepIntervalMs = 5; static bool IsClientRequestValid(const ProtocolMessage& msg) { - return msg.tag == MESSAGE_TAG_REGISTRATION_REQUEST && - msg.pid != 0 && - msg.thread_id != NULL && - msg.exception_pointers != NULL && - msg.assert_info != NULL; + return msg.tag == MESSAGE_TAG_UPLOAD_REQUEST || + (msg.tag == MESSAGE_TAG_REGISTRATION_REQUEST && + msg.id != 0 && + msg.thread_id != NULL && + msg.exception_pointers != NULL && + msg.assert_info != NULL); } CrashGenerationServer::CrashGenerationServer( @@ -100,6 +101,8 @@ CrashGenerationServer::CrashGenerationServer( void* dump_context, OnClientExitedCallback exit_callback, void* exit_context, + OnClientUploadRequestCallback upload_request_callback, + void* upload_context, bool generate_dumps, const std::wstring* dump_path) : pipe_name_(pipe_name), @@ -113,6 +116,8 @@ CrashGenerationServer::CrashGenerationServer( dump_context_(dump_context), exit_callback_(exit_callback), exit_context_(exit_context), + upload_request_callback_(upload_request_callback), + upload_context_(upload_context), generate_dumps_(generate_dumps), dump_generator_(NULL), server_state_(IPC_SERVER_STATE_UNINITIALIZED), @@ -416,9 +421,15 @@ void CrashGenerationServer::HandleReadDoneState() { return; } + if (msg_.tag == MESSAGE_TAG_UPLOAD_REQUEST) { + if (upload_request_callback_) + upload_request_callback_(upload_context_, msg_.id); + return; + } + scoped_ptr client_info( new ClientInfo(this, - msg_.pid, + msg_.id, msg_.dump_type, msg_.thread_id, msg_.exception_pointers, @@ -582,7 +593,7 @@ void CrashGenerationServer::EnterStateImmediately(IPCServerState state) { bool CrashGenerationServer::PrepareReply(const ClientInfo& client_info, ProtocolMessage* reply) const { reply->tag = MESSAGE_TAG_REGISTRATION_RESPONSE; - reply->pid = GetCurrentProcessId(); + reply->id = GetCurrentProcessId(); if (CreateClientHandles(client_info, reply)) { return true; diff --git a/src/client/windows/crash_generation/crash_generation_server.h b/src/client/windows/crash_generation/crash_generation_server.h index 31a353bf..2f0a222c 100644 --- a/src/client/windows/crash_generation/crash_generation_server.h +++ b/src/client/windows/crash_generation/crash_generation_server.h @@ -59,6 +59,9 @@ class CrashGenerationServer { typedef void (*OnClientExitedCallback)(void* context, const ClientInfo* client_info); + typedef void (*OnClientUploadRequestCallback)(void* context, + const DWORD crash_id); + // Creates an instance with the given parameters. // // Parameter pipe_name: Name of the Windows named pipe @@ -86,6 +89,8 @@ class CrashGenerationServer { void* dump_context, OnClientExitedCallback exit_callback, void* exit_context, + OnClientUploadRequestCallback upload_request_callback, + void* upload_context, bool generate_dumps, const std::wstring* dump_path); @@ -250,6 +255,12 @@ class CrashGenerationServer { // Context for client process exit callback. void* exit_context_; + // Callback for upload request. + OnClientUploadRequestCallback upload_request_callback_; + + // Context for upload request callback. + void* upload_context_; + // Whether to generate dumps. bool generate_dumps_; 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..e36c1d1e 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 @@ -291,6 +291,8 @@ void CrashServerStart() { NULL, ShowClientExited, NULL, + NULL, + NULL, true, &dump_path); diff --git a/src/client/windows/unittests/crash_generation_server_test.cc b/src/client/windows/unittests/crash_generation_server_test.cc index ce49439c..cf95d43f 100644 --- a/src/client/windows/unittests/crash_generation_server_test.cc +++ b/src/client/windows/unittests/crash_generation_server_test.cc @@ -65,6 +65,7 @@ class CrashGenerationServerTest : public ::testing::Test { CallOnClientConnected, &mock_callbacks_, CallOnClientDumpRequested, &mock_callbacks_, CallOnClientExited, &mock_callbacks_, + CallOnClientUploadRequested, &mock_callbacks_, false, NULL), thread_id_(0), @@ -82,6 +83,8 @@ class CrashGenerationServerTest : public ::testing::Test { const std::wstring* file_path)); MOCK_METHOD1(OnClientExited, void(const google_breakpad::ClientInfo* client_info)); + MOCK_METHOD1(OnClientUploadRequested, + void(const DWORD crash_id)); }; enum ClientFault { @@ -248,6 +251,11 @@ class CrashGenerationServerTest : public ::testing::Test { OnClientExited(client_info); } + static void CallOnClientUploadRequested(void* context, const DWORD crash_id) { + static_cast(context)-> + OnClientUploadRequested(crash_id); + } + DWORD thread_id_; EXCEPTION_POINTERS* exception_pointers_; MDRawAssertionInfo assert_info_; diff --git a/src/client/windows/unittests/exception_handler_death_test.cc b/src/client/windows/unittests/exception_handler_death_test.cc index adea044f..514ee72a 100644 --- a/src/client/windows/unittests/exception_handler_death_test.cc +++ b/src/client/windows/unittests/exception_handler_death_test.cc @@ -161,8 +161,8 @@ TEST_F(ExceptionHandlerDeathTest, OutOfProcTest) { ASSERT_TRUE(DoesPathExist(temp_path_)); std::wstring dump_path(temp_path_); google_breakpad::CrashGenerationServer server( - kPipeName, NULL, NULL, NULL, &clientDumpCallback, NULL, NULL, NULL, true, - &dump_path); + kPipeName, NULL, NULL, NULL, &clientDumpCallback, NULL, NULL, NULL, NULL, + NULL, true, &dump_path); // This HAS to be EXPECT_, because when this test case is executed in the // child process, the server registration will fail due to the named pipe diff --git a/src/client/windows/unittests/exception_handler_test.cc b/src/client/windows/unittests/exception_handler_test.cc index f0341880..74d9a9be 100644 --- a/src/client/windows/unittests/exception_handler_test.cc +++ b/src/client/windows/unittests/exception_handler_test.cc @@ -220,8 +220,8 @@ TEST_F(ExceptionHandlerTest, InvalidParameterMiniDumpTest) { ASSERT_TRUE(DoesPathExist(temp_path_)); wstring dump_path(temp_path_); google_breakpad::CrashGenerationServer server( - kPipeName, NULL, NULL, NULL, ClientDumpCallback, NULL, NULL, NULL, true, - &dump_path); + kPipeName, NULL, NULL, NULL, ClientDumpCallback, NULL, NULL, NULL, NULL, + NULL, true, &dump_path); ASSERT_TRUE(dump_file.empty() && full_dump_file.empty()); @@ -291,8 +291,8 @@ TEST_F(ExceptionHandlerTest, PureVirtualCallMiniDumpTest) { ASSERT_TRUE(DoesPathExist(temp_path_)); wstring dump_path(temp_path_); google_breakpad::CrashGenerationServer server( - kPipeName, NULL, NULL, NULL, ClientDumpCallback, NULL, NULL, NULL, true, - &dump_path); + kPipeName, NULL, NULL, NULL, ClientDumpCallback, NULL, NULL, NULL, NULL, + NULL, true, &dump_path); ASSERT_TRUE(dump_file.empty() && full_dump_file.empty());