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 9fc453127f3215c29f3f2f59fdc8600a7c93f43d..3a70725200466ea1c188e48602a3968b7b7c2139 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"
@@ -121,55 +122,73 @@ 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] = { 0 };

[grt (UTC plus 2), 2013/09/06 15:53:23]

grt's pet peeve: "{ 0 }" -> "{}"

[Roger McFarlane (Chromium), 2013/09/10 19:43:44]

Done.

+
+// 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;
   }
 
+  // Hold the registry key in a global for update on crash dump.
   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);

[grt (UTC plus 2), 2013/09/06 15:53:23]

how about being a little more defensive here:
  if (length > 0 && length < arraysize(g_browser_crash_dump_prefix)) {
    // Hold the registry key in a global for update on crash dump.
    g_browser_crash_dump_regkey = regkey.Take();
  } else {
    g_browser_crash_dump_prefix[0] = '\0';
  }

[Roger McFarlane (Chromium), 2013/09/10 19:43:44]

Done.

+  DCHECK_LT(length, kBrowserCrashDumpPrefixLength);

[grt (UTC plus 2), 2013/09/06 15:53:23]

DCHECK_LE since kBrowserCrashDumpPrefixLength is already one less than
arraysize(g_browser_crash_dump_prefix)?

[Roger McFarlane (Chromium), 2013/09/10 19:43:44]

Done.

 }
 
 void SendSmokeSignalForCrashDump() {

[grt (UTC plus 2), 2013/09/06 15:53:23]

Send -> Write or Store since this function doesn't send anything.

[Roger McFarlane (Chromium), 2013/09/10 19:43:44]

RecordCrashDumpAttempt (the smoke signal terminology is defunct)

-  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;
-  }
+  // 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).
+  wchar_t value[2 * arraysize(g_browser_crash_dump_prefix)] = { 0 };

[grt (UTC plus 2), 2013/09/06 15:53:23]

initializer isn't necessary here if you check the return value below.

[Roger McFarlane (Chromium), 2013/09/10 19:43:44]

Don't we always initialize our vars in Chrome?

+  int length = swprintf(
+      value, arraysize(value), L"%ls-%x",
+      g_browser_crash_dump_prefix,
+      base::subtle::NoBarrier_AtomicIncrement(&g_browser_crash_dump_count, 1));
+

[grt (UTC plus 2), 2013/09/06 15:53:23]

  if (length > 0 && length < arraysize(value)) {
    // Write the value to the registry.
    ....
  }

[Roger McFarlane (Chromium), 2013/09/10 19:43:44]

Done.

+  // Write the value to the registry.
+  base::win::RegKey regkey(g_browser_crash_dump_regkey);
+  regkey.WriteValue(value, 1);

[grt (UTC plus 2), 2013/09/06 15:53:23]

since you don't need data in the value, how about:
  regkey.WriteValue(value, NULL, 0, REG_NONE);

[Roger McFarlane (Chromium), 2013/09/10 19:43:44]

I now need to data in the value (to tell between a crashing and non-crashing
dump).

+
+  // 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();
   }
 }
@@ -177,7 +196,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();
   }
 }
@@ -224,7 +242,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();
@@ -601,6 +618,7 @@ volatile LONG handling_exception = 0;
 // to implement it.
 bool FilterCallbackWhenNoCrash(
     void*, EXCEPTION_POINTERS*, MDRawAssertionInfo*) {
+  SendSmokeSignalForCrashDump();
   return true;
 }
 
@@ -614,6 +632,7 @@ bool FilterCallback(void*, EXCEPTION_POINTERS*, MDRawAssertionInfo*) {
   if (::InterlockedCompareExchange(&handling_exception, 1, 0) == 1) {
     ::Sleep(INFINITE);
   }
+  SendSmokeSignalForCrashDump();
   return true;
 }
 
@@ -804,7 +823,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