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

2 files changed, 18 insertions, 62 deletions

diff --git a/src/client/windows/handler/exception_handler.cc b/src/client/windows/handler/exception_handler.cc
index 9ede97f0..6e5b724a 100755..100644
--- a/src/client/windows/handler/exception_handler.cc
+++ b/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
diff --git a/src/client/windows/handler/exception_handler.h b/src/client/windows/handler/exception_handler.h
index 38c2c3ca..09f5177c 100644
--- a/src/client/windows/handler/exception_handler.h
+++ b/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