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..5b1672d72253b0b16270de6df4b093fed64983ee 100644
--- a/chrome/app/breakpad_win.cc
+++ b/chrome/app/breakpad_win.cc
@@ -13,6 +13,7 @@
 #include <algorithm>
 #include <vector>
 
+#include "base/atomicops.h"
 #include "base/basictypes.h"
 #include "base/base_switches.h"
 #include "base/command_line.h"
@@ -119,55 +120,83 @@ 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 wchar_t kBrowserCrashDumpPrefixTemplate[] = L"%hs-%08x-%08x";
+const int kBrowserCrashDumpPrefixLength = 63;
+wchar_t 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,
+  int length = swprintf(g_browser_crash_dump_prefix,
+                        arraysize(g_browser_crash_dump_prefix),
+                        kBrowserCrashDumpPrefixTemplate,
+                        chrome::kChromeVersion,
                         ::GetCurrentProcessId(),
                         ::GetTickCount());
-  DCHECK_EQ(kBrowserCrashDumpValueLength, length);
+  DCHECK_GT(length, 0);
+  DCHECK_LE(length, kBrowserCrashDumpPrefixLength);
+
+  // Hold the registry key in a global for update on crash dump.
+  if (length > 0 && length < arraysize(g_browser_crash_dump_prefix)) {

[grt (UTC plus 2), 2013/09/11 03:25:56]

nit: remove braces since the conditional and the two bodies are each one liners.

[Roger McFarlane (Chromium), 2013/09/11 15:42:52]

Done.

+    g_browser_crash_dump_regkey = regkey.Take();
+  } else {
+    g_browser_crash_dump_prefix[0] = L'\0';
+  }
 }
 
-void SendSmokeSignalForCrashDump() {
-  if (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 int kMaxValueSize = 2 * arraysize(g_browser_crash_dump_prefix);

[grt (UTC plus 2), 2013/09/11 03:25:56]

int -> size_t since this is counting things (and arraysize returns size_t)

[Roger McFarlane (Chromium), 2013/09/11 15:42:52]

Done.

[rvargas (doing something else), 2013/09/12 20:15:59]

Small clarification about the first part: something that is counting things
should be an int
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Integer_Types),
even if negative things doesn't make sense. Of course I'm fine with size_t given
that we interact with arraysize...

[grt (UTC plus 2), 2013/09/13 18:39:29]

ah, i chose words poorly. thanks for pointing that out, Ricardo. my
understanding of the style guide was that when you're specifically talking about
the size of something measured by sizeof() or an STL thing that is size_type,
then size_t is the right choice. i didn't mean "counting sheep" sort of
counting. does that seem correct to you?

[rvargas (doing something else), 2013/09/13 19:45:29]

yes, that's also my reading of the guide. Once something is returning size_t,
the code should adapt to it and the simplest way to do it is to also use size_t
around it (reduce unsafe casts etc). It is unfortunate that size_t is unsigned
and 64 bits wide, but there's nothing to do about it and it hardly ever matters.

+  wchar_t value[kMaxValueSize + 1] = {};
+  int length = swprintf(
+      value, arraysize(value), L"%ls-%x",
+      g_browser_crash_dump_prefix,
+      base::subtle::NoBarrier_AtomicIncrement(&g_browser_crash_dump_count, 1));
+
+  DCHECK_GT(length, 0);
+  DCHECK_LE(length, kMaxValueSize);
+
+  if (length > 0 && length < arraysize(value)) {
+    // Write the value to the registry.
     base::win::RegKey regkey(g_browser_crash_dump_regkey);
-    regkey.WriteValue(g_browser_crash_dump_value, 1);
-    g_browser_crash_dump_regkey = NULL;
+    regkey.WriteValue(value, is_real_crash ? 1 : 0);
+
+    // Don't let regkey auto-close the key. More crash dumps may follow.
+    ignore_result(regkey.Take());
   }
 }
 
 // Dumps the current process memory.
 extern "C" void __declspec(dllexport) __cdecl DumpProcess() {
   if (g_breakpad) {
-    SendSmokeSignalForCrashDump();
     g_breakpad->WriteMinidump();
   }
 }
@@ -175,7 +204,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 +250,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 +615,7 @@ volatile LONG handling_exception = 0;
 // to implement it.
 bool FilterCallbackWhenNoCrash(
     void*, EXCEPTION_POINTERS*, MDRawAssertionInfo*) {
+  RecordCrashDumpAttempt(false);
   return true;
 }
 
@@ -601,6 +629,7 @@ bool FilterCallback(void*, EXCEPTION_POINTERS*, MDRawAssertionInfo*) {
   if (::InterlockedCompareExchange(&handling_exception, 1, 0) == 1) {
     ::Sleep(INFINITE);
   }
+  RecordCrashDumpAttempt(true);
   return true;
 }
 
@@ -773,7 +802,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