Collect field trial information from the stability file

components/browser_watcher/postmortem_report_collector.cc

Index: components/browser_watcher/postmortem_report_collector.cc
diff --git a/components/browser_watcher/postmortem_report_collector.cc b/components/browser_watcher/postmortem_report_collector.cc
index aa3845f22c3f01d7bd274b8e771745ba460045bd..745b1087175f329d38f595c81122e6310f7cec16 100644
--- a/components/browser_watcher/postmortem_report_collector.cc
+++ b/components/browser_watcher/postmortem_report_collector.cc
@@ -13,8 +13,10 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/path_service.h"
 #include "base/strings/string_piece.h"
+#include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/browser_watcher/postmortem_minidump_writer.h"
+#include "components/variations/active_field_trials.h"
 #include "third_party/crashpad/crashpad/client/settings.h"
 #include "third_party/crashpad/crashpad/util/misc/uuid.h"
 
@@ -31,14 +33,18 @@ using crashpad::CrashReportDatabase;
 
 namespace {
 
+const char kFieldTrialKeyPrefix[] = "FieldTrial.";
+
 // Collects stability user data from the recorded format to the collected
 // format.
 void CollectUserData(
     const ActivityUserData::Snapshot& recorded_map,
-    google::protobuf::Map<std::string, TypedValue>* collected_map) {
+    google::protobuf::Map<std::string, TypedValue>* collected_map,
+    StabilityReport* report) {
   DCHECK(collected_map);
 
   for (const auto& name_and_value : recorded_map) {
+    const std::string& key = name_and_value.first;
     const ActivityUserData::TypedValue& recorded_value = name_and_value.second;
     TypedValue collected_value;
 
@@ -58,10 +64,25 @@ void CollectUserData(
         collected_ref->set_size(recorded_ref.size());
         break;
       }
-      case ActivityUserData::STRING_VALUE:
-        collected_value.set_string_value(
-            recorded_value.GetString().as_string());
+      case ActivityUserData::STRING_VALUE: {
+        std::string value = recorded_value.GetString().as_string();
+
+        if (report && base::StartsWith(key, kFieldTrialKeyPrefix,

[bcwhite, 2017/02/14 15:42:14]

To be clear, you want field-trial strings to go into the collected values if no
report is requested?

[manzagop (departed), 2017/02/14 19:21:15]

Yeah, this is clunky. I'm using this function to collect both the global data,
which may contain finch data, and activity data, which should not. In the first
case I pass in the report, in the second I don't have one.

[bcwhite, 2017/02/14 21:05:16]

On the second pass, do you want the field trials to be ignored or included with
other global data?

[manzagop (departed), 2017/02/14 21:27:57]

Not sure I follow: what's the second pass? Do you mean for activity-local data?
Those shouldn't have field trial data. If they do, it's fine to leave it in,
that way we'll know we're doing something wrong.

[bcwhite, 2017/02/15 16:24:19]

You said that "in the second I don't have one" so I just wanted to be sure that,
during that case, you want all the field trials added as their existing strings
into the collected_map.

[manzagop (departed), 2017/02/15 18:35:07]

Acknowledged.

+                                       base::CompareCase::SENSITIVE)) {
+          // This entry represents an active Finch experiment.
+          std::string trial_name =
+              key.substr(std::strlen(kFieldTrialKeyPrefix));
+          variations::ActiveGroupId group_id =
+              variations::MakeActiveGroupId(trial_name, value);
+          FieldTrial* field_trial = report->add_field_trials();
+          field_trial->set_name_id(group_id.name);
+          field_trial->set_group_id(group_id.group);
+          continue;
+        }
+
+        collected_value.set_string_value(value);

[bcwhite, 2017/02/14 15:42:14]

I believe set_string_value will accept (char*, len) which would be more
efficient that creating a std::string from the returned StringPiece.

That's probably also true for set_bytes_value and set_char_value.

[manzagop (departed), 2017/02/14 19:21:15]

Nice! Ah, but MakeActiveGroupId wants a string. Still changed it, if that
function ever changes. Also changed the other cases.

         break;
+      }
       case ActivityUserData::STRING_VALUE_REFERENCE: {
         base::StringPiece recorded_ref = recorded_value.GetStringReference();
         TypedValue::Reference* collected_ref =
@@ -86,7 +107,7 @@ void CollectUserData(
         break;
     }
 
-    (*collected_map)[name_and_value.first].Swap(&collected_value);
+    (*collected_map)[key].Swap(&collected_value);
   }
 }
 
@@ -285,7 +306,8 @@ PostmortemReportCollector::CollectionStatus PostmortemReportCollector::Collect(
   }
 
   // Collect global user data.
-  CollectUserData(global_data_snapshot, (*report)->mutable_global_data());
+  CollectUserData(global_data_snapshot, (*report)->mutable_global_data(),
+                  report->get());
 
   // Collect thread activity data.
   // Note: a single process is instrumented.
@@ -356,7 +378,7 @@ void PostmortemReportCollector::CollectThread(
     // Collect user data
     if (i < snapshot.user_data_stack.size()) {
       CollectUserData(snapshot.user_data_stack[i],
-                      collected->mutable_user_data());
+                      collected->mutable_user_data(), nullptr);
     }
   }
 }