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 88 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] = { 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();
   }
 }
 
 // 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 356 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*) {
+  SendSmokeSignalForCrashDump();
   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);
   }
+  SendSmokeSignalForCrashDump();
   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 170 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());
 }