Add experiments info to crash dumps.

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 <windows.h>
 #include <shellapi.h>
 #include <tchar.h>
 
 #include <algorithm>
 #include <vector>
 
 #include "base/base_switches.h"
 #include "base/command_line.h"
 #include "base/environment.h"
 #include "base/file_util.h"
 #include "base/file_version_info.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/string_split.h"
 #include "base/string_util.h"
+#include "base/string16.h"
 #include "base/stringprintf.h"
 #include "base/utf_string_conversions.h"
 #include "base/win/registry.h"
 #include "base/win/win_util.h"
 #include "breakpad/src/client/windows/handler/exception_handler.h"
 #include "chrome/app/hard_error_handler_win.h"
 #include "chrome/common/child_process_logging.h"
 #include "chrome/common/chrome_result_codes.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/env_vars.h"
 #include "chrome/installer/util/google_chrome_sxs_distribution.h"
 #include "chrome/installer/util/google_update_settings.h"
 #include "chrome/installer/util/install_util.h"
 #include "policy/policy_constants.h"
 
+// NACL_WIN64 build doesn't include field trials.
+#if !defined(NACL_WIN64)
+#include "base/metrics/field_trial.h"
+#endif
+
 namespace {
 
 // Minidump with stacks, PEB, TEB, and unloaded module list.
 const MINIDUMP_TYPE kSmallDumpType = static_cast<MINIDUMP_TYPE>(
     MiniDumpWithProcessThreadData |  // Get PEB and TEB.
     MiniDumpWithUnloadedModules);  // Get unloaded modules when available.
 
 // Minidump with all of the above, plus memory referenced from stack.
 const MINIDUMP_TYPE kLargerDumpType = static_cast<MINIDUMP_TYPE>(
     MiniDumpWithProcessThreadData |  // Get PEB and TEB.
@@ skipping 23 matching lines @@
 // data updated as the state of the browser changes.
 static std::vector<google_breakpad::CustomInfoEntry>* g_custom_entries = NULL;
 static size_t g_url_chunks_offset;
 static size_t g_num_of_extensions_offset;
 static size_t g_extension_ids_offset;
 static size_t g_client_id_offset;
 static size_t g_gpu_info_offset;
 static size_t g_num_of_views_offset;
 static size_t g_num_switches_offset;
 static size_t g_switches_offset;
+static size_t g_num_of_experiments_offset;
+static size_t g_experiment_chunks_offset;

[robertshield, 2012/03/14 13:51:48]

For clarity I'd prefer these to be initialized explicitly.

[MAD, 2012/03/15 22:37:36]

I don't understand... I just did the same thing as the other variables of the
same usage...

The only thing I see I could do is initialize them to an invalid value and then
DCHECK that the value is valid before using them... Is this what you meant?

[robertshield, 2012/03/20 19:24:31]

Yes, this wasn't a criticism of your code :-)
I just mean stick an "= 0;" at the end of each of the above lines. I know it
doesn't make a difference, but it seems more conventional to explicitly
initialize static vars in the Chrome codebase.

 
 // Maximum length for plugin path to include in plugin crash reports.
 const size_t kMaxPluginPathLength = 256;
 
 // Dumps the current process memory.
 extern "C" void __declspec(dllexport) __cdecl DumpProcess() {
   if (g_breakpad)
     g_breakpad->WriteMinidump();
 }
 
@@ skipping 71 matching lines @@
                   argv[argv_i].c_str(),
                   google_breakpad::CustomInfoEntry::kValueMaxLength);
     num_added++;
   }
 
   // Make note of the total number of switches. This is useful in case we have
   // truncated at kMaxSwitches, to see how many were unaccounted for.
   SetIntegerValue(g_num_switches_offset, static_cast<int>(argv.size()) - 1);
 }
 
+#if !defined(NACL_WIN64)

[robertshield, 2012/03/14 13:51:48]

Would you consider moving this code to a new file that is not built by 64-bit
NACL instead? This file is already quite long, does the field trial stuff need
to live here? 

[MAD, 2012/03/15 22:37:36]

It's doable, I guess I could call the function implemented in another file and
pass it the pointers to the global variables.

[robertshield, 2012/03/20 19:24:31]

Yes, that's what I meant. It would be a first step to breaking the tradition of
adding more fairly independent stuff to this file.

