Chrome.BrowserCrashDumpAttempts needs to account for multiple dumps from the same browser process.

chrome/app/breakpad_win.cc

Index: chrome/app/breakpad_win.cc
diff --git a/chrome/app/breakpad_win.cc b/chrome/app/breakpad_win.cc
index b77b3689080ffa40af4850d3b0976cc0bbde15a1..3350959e32e8c31907282b81e3e9365d4b8635f2 100644
--- a/chrome/app/breakpad_win.cc
+++ b/chrome/app/breakpad_win.cc
@@ -13,12 +13,14 @@
 #include <algorithm>
 #include <vector>
 
+#include "base/atomicops.h"
 #include "base/basictypes.h"
 #include "base/base_switches.h"
 #include "base/command_line.h"
 #include "base/debug/crash_logging.h"
 #include "base/environment.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/strings/safe_sprintf.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
@@ -119,55 +121,77 @@ const size_t kMaxDynamicEntries = 256;
 // Maximum length for plugin path to include in plugin crash reports.
 const size_t kMaxPluginPathLength = 256;
 
-// These values track the browser crash dump registry key and pre-computed
-// registry value name, which we use as a "smoke-signal" for counting dumps.
-static HKEY g_browser_crash_dump_regkey = NULL;
-static const wchar_t kBrowserCrashDumpValueFormatStr[] = L"%08x-%08x";
-static const int kBrowserCrashDumpValueLength = 17;
-static wchar_t g_browser_crash_dump_value[kBrowserCrashDumpValueLength+1] = {0};
+// The value name prefix will be of the form {chrome-version}-{pid}-{timestamp}
+// (i.e., "#####.#####.#####.#####-########-########") which easily fits into a
+// 63 character buffer.
+const char kBrowserCrashDumpPrefixTemplate[] = "%s-%08x-%08x";
+const size_t kBrowserCrashDumpPrefixLength = 63;
+char g_browser_crash_dump_prefix[kBrowserCrashDumpPrefixLength + 1] = {};
+
+// These registry key to which we'll write a value for each crash dump attempt.
+HKEY g_browser_crash_dump_regkey = NULL;
+
+// A atomic counter to make each crash dump value name unique.
+base::subtle::Atomic32 g_browser_crash_dump_count = 0;
 
 void InitBrowserCrashDumpsRegKey() {
   DCHECK(g_browser_crash_dump_regkey == NULL);
 
-  base::string16 key_str(chrome::kBrowserCrashDumpAttemptsRegistryPath);
-  key_str += L"\\";
-  key_str += UTF8ToWide(chrome::kChromeVersion);
-
   base::win::RegKey regkey;
   if (regkey.Create(HKEY_CURRENT_USER,
-                    key_str.c_str(),
+                    chrome::kBrowserCrashDumpAttemptsRegistryPath,
                     KEY_ALL_ACCESS) != ERROR_SUCCESS) {
     return;
   }
 
-  g_browser_crash_dump_regkey = regkey.Take();
-
-  // We use the current process id and the curren tick count as a (hopefully)
+  // We use the current process id and the current tick count as a (hopefully)
   // unique combination for the crash dump value. There's a small chance that
   // across a reboot we might have a crash dump signal written, and the next
   // browser process might have the same process id and tick count, but crash
   // before consuming the signal (overwriting the signal with an identical one).
   // For now, we're willing to live with that risk.
-  int length = swprintf(g_browser_crash_dump_value,
-                        arraysize(g_browser_crash_dump_value),
-                        kBrowserCrashDumpValueFormatStr,
-                        ::GetCurrentProcessId(),
-                        ::GetTickCount());
-  DCHECK_EQ(kBrowserCrashDumpValueLength, length);
+  int length = base::strings::SafeSPrintf(g_browser_crash_dump_prefix,
+                                          kBrowserCrashDumpPrefixTemplate,
+                                          chrome::kChromeVersion,
+                                          ::GetCurrentProcessId(),
+                                          ::GetTickCount());
+  if (length <= 0) {
+    NOTREACHED();
+    g_browser_crash_dump_prefix[0] = '\0';
+    return;
+  }
+
+  // Hold the registry key in a global for update on crash dump.
+  g_browser_crash_dump_regkey = regkey.Take();
 }
 
