Store field trial parameters in shared memory

base/metrics/field_trial.cc

Index: base/metrics/field_trial.cc
diff --git a/base/metrics/field_trial.cc b/base/metrics/field_trial.cc
index 647234389b624c3b0e59e6c821952d7bd33f37c7..ff75ba2e192db77856e08f726d7595de15faf657 100644
--- a/base/metrics/field_trial.cc
+++ b/base/metrics/field_trial.cc
@@ -13,6 +13,7 @@
 #include "base/debug/alias.h"
 #include "base/feature_list.h"
 #include "base/logging.h"
+#include "base/metrics/field_trial_param_associator.h"
 #include "base/pickle.h"
 #include "base/process/memory.h"
 #include "base/rand_util.h"
@@ -71,25 +72,81 @@ struct FieldTrialEntry {
   // Size of the pickled structure, NOT the total size of this entry.
   uint32_t size;
 
-  // Calling this is only valid when the entry is initialized. That is, it
-  // resides in shared memory and has a pickle containing the trial name and
-  // group name following it.
-  bool GetTrialAndGroupName(StringPiece* trial_name,
-                            StringPiece* group_name) const {
+  // Returns an iterator over the data containing names and params.
+  PickleIterator GetPickleIterator() const {
     char* src = reinterpret_cast<char*>(const_cast<FieldTrialEntry*>(this)) +
                 sizeof(FieldTrialEntry);
 
     Pickle pickle(src, size);
-    PickleIterator pickle_iter(pickle);
+    return PickleIterator(pickle);
+  }
 
-    if (!pickle_iter.ReadStringPiece(trial_name))
+  // Takes the iterator and writes out the first two items into |trial_name| and
+  // |group_name|.
+  bool ReadStringPair(PickleIterator* iter,
+                      StringPiece* trial_name,

[bcwhite, 2016/11/23 21:03:52]

The name of this method is generic.  If you use string1_out and string2_out for
parameter names then you can reuse this method.

Might as well create a WriteStringPair and use that, too.  Or do away with this
completely.  It doesn't make much sense to have one but not the other.

[lawrencewu, 2016/11/23 21:49:20]

I don't think we can totally reuse this method, since we'll miss out on the case
where we can't read anymore, and we'll return false there.

I'm not opposed to the WriteStringPair method though -- Changed to add that.

+                      StringPiece* group_name) const {
+    if (!iter->ReadStringPiece(trial_name))
       return false;
-    if (!pickle_iter.ReadStringPiece(group_name))
+    if (!iter->ReadStringPiece(group_name))
       return false;
     return true;
   }
+
+  // Calling this is only valid when the entry is initialized. That is, it
+  // resides in shared memory and has a pickle containing the trial name and
+  // group name following it.
+  bool GetTrialAndGroupName(StringPiece* trial_name,
+                            StringPiece* group_name) const {
+    PickleIterator iter = GetPickleIterator();
+    return ReadStringPair(&iter, trial_name, group_name);
+  }
+
+  // Calling this is only valid when the entry is initialized as well. Reads the
+  // parameters following the trial and group name and stores them as key-value
+  // mappings in |params|.
+  bool GetParams(std::map<std::string, std::string>* params) const {
+    PickleIterator iter = GetPickleIterator();
+    StringPiece tmp;
+    if (!ReadStringPair(&iter, &tmp, &tmp))
+      return false;
+
+    while (true) {
+      StringPiece key;
+      StringPiece value;
+      if (!iter.ReadStringPiece(&key))

[bcwhite, 2016/11/23 21:03:52]

Can you just reuse ReadStringPair here?  iter must have some kind of "has more"
flag you can test, right?

[lawrencewu, 2016/11/23 21:49:20]

Unfortunately, no, there doesn't seem to be a "has more" method:
https://cs.chromium.org/chromium/src/base/pickle.h?q=PickleIterator&sq=packag...

[bcwhite, 2016/11/24 18:29:47]

You can reuse the existing method with a little creativity.

  if (!ReadStringPair(&key, &value))
    return key.empty();  // Non-empty is bad: got one of a pair.

[lawrencewu, 2016/11/24 21:50:35]

That's very clever -- done.

+        return true;  // No more params to read.
+      if (!iter.ReadStringPiece(&value))
+        return false;
+      (*params)[key.as_string()] = value.as_string();
+    }
+  }
 };
 
+// Writes out the field trial's contents (via trial_state) to the pickle. The
+// format of the pickle looks like:
+// TrialName, GroupName, ParamKey1, ParamValue1, ParamKey2, ParamValue2, ...
+// If there are no parameters, then it just ends at GroupName.
+bool PickleFieldTrial(const FieldTrial::State& trial_state, Pickle* pickle) {
+  if (!pickle->WriteString(trial_state.trial_name))
+    return false;
+  if (!pickle->WriteString(trial_state.group_name))
+    return false;
+
+  // Get field trial params.
+  std::map<std::string, std::string> params;
+  FieldTrialParamAssociator::GetInstance()->GetFieldTrialParamsWithoutFallback(
+      trial_state.trial_name.as_string(), trial_state.group_name.as_string(),
+      &params);
+
+  // Write params to pickle.
+  for (const auto& param : params) {
+    pickle->WriteString(StringPiece(param.first));

[bcwhite, 2016/11/23 21:03:52]

You're testing the return value of WriteString above but not here.

[lawrencewu, 2016/11/23 21:49:20]

Fixed.

+    pickle->WriteString(StringPiece(param.second));
+  }
+  return true;
+}
+
 // Created a time value based on |year|, |month| and |day_of_month| parameters.
 Time CreateTimeFromParams(int year, int month, int day_of_month) {
   DCHECK_GT(year, 1970);
@@ -893,6 +950,81 @@ size_t FieldTrialList::GetFieldTrialCount() {
   return global_->registered_.size();
 }
 
+// static
+bool FieldTrialList::GetParamsFromSharedMemory(
+    FieldTrial* field_trial,
+    std::map<std::string, std::string>* params) {
+  DCHECK(global_);
+  // If the field trial allocator is not set up yet, then there are several
+  // cases:
+  //   - We are in the browser process and the allocator has not been set up
+  //   yet. If we got here, then we couldn't find the params in
+  //   FieldTrialParamAssociator, so it's definitely not here. Return false.
+  //   - Using shared memory for field trials is not enabled. If we got here,
+  //   then there's nothing in shared memory. Return false.
+  //   - We are in the child process and the allocator has not been set up yet.
+  //   If this is the case, then you are calling this too early. The field trial
+  //   allocator should get set up very early in the lifecycle. Try to see if
+  //   you can call it after it's been set up.
+  if (!global_->field_trial_allocator_)
+    return false;
+
+  // If ref_ isn't set, then the field trial data can't be in shared memory.
+  if (!field_trial->ref_)
+    return false;
+
+  const FieldTrialEntry* entry =
+      global_->field_trial_allocator_->GetAsObject<const FieldTrialEntry>(
+          field_trial->ref_, kFieldTrialType);

[bcwhite, 2016/11/23 21:03:52]

Verify: GetAllocSize() >= sizeof(entry) + entry->size

Since you're checking all the read/write results, you're concerned that the data
might be bad so you check this in code as well and not in a DCHECK.

[lawrencewu, 2016/11/23 21:49:20]

Added.

+  return entry->GetParams(params);
+}
+
+// static
+void FieldTrialList::ClearParamsFromSharedMemoryForTesting() {
+  if (!global_ || !global_->field_trial_allocator_)
+    return;
+  AutoLock auto_lock(global_->lock_);

[bcwhite, 2016/11/23 21:03:52]

The comment in the .h file says lock_ protects access to registered_ but you're
not accessing that in this method.

[lawrencewu, 2016/11/23 21:49:20]

Changed comment to reflect that lock_ also protects access to
field_trial_allocator_.

+
+  // To clear the params, we iterate through every item in the allocator, copy
+  // just the trial and group name into a newly-allocated segment and then clear
+  // the existing item.
+  FieldTrialAllocator* allocator = global_->field_trial_allocator_.get();
+  FieldTrialAllocator::Iterator mem_iter(allocator);
+
+  FieldTrial::FieldTrialRef prev_ref;
+  while ((prev_ref = mem_iter.GetNextOfType(kFieldTrialType)) !=
+         FieldTrialAllocator::kReferenceNull) {
+    // Get the existing field trial entry in shared memory.
+    const FieldTrialEntry* prev_entry =
+        allocator->GetAsObject<const FieldTrialEntry>(prev_ref,
+                                                      kFieldTrialType);
+    StringPiece trial_name;
+    StringPiece group_name;
+    if (!prev_entry->GetTrialAndGroupName(&trial_name, &group_name))
+      continue;
+
+    // Write a new entry, minus the params.
+    Pickle pickle;
+    pickle.WriteString(trial_name);
+    pickle.WriteString(group_name);
+    size_t total_size = sizeof(FieldTrialEntry) + pickle.size();
+    FieldTrial::FieldTrialRef new_ref =
+        allocator->Allocate(total_size, kFieldTrialType);
+    FieldTrialEntry* new_entry =
+        allocator->GetAsObject<FieldTrialEntry>(new_ref, kFieldTrialType);
+    new_entry->activated = prev_entry->activated;
+    new_entry->size = pickle.size();

[bcwhite, 2016/11/23 21:03:52]

Don't you need to copy the pickle data?

[lawrencewu, 2016/11/23 21:49:20]

Whoops, good catch -- yes, fixed.

[bcwhite, 2016/11/24 18:29:47]

If your tests didn't catch that then your tests are incomplete.  Make sure there
is a test that stores and restores a complete set of data.

[lawrencewu, 2016/11/24 21:50:35]

Done. I found another bug while writing the test, too.

+
+    // Update the ref on the field trial.
+    FieldTrial* trial = Find(trial_name.as_string());
+    trial->ref_ = new_ref;
+
+    // Mark the existing entry as unused.
+    uint32_t old_type = allocator->GetType(prev_ref);

[bcwhite, 2016/11/23 21:03:52]

You already know from line 995 that the type is kFieldTrialType.

[lawrencewu, 2016/11/23 21:49:20]

Fixed.

+    allocator->ChangeType(prev_ref, 0, old_type);
+  }
+}
+
 #if defined(OS_WIN)
 // static
 bool FieldTrialList::CreateTrialsFromWindowsHandle(HANDLE handle) {
@@ -1036,8 +1168,8 @@ void FieldTrialList::AddToAllocatorWhileLocked(FieldTrial* field_trial) {
     return;
 
   Pickle pickle;
-  pickle.WriteString(trial_state.trial_name);
-  pickle.WriteString(trial_state.group_name);
+  if (!PickleFieldTrial(trial_state, &pickle))
+    return;
 
   size_t total_size = sizeof(FieldTrialEntry) + pickle.size();
   FieldTrial::FieldTrialRef ref =