Allow option for efficient and safe opt out of in-proc dump generation for Windows breakpad clients.

https://breakpad.appspot.com/549002/



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

4 files changed, 143 insertions, 41 deletions

diff --git a/src/client/windows/crash_generation/crash_generation_client.cc b/src/client/windows/crash_generation/crash_generation_client.cc
index b0d3d04d..1e86dcec 100644
--- a/src/client/windows/crash_generation/crash_generation_client.cc
+++ b/src/client/windows/crash_generation/crash_generation_client.cc
@@ -178,6 +178,10 @@ CrashGenerationClient::~CrashGenerationClient() {
 //
 // Returns true if the registration is successful; false otherwise.
 bool CrashGenerationClient::Register() {
+  if (IsRegistered()) {
+    return true;
+  }
+
   HANDLE pipe = ConnectToServer();
   if (!pipe) {
     return false;
diff --git a/src/client/windows/handler/exception_handler.cc b/src/client/windows/handler/exception_handler.cc
index 59a63144..14c2cf86 100644
--- a/src/client/windows/handler/exception_handler.cc
+++ b/src/client/windows/handler/exception_handler.cc
@@ -73,7 +73,8 @@ ExceptionHandler::ExceptionHandler(const wstring& dump_path,
              handler_types,
              dump_type,
              pipe_name,
-             NULL,
+             NULL,  // pipe_handle
+             NULL,  // crash_generation_client
              custom_info);
 }
 
@@ -91,11 +92,33 @@ ExceptionHandler::ExceptionHandler(const wstring& dump_path,
              callback_context,
              handler_types,
              dump_type,
-             NULL,
+             NULL,  // pipe_name
              pipe_handle,
+             NULL,  // crash_generation_client
              custom_info);
 }  
 
+ExceptionHandler::ExceptionHandler(
+    const wstring& dump_path,
+    FilterCallback filter,
+    MinidumpCallback callback,
+    void* callback_context,
+    int handler_types,
+    MINIDUMP_TYPE dump_type,
+    CrashGenerationClient* crash_generation_client,
+    const CustomClientInfo* custom_info) {
+  Initialize(dump_path,
+             filter,
+             callback,
+             callback_context,
+             handler_types,
+             MiniDumpNormal,
+             NULL,  // pipe_name
+             NULL,  // pipe_handle
+             crash_generation_client,
+             custom_info);
+}
+
 ExceptionHandler::ExceptionHandler(const wstring &dump_path,
                                    FilterCallback filter,
                                    MinidumpCallback callback,
@@ -107,20 +130,23 @@ ExceptionHandler::ExceptionHandler(const wstring &dump_path,
              callback_context,
              handler_types,
              MiniDumpNormal,
-             NULL,
-             NULL,
-             NULL);
+             NULL,   // pipe_name
+             NULL,   // pipe_handle
+             NULL,   // crash_generation_client
+             NULL);  // custom_info
 }
 