+void UpdateExperiments() {
+  std::vector<base::FieldTrial::NameGroupId> name_group_ids;
+  base::FieldTrialList::GetFieldTrialNameGroupIds(&name_group_ids);
+
+  std::vector<string16> experiment_strings(name_group_ids.size());
+  for (size_t i = 0; i < name_group_ids.size(); ++i) {
+    experiment_strings[i] = base::StringPrintf(
+        L"%x-%x", name_group_ids[i].name, name_group_ids[i].group);
+  }
+
+  size_t num_chunks = 0;
+  size_t current_experiment = 0;
+  string16 current_chunk;
+  // Preallocate the string...
+  current_chunk.resize(google_breakpad::CustomInfoEntry::kValueMaxLength);
+  current_chunk.erase();  // But start empty... This won't force a realloc.
+  while (current_experiment < experiment_strings.size() &&
+         num_chunks < kMaxReportedExperimentChunks) {
+    // Check if we have enough room to add another experiment to the current
+    // chunk string. If not, we commit the current chunk string and start over.
+    if (current_chunk.size() + experiment_strings[current_experiment].size() >
+        google_breakpad::CustomInfoEntry::kValueMaxLength) {
+      base::wcslcpy(
+          (*g_custom_entries)[g_experiment_chunks_offset + num_chunks].value,

[robertshield, 2012/03/14 13:51:48]

Not 100% sure, but I believe this will do the wrong thing if UpdateExperiments
is called before GetCustomInfo. If so, this should be guarded against.

[MAD, 2012/03/15 22:37:36]

You're right, I'll add a DCHECK...

[robertshield, 2012/03/20 19:24:31]

Sounds good. I suggest also handling this case in release mode.

+          current_chunk.c_str(),
+          current_chunk.size() + 1);  // This must include the NULL termination.
+      ++num_chunks;
+      current_chunk = experiment_strings[current_experiment];
+    } else {
+      if (!current_chunk.empty())
+        current_chunk += L",";
+      current_chunk += experiment_strings[current_experiment];
+    }
+    ++current_experiment;
+  }
+
+  // Commit the last chunk that didn't get big enough yet.
+  if (!current_chunk.empty() && num_chunks < kMaxReportedExperimentChunks) {
+    base::wcslcpy(
+        (*g_custom_entries)[g_experiment_chunks_offset + num_chunks].value,
+        current_chunk.c_str(),
+        current_chunk.size() + 1);  // This must include the NULL termination.
+  }
+
+  // Make note of the total number of experiments,
+  // even if it's > kMaxReportedExperimentChunks. This is useful when
+  // correlating stability with the number of experiments running
+  // simultaneously.
+  SetIntegerValue(g_num_of_experiments_offset,
+                  static_cast<int>(name_group_ids.size()));
+}
+
+extern "C" void __declspec(dllexport) __cdecl InitExperimentList() {
+  UpdateExperiments();
+}
+
+extern "C" void __declspec(dllexport) __cdecl AddFieldTrialGroup(
+    const std::string& field_trial_name, const std::string& group_name) {
+  // The trial is already in the list, we only use this as a notification.
+  // TODO(mad): Find a way to just add the new info instead of starting over.

[robertshield, 2012/03/14 13:51:48]

please clarify that "the new info" is the data stored in |field_trial_name| and
|group_name|.

[MAD, 2012/03/15 22:37:36]

Done.

[robertshield, 2012/03/20 19:24:31]

Where?

+  UpdateExperiments();
+}
+#endif  // NACL_WIN64
+
 // Appends the plugin path to |g_custom_entries|.
 void SetPluginPath(const std::wstring& path) {
   DCHECK(g_custom_entries);
 
   if (path.size() > kMaxPluginPathLength) {
     // If the path is too long, truncate from the start rather than the end,
     // since we want to be able to recover the DLL name.
     SetPluginPath(path.substr(path.size() - kMaxPluginPathLength));
     return;
   }
@@ skipping 133 matching lines @@
       std::wstring plugin_path =
           CommandLine::ForCurrentProcess()->GetSwitchValueNative("plugin-path");
       if (!plugin_path.empty())
         SetPluginPath(plugin_path);
     }
   } else {
     g_custom_entries->push_back(
         google_breakpad::CustomInfoEntry(L"num-views", L"N/A"));
   }
 
+  g_num_of_experiments_offset = g_custom_entries->size();
+  g_custom_entries->push_back(
+      google_breakpad::CustomInfoEntry(L"num-experiments", L"N/A"));
+
+  g_experiment_chunks_offset = g_custom_entries->size();
+  for (int i = 0; i < kMaxReportedExperimentChunks; ++i) {
+    g_custom_entries->push_back(google_breakpad::CustomInfoEntry(
+        base::StringPrintf(L"experiment-chunk-%i", i + 1).c_str(), L""));
+  }
+
   static google_breakpad::CustomClientInfo custom_client_info;
   custom_client_info.entries = &g_custom_entries->front();
   custom_client_info.count = g_custom_entries->size();
 
   return &custom_client_info;
 }
 
 // This callback is used when we want to get a dump without crashing the
 // process.
 bool DumpDoneCallbackWhenNoCrash(const wchar_t*, const wchar_t*, void*,
@@ skipping 170 matching lines @@
                 google_breakpad::CustomInfoEntry::kValueMaxLength);
 }
 
 extern "C" void __declspec(dllexport) __cdecl SetNumberOfViews(
     int number_of_views) {
   SetIntegerValue(g_num_of_views_offset, number_of_views);
 }
 
 }  // namespace
 
+namespace testing {
+
+// Access to namespace protected functions for test purposes.
+google_breakpad::CustomClientInfo* TestGetCustomInfo() {
+  return GetCustomInfo(L"", L"", L"");
+}
+
+#if !defined(NACL_WIN64)
+void TestUpdateExperiments() {
+  UpdateExperiments();
+}
+#endif  // NACL_WIN64
+
+}  // namespace testing
+
 bool WrapMessageBoxWithSEH(const wchar_t* text, const wchar_t* caption,
                            UINT flags, bool* exit_now) {
   // We wrap the call to MessageBoxW with a SEH handler because it some
   // machines with CursorXP, PeaDict or with FontExplorer installed it crashes
   // uncontrollably here. Being this a best effort deal we better go away.
   __try {
     *exit_now = (IDOK != ::MessageBoxW(NULL, text, caption, flags));
   } __except(EXCEPTION_EXECUTE_HANDLER) {
     // Its not safe to continue executing, exit silently here.
     ::ExitProcess(chrome::RESULT_CODE_RESPAWN_FAILED);
@@ skipping 229 matching lines @@
     // Tells breakpad to handle breakpoint and single step exceptions.
     // This might break JIT debuggers, but at least it will always
     // generate a crashdump for these exceptions.
     g_breakpad->set_handle_debug_exceptions(true);
   }
 }
 
 void InitDefaultCrashCallback(LPTOP_LEVEL_EXCEPTION_FILTER filter) {
   previous_filter = SetUnhandledExceptionFilter(filter);
 }