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

chrome/browser/metrics/metrics_service.cc

Index: chrome/browser/metrics/metrics_service.cc
diff --git a/chrome/browser/metrics/metrics_service.cc b/chrome/browser/metrics/metrics_service.cc
index b356a00dbb3d2c85966f560613760558055b2810..230035b0d874d2148ebdac5bbdc2aa1ca22c0c8b 100644
--- a/chrome/browser/metrics/metrics_service.cc
+++ b/chrome/browser/metrics/metrics_service.cc
@@ -797,26 +797,24 @@ void MetricsService::RecordBreakpadHasDebugger(bool has_debugger) {
 
 #if defined(OS_WIN)
 void MetricsService::CountBrowserCrashDumpAttempts() {
-  base::string16 key_str(chrome::kBrowserCrashDumpAttemptsRegistryPath);
-  key_str += L"\\";
-  key_str += UTF8ToWide(chrome::kChromeVersion);
-
   base::win::RegKey regkey;
   if (regkey.Open(HKEY_CURRENT_USER,
-                  key_str.c_str(),
+                  chrome::kBrowserCrashDumpAttemptsRegistryPath,
                   KEY_ALL_ACCESS) != ERROR_SUCCESS) {
     return;
   }
 
+  base::string16 chrome_version(base::ASCIIToUTF16(chrome::kChromeVersion));
+
   base::string16 temp_name;
   DWORD temp_value = 0;
   int crash_dump_attempts = 0;
   for (int i = regkey.GetValueCount() - 1; i >= 0; --i) {
     if (regkey.GetValueNameAt(i, &temp_name) == ERROR_SUCCESS &&

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

i don't think that this will work the way you hope it will since there's no
guarantee on a stable ordering of registry values. i suggest you do a simple
walk through the values via RegistryValueIterator and keep track of the names of
the to-be-deleted values as you go (in a deque or vector, for example) which you
then delete after you finish iterating. one idea:
{
  base::win::RegKey regkey;
  // Don't report any value in the histogram if the registry key doesn't exist
  // so that the zero-crashes bucket isn't polluted.
  if (regkey.Open(HKEY_CURRENT_USER,
                  chrome::kBrowserCrashDumpAttemptsRegistryPath,
                  KEY_SET_VALUE) == ERROR_SUCCESS) {
    base::string16 chrome_version(base::ASCIIToUTF16(chrome::kChromeVersion));
    std::deque<base::string16> crash_dump_attempts;
    for (base::win::RegistryValueIterator iter(
             HKEY_CURRENT_USER,
             chrome::kBrowserCrashDumpAttemptsRegistryPath);
         iter.Valid();
         ++iter) {
      if (StartsWith(iter.Name(), chrome_version, false))
        crash_dump_attempts.push_back(base::string16(iter.Name()));
    }

    UMA_HISTOGRAM_COUNTS("Chrome.BrowserCrashDumpAttempts",
                         crash_dump_attempts.size());

    while (!crash_dump_attempts.empty()) {
      regkey.DeleteValue(crash_dump_attempts.back().c_str());
      crash_dump_attempts.pop_back();
    }
  }
}

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

I've done as you suggest per the walkthrough and delete after.

I've also limited the reporting to non-zero counts. It was a suggestion on the
initial Cl creating this code that it always be reported, but have the
zero-valued samples does seem to confuse consumers of the histograms.

+        StartsWith(temp_name, chrome_version, false) &&

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

how about also deleting the values for older versions? or maybe unconditionally
delete all values after counting the ones corresponding to this version? is it
useful to have another UMA stat for the number of other-version crashes?

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

Hmm...

That would be wonky in multi-install, which is probably rare outside the
chromium developer community. Since I'm not immediately sure what the right
answer is on that one, how about I punt it to a future CL?

         regkey.ReadValueDW(temp_name.c_str(), &temp_value) == ERROR_SUCCESS) {
       regkey.DeleteValue(temp_name.c_str());
-      if (temp_value != 0)
-        ++crash_dump_attempts;
+      crash_dump_attempts += temp_value;
     }
   }
   UMA_HISTOGRAM_COUNTS("Chrome.BrowserCrashDumpAttempts", crash_dump_attempts);