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 size_t 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),

[rvargas (doing something else), 2013/09/12 20:15:59]

why not use kBrowserCrashDumpPrefixLength directly? (and yes, I understand
there's a + 1 going on)

[Roger McFarlane (Chromium), 2013/09/13 18:34:23]

Switched to use SafeSPrintf here as well, no need to specify the size.

+                        kBrowserCrashDumpPrefixTemplate,
+                        chrome::kChromeVersion,
                         ::GetCurrentProcessId(),
                         ::GetTickCount());
-  DCHECK_EQ(kBrowserCrashDumpValueLength, length);
+  DCHECK_GT(length, 0);
+  DCHECK_LE(length, static_cast<int>(kBrowserCrashDumpPrefixLength));
+
+  // Hold the registry key in a global for update on crash dump.
+  if (length > 0 && length < arraysize(g_browser_crash_dump_prefix))
+    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 size_t kMaxValueSize = 2 * arraysize(g_browser_crash_dump_prefix);

[rvargas (doing something else), 2013/09/12 20:15:59]

kBrowserCrashDumpPrefixLength ?

[Roger McFarlane (Chromium), 2013/09/13 18:34:23]

Done.

+  wchar_t value[kMaxValueSize + 1] = {};
+  int length = swprintf(

[rvargas (doing something else), 2013/09/12 02:50:21]

This is a non trivial CL. Do you mind creating a bug so that it is easier to
track what we are doing and why?

I wonder if we want to do this work pre-exception processing (as is currently
the case) or post- exception. I mean, we could as well do work on the callback
as opposed to the filter.

One approach focuses on issues uploading crash reports (I though that was the
main objective), while the other includes issues generating the dump as well,
but has the potential to significantly affect the object being measured.

On that topic, see ExceptionHandler::HandleInvalidParameter... breakpad code is
careful to minimize work when handling exceptions, as anything may result in not
being able to generate a report, or getting wrong reports. printf is one of
those things...

[Roger McFarlane (Chromium), 2013/09/12 17:51:38]

Done. http://crbug/290196
I put it pre-exception in case there are instances where the crash reporting
mechanism itself might be failing, which is a large part of what these
histograms are trying to confirm/deny.
So, in theory, this could be problematic. But we're not calling printf in a
potentially error inducing manner. The alternative is to implement a custom
string formatting function here. While certainly do-able, I think that would be
hard to justify.

Ideally, I would have pre-computed the entirety of the string outside the
exception handler, but we need to be able to reliably differentiate between
multiple instances occurring in the same process, and the exception handler
chain is our only hook for doing so (AFAIK).

[rvargas (doing something else), 2013/09/12 20:15:59]

famous last words :)
That code already exists: base::strings::SafeSPrintf.

That said, what is the issue with pre-computing the string? Why not two fixed
names with the counter as the value? Do we really worry about races between two
threads attempting to dump without crash at the same time? I think the code
would be simpler that way, no?

[Roger McFarlane (Chromium), 2013/09/13 18:34:23]

Re: famous last words

Indeed. After I hit 'send' I thought: I'm going to live to regret those words.

re: SafeSPrintf

Nice! Didn't know that was there (looks like it landed just over a week ago).
Will use that instead.

Re: Concurrent non-crashing dumps

While unlikely, we've got a lot a users, so eventually this is going to happen
(it's called out in the comments for the non-crashing filter callback as well).

Actually, the problem is slightly bigger than just concurrency, it's that
writing the value as a counter (instead of counted booleans) makes it possible
to double count dump attempts.

 1 - B1 records a dump attempt but keeps running
 2 - B2 consumes the registry key
 3 - B1 records another dump attempt, value now equals 2
 4 - Next consumer double counts the first attempt.

[rvargas (doing something else), 2013/09/13 19:45:29]

yeah... it took quite a while to land.
sure, but for this code we only care if it actually affects the result of the
measurement in a way that we can notice :)
That should not be a concern because B2 should have a lock against B1 before
MetricsService starts. If that is not the case (or we worry about the very small
group of users who have multiple user data dirs), then this code is also wrong
in that reading back the registry keys is not locked against another browser
doing the same... two browsers may gather keys, report and attempt to delete
keys at the same time.

I'm not saying that we should add more synchronization to cover more edge cases.
And of course I'm not opposed to code that actually takes into account more edge
cases. What I was trying to say is that sometimes having simpler code is better,
as long as the consequences of not being exhaustive are acceptable.

The goal here is to correlate other metrics that have a 10x factor of
disagreement, so unless we have a significant source of errors (we miscount a
very large percentage of cases), the end result is not going to change. On the
other hand, I'm always supper cautious about doing any work at all while dealing
with a process that is in the middle of crashing, so my opinion will be weighted
towards simpler code.

+      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);

[rvargas (doing something else), 2013/09/12 02:50:21]

What does it mean to DCHECK in the middle of handling an exception?

[Roger McFarlane (Chromium), 2013/09/12 17:51:38]

DCHECKs look for programming errors, so in reality this should never fire. That
said, in a debug build this should cause chrome to die a stupendously fiery
death (after giving the debugger a chance to inspect the problem)... alerting us
to something wrong.

:)

[rvargas (doing something else), 2013/09/12 20:15:59]

Maybe it is just me, but I don't think intentionally reentering code while
processing exceptions is a sane thing to do, even for debug builds.

[Roger McFarlane (Chromium), 2013/09/13 18:34:23]

Fair enough, I guess. The intent of the DCHECK was call attention to a problem
instead of just silently becoming a NOP, which is what the defensive programming
will result in. In this case, I agree that falling back to a NOP in release
builds is the right approach, but in debug it would be nice to fail noticeably.

While in principle, I'd prefer to leave the DCHECKs there, I'm not averse to
removing them.

Done.

+  DCHECK_LE(length, static_cast<int>(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());
 }