-void ExceptionHandler::Initialize(const wstring& dump_path,
-                                  FilterCallback filter,
-                                  MinidumpCallback callback,
-                                  void* callback_context,
-                                  int handler_types,
-                                  MINIDUMP_TYPE dump_type,
-                                  const wchar_t* pipe_name,
-                                  HANDLE pipe_handle,
-                                  const CustomClientInfo* custom_info) {
+void ExceptionHandler::Initialize(
+    const wstring& dump_path,
+    FilterCallback filter,
+    MinidumpCallback callback,
+    void* callback_context,
+    int handler_types,
+    MINIDUMP_TYPE dump_type,
+    const wchar_t* pipe_name,
+    HANDLE pipe_handle,
+    CrashGenerationClient* crash_generation_client,
+    const CustomClientInfo* custom_info) {
   LONG instance_count = InterlockedIncrement(&instance_count_);
   filter_ = filter;
   callback_ = callback;
@@ -149,23 +175,20 @@ void ExceptionHandler::Initialize(const wstring& dump_path,
   handler_return_value_ = false;
   handle_debug_exceptions_ = false;
 
-  // Attempt to use out-of-process if user has specified a pipe.
-  if (pipe_name != NULL || pipe_handle != NULL) {
-    assert(!(pipe_name && pipe_handle));
-
-    scoped_ptr<CrashGenerationClient> client;
-    if (pipe_name) {
-      client.reset(
-        new CrashGenerationClient(pipe_name,
-                                  dump_type_,
-                                  custom_info));
-    } else {
-      client.reset(
-        new CrashGenerationClient(pipe_handle,
-                                  dump_type_,
-                                  custom_info));
-    }
+  // Attempt to use out-of-process if user has specified a pipe or a
+  // crash generation client.
+  scoped_ptr<CrashGenerationClient> client;
+  if (crash_generation_client) {
+    client.reset(crash_generation_client);
+  } else if (pipe_name) {
+    client.reset(
+      new CrashGenerationClient(pipe_name, dump_type_, custom_info));
+  } else if (pipe_handle) {
+    client.reset(
+      new CrashGenerationClient(pipe_handle, dump_type_, custom_info));
+  }
 
+  if (client.get() != NULL) {
     // If successful in registering with the monitoring process,
     // there is no need to setup in-process crash generation.
     if (client->Register()) {
diff --git a/src/client/windows/handler/exception_handler.h b/src/client/windows/handler/exception_handler.h
index ec9aabcd..2c8ff7d9 100644
--- a/src/client/windows/handler/exception_handler.h
+++ b/src/client/windows/handler/exception_handler.h
@@ -195,6 +195,27 @@ class ExceptionHandler {
                    HANDLE pipe_handle,
                    const CustomClientInfo* custom_info);
 
+  // ExceptionHandler that ENSURES out-of-process dump generation.  Expects a
+  // crash generation client that is already registered with a crash generation
+  // server.  Takes ownership of the passed-in crash_generation_client.
+  //
+  // Usage example:
+  //   crash_generation_client = new CrashGenerationClient(..);
+  //   if (crash_generation_client->Register()) {
+  //     // Registration with the crash generation server succeeded.
+  //     // Out-of-process dump generation is guaranteed.
+  //     g_handler = new ExceptionHandler(.., crash_generation_client, ..);
+  //     return true;
+  //   }
+  ExceptionHandler(const wstring& dump_path,
+                   FilterCallback filter,
+                   MinidumpCallback callback,
+                   void* callback_context,
+                   int handler_types,
+                   MINIDUMP_TYPE dump_type,
+                   CrashGenerationClient* crash_generation_client,
+                   const CustomClientInfo* custom_info);
+
   ~ExceptionHandler();
 
   // Get and set the minidump path.
@@ -264,6 +285,7 @@ class ExceptionHandler {
                   MINIDUMP_TYPE dump_type,
                   const wchar_t* pipe_name,
                   HANDLE pipe_handle,
+                  CrashGenerationClient* crash_generation_client,
                   const CustomClientInfo* custom_info);
 
   // Function pointer type for MiniDumpWriteDump, which is looked up
diff --git a/src/client/windows/unittests/exception_handler_death_test.cc b/src/client/windows/unittests/exception_handler_death_test.cc
index c4cb9930..ba7be920 100644
--- a/src/client/windows/unittests/exception_handler_death_test.cc
+++ b/src/client/windows/unittests/exception_handler_death_test.cc
@@ -54,6 +54,11 @@ const char kFailureIndicator[] = "failure";
 // Utility function to test for a path's existence.
 BOOL DoesPathExist(const TCHAR *path_name);
 
+enum OutOfProcGuarantee {
+  OUT_OF_PROC_GUARANTEED,
+  OUT_OF_PROC_BEST_EFFORT,
+};
+
 class ExceptionHandlerDeathTest : public ::testing::Test {
  protected:
   // Member variable for each test that they can use
@@ -62,7 +67,7 @@ class ExceptionHandlerDeathTest : public ::testing::Test {
   // Actually constructs a temp path name.
   virtual void SetUp();
   // A helper method that tests can use to crash.
-  void DoCrashAccessViolation();
+  void DoCrashAccessViolation(const OutOfProcGuarantee out_of_proc_guarantee);
   void DoCrashPureVirtualCall();
 };
 
@@ -140,12 +145,37 @@ void clientDumpCallback(void *dump_context,
   gDumpCallbackCalled = true;
 }
 
-void ExceptionHandlerDeathTest::DoCrashAccessViolation() {
-  google_breakpad::ExceptionHandler *exc =
-    new google_breakpad::ExceptionHandler(
-    temp_path_, NULL, NULL, NULL,
-    google_breakpad::ExceptionHandler::HANDLER_ALL, MiniDumpNormal, kPipeName,
-    NULL);
+void ExceptionHandlerDeathTest::DoCrashAccessViolation(
+    const OutOfProcGuarantee out_of_proc_guarantee) {
+  google_breakpad::ExceptionHandler *exc = NULL;
+
+  if (out_of_proc_guarantee == OUT_OF_PROC_GUARANTEED) {
+    google_breakpad::CrashGenerationClient *client = 
+        new google_breakpad::CrashGenerationClient(kPipeName,
+                                                   MiniDumpNormal,
+                                                   NULL);  // custom_info
+    ASSERT_TRUE(client->Register());
+    exc = new google_breakpad::ExceptionHandler(
+        temp_path_,
+        NULL,   // filter
+        NULL,   // callback
+        NULL,   // callback_context
+        google_breakpad::ExceptionHandler::HANDLER_ALL,
+        MiniDumpNormal,
+        client,
+        NULL);  // custom_info
+  } else {
+    ASSERT_TRUE(out_of_proc_guarantee == OUT_OF_PROC_BEST_EFFORT);
+    exc = new google_breakpad::ExceptionHandler(
+        temp_path_,
+        NULL,   // filter
+        NULL,   // callback
+        NULL,   // callback_context
+        google_breakpad::ExceptionHandler::HANDLER_ALL,
+        MiniDumpNormal,
+        kPipeName,
+        NULL);  // custom_info
+  }
 
   // Disable GTest SEH handler
   testing::DisableExceptionHandlerInScope disable_exception_handler;
@@ -170,15 +200,38 @@ 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, 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
+  // being the same.
+  EXPECT_TRUE(server.Start());
+  gDumpCallbackCalled = false;
+  ASSERT_DEATH(this->DoCrashAccessViolation(OUT_OF_PROC_BEST_EFFORT), "");
+  EXPECT_TRUE(gDumpCallbackCalled);
+}
+
+TEST_F(ExceptionHandlerDeathTest, OutOfProcGuaranteedTest) {
+  // This is similar to the previous test (OutOfProcTest).  The only difference
+  // is that in this test, the crash generation client is created and registered
+  // with the crash generation server outside of the ExceptionHandler
+  // constructor which allows breakpad users to opt out of the default
+  // in-process dump generation when the registration with the crash generation
+  // server fails.
+
+  ASSERT_TRUE(DoesPathExist(temp_path_));
+  std::wstring dump_path(temp_path_);
+  google_breakpad::CrashGenerationServer server(
+      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
   // being the same.
   EXPECT_TRUE(server.Start());
-  EXPECT_FALSE(gDumpCallbackCalled);
-  ASSERT_DEATH(this->DoCrashAccessViolation(), "");
+  gDumpCallbackCalled = false;
+  ASSERT_DEATH(this->DoCrashAccessViolation(OUT_OF_PROC_GUARANTEED), "");
   EXPECT_TRUE(gDumpCallbackCalled);
 }