Stability instrumentation: add a Vectored Exception Handler

components/browser_watcher/stability_debugging.cc

Index: components/browser_watcher/stability_debugging.cc
diff --git a/components/browser_watcher/stability_debugging.cc b/components/browser_watcher/stability_debugging.cc
index 3c8cc6492928043dbe0de2306bef4581e992e044..c522687f05af96a6044d1602a6adb145c6f9061a 100644
--- a/components/browser_watcher/stability_debugging.cc
+++ b/components/browser_watcher/stability_debugging.cc
@@ -4,10 +4,41 @@
 
 #include "components/browser_watcher/stability_debugging.h"
 
+#include <windows.h>
+
 #include "base/debug/activity_tracker.h"
+#include "build/build_config.h"
 
 namespace browser_watcher {
 
+namespace {
+
+// A registered vecotred exception handler.

[Sigurður Ásgeirsson, 2017/06/07 17:43:25]

speling [sic]

[manzagop (departed), 2017/06/08 22:46:38]

Done.

+void* g_veh_handler = nullptr;
+
+uintptr_t GetProgramCounter(const CONTEXT& context) {
+#if defined(ARCH_CPU_X86)
+  return context.Eip;
+#elif defined(ARCH_CPU_X86_64)
+  return context.Rip;
+#endif
+}
+
+LONG CALLBACK VectoredExceptionHandler(PEXCEPTION_POINTERS exception_pointers) {

[Sigurður Ásgeirsson, 2017/06/07 17:43:25]

I think we shy away from the Hungarian notation PFoo, and use Foo* instead?

[manzagop (departed), 2017/06/08 22:46:38]

Done.

+  base::debug::GlobalActivityTracker* tracker =
+      base::debug::GlobalActivityTracker::Get();

[Sigurður Ásgeirsson, 2017/06/07 17:43:25]

QQ: is this a straight-up accessor, or can this cause allocations and such? If
the latter, maybe you want to implement a straight-up accessor, as allocations
and locking and such in vectored exception handling is bad...

[manzagop (departed), 2017/06/08 22:46:38]

This is a straight up accessor. ACK.

+  if (tracker) {
+    EXCEPTION_RECORD* record = exception_pointers->ExceptionRecord;
+    uintptr_t pc = GetProgramCounter(*exception_pointers->ContextRecord);
+    tracker->RecordException(reinterpret_cast<void*>(pc),
+                             record->ExceptionAddress, record->ExceptionCode);
+  }
+
+  return EXCEPTION_CONTINUE_SEARCH;  // Continue to the next handler.
+}
+
+}  // namespace
+
 void SetStabilityDataBool(base::StringPiece name, bool value) {
   base::debug::GlobalActivityTracker* global_tracker =
       base::debug::GlobalActivityTracker::Get();
@@ -26,4 +57,16 @@ void SetStabilityDataInt(base::StringPiece name, int64_t value) {
   global_tracker->process_data().SetInt(name, value);
 }
 
+void RegisterStabilityVEH() {
+  DCHECK(!g_veh_handler);
+  // Register a vectored exception handler and request it be first. Note that
+  // subsequent registrations may also request to be first, in which case this
+  // one will be bumped.
+  // TODO(manzagop): Depending on observations, it may be necessary to
+  // consider refreshing the registration, either periodically or at opportune
+  // (e.g. risky) times.
+  // DO NOT SUBMIT: figure out when to ::RemoveVectoredExceptionHandler.

[Sigurður Ásgeirsson, 2017/06/07 17:43:25]

pro tip, there really isn't a good time to RemoveVEH, as is true for all kinds
of registrations that are process global. In Chrome this'd be a non-issue, as
chrome.dll doesn't really unload, but using e.g. a static uninitializer or any
other DllMain-time invocation would be your best bet.

[manzagop (departed), 2017/06/08 22:46:38]

Does this look ok?

[Sigurður Ásgeirsson, 2017/06/09 13:55:15]

looks good to me.

[manzagop (departed), 2017/06/09 21:27:35]

Acknowledged.

+  g_veh_handler = ::AddVectoredExceptionHandler(1, &VectoredExceptionHandler);
+}
+
 }  // namespace browser_watcher