Added a death test for the pure virtual function call.

Added a test for the minidump generated by a pure virtual function call.
Changed the pure virtual function call handler so that it creates a minidump with Exception info and a stack.

Review URL: http://codereview.chromium.org/2050013

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

3 files changed, 174 insertions, 8 deletions

diff --git a/src/client/windows/handler/exception_handler.cc b/src/client/windows/handler/exception_handler.cc
index a877b2f1..d27ddcc1 100644
--- a/src/client/windows/handler/exception_handler.cc
+++ b/src/client/windows/handler/exception_handler.cc
@@ -550,6 +550,8 @@ void ExceptionHandler::HandleInvalidParameter(const wchar_t* expression,
 
 // static
 void ExceptionHandler::HandlePureVirtualCall() {
+  // This is an pure virtual funciton call, not an exception.  It's safe to
+  // play with sprintf here.
   AutoExceptionHandler auto_exception_handler;
   ExceptionHandler* current_handler = auto_exception_handler.get_handler();
 
@@ -557,6 +559,26 @@ void ExceptionHandler::HandlePureVirtualCall() {
   memset(&assertion, 0, sizeof(assertion));
   assertion.type = MD_ASSERTION_INFO_TYPE_PURE_VIRTUAL_CALL;
 
+  // Make up an exception record for the current thread and CPU context
+  // to make it possible for the crash processor to classify these
+  // as do regular crashes, and to make it humane for developers to
+  // analyze them.
+  EXCEPTION_RECORD exception_record = {};
+  CONTEXT exception_context = {};
+  EXCEPTION_POINTERS exception_ptrs = { &exception_record, &exception_context };
+  RtlCaptureContext(&exception_context);
+  exception_record.ExceptionCode = STATUS_NONCONTINUABLE_EXCEPTION;
+
+  // We store pointers to the the expression and function strings,
+  // and the line as exception parameters to make them easy to
+  // access by the developer on the far side.
+  exception_record.NumberParameters = 3;
+  exception_record.ExceptionInformation[0] =
+      reinterpret_cast<ULONG_PTR>(&assertion.expression);
+  exception_record.ExceptionInformation[1] =
+      reinterpret_cast<ULONG_PTR>(&assertion.file);
+  exception_record.ExceptionInformation[2] = assertion.line;
+
   bool success = false;
   // In case of out-of-process dump generation, directly call
   // WriteMinidumpWithException since there is no separate thread running.
@@ -564,10 +586,11 @@ void ExceptionHandler::HandlePureVirtualCall() {
   if (current_handler->IsOutOfProcess()) {
     success = current_handler->WriteMinidumpWithException(
         GetCurrentThreadId(),
-        NULL,
+        &exception_ptrs,
         &assertion);
   } else {
-    success = current_handler->WriteMinidumpOnHandlerThread(NULL, &assertion);
+    success = current_handler->WriteMinidumpOnHandlerThread(&exception_ptrs,
+                                                            &assertion);
   }
 
   if (!success) {
diff --git a/src/client/windows/unittests/exception_handler_death_test.cc b/src/client/windows/unittests/exception_handler_death_test.cc
index a03ad011..36131403 100755
--- a/src/client/windows/unittests/exception_handler_death_test.cc
+++ b/src/client/windows/unittests/exception_handler_death_test.cc
@@ -53,7 +53,8 @@ class ExceptionHandlerDeathTest : public ::testing::Test {
   // Actually constructs a temp path name.
   virtual void SetUp();
   // A helper method that tests can use to crash.
-  void DoCrash();
+  void DoCrashAccessViolation();
+  void DoCrashPureVirtualCall();
 };
 
 void ExceptionHandlerDeathTest::SetUp() {
@@ -126,7 +127,7 @@ void clientDumpCallback(void *dump_context,
   gDumpCallbackCalled = true;
 }
 
-void ExceptionHandlerDeathTest::DoCrash() {
+void ExceptionHandlerDeathTest::DoCrashAccessViolation() {
   google_breakpad::ExceptionHandler *exc =
     new google_breakpad::ExceptionHandler(
     temp_path_, NULL, NULL, NULL,
@@ -160,7 +161,7 @@ TEST_F(ExceptionHandlerDeathTest, OutOfProcTest) {
   // being the same.
   EXPECT_TRUE(server.Start());
   EXPECT_FALSE(gDumpCallbackCalled);
-  ASSERT_DEATH(this->DoCrash(), "");
+  ASSERT_DEATH(this->DoCrashAccessViolation(), "");
   EXPECT_TRUE(gDumpCallbackCalled);
 }
 
@@ -178,4 +179,37 @@ TEST_F(ExceptionHandlerDeathTest, InvalidParameterTest) {
   // and a dump will be generated, the process will exit(0).
   ASSERT_EXIT(printf(NULL), ::testing::ExitedWithCode(0), "");
 }
+
+
+struct PureVirtualCallBase {
+  PureVirtualCallBase() {
+    // We have to reinterpret so the linker doesn't get confused because the
+    // method isn't defined.
+    reinterpret_cast<PureVirtualCallBase*>(this)->PureFunction();
+  }
+  virtual ~PureVirtualCallBase() {}
+  virtual void PureFunction() const = 0;
+};
+struct PureVirtualCall : public PureVirtualCallBase {
+  PureVirtualCall() { PureFunction(); }
+  virtual void PureFunction() const {}
+};
+
+void ExceptionHandlerDeathTest::DoCrashPureVirtualCall() {
+  PureVirtualCall instance;
+}
+
+TEST_F(ExceptionHandlerDeathTest, PureVirtualCallTest) {
+  using google_breakpad::ExceptionHandler;
+
+  ASSERT_TRUE(DoesPathExist(temp_path_));
+  ExceptionHandler handler(temp_path_, NULL, NULL, NULL,
+                           ExceptionHandler::HANDLER_PURECALL);
+
+  // Disable the message box for assertions
+  _CrtSetReportMode(_CRT_ASSERT, 0);
+
+  // Calls a pure virtual function.
+  EXPECT_EXIT(DoCrashPureVirtualCall(), ::testing::ExitedWithCode(0), "");
+}
 }
diff --git a/src/client/windows/unittests/exception_handler_test.cc b/src/client/windows/unittests/exception_handler_test.cc
index b8b58762..ed1cec64 100755
--- a/src/client/windows/unittests/exception_handler_test.cc
+++ b/src/client/windows/unittests/exception_handler_test.cc
@@ -60,7 +60,8 @@ class ExceptionHandlerTest : public ::testing::Test {
   // Deletes temporary files.
   virtual void TearDown();
 
-  void DoCrash();
+  void DoCrashInvalidParameter();
+  void DoCrashPureVirtualCall();
 
   // Utility function to test for a path's existence.
   static BOOL DoesPathExist(const TCHAR *path_name);
@@ -127,7 +128,7 @@ void ExceptionHandlerTest::ClientDumpCallback(
   full_dump_file = dump_file.substr(0, dump_file.length() - 4) + L"-full.dmp";
 }
 
-void ExceptionHandlerTest::DoCrash() {
+void ExceptionHandlerTest::DoCrashInvalidParameter() {
   google_breakpad::ExceptionHandler *exc =
       new google_breakpad::ExceptionHandler(
           temp_path_, NULL, NULL, NULL,
@@ -144,6 +145,43 @@ void ExceptionHandlerTest::DoCrash() {
   printf(NULL);
 }
 
+
+struct PureVirtualCallBase {
+  PureVirtualCallBase() {
+    // We have to reinterpret so the linker doesn't get confused because the
+    // method isn't defined.
+    reinterpret_cast<PureVirtualCallBase*>(this)->PureFunction();
+  }
+  virtual ~PureVirtualCallBase() {}
+  virtual void PureFunction() const = 0;
+};
+struct PureVirtualCall : public PureVirtualCallBase {
+  PureVirtualCall() { PureFunction(); }
+  virtual void PureFunction() const {}
+};
+
+void ExceptionHandlerTest::DoCrashPureVirtualCall() {
+  google_breakpad::ExceptionHandler *exc =
+      new google_breakpad::ExceptionHandler(
+          temp_path_, NULL, NULL, NULL,
+          google_breakpad::ExceptionHandler::HANDLER_PURECALL,
+          kFullDumpType, kPipeName, NULL);
+
+  // Disable the message box for assertions
+  _CrtSetReportMode(_CRT_ASSERT, 0);
+
+  // Although this is executing in the child process of the death test,
+  // if it's not true we'll still get an error rather than the crash
+  // being expected.
+  ASSERT_TRUE(exc->IsOutOfProcess());
+
+  // Create a new frame to ensure PureVirtualCall is not optimized to some
+  // other line in this function.
+  {
+    PureVirtualCall instance;
+  }
+}
+
 // This test validates that the minidump is written correctly.
 TEST_F(ExceptionHandlerTest, InvalidParameterMiniDumpTest) {
   ASSERT_TRUE(DoesPathExist(temp_path_));
@@ -161,7 +199,78 @@ TEST_F(ExceptionHandlerTest, InvalidParameterMiniDumpTest) {
   // child process, the server registration will fail due to the named pipe
   // being the same.
   EXPECT_TRUE(server.Start());
-  EXPECT_EXIT(this->DoCrash(), ::testing::ExitedWithCode(0), "");
+  EXPECT_EXIT(DoCrashInvalidParameter(), ::testing::ExitedWithCode(0), "");
+  ASSERT_TRUE(!dump_file.empty() && !full_dump_file.empty());
+  ASSERT_TRUE(DoesPathExist(dump_file.c_str()));
+
+  // Verify the dump for infos.
+  DumpAnalysis mini(dump_file);
+  DumpAnalysis full(full_dump_file);
+
+  // The dump should have all of these streams.
+  EXPECT_TRUE(mini.HasStream(ThreadListStream));
+  EXPECT_TRUE(full.HasStream(ThreadListStream));
+  EXPECT_TRUE(mini.HasStream(ModuleListStream));
+  EXPECT_TRUE(full.HasStream(ModuleListStream));
+  EXPECT_TRUE(mini.HasStream(ExceptionStream));
+  EXPECT_TRUE(full.HasStream(ExceptionStream));
+  EXPECT_TRUE(mini.HasStream(SystemInfoStream));
+  EXPECT_TRUE(full.HasStream(SystemInfoStream));
+  EXPECT_TRUE(mini.HasStream(MiscInfoStream));
+  EXPECT_TRUE(full.HasStream(MiscInfoStream));
+  EXPECT_TRUE(mini.HasStream(HandleDataStream));
+  EXPECT_TRUE(full.HasStream(HandleDataStream));
+
+  // We expect PEB and TEBs in this dump.
+  EXPECT_TRUE(mini.HasTebs() || full.HasTebs());
+  EXPECT_TRUE(mini.HasPeb() || full.HasPeb());
+
+  // Minidump should have a memory listing, but no 64-bit memory.
+  EXPECT_TRUE(mini.HasStream(MemoryListStream));
+  EXPECT_FALSE(mini.HasStream(Memory64ListStream));
+
+  EXPECT_FALSE(full.HasStream(MemoryListStream));
+  EXPECT_TRUE(full.HasStream(Memory64ListStream));
+
+  // This is the only place we don't use OR because we want both not
+  // to have the streams.
+  EXPECT_FALSE(mini.HasStream(ThreadExListStream));
+  EXPECT_FALSE(full.HasStream(ThreadExListStream));
+  EXPECT_FALSE(mini.HasStream(CommentStreamA));
+  EXPECT_FALSE(full.HasStream(CommentStreamA));
+  EXPECT_FALSE(mini.HasStream(CommentStreamW));
+  EXPECT_FALSE(full.HasStream(CommentStreamW));
+  EXPECT_FALSE(mini.HasStream(FunctionTableStream));
+  EXPECT_FALSE(full.HasStream(FunctionTableStream));
+  EXPECT_FALSE(mini.HasStream(MemoryInfoListStream));
+  EXPECT_FALSE(full.HasStream(MemoryInfoListStream));
+  EXPECT_FALSE(mini.HasStream(ThreadInfoListStream));
+  EXPECT_FALSE(full.HasStream(ThreadInfoListStream));
+  EXPECT_FALSE(mini.HasStream(HandleOperationListStream));
+  EXPECT_FALSE(full.HasStream(HandleOperationListStream));
+  EXPECT_FALSE(mini.HasStream(TokenStream));
+  EXPECT_FALSE(full.HasStream(TokenStream));
+}
+
+
+// This test validates that the minidump is written correctly.
+TEST_F(ExceptionHandlerTest, PureVirtualCallMiniDumpTest) {
+  ASSERT_TRUE(DoesPathExist(temp_path_));
+
+  // Call with a bad argument
+  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);
+
+  ASSERT_TRUE(dump_file.empty() && full_dump_file.empty());
+
+  // 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_EXIT(DoCrashPureVirtualCall(), ::testing::ExitedWithCode(0), "");
   ASSERT_TRUE(!dump_file.empty() && !full_dump_file.empty());
   ASSERT_TRUE(DoesPathExist(dump_file.c_str()));