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

chrome/browser/metrics/metrics_service.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 //------------------------------------------------------------------------------
 // Description of the life cycle of a instance of MetricsService.
 //
 //  OVERVIEW
 //
 // A MetricsService instance is typically created at application startup.  It is
@@ skipping 779 matching lines @@
 
 void MetricsService::RecordBreakpadHasDebugger(bool has_debugger) {
   if (!has_debugger)
     IncrementPrefValue(prefs::kStabilityDebuggerNotPresent);
   else
     IncrementPrefValue(prefs::kStabilityDebuggerPresent);
 }
 
 #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);
 }
 #endif  // defined(OS_WIN)
 
 //------------------------------------------------------------------------------
 // private methods
 //------------------------------------------------------------------------------
 
@@ skipping 949 matching lines @@
   if (local_state) {
     const PrefService::Preference* uma_pref =
         local_state->FindPreference(prefs::kMetricsReportingEnabled);
     if (uma_pref) {
       bool success = uma_pref->GetValue()->GetAsBoolean(&result);
       DCHECK(success);
     }
   }
   return result;
 }