Use pickle for field trial entries 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 bb6ae0cb83fd02fa52b8d17584ee911fcef105ca..a3e5bdad33b72bf795634f4ae2231e177677e583 100644
--- a/base/metrics/field_trial.cc
+++ b/base/metrics/field_trial.cc
@@ -13,6 +13,7 @@
 #include "base/feature_list.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/pickle.h"
 #include "base/process/memory.h"
 #include "base/rand_util.h"
 #include "base/strings/string_number_conversions.h"
@@ -46,29 +47,27 @@ const uint32_t kFieldTrialType = 0xABA17E13 + 1;  // SHA1(FieldTrialEntry) v1
 const size_t kFieldTrialAllocationSize = 4 << 10;  // 4 KiB = one page
 #endif
 
-// We create one FieldTrialEntry struct per field trial in shared memory (via
-// field_trial_allocator_). It contains whether or not it's activated, and the
-// offset from the start of the allocated memory to the group name. We don't
-// need the offset into the trial name because that's always a fixed position
-// from the start of the struct. Two strings will be appended to the end of this
-// structure in allocated memory, so the result will look like this:
-// ---------------------------------
-// | fte | trial_name | group_name |
-// ---------------------------------
+// We create one FieldTrialEntry per field trial in shared memory, via
+// AddToAllocatorWhileLocked. It contains whether or not it's activated, and a
+// base::Pickle object that we unpickle and read from.
 struct FieldTrialEntry {
   bool activated;
-  uint32_t group_name_offset;
+  // Size of the pickled structure, NOT the total size of this entry.
+  uint32_t size;
 
-  const char* GetTrialName() const {
-    const char* src =
-        reinterpret_cast<char*>(const_cast<FieldTrialEntry*>(this));
-    return src + sizeof(FieldTrialEntry);
-  }
+  bool GetTrialAndGroupName(StringPiece* trial_name,

[Alexei Svitkine (slow), 2016/10/28 15:32:22]

Please add a comment, specifically mentioning that this is only valid to do when
the entry resides in shared memory with name/group data following it.

+                            StringPiece* group_name) const {
+    char* src = reinterpret_cast<char*>(const_cast<FieldTrialEntry*>(this)) +
+                sizeof(FieldTrialEntry);
+
+    Pickle pickle(src, this->size);

[Alexei Svitkine (slow), 2016/10/28 15:32:22]

Nit: No this->

[lawrencewu, 2016/10/28 15:38:02]

Done.

+    PickleIterator pickle_iter(pickle);
 
-  const char* GetGroupName() const {
-    const char* src =
-        reinterpret_cast<char*>(const_cast<FieldTrialEntry*>(this));
-    return src + this->group_name_offset;
+    if (!pickle_iter.ReadStringPiece(trial_name))
+      return false;
+    if (!pickle_iter.ReadStringPiece(group_name))
+      return false;
+    return true;
   }
 };
 
@@ -798,15 +797,22 @@ void FieldTrialList::CreateTrialsFromSharedMemory(
     std::unique_ptr<SharedMemory> shm) {
   const SharedPersistentMemoryAllocator shalloc(std::move(shm), 0,
                                                 kAllocatorName, true);
-  PersistentMemoryAllocator::Iterator iter(&shalloc);
+  PersistentMemoryAllocator::Iterator mem_iter(&shalloc);
 
   SharedPersistentMemoryAllocator::Reference ref;
-  while ((ref = iter.GetNextOfType(kFieldTrialType)) !=
+  while ((ref = mem_iter.GetNextOfType(kFieldTrialType)) !=
          SharedPersistentMemoryAllocator::kReferenceNull) {
     const FieldTrialEntry* entry =
         shalloc.GetAsObject<const FieldTrialEntry>(ref, kFieldTrialType);
+
+    StringPiece trial_name;
+    StringPiece group_name;
+    if (!entry->GetTrialAndGroupName(&trial_name, &group_name))
+      NOTREACHED();

[Alexei Svitkine (slow), 2016/10/28 15:32:22]

You can still do a continue after the NOTREACHED just in case.

[lawrencewu, 2016/10/28 15:38:02]

Done.

+
+    // TODO: Convert the API for CreateFieldTrial to take StringPieces.

[Alexei Svitkine (slow), 2016/10/28 15:32:22]

TODOs should have a username or crbug here.

So you can make this a TODO(lawrencewu):

[lawrencewu, 2016/10/28 15:38:02]

Done.

     FieldTrial* trial =
-        CreateFieldTrial(entry->GetTrialName(), entry->GetGroupName());
+        CreateFieldTrial(trial_name.as_string(), group_name.as_string());
 
     if (entry->activated) {
       // Call |group()| to mark the trial as "used" and notify observers, if
@@ -866,34 +872,25 @@ void FieldTrialList::AddToAllocatorWhileLocked(FieldTrial* field_trial) {
   if (!field_trial->GetState(&trial_state))
     return;
 
-  size_t trial_name_size = trial_state.trial_name.size() + 1;
-  size_t group_name_size = trial_state.group_name.size() + 1;
-  size_t size = sizeof(FieldTrialEntry) + trial_name_size + group_name_size;
+  Pickle pickle;
+  pickle.WriteString(trial_state.trial_name);
+  pickle.WriteString(trial_state.group_name);
 
-  // Allocate just enough memory to fit the FieldTrialEntry struct, trial name,
-  // and group name.
+  size_t total_size = sizeof(FieldTrialEntry) + pickle.size();
   SharedPersistentMemoryAllocator::Reference ref =
-      allocator->Allocate(size, kFieldTrialType);
+      allocator->Allocate(total_size, kFieldTrialType);
   if (ref == SharedPersistentMemoryAllocator::kReferenceNull)
     return;
 
-  // Calculate offsets into the shared memory segment.
   FieldTrialEntry* entry =
       allocator->GetAsObject<FieldTrialEntry>(ref, kFieldTrialType);
-  uint32_t trial_name_offset = sizeof(FieldTrialEntry);
-  uint32_t group_name_offset = trial_name_offset + trial_name_size;
-
-  // Copy over the data.
   entry->activated = trial_state.activated;
-  entry->group_name_offset = group_name_offset;
-  char* trial_name = reinterpret_cast<char*>(entry) + trial_name_offset;
-  char* group_name = reinterpret_cast<char*>(entry) + group_name_offset;
-  memcpy(trial_name, trial_state.trial_name.data(), trial_name_size);
-  memcpy(group_name, trial_state.group_name.data(), group_name_size);
-
-  // Null terminate the strings.
-  trial_name[trial_state.trial_name.size()] = '\0';
-  group_name[trial_state.group_name.size()] = '\0';
+  entry->size = pickle.size();
+
+  // TODO: Modify base::Pickle to be able to write over a section in memory, so

[Alexei Svitkine (slow), 2016/10/28 15:32:22]

Same comment about the TODO.

[lawrencewu, 2016/10/28 15:38:02]

Done.

+  // we can avoid this memcpy.
+  char* dst = reinterpret_cast<char*>(entry) + sizeof(FieldTrialEntry);
+  memcpy(dst, pickle.data(), pickle.size());
 
   allocator->MakeIterable(ref);
   field_trial->ref_ = ref;