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/string16.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
@@ skipping 86 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 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();
   }
 }
 
 // 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);
   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());
 }