From bcf885c8079b1742bfd93de722afceb26237ce4f Mon Sep 17 00:00:00 2001 From: "hansl@google.com" Date: Thu, 13 May 2010 21:00:43 +0000 Subject: [PATCH] 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 --- .../windows/handler/exception_handler.cc | 27 +++- .../unittests/exception_handler_death_test.cc | 40 +++++- .../unittests/exception_handler_test.cc | 115 +++++++++++++++++- 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(&assertion.expression); + exception_record.ExceptionInformation[1] = + reinterpret_cast(&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(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(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()));