Add an ExceptionBarrier around outbound calls to patched methods in IE. In so doing, we have an SEH present in the SEH chain and so the VEH won't erroneously report crashes that occur in other modules when we happen to be on the stack.

BUG=42660
TEST=Less false positives in the crash reports.


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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@45764 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 276 insertions, 14 deletions

diff --git a/chrome_frame/chrome_frame.gyp b/chrome_frame/chrome_frame.gyp
index 245bd3f..a5c86d6 100644
--- a/chrome_frame/chrome_frame.gyp
+++ b/chrome_frame/chrome_frame.gyp
@@ -647,6 +647,9 @@
         'com_type_info_holder.h',
         'delete_chrome_history.cc',
         'delete_chrome_history.h',
+        'exception_barrier.cc',
+        'exception_barrier.h',
+        'exception_barrier_lowlevel.asm',
         'find_dialog.cc',
         'find_dialog.h',
         'function_stub.h',
@@ -705,6 +708,31 @@
           ],
         },],
       ],
+      'rules': [
+        {
+          'rule_name': 'Assemble',
+          'extension': 'asm',
+          'inputs': [],
+          'outputs': [
+            '<(INTERMEDIATE_DIR)/<(RULE_INPUT_ROOT).obj',
+          ],
+          'action': [
+            'ml',
+            '/safeseh',
+            '/Fo', '<(INTERMEDIATE_DIR)\<(RULE_INPUT_ROOT).obj',
+            '/c', '<(RULE_INPUT_PATH)',
+          ],
+          'process_outputs_as_sources': 0,
+          'message': 'Assembling <(RULE_INPUT_PATH) to <(INTERMEDIATE_DIR)\<(RULE_INPUT_ROOT).obj.',
+        },
+      ],
+      'msvs_settings': {
+        'VCLinkerTool': {
+          'AdditionalOptions': [
+            '/safeseh',
+          ],
+        },
+      },
     },
     {
       'target_name': 'npchrome_frame',
diff --git a/chrome_frame/chrome_frame_reporting.cc b/chrome_frame/chrome_frame_reporting.cc
index 202d43c..556747b2 100644
--- a/chrome_frame/chrome_frame_reporting.cc
+++ b/chrome_frame/chrome_frame_reporting.cc
@@ -11,6 +11,7 @@
 #include "chrome/installer/util/google_update_settings.h"
 #include "chrome/installer/util/install_util.h"
 #include "chrome_frame/chrome_frame_reporting.h"
+#include "chrome_frame/exception_barrier.h"
 #include "chrome_frame/utils.h"
 
 // Well known SID for the system principal.
@@ -45,6 +46,11 @@ google_breakpad::CustomClientInfo* GetCustomInfo(const wchar_t* dll_path) {
   return &custom_info;
 }
 
+
+void CALLBACK BreakpadHandler(EXCEPTION_POINTERS *ptrs) {
+  WriteMinidumpForException(ptrs);
+}
+
 extern "C" IMAGE_DOS_HEADER __ImageBase;
 
 bool InitializeCrashReporting() {
@@ -55,6 +61,10 @@ bool InitializeCrashReporting() {
   if (!always_take_dump && !GoogleUpdateSettings::GetCollectStatsConsent())
       return true;
 
+  // Set the handler for ExceptionBarrier for this module:
+  DCHECK(ExceptionBarrier::handler() == NULL);
+  ExceptionBarrier::set_handler(BreakpadHandler);
+
   // Get the alternate dump directory. We use the temp path.
   FilePath temp_directory;
   if (!file_util::GetTempDir(&temp_directory) || temp_directory.empty()) {
diff --git a/chrome_frame/crash_reporting/crash_report.cc b/chrome_frame/crash_reporting/crash_report.cc
index 89dd409..529d1cd 100644
--- a/chrome_frame/crash_reporting/crash_report.cc
+++ b/chrome_frame/crash_reporting/crash_report.cc
@@ -7,11 +7,13 @@
 #include "chrome_frame/crash_reporting/crash_report.h"
 
 #include "base/basictypes.h"
+#include "base/lock.h"
 #include "breakpad/src/client/windows/handler/exception_handler.h"
 
 // TODO(joshia): factor out common code with chrome used for crash reporting
 const wchar_t kGoogleUpdatePipeName[] = L"\\\\.\\pipe\\GoogleCrashServices\\";
 static google_breakpad::ExceptionHandler * g_breakpad = NULL;
+static Lock g_breakpad_lock;
 
 // These minidump flag combinations have been tested safe agains the
 // DbgHelp.dll version that ships with Windows XP SP2.
@@ -47,9 +49,14 @@ static void veh_segment_end() {}
 class CrashHandlerTraits : public Win32VEHTraits,
                            public ModuleOfInterestWithExcludedRegion {
  public:
-  CrashHandlerTraits() : breakpad_(NULL) {}
-  void Init(google_breakpad::ExceptionHandler* breakpad) {
-    breakpad_ = breakpad;
+  CrashHandlerTraits() {}
+
+  // Note that breakpad_lock must be held when this is called.
+  void Init(google_breakpad::ExceptionHandler* breakpad, Lock* breakpad_lock) {
+    DCHECK(breakpad);
+    DCHECK(breakpad_lock);
+    breakpad_lock->AssertAcquired();
+
     Win32VEHTraits::InitializeIgnoredBlocks();
     ModuleOfInterestWithExcludedRegion::SetCurrentModule();
     // Pointers to static (non-extern) functions take the address of the
@@ -61,21 +68,21 @@ class CrashHandlerTraits : public Win32VEHTraits,
   }
 
   void Shutdown() {
-    breakpad_ = 0;
   }
 
   inline bool WriteDump(EXCEPTION_POINTERS* p) {
-    return breakpad_->WriteMinidumpForException(p);
+    return WriteMinidumpForException(p);
   }
-
- private:
-  google_breakpad::ExceptionHandler* breakpad_;
 };
 
 class CrashHandler {
  public:
   CrashHandler() : veh_id_(NULL), handler_(&crash_api_) {}
-  bool Init(google_breakpad::ExceptionHandler* breakpad);
+
+  // Note that breakpad_lock is used to protect accesses to breakpad and must
+  // be held when Init() is called.
+  bool Init(google_breakpad::ExceptionHandler* breakpad, Lock* breakpad_lock);
+
   void Shutdown();
  private:
   VectoredHandlerT<CrashHandlerTraits> handler_;
@@ -97,14 +104,19 @@ LONG WINAPI CrashHandler::VectoredHandlerEntryPoint(
 
 #pragma code_seg(pop)
 
-bool CrashHandler::Init(google_breakpad::ExceptionHandler* breakpad) {
+bool CrashHandler::Init(google_breakpad::ExceptionHandler* breakpad,
+                        Lock* breakpad_lock) {
+  DCHECK(breakpad);
+  DCHECK(breakpad_lock);
+  breakpad_lock->AssertAcquired();
+
   if (veh_id_)
     return true;
 
   void* id = ::AddVectoredExceptionHandler(FALSE, &VectoredHandlerEntryPoint);
   if (id != NULL) {
     veh_id_ = id;
-    crash_api_.Init(breakpad);
+    crash_api_.Init(breakpad, breakpad_lock);
     return true;
   }
 
@@ -131,6 +143,7 @@ bool InitializeVectoredCrashReportingWithPipeName(
     const wchar_t* pipe_name,
     const std::wstring& dump_path,
     google_breakpad::CustomClientInfo* client_info) {
+  AutoLock lock(g_breakpad_lock);
   if (g_breakpad)
     return true;
 
@@ -149,7 +162,7 @@ bool InitializeVectoredCrashReportingWithPipeName(
   if (!g_breakpad)
     return false;
 
-  if (!g_crash_handler.Init(g_breakpad)) {
+  if (!g_crash_handler.Init(g_breakpad, &g_breakpad_lock)) {
     delete g_breakpad;
     g_breakpad = NULL;
     return false;
@@ -176,7 +189,17 @@ bool InitializeVectoredCrashReporting(
 
 bool ShutdownVectoredCrashReporting() {
   g_crash_handler.Shutdown();
+  AutoLock lock(g_breakpad_lock);
   delete g_breakpad;
   g_breakpad = NULL;
   return true;
 }
+
+bool WriteMinidumpForException(EXCEPTION_POINTERS* p) {
+  AutoLock lock(g_breakpad_lock);
+  bool success = false;
+  if (g_breakpad) {
+    success = g_breakpad->WriteMinidumpForException(p);
+  }
+  return success;
+}
diff --git a/chrome_frame/crash_reporting/crash_report.h b/chrome_frame/crash_reporting/crash_report.h
index d9fd42f..3336890 100644
--- a/chrome_frame/crash_reporting/crash_report.h
+++ b/chrome_frame/crash_reporting/crash_report.h
@@ -9,6 +9,7 @@
 
 #include <string>
 
+#include "base/lock.h"
 #include "base/logging.h"
 #include "breakpad/src/client/windows/handler/exception_handler.h"
 
@@ -29,4 +30,7 @@ bool InitializeVectoredCrashReportingWithPipeName(
 
 bool ShutdownVectoredCrashReporting();
 
+bool WriteMinidumpForException(EXCEPTION_POINTERS* p);
+
+
 #endif  // CHROME_FRAME_CRASH_REPORTING_CRASH_REPORT_H_
diff --git a/chrome_frame/exception_barrier.cc b/chrome_frame/exception_barrier.cc
new file mode 100644
index 0000000..95c12f3
--- /dev/null
+++ b/chrome_frame/exception_barrier.cc
@@ -0,0 +1,41 @@
+// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// A class to make it easy to tag exception propagation boundaries and
+// get crash reports of exceptions that pass over same.
+#include "chrome_frame/exception_barrier.h"
+
+enum {
+  // Flag set by exception handling machinery when unwinding
+  EH_UNWINDING = 0x00000002
+};
+
+ExceptionBarrier::ExceptionHandler ExceptionBarrier::s_handler_ = NULL;
+
+// This function must be extern "C" to match up with the SAFESEH
+// declaration in our corresponding ASM file
+extern "C" EXCEPTION_DISPOSITION __cdecl
+ExceptionBarrierHandler(struct _EXCEPTION_RECORD *exception_record,
+                        void * establisher_frame,
+                        struct _CONTEXT *context,
+                        void * reserved) {
+  establisher_frame;  // unreferenced formal parameter
+  reserved;
+  if (!(exception_record->ExceptionFlags & EH_UNWINDING)) {
+    // When the exception is really propagating through us, we'd like to be
+    // called before the state of the program has been modified by the stack
+    // unwinding. In the absence of an exception handler, the unhandled
+    // exception filter gets called between the first chance and the second
+    // chance exceptions, so Windows pops either the JIT debugger or WER UI.
+    // This is not desirable in most of the cases.
+    ExceptionBarrier::ExceptionHandler handler = ExceptionBarrier::handler();
+    if (handler) {
+      EXCEPTION_POINTERS ptrs = { exception_record, context };
+
+      handler(&ptrs);
+    }
+  }
+
+  return ExceptionContinueSearch;
+}
diff --git a/chrome_frame/exception_barrier.h b/chrome_frame/exception_barrier.h
new file mode 100644
index 0000000..26cbc44
--- /dev/null
+++ b/chrome_frame/exception_barrier.h
@@ -0,0 +1,96 @@
+// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// A class to make it easy to tag exception propagation boundaries and
+// get crash reports of exceptions that pass over same.
+#ifndef CHROME_FRAME_EXCEPTION_BARRIER_H_
+#define CHROME_FRAME_EXCEPTION_BARRIER_H_
+
+#include <windows.h>
+
+/// This is the type dictated for an exception handler by the platform ABI
+/// @see _except_handler in excpt.h
+typedef EXCEPTION_DISPOSITION (__cdecl *ExceptionHandlerFunc)(
+                                struct _EXCEPTION_RECORD *exception_record,
+                                void * establisher_frame,
+                                struct _CONTEXT *context,
+                                void * reserved);
+
+/// The type of an exception record in the exception handler chain
+struct EXCEPTION_REGISTRATION {
+  EXCEPTION_REGISTRATION *prev;
+  ExceptionHandlerFunc  handler;
+};
+
+/// This is our raw exception handler, it must be declared extern "C" to
+/// match up with the SAFESEH declaration in our corresponding ASM file
+extern "C" EXCEPTION_DISPOSITION __cdecl
+ExceptionBarrierHandler(struct _EXCEPTION_RECORD *exception_record,
+                        void * establisher_frame,
+                        struct _CONTEXT *context,
+                        void * reserved);
+
+/// An exception barrier is used to report exceptions that pass through
+/// a boundary where exceptions shouldn't pass, such as e.g. COM interface
+/// boundaries.
+/// This is handy for any kind of plugin code, where if the exception passes
+/// through unhindered, it'll either be swallowed by an SEH exception handler
+/// above us on the stack, or be reported as an unhandled exception for
+/// the application hosting the plugin code.
+///
+/// To use this class, simply instantiate an ExceptionBarrier just inside
+/// the code boundary, like this:
+/// @code
+/// HRESULT SomeObject::SomeCOMMethod(...) {
+///   ExceptionBarrier report_crashes;
+///
+///   ... other code here ...
+/// }
+/// @endcode
+class ExceptionBarrier {
+ public:
+  /// Register the barrier in the SEH chain
+  ExceptionBarrier();
+
+  /// And unregister on destruction
+  ~ExceptionBarrier();
+
+  /// Signature of the handler function which gets notified when
+  /// an exception propagates through a barrier.
+  typedef void (CALLBACK *ExceptionHandler)(EXCEPTION_POINTERS *ptrs);
+
+  /// @name Accessors
+  /// @{
+  static void set_handler(ExceptionHandler handler) { s_handler_ = handler; }
+  static ExceptionHandler handler() { return s_handler_; }
+  /// @}
+
+ private:
+  /// Our SEH frame
+  EXCEPTION_REGISTRATION registration_;
+
+  /// The function that gets invoked if an exception
+  /// propagates through a barrier
+  static ExceptionHandler s_handler_;
+};
+
+/// @name These are implemented in the associated .asm file
+/// @{
+extern "C" void WINAPI RegisterExceptionRecord(
+                          EXCEPTION_REGISTRATION *registration,
+                          ExceptionHandlerFunc func);
+extern "C" void WINAPI UnregisterExceptionRecord(
+                          EXCEPTION_REGISTRATION *registration);
+/// @}
+
+
+inline ExceptionBarrier::ExceptionBarrier() {
+  RegisterExceptionRecord(&registration_, ExceptionBarrierHandler);
+}
+
+inline ExceptionBarrier::~ExceptionBarrier() {
+  UnregisterExceptionRecord(&registration_);
+}
+
+#endif  // CHROME_FRAME_EXCEPTION_BARRIER_H_
diff --git a/chrome_frame/exception_barrier_lowlevel.asm b/chrome_frame/exception_barrier_lowlevel.asm
new file mode 100644
index 0000000..b759990
--- /dev/null
+++ b/chrome_frame/exception_barrier_lowlevel.asm
@@ -0,0 +1,52 @@
+; Copyright (c) 2010 The Chromium Authors. All rights reserved.
+; Use of this source code is governed by a BSD-style license that can be
+; found in the LICENSE file.
+;
+; Tag the exception handler as an SEH handler in case the executable
+; is linked with /SAFESEH (which is the default).
+;
+; MASM 8.0 inserts an additional leading underscore in front of names
+; and this is an attempted fix until we understand why.
+IF @version LT 800
+_ExceptionBarrierHandler PROTO
+.SAFESEH _ExceptionBarrierHandler
+ELSE
+ExceptionBarrierHandler PROTO
+.SAFESEH ExceptionBarrierHandler
+ENDIF
+
+.586
+.MODEL FLAT, STDCALL
+ASSUME FS:NOTHING
+.CODE
+
+; extern "C" void WINAPI RegisterExceptionRecord(
+;                          EXCEPTION_REGISTRATION *registration,
+;                          ExceptionHandlerFunc func);
+RegisterExceptionRecord PROC registration:DWORD, func:DWORD
+OPTION PROLOGUE:None
+OPTION EPILOGUE:None
+  mov   edx, DWORD PTR [esp + 4]  ; edx is registration
+  mov   eax, DWORD PTR [esp + 8] ; eax is func
+  mov   DWORD PTR [edx + 4], eax
+  mov   eax, FS:[0]
+  mov   DWORD PTR [edx], eax
+  mov   FS:[0], edx
+  ret   8
+
+RegisterExceptionRecord ENDP
+
+; extern "C" void UnregisterExceptionRecord(
+;                           EXCEPTION_REGISTRATION *registration);
+UnregisterExceptionRecord PROC registration:DWORD
+OPTION PROLOGUE:None
+OPTION EPILOGUE:None
+
+  mov   edx, DWORD PTR [esp + 4]
+  mov   eax, [edx]
+  mov   FS:[0], eax
+  ret   4
+
+UnregisterExceptionRecord ENDP
+
+END
diff --git a/chrome_frame/urlmon_moniker.cc b/chrome_frame/urlmon_moniker.cc
index 4713da0..fe064ce 100644
--- a/chrome_frame/urlmon_moniker.cc
+++ b/chrome_frame/urlmon_moniker.cc
@@ -9,6 +9,7 @@
 #include "base/string_util.h"
 #include "chrome_frame/bho.h"
 #include "chrome_frame/bind_context_info.h"
+#include "chrome_frame/exception_barrier.h"
 #include "chrome_frame/chrome_active_document.h"
 #include "chrome_frame/urlmon_bind_status_callback.h"
 #include "chrome_frame/utils.h"
@@ -161,6 +162,8 @@ HRESULT MonikerPatch::BindToObject(IMoniker_BindToObject_Fn original,
   DLOG(INFO) << __FUNCTION__;
   DCHECK(to_left == NULL);
 
+  ExceptionBarrier barrier;
+
   HRESULT hr = S_OK;
   // Bind context is marked for switch when we sniff data in BSCBStorageBind
   // and determine that the renderer to be used is Chrome.
@@ -201,9 +204,14 @@ HRESULT MonikerPatch::BindToStorage(IMoniker_BindToStorage_Fn original,
     callback->AddRef();
     hr = callback->Initialize(me, bind_ctx);
     DCHECK(SUCCEEDED(hr));
-  }
 
-  hr = original(me, bind_ctx, to_left, iid, obj);
+    // Call the original back under an exception barrier only if we should
+    // wrap the callback.
+    ExceptionBarrier barrier;
+    hr = original(me, bind_ctx, to_left, iid, obj);
+  } else {
+    hr = original(me, bind_ctx, to_left, iid, obj);
+  }
 
   // If the binding terminates before the data could be played back
   // now is the chance. Sometimes OnStopBinding happens after this returns