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

chrome/app/breakpad_win.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.
 
 #include "chrome/app/breakpad_win.h"
 
 #include <shellapi.h>
 #include <tchar.h>
 #include <userenv.h>
 #include <windows.h>
 #include <winnt.h>
 
 #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"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/win/metro.h"
 #include "base/win/pe_image.h"
 #include "base/win/registry.h"
 #include "base/win/win_util.h"
 #include "breakpad/src/client/windows/handler/exception_handler.h"
@@ skipping 80 matching lines @@
     DynamicEntriesMap;
 DynamicEntriesMap* g_dynamic_entries = NULL;
 // Allow for 128 entries. POSIX uses 64 entries of 256 bytes, so Windows needs
 // 256 entries of 64 bytes to match. See CustomInfoEntry::kValueMaxLength in
 // Breakpad.
 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();
   }
 }
 
 // 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();
   }
 }
 
 // We need to prevent ICF from folding DumpForHangDebuggingThread() and
 // DumpProcessWithoutCrashThread() together, since that makes them
 // indistinguishable in crash dumps. We do this by making the function
 // bodies unique, and prevent optimization from shuffling things around.
 MSVC_DISABLE_OPTIMIZE()
 MSVC_PUSH_DISABLE_WARNING(4748)
@@ skipping 26 matching lines @@
 
 extern "C" HANDLE __declspec(dllexport) __cdecl
 InjectDumpForHangDebugging(HANDLE process) {
   return CreateRemoteThread(process, NULL, 0, DumpForHangDebuggingThread,
                             0, 0, NULL);
 }
 
 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();
 }
 
 // Reduces the size of the string |str| to a max of 64 chars. Required because
 // breakpad's CustomInfoEntry raises an invalid_parameter error if the string
 // we want to set is longer.
 std::wstring TrimToBreakpadMax(const std::wstring& str) {
   std::wstring shorter(str);
@@ skipping 345 matching lines @@
 }
 
 // flag to indicate that we are already handling an exception.
 volatile LONG handling_exception = 0;
 
 // This callback is used when there is no crash. Note: Unlike the
 // |FilterCallback| below this does not do dupe detection. It is upto the caller
 // to implement it.
 bool FilterCallbackWhenNoCrash(
     void*, EXCEPTION_POINTERS*, MDRawAssertionInfo*) {
+  RecordCrashDumpAttempt(false);
   return true;
 }
 
 // This callback is executed when the Chrome process has crashed and *before*
 // the crash dump is created. To prevent duplicate crash reports we
 // make every thread calling this method, except the very first one,
 // go to sleep.
 bool FilterCallback(void*, EXCEPTION_POINTERS*, MDRawAssertionInfo*) {
   // Capture every thread except the first one in the sleep. We don't
   // want multiple threads to concurrently report exceptions.
   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;
 }
 
 // Previous unhandled filter. Will be called if not null when we
 // intercept a crash.
 LPTOP_LEVEL_EXCEPTION_FILTER previous_filter = NULL;
 
 // Exception filter used when breakpad is not enabled. We just display
 // the "Do you want to restart" message and then we call the previous filter.
 long WINAPI ChromeExceptionFilter(EXCEPTION_POINTERS* info) {
@@ skipping 152 matching lines @@
 
 // Crashes the process after generating a dump for the provided exception. Note
 // that the crash reporter should be initialized before calling this function
 // for it to do anything.
 // NOTE: This function is used by SyzyASAN to invoke a crash. If you change the
 // the name or signature of this function you will break SyzyASAN instrumented
 // releases of Chrome. Please contact syzygy-team@chromium.org before doing so!
 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
     // WindowProcExceptionFilter.
     if (g_real_terminate_process_stub == NULL) {
       ::TerminateProcess(::GetCurrentProcess(), content::RESULT_CODE_KILLED);
     } else {
       NtTerminateProcessPtr real_terminate_proc =
           reinterpret_cast<NtTerminateProcessPtr>(
               static_cast<char*>(g_real_terminate_process_stub));
@@ skipping 238 matching lines @@
   previous_filter = SetUnhandledExceptionFilter(filter);
 }
 
 void StringVectorToCStringVector(const std::vector<std::wstring>& wstrings,
                                  std::vector<const wchar_t*>* cstrings) {
   cstrings->clear();
   cstrings->reserve(wstrings.size());
   for (size_t i = 0; i < wstrings.size(); ++i)
     cstrings->push_back(wstrings[i].c_str());
 }