Use pickle for field trial entries in shared memory

base/metrics/field_trial.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 "base/metrics/field_trial.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/base_switches.h"
 #include "base/build_time.h"
 #include "base/command_line.h"
 #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"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 
 namespace base {
 
 namespace {
@@ skipping 13 matching lines @@
 // crbug.com/653874
 const bool kUseSharedMemoryForFieldTrials = false;
 
 // Constants for the field trial allocator.
 const char kAllocatorName[] = "FieldTrialAllocator";
 const uint32_t kFieldTrialType = 0xABA17E13 + 1;  // SHA1(FieldTrialEntry) v1
 #if !defined(OS_NACL)
 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);
 
-  const char* GetGroupName() const {
-    const char* src =
-        reinterpret_cast<char*>(const_cast<FieldTrialEntry*>(this));
-    return src + this->group_name_offset;
+    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);
+
+    if (!pickle_iter.ReadStringPiece(trial_name))
+      return false;
+    if (!pickle_iter.ReadStringPiece(group_name))
+      return false;
+    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);
   DCHECK_GT(month, 0);
   DCHECK_LT(month, 13);
   DCHECK_GT(day_of_month, 0);
   DCHECK_LT(day_of_month, 32);
@@ skipping 709 matching lines @@
     return 0;
   AutoLock auto_lock(global_->lock_);
   return global_->registered_.size();
 }
 
 // static
 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
       // any. This is useful to ensure that field trials created in child
       // processes are properly reported in crash reports.
       trial->group();
     }
   }
 }
 
@@ skipping 39 matching lines @@
     return;
 
   // Or if we've already added it.
   if (field_trial->ref_ != SharedPersistentMemoryAllocator::kReferenceNull)
     return;
 
   FieldTrial::State trial_state;
   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;
+  entry->activated = trial_state.activated;
+  entry->size = pickle.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';
+  // 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;
 }
 
 // static
 void FieldTrialList::ActivateFieldTrialEntryWhileLocked(
     FieldTrial* field_trial) {
   SharedPersistentMemoryAllocator* allocator =
       global_->field_trial_allocator_.get();
@@ skipping 37 matching lines @@
     return;
   }
   AutoLock auto_lock(global_->lock_);
   CHECK(!global_->PreLockedFind(trial->trial_name())) << trial->trial_name();
   trial->AddRef();
   trial->SetTrialRegistered();
   global_->registered_[trial->trial_name()] = trial;
 }
 
 }  // namespace base