Add experiments info to crash dumps.

chrome/app/breakpad_win.cc

Index: chrome/app/breakpad_win.cc
===================================================================
--- chrome/app/breakpad_win.cc	(revision 125856)
+++ chrome/app/breakpad_win.cc	(working copy)
@@ -19,6 +19,7 @@
 #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"
@@ -34,6 +35,11 @@
 #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.
@@ -77,6 +83,8 @@
 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;
@@ -168,6 +176,71 @@
   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);
@@ -321,6 +394,16 @@
         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();
@@ -511,6 +594,21 @@
 
 }  // 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