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 94 matching lines @@
 
 // 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};
+static base::subtle::Atomic32 g_browser_crash_dump_count = 0;

[cpu_(ooo_6.6-7.5), 2013/09/04 23:29:45]

aka int32 btw

[Roger McFarlane (Chromium), 2013/09/05 13:57:53]

Yes, I know. But this communicates the intended usage.

 
 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,

[grt (UTC plus 2), 2013/09/05 21:09:13]

when are these keys deleted?

[grt (UTC plus 2), 2013/09/06 02:21:35]

one possibility: the installer could delete the keys for previous versions when
an update is applied, and likewise delete all keys when chrome is uninstalled.
if you wish, you could preserve the key for the
version-from-which-chrome-was-just-updated when an update is applied so that you
could somehow send this data up rather than losing it.

[Roger McFarlane (Chromium), 2013/09/06 02:59:20]

Indeed.

I got another ping from a concerned dev shortly after you wrote this. But he was
asking about post uninstall.

Another approach would be to put the version identifier into the value. All
chromen would share a key but write/delete version specific values. That still
doesn't address uninstall, but it does prevent the proliferation of keys.

[Roger McFarlane (Chromium), 2013/09/06 02:59:20]

That sounds like the makings of a plan.

                     key_str.c_str(),
                     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)
   // 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);
 }
 
 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;
-  }
+  // 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;
+
+  // Increment the number of crash dumps and persist it to the registry.
+  // Note that there is a race condition here: the final count could be off by
+  // one if two dumps are triggered at the same moment and the registry writes
+  // happen to be committed in the reverse order of the atomic increments.
+  // We'll live with this, as we don't want to attempt any "real" work while
+  // we may be in a crashing state.
+  base::win::RegKey regkey(g_browser_crash_dump_regkey);
+  regkey.WriteValue(
+      g_browser_crash_dump_value,
+      base::subtle::NoBarrier_AtomicIncrement(&g_browser_crash_dump_count, 1));
+
+  // 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();
   }
 }
 
@@ skipping 392 matching lines @@
 // facilities such as the i18n helpers.
 bool DumpDoneCallback(const wchar_t*, const wchar_t*, void*,
                       EXCEPTION_POINTERS* ex_info,
                       MDRawAssertionInfo*, bool) {
   // Check if the exception is one of the kind which would not be solved
   // by simply restarting chrome. In this case we show a message box with
   // and exit silently. Remember that chrome is in a crashed state so we
   // can't show our own UI from this process.
   if (HardErrorHandler(ex_info))
     return true;
 

[cpu_(ooo_6.6-7.5), 2013/09/04 23:32:20]

we don't have the smoke signal here ...

[Roger McFarlane (Chromium), 2013/09/05 13:57:53]

We send the smoke signal before generating the dump, or at least that's the
intent. AFAICT, I've hit all of the spots just before we write a minidump.

I did it in this order to count all the times we intend to generate a dump, and
if the dump generation should fail we should still have a count of the attempt.

[Roger McFarlane (Chromium), 2013/09/06 02:59:20]

I moved the call to the filter callbacks (which are called by breakpad before
generating any dumps).

   if (!breakpad::GetBreakpadClient()->AboutToRestart())
     return true;
 
   // Now we just start chrome browser with the same command line.
   STARTUPINFOW si = {sizeof(si)};
   PROCESS_INFORMATION pi;
   if (::CreateProcessW(NULL, ::GetCommandLineW(), NULL, NULL, FALSE,
                        CREATE_UNICODE_ENVIRONMENT, NULL, NULL, &si, &pi)) {
     ::CloseHandle(pi.hProcess);
     ::CloseHandle(pi.hThread);
@@ skipping 466 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());
 }