-void SendSmokeSignalForCrashDump() {
-  if (g_browser_crash_dump_regkey != NULL) {
-    base::win::RegKey regkey(g_browser_crash_dump_regkey);
-    regkey.WriteValue(g_browser_crash_dump_value, 1);
-    g_browser_crash_dump_regkey = NULL;
+void RecordCrashDumpAttempt(bool is_real_crash) {
+  // If we're not a browser (or the registry is unavailable to us for some
+  // reason) then there's nothing to do.
+  if (g_browser_crash_dump_regkey == NULL)
+    return;
+
+  // Generate the final value name we'll use (appends the crash number to the
+  // base value name).
+  const size_t kMaxValueSize = 2 * kBrowserCrashDumpPrefixLength;
+  char value_name[kMaxValueSize + 1] = {};
+  int length = base::strings::SafeSPrintf(
+      value_name,
+      "%s-%x",
+      g_browser_crash_dump_prefix,
+      base::subtle::NoBarrier_AtomicIncrement(&g_browser_crash_dump_count, 1));
+
+  if (length > 0) {
+    DWORD value_dword = is_real_crash ? 1 : 0;
+    ::RegSetValueExA(g_browser_crash_dump_regkey, value_name, 0, REG_DWORD,
+                     reinterpret_cast<BYTE*>(&value_dword),
+                     sizeof(value_dword));
   }
 }
 
 // Dumps the current process memory.
 extern "C" void __declspec(dllexport) __cdecl DumpProcess() {
   if (g_breakpad) {
-    SendSmokeSignalForCrashDump();
     g_breakpad->WriteMinidump();
   }
 }
@@ -175,7 +199,6 @@ extern "C" void __declspec(dllexport) __cdecl DumpProcess() {
 // Used for dumping a process state when there is no crash.
 extern "C" void __declspec(dllexport) __cdecl DumpProcessWithoutCrash() {
   if (g_dumphandler_no_crash) {
-    SendSmokeSignalForCrashDump();
     g_dumphandler_no_crash->WriteMinidump();
   }
 }
@@ -222,7 +245,6 @@ InjectDumpForHangDebugging(HANDLE process) {
 extern "C" void DumpProcessAbnormalSignature() {
   if (!g_breakpad)
     return;
-  SendSmokeSignalForCrashDump();
   g_custom_entries->push_back(
       google_breakpad::CustomInfoEntry(L"unusual-crash-signature", L""));
   g_breakpad->WriteMinidump();
@@ -588,6 +610,7 @@ volatile LONG handling_exception = 0;
 // to implement it.
 bool FilterCallbackWhenNoCrash(
     void*, EXCEPTION_POINTERS*, MDRawAssertionInfo*) {
+  RecordCrashDumpAttempt(false);
   return true;
 }
 
@@ -601,6 +624,7 @@ bool FilterCallback(void*, EXCEPTION_POINTERS*, MDRawAssertionInfo*) {
   if (::InterlockedCompareExchange(&handling_exception, 1, 0) == 1) {
     ::Sleep(INFINITE);
   }
+  RecordCrashDumpAttempt(true);

[cpu_(ooo_6.6-7.5), 2013/09/13 21:04:28]

Is this thing here temporary? I want to keep the code we execute while crashed
(for realz) to the minimun possible. For example, if the stack is corrupted we
would get less crashes the more we executue code here.

[Roger McFarlane (Chromium), 2013/09/13 21:30:46]

It's here for as long as want to keep these histograms alive.

For the most recent snapshots of this CL I've simply move the call from being
sprinkled before every WriteMiniDump call to being in the callback(s) invoked by
WriteMiniDump, so we don't miss any. It's otherwise the same basic set of
actions as the previous iterations.

In sum, the "new" stuff happening on crash dump are a SafeSPrintf and a
RegSetValueEx.

   return true;
 }
 
@@ -773,7 +797,6 @@ bool ShowRestartDialogIfCrashed(bool* exit_now) {
 extern "C" int __declspec(dllexport) CrashForException(
     EXCEPTION_POINTERS* info) {
   if (g_breakpad) {
-    SendSmokeSignalForCrashDump();
     g_breakpad->WriteMinidumpForException(info);
     // Patched stub exists based on conditions (See InitCrashReporter).
     // As a side note this function also gets called from