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

@@ -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

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/<pid>/maps.
   MappingList mapping_list_;
-
-  // Callers can request additional memory regions to be included in
-  // the dump.
-  AppMemoryList app_memory_list_;
 };
 
 }  // namespace google_breakpad

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<uintptr_t>(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;
-}

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<MDMemoryDescriptor> 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<uintptr_t>(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();

src/client/linux/minidump_writer/minidump_writer.h

@@ -51,16 +51,6 @@ struct MappingEntry {
 // A list of <MappingInfo, GUID>
 typedef std::list<MappingEntry> 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<AppMemory> 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<AppMemory> 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

src/client/linux/minidump_writer/minidump_writer_unittest.cc

@@ -32,7 +32,6 @@
 #include <sys/stat.h>
 #include <sys/syscall.h>
 #include <sys/types.h>
-#include <ucontext.h>
 #include <unistd.h>
 
 #include <string>
@@ -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<uintptr_t>(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]);
-}

src/client/windows/handler/exception_handler.cc

@@ -46,7 +46,9 @@ static const int kExceptionHandlerThreadInitialStackSize = 64 * 1024;
 
 // This is passed as the context to the MinidumpWriteDump callback.
 typedef struct {
-  AppMemoryList::const_iterator iter, end;
+  ULONG64 memory_base;
+  ULONG memory_size;
+  bool finished;
 } MinidumpCallbackContext;
 
 vector<ExceptionHandler*>* ExceptionHandler::handler_stack_ = NULL;
@@ -218,9 +220,6 @@ void ExceptionHandler::Initialize(const wstring& dump_path,
     set_dump_path(dump_path);
   }
 
-  // Reserve one element for the instruction memory
-  app_memory_info_.push_back(AppMemory(0, 0));
-
   // There is a race condition here. If the first instance has not yet
   // initialized the critical section, the second (and later) instances may
   // try to use uninitialized critical section object. The feature of multiple
@@ -796,6 +795,9 @@ bool ExceptionHandler::WriteMinidumpWithException(
           ++user_streams.UserStreamCount;
         }
 
+        MINIDUMP_CALLBACK_INFORMATION callback;
+        MinidumpCallbackContext context;
+        MINIDUMP_CALLBACK_INFORMATION* callback_pointer = NULL;
         // Older versions of DbgHelp.dll don't correctly put the memory around
         // the faulting instruction pointer into the minidump. This
         // callback will ensure that it gets included.
@@ -820,33 +822,23 @@ bool ExceptionHandler::WriteMinidumpWithException(
             // pointer, but settle for whatever's available up to the
             // boundaries of the memory region.
             const ULONG64 kIPMemorySize = 256;
-            ULONG64 base =
+            context.memory_base =
               (std::max)(reinterpret_cast<ULONG64>(info.BaseAddress),
                        instruction_pointer - (kIPMemorySize / 2));
             ULONG64 end_of_range =
               (std::min)(instruction_pointer + (kIPMemorySize / 2),
                        reinterpret_cast<ULONG64>(info.BaseAddress)
                        + info.RegionSize);
-            ULONG size = static_cast<ULONG>(end_of_range - base);
+            context.memory_size =
+                static_cast<ULONG>(end_of_range - context.memory_base);
 
-           AppMemory &elt = app_memory_info_.front();
-           elt.ptr = base;
-           elt.length = size;
+            context.finished = false;
+            callback.CallbackRoutine = MinidumpWriteDumpCallback;
+            callback.CallbackParam = reinterpret_cast<void*>(&context);
+            callback_pointer = &callback;
           }
         }
 
-        MinidumpCallbackContext context;
-        context.iter = app_memory_info_.begin();
-        context.end = app_memory_info_.end();
-
-       // Skip the reserved element if there was no instruction memory
-       if (context.iter->ptr == 0)
- 	context.iter++;
-        
-        MINIDUMP_CALLBACK_INFORMATION callback;
-        callback.CallbackRoutine = MinidumpWriteDumpCallback;
-        callback.CallbackParam = reinterpret_cast<void*>(&context);
-
         // The explicit comparison to TRUE avoids a warning (C4800).
         success = (minidump_write_dump_(GetCurrentProcess(),
                                         GetCurrentProcessId(),
@@ -854,7 +846,7 @@ bool ExceptionHandler::WriteMinidumpWithException(
                                         dump_type_,
                                         exinfo ? &except_info : NULL,
                                         &user_streams,
-                                        &callback) == TRUE);
+                                        callback_pointer) == TRUE);
 
         CloseHandle(dump_file);
       }
@@ -882,13 +874,13 @@ BOOL CALLBACK ExceptionHandler::MinidumpWriteDumpCallback(
   case MemoryCallback: {
     MinidumpCallbackContext* callback_context =
         reinterpret_cast<MinidumpCallbackContext*>(context);
-    if (callback_context->iter == callback_context->end)
+    if (callback_context->finished)
       return FALSE;
 
     // Include the specified memory region.
-    callback_output->MemoryBase = callback_context->iter->ptr;
-    callback_output->MemorySize = callback_context->iter->length;
-    callback_context->iter++;
+    callback_output->MemoryBase = callback_context->memory_base;
+    callback_output->MemorySize = callback_context->memory_size;
+    callback_context->finished = true;
     return TRUE;
   }
     
@@ -932,20 +924,4 @@ void ExceptionHandler::UpdateNextID() {
   next_minidump_path_c_ = next_minidump_path_.c_str();
 }
 
-void ExceptionHandler::RegisterAppMemory(void *ptr, size_t length) {
-  app_memory_info_.push_back(AppMemory(reinterpret_cast<ULONG64>(ptr),
-                                       static_cast<ULONG>(length)));
-}
-
-void ExceptionHandler::UnregisterAppMemory(void *ptr) {
-  for (AppMemoryList::iterator iter = app_memory_info_.begin();
-       iter != app_memory_info_.end();
-       ++iter) {
-    if (iter->ptr == reinterpret_cast<ULONG64>(ptr)) {
-      app_memory_info_.erase(iter);
-      return;
-    }
-  }
-}
-
 }  // namespace google_breakpad

src/client/windows/handler/exception_handler.h

@@ -67,7 +67,6 @@
 
 #include <string>
 #include <vector>
-#include <list>
 
 #include "client/windows/common/ipc_protocol.h"
 #include "client/windows/crash_generation/crash_generation_client.h"
@@ -79,16 +78,6 @@ namespace google_breakpad {
 using std::vector;
 using std::wstring;
 
-// These entries store a list of memory regions that the client wants included
-// in the minidump.
-struct AppMemory {
-  AppMemory(ULONG64 ptr, ULONG length) : ptr(ptr), length(length) {}
-
-  ULONG64 ptr;
-  ULONG length;
-};
-typedef std::list<AppMemory> AppMemoryList;
-
 class ExceptionHandler {
  public:
   // A callback function to run before Breakpad performs any substantial
@@ -230,11 +219,6 @@ class ExceptionHandler {
   // Returns whether out-of-process dump generation is used or not.
   bool IsOutOfProcess() const { return crash_generation_client_.get() != NULL; }
 
-  // Calling RegisterAppMemory(p, len) causes len bytes starting
-  // at address p to be copied to the minidump when a crash happens.
-  void RegisterAppMemory(void *ptr, size_t length);
-  void UnregisterAppMemory(void *ptr);
-
  private:
   friend class AutoExceptionHandler;
 
@@ -420,10 +404,6 @@ class ExceptionHandler {
   // to not interfere with debuggers.
   bool handle_debug_exceptions_;
 
-  // Callers can request additional memory regions to be included in
-  // the dump.
-  AppMemoryList app_memory_info_;
-
   // A stack of ExceptionHandler objects that have installed unhandled
   // exception filters.  This vector is used by HandleException to determine
   // which ExceptionHandler object to route an exception to.  When an

src/client/windows/unittests/exception_handler_test.cc

@@ -374,50 +374,4 @@ TEST_F(ExceptionHandlerTest, WriteMinidumpTest) {
   //TODO(ted): more comprehensive tests...
 }
 
-TEST_F(ExceptionHandlerTest, AdditionalMemory) {
-  SYSTEM_INFO si;
-  GetSystemInfo(&si);
-  const u_int32_t kMemorySize = si.dwPageSize;
-  // Get some heap memory.
-  u_int8_t* memory = new u_int8_t[kMemorySize];
-  const uintptr_t kMemoryAddress = reinterpret_cast<uintptr_t>(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;
-  }
-
-  ExceptionHandler handler(temp_path_,
-                           NULL,
-                           DumpCallback,
-                           NULL,
-                           ExceptionHandler::HANDLER_ALL);
-  // Add the memory region to the list of memory to be included.
-  handler.RegisterAppMemory(memory, kMemorySize);
-  ASSERT_TRUE(handler.WriteMinidump());
-  ASSERT_FALSE(dump_file.empty());
-
-  string minidump_filename;
-  ASSERT_TRUE(WindowsStringUtils::safe_wcstombs(dump_file,
-                                                &minidump_filename));
-
-  // 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;
-}
-
 }  // namespace

src/tools/linux/core2md/core2md.cc

@@ -34,7 +34,6 @@
 #include "client/linux/minidump_writer/minidump_writer.h"
 #include "client/linux/minidump_writer/linux_core_dumper.h"
 
-using google_breakpad::AppMemoryList;
 using google_breakpad::MappingList;
 using google_breakpad::LinuxCoreDumper;
 
@@ -47,10 +46,8 @@ bool WriteMinidumpFromCore(const char* filename,
                            const char* core_path,
                            const char* procfs_override) {
   MappingList mappings;
-  AppMemoryList memory_list;
   LinuxCoreDumper dumper(0, core_path, procfs_override);
-  return google_breakpad::WriteMinidump(filename, mappings, memory_list,
-                                        &dumper);
+  return google_breakpad::WriteMinidump(filename, mappings, &dumper);
 }
 
 int main(int argc, char *argv[]) {