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

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<ClientInfo> 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<MockCrashGenerationServerCallbacks*>(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());