From 3279794487885ea6c5ac30111a35264a51d83e4d Mon Sep 17 00:00:00 2001 From: "mark@chromium.org" Date: Tue, 24 Jul 2012 22:45:30 +0000 Subject: Speculatively back out r989 per http://codereview.chromium.org/10805065/ : Ted Mielczarek: > You could try backing out r989, although Mozilla has been running with that > patch for months without issue. Me: > src/client/windows/handler/exception_handler.cc in r989 appears to have > formatting problems, an unwanted property change, and no real Breakpad review > history, so maybe we should back it out anyway until the proper process is > followed. NACL Tests nacl_integration failures: http://build.chromium.org/p/chromium/builders/NACL%20Tests/builds/30138 chrome src/native_client/tests/inbrowser_crash_test/crash_dump_tester.py says that the observed failures are a symptom of crash_service.exe itself crashing. git-svn-id: http://google-breakpad.googlecode.com/svn/trunk@998 4c0a9323-5329-0410-9bdc-e9ce6186880e --- src/client/linux/handler/exception_handler.cc | 18 +----- src/client/linux/handler/exception_handler.h | 11 ---- .../linux/handler/exception_handler_unittest.cc | 39 ------------ .../linux/minidump_writer/minidump_writer.cc | 45 ++----------- src/client/linux/minidump_writer/minidump_writer.h | 17 +---- .../minidump_writer/minidump_writer_unittest.cc | 73 +--------------------- 6 files changed, 10 insertions(+), 193 deletions(-) (limited to 'src/client/linux') diff --git a/src/client/linux/handler/exception_handler.cc b/src/client/linux/handler/exception_handler.cc index fffc3e64..f505ff33 100644 --- a/src/client/linux/handler/exception_handler.cc +++ b/src/client/linux/handler/exception_handler.cc @@ -465,8 +465,7 @@ bool ExceptionHandler::DoDump(pid_t crashing_process, const void* context, crashing_process, context, context_size, - mapping_list_, - app_memory_list_); + mapping_list_); } // static @@ -516,19 +515,4 @@ void ExceptionHandler::AddMappingInfo(const string& name, mapping_list_.push_back(mapping); } -void ExceptionHandler::RegisterAppMemory(void *ptr, size_t length) { - app_memory_list_.push_back(AppMemory(ptr, length)); -} - -void ExceptionHandler::UnregisterAppMemory(void *ptr) { - for (AppMemoryList::iterator iter = app_memory_list_.begin(); - iter != app_memory_list_.end(); - ++iter) { - if (iter->ptr == ptr) { - app_memory_list_.erase(iter); - return; - } - } -} - } // namespace google_breakpad diff --git a/src/client/linux/handler/exception_handler.h b/src/client/linux/handler/exception_handler.h index fab74d28..4bc8ba72 100644 --- a/src/client/linux/handler/exception_handler.h +++ b/src/client/linux/handler/exception_handler.h @@ -194,13 +194,6 @@ class ExceptionHandler { size_t mapping_size, size_t file_offset); - // Register a block of memory of len bytes starting at address p - // to be copied to the minidump when a crash happens. - void RegisterAppMemory(void *ptr, size_t length); - - // Unregister a block of memory that was registered with RegisterAppMemory. - void UnregisterAppMemory(void *ptr); - private: void Init(const string &dump_path, const int server_fd); @@ -259,10 +252,6 @@ class ExceptionHandler { // Callers can add extra info about mappings for cases where the // dumper code cannot extract enough information from /proc//maps. MappingList mapping_list_; - - // Callers can request additional memory regions to be included in - // the dump. - AppMemoryList app_memory_list_; }; } // namespace google_breakpad diff --git a/src/client/linux/handler/exception_handler_unittest.cc b/src/client/linux/handler/exception_handler_unittest.cc index 3ce93adc..ab3e3072 100644 --- a/src/client/linux/handler/exception_handler_unittest.cc +++ b/src/client/linux/handler/exception_handler_unittest.cc @@ -788,42 +788,3 @@ TEST(ExceptionHandlerTest, ExternalDumper) { ASSERT_GT(st.st_size, 0u); unlink(templ.c_str()); } - -// Test that an additional memory region can be added to the minidump. -TEST(ExceptionHandlerTest, AdditionalMemory) { - const u_int32_t kMemorySize = sysconf(_SC_PAGESIZE); - // Get some heap memory. - u_int8_t* memory = new u_int8_t[kMemorySize]; - const uintptr_t kMemoryAddress = reinterpret_cast(memory); - ASSERT_TRUE(memory); - // Stick some data into the memory so the contents can be verified. - for (unsigned int i = 0; i < kMemorySize; ++i) { - memory[i] = i % 255; - } - - string minidump_filename; - AutoTempDir temp_dir; - ExceptionHandler handler(temp_dir.path(), NULL, SimpleCallback, - (void*)&minidump_filename, true); - // Add the memory region to the list of memory to be included. - handler.RegisterAppMemory(memory, kMemorySize); - handler.WriteMinidump(); - - // Read the minidump. Ensure that the memory region is present - Minidump minidump(minidump_filename); - ASSERT_TRUE(minidump.Read()); - - MinidumpMemoryList* dump_memory_list = minidump.GetMemoryList(); - ASSERT_TRUE(dump_memory_list); - const MinidumpMemoryRegion* region = - dump_memory_list->GetMemoryRegionForAddress(kMemoryAddress); - ASSERT_TRUE(region); - - EXPECT_EQ(kMemoryAddress, region->GetBase()); - EXPECT_EQ(kMemorySize, region->GetSize()); - - // Verify memory contents. - EXPECT_EQ(0, memcmp(region->GetMemory(), memory, kMemorySize)); - - delete[] memory; -} diff --git a/src/client/linux/minidump_writer/minidump_writer.cc b/src/client/linux/minidump_writer/minidump_writer.cc index 9c834791..e0aeb3bd 100644 --- a/src/client/linux/minidump_writer/minidump_writer.cc +++ b/src/client/linux/minidump_writer/minidump_writer.cc @@ -373,7 +373,6 @@ class MinidumpWriter { MinidumpWriter(const char* filename, const ExceptionHandler::CrashContext* context, const MappingList& mappings, - const AppMemoryList& appmem, LinuxDumper* dumper) : filename_(filename), ucontext_(context ? &context->context : NULL), @@ -385,8 +384,7 @@ class MinidumpWriter { #endif dumper_(dumper), memory_blocks_(dumper_->allocator()), - mapping_list_(mappings), - app_memory_list_(appmem) { + mapping_list_(mappings) { } bool Init() { @@ -460,9 +458,6 @@ class MinidumpWriter { return false; dir.CopyIndex(dir_index++, &dirent); - if (!WriteAppMemory()) - return false; - if (!WriteMemoryListStream(&dirent)) return false; dir.CopyIndex(dir_index++, &dirent); @@ -764,30 +759,6 @@ class MinidumpWriter { return true; } - // Write application-provided memory regions. - bool WriteAppMemory() { - for (AppMemoryList::const_iterator iter = app_memory_list_.begin(); - iter != app_memory_list_.end(); - ++iter) { - uint8_t* data_copy = - (uint8_t*) dumper_->allocator()->Alloc(iter->length); - dumper_->CopyFromProcess(data_copy, GetCrashThread(), iter->ptr, - iter->length); - - UntypedMDRVA memory(&minidump_writer_); - if (!memory.Allocate(iter->length)) { - return false; - } - memory.Copy(data_copy, iter->length); - MDMemoryDescriptor desc; - desc.start_of_memory_range = (uintptr_t)iter->ptr; - desc.memory = memory.location(); - memory_blocks_.push_back(desc); - } - - return true; - } - static bool ShouldIncludeMapping(const MappingInfo& mapping) { if (mapping.name[0] == 0 || // only want modules with filenames. mapping.offset || // only want to include one mapping per shared lib. @@ -1363,22 +1334,17 @@ class MinidumpWriter { wasteful_vector memory_blocks_; // Additional information about some mappings provided by the caller. const MappingList& mapping_list_; - // Additional memory regions to be included in the dump, - // provided by the caller. - const AppMemoryList& app_memory_list_; }; bool WriteMinidump(const char* filename, pid_t crashing_process, const void* blob, size_t blob_size) { MappingList m; - AppMemoryList a; - return WriteMinidump(filename, crashing_process, blob, blob_size, m, a); + return WriteMinidump(filename, crashing_process, blob, blob_size, m); } bool WriteMinidump(const char* filename, pid_t crashing_process, const void* blob, size_t blob_size, - const MappingList& mappings, - const AppMemoryList& appmem) { + const MappingList& mappings) { if (blob_size != sizeof(ExceptionHandler::CrashContext)) return false; const ExceptionHandler::CrashContext* context = @@ -1388,7 +1354,7 @@ bool WriteMinidump(const char* filename, pid_t crashing_process, reinterpret_cast(context->siginfo.si_addr)); dumper.set_crash_signal(context->siginfo.si_signo); dumper.set_crash_thread(context->tid); - MinidumpWriter writer(filename, context, mappings, appmem, &dumper); + MinidumpWriter writer(filename, context, mappings, &dumper); if (!writer.Init()) return false; return writer.Dump(); @@ -1396,9 +1362,8 @@ bool WriteMinidump(const char* filename, pid_t crashing_process, bool WriteMinidump(const char* filename, const MappingList& mappings, - const AppMemoryList& appmem, LinuxDumper* dumper) { - MinidumpWriter writer(filename, NULL, mappings, appmem, dumper); + MinidumpWriter writer(filename, NULL, mappings, dumper); if (!writer.Init()) return false; return writer.Dump(); diff --git a/src/client/linux/minidump_writer/minidump_writer.h b/src/client/linux/minidump_writer/minidump_writer.h index 4ff3b469..e79eb79b 100644 --- a/src/client/linux/minidump_writer/minidump_writer.h +++ b/src/client/linux/minidump_writer/minidump_writer.h @@ -51,16 +51,6 @@ struct MappingEntry { // A list of typedef std::list MappingList; -// These entries store a list of memory regions that the client wants included -// in the minidump. -struct AppMemory { - AppMemory(void *ptr, size_t length) : ptr(ptr), length(length) {} - - void *ptr; - size_t length; -}; -typedef std::list AppMemoryList; - // Write a minidump to the filesystem. This function does not malloc nor use // libc functions which may. Thus, it can be used in contexts where the state // of the heap may be corrupt. @@ -74,16 +64,13 @@ typedef std::list AppMemoryList; bool WriteMinidump(const char* filename, pid_t crashing_process, const void* blob, size_t blob_size); -// This overload also allows passing a list of known mappings and -// a list of additional memory regions to be included in the minidump. +// This overload also allows passing a list of known mappings. bool WriteMinidump(const char* filename, pid_t crashing_process, const void* blob, size_t blob_size, - const MappingList& mappings, - const AppMemoryList& appdata); + const MappingList& mappings); bool WriteMinidump(const char* filename, const MappingList& mappings, - const AppMemoryList& appdata, LinuxDumper* dumper); } // namespace google_breakpad diff --git a/src/client/linux/minidump_writer/minidump_writer_unittest.cc b/src/client/linux/minidump_writer/minidump_writer_unittest.cc index 4f69013a..31e1440d 100644 --- a/src/client/linux/minidump_writer/minidump_writer_unittest.cc +++ b/src/client/linux/minidump_writer/minidump_writer_unittest.cc @@ -32,7 +32,6 @@ #include #include #include -#include #include #include @@ -154,13 +153,12 @@ TEST(MinidumpWriterTest, MappingInfo) { strcpy(info.name, kMemoryName); MappingList mappings; - AppMemoryList memory_list; MappingEntry mapping; mapping.first = info; memcpy(mapping.second, kModuleGUID, sizeof(MDGUID)); mappings.push_back(mapping); ASSERT_TRUE(WriteMinidump(templ.c_str(), child, &context, sizeof(context), - mappings, memory_list)); + mappings)); // Read the minidump. Load the module list, and ensure that // the mmap'ed |memory| is listed with the given module name @@ -259,14 +257,13 @@ TEST(MinidumpWriterTest, MappingInfoContained) { strcpy(info.name, kMemoryName); MappingList mappings; - AppMemoryList memory_list; MappingEntry mapping; mapping.first = info; memcpy(mapping.second, kModuleGUID, sizeof(MDGUID)); mappings.push_back(mapping); ASSERT_TRUE( WriteMinidump(dumpfile.c_str(), child, &context, sizeof(context), - mappings, memory_list)); + mappings)); // Read the minidump. Load the module list, and ensure that // the mmap'ed |memory| is listed with the given module name @@ -387,69 +384,3 @@ TEST(MinidumpWriterTest, DeletedBinary) { module_identifier += "0"; EXPECT_EQ(module_identifier, module->debug_identifier()); } - -// Test that an additional memory region can be added to the minidump. -TEST(MinidumpWriterTest, AdditionalMemory) { - int fds[2]; - ASSERT_NE(-1, pipe(fds)); - - // These are defined here so the parent can use them to check the - // data from the minidump afterwards. - const u_int32_t kMemorySize = sysconf(_SC_PAGESIZE); - // Get some heap memory. - u_int8_t* memory = new u_int8_t[kMemorySize]; - const uintptr_t kMemoryAddress = reinterpret_cast(memory); - ASSERT_TRUE(memory); - // Stick some data into the memory so the contents can be verified. - for (int i = 0; i < kMemorySize; ++i) { - memory[i] = i % 255; - } - - const pid_t child = fork(); - if (child == 0) { - close(fds[1]); - char b; - HANDLE_EINTR(read(fds[0], &b, sizeof(b))); - close(fds[0]); - syscall(__NR_exit); - } - close(fds[0]); - - ExceptionHandler::CrashContext context; - // This needs a valid context for minidump writing to work, but getting - // a useful one from the child is too much work, so just use one from - // the parent since the child is just a forked copy anyway. - //TODO(ted): this won't work for Android if unit tests ever get run there. - ASSERT_EQ(0, getcontext(&context.context)); - context.tid = child; - - AutoTempDir temp_dir; - string templ = "/tmp/minidump-memory.dmp"; //temp_dir.path() + "/minidump-writer-unittest"; - unlink(templ.c_str()); - - MappingList mappings; - AppMemoryList memory_list; - // Add the memory region to the list of memory to be included. - memory_list.push_back(AppMemory(memory, kMemorySize)); - ASSERT_TRUE(WriteMinidump(templ.c_str(), child, &context, sizeof(context), - mappings, memory_list)); - - // Read the minidump. Ensure that the memory region is present - Minidump minidump(templ.c_str()); - ASSERT_TRUE(minidump.Read()); - - MinidumpMemoryList* dump_memory_list = minidump.GetMemoryList(); - ASSERT_TRUE(dump_memory_list); - const MinidumpMemoryRegion* region = - dump_memory_list->GetMemoryRegionForAddress(kMemoryAddress); - ASSERT_TRUE(region); - - EXPECT_EQ(kMemoryAddress, region->GetBase()); - EXPECT_EQ(kMemorySize, region->GetSize()); - - // Verify memory contents. - EXPECT_EQ(0, memcmp(region->GetMemory(), memory, kMemorySize)); - - delete[] memory; - close(fds[1]); -} -- cgit v1.2.1