Use SharedPersistentMemoryAllocator to share field trial state

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 4f3e0c76fcd58c606f3b98f90718f8dc63538d5f..2f993402c548bca98d24de03f32e108d5f300496 100644
--- a/base/metrics/field_trial.cc
+++ b/base/metrics/field_trial.cc
@@ -35,10 +35,46 @@ const char kActivationMarker = '*';
 // for now while the implementation is fleshed out (e.g. data format, single
 // shared memory segment). See https://codereview.chromium.org/2365273004/ and
 // crbug.com/653874
-#if defined(OS_WIN)
 const bool kUseSharedMemoryForFieldTrials = false;
+
+// Allocator used to instantiate field trial in child processes. In the
+// future, we may want to move this to a more generic place if we want to
+// start passing more data other than field trials.
+base::SharedPersistentMemoryAllocator* g_field_trial_allocator = nullptr;
+#if defined(OS_WIN)
+HANDLE g_readonly_allocator_handle = 0;
 #endif
 
+// Constants for the field trial allocator.
+const char kAllocatorName[] = "FieldTrialAllocator";
+const uint32_t kFieldTrialType = 0xABA17E13 + 1;  // SHA1(FieldTrialEntry) v1
+
+// We create one FieldTrialEntry struct per field trial in shared memory (via
+// g_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 |
+// ---------------------------------
+struct FieldTrialEntry {
+  bool activated;
+  uint32_t group_name_offset;
+
+  const char* GetTrialName() const {
+    const char* src =
+        reinterpret_cast<char*>(const_cast<FieldTrialEntry*>(this));
+    return src + sizeof(FieldTrialEntry);
+  }
+
+  const char* GetGroupName() const {
+    const char* src =
+        reinterpret_cast<char*>(const_cast<FieldTrialEntry*>(this));
+    return src + this->group_name_offset;
+  }
+};
+
 // 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);
@@ -129,6 +165,7 @@ bool ParseFieldTrialsString(const std::string& trials_string,
 // statics
 const int FieldTrial::kNotFinalized = -1;
 const int FieldTrial::kDefaultGroupNumber = 0;
+const int kFieldTrialAllocationSize = 4 << 10;  // 4 KiB = one page
 bool FieldTrial::enable_benchmarking_ = false;
 
 int FieldTrialList::kNoExpirationYear = 0;
@@ -586,14 +623,12 @@ void FieldTrialList::CreateTrialsFromCommandLine(
         base::SharedMemoryHandle(handle, base::GetCurrentProcId());
 
     // Gets deleted when it gets out of scope, but that's OK because we need it
-    // only for the duration of this call currently anyway.
-    base::SharedMemory shared_memory(shm_handle, false);
-    shared_memory.Map(field_trial_length);
+    // only for the duration of this method.
+    std::unique_ptr<base::SharedMemory> shm(
+        new base::SharedMemory(shm_handle, true));
+    shm.get()->Map(field_trial_length);

[bcwhite, 2016/10/20 21:14:06]

Map() will return false if it fails which can happen even on otherwise healthy
systems.  You should handle this case or it'll crash later on.

[lawrencewu, 2016/10/21 16:41:58]

Done.

 
-    char* field_trial_state = static_cast<char*>(shared_memory.memory());
-    bool result = FieldTrialList::CreateTrialsFromString(
-        std::string(field_trial_state), std::set<std::string>());
-    DCHECK(result);
+    FieldTrialList::CreateTrialsFromSharedMemory(std::move(shm));
     return;
   }
 #endif
@@ -606,37 +641,78 @@ void FieldTrialList::CreateTrialsFromCommandLine(
   }
 }
 
+#if defined(OS_WIN)
+// static
+void FieldTrialList::AppendFieldTrialHandleIfNeeded(
+    base::HandlesToInheritVector* handles) {
+  FieldTrialList::InstantiateFieldTrialAllocatorIfNeeded();
+
+  // Need to get a new, read-only handle to share to the child.
+  handles->push_back(g_readonly_allocator_handle);
+}
+#endif
+
 // static
-std::unique_ptr<base::SharedMemory> FieldTrialList::CopyFieldTrialStateToFlags(
+void FieldTrialList::CreateTrialsFromSharedMemory(
+    std::unique_ptr<base::SharedMemory> shm) {
+  const base::SharedPersistentMemoryAllocator shalloc(std::move(shm), 0,
+                                                      kAllocatorName, true);
+  base::PersistentMemoryAllocator::Iterator iter(&shalloc);
+
+  base::SharedPersistentMemoryAllocator::Reference ref;
+  while ((ref = iter.GetNextOfType(kFieldTrialType)) !=
+         base::SharedPersistentMemoryAllocator::kReferenceNull) {
+    const FieldTrialEntry* entry =
+        shalloc.GetAsObject<const FieldTrialEntry>(ref, kFieldTrialType);
+    FieldTrial* trial =
+        CreateFieldTrial(entry->GetTrialName(), entry->GetGroupName());
+
+    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();
+    }
+  }
+}
+
+// static
+void FieldTrialList::CopyFieldTrialStateToFlags(
     const char* field_trial_handle_switch,
     base::CommandLine* cmd_line) {
+#if defined(OS_WIN)
+  // Use shared memory to pass the state if the feature is enabled, otherwise
+  // fallback to passing it via the command line as a string.
+  if (kUseSharedMemoryForFieldTrials) {
+    FieldTrialList::InstantiateFieldTrialAllocatorIfNeeded();
+    // HANDLE is just typedef'd to void *. We basically cast the handle into an
+    // int (uintptr_t, to be exact), stringify the int, and pass it as a
+    // command-line flag. The child process will do the reverse conversions to
+    // retrieve the handle. See http://stackoverflow.com/a/153077
+    auto uintptr_handle =
+        reinterpret_cast<uintptr_t>(g_readonly_allocator_handle);
+    size_t field_trial_length =
+        g_field_trial_allocator->shared_memory()->mapped_size();

[bcwhite, 2016/10/20 21:14:06]

Instead of UpdateTrackingHistograms you could also create a histogram here and
get the used() size from the allocator directly.  Perhaps a better option;
doesn't have to be done now.

[lawrencewu, 2016/10/23 20:01:56]

Ack, will do later.

+    std::string field_trial_handle = std::to_string(uintptr_handle) + "," +
+                                     std::to_string(field_trial_length);
+
+    cmd_line->AppendSwitchASCII(field_trial_handle_switch, field_trial_handle);
+    return;
+  }
+#endif
+
   std::string field_trial_states;
   base::FieldTrialList::AllStatesToString(&field_trial_states);
   if (!field_trial_states.empty()) {
-// Use shared memory to pass the state if the feature is enabled, otherwise
-// fallback to passing it via the command line as a string.
-#if defined(OS_WIN)
-    if (kUseSharedMemoryForFieldTrials) {
-      std::unique_ptr<base::SharedMemory> shm(new base::SharedMemory());
-      size_t length = field_trial_states.size() + 1;
-      shm->CreateAndMapAnonymous(length);
-      memcpy(shm->memory(), field_trial_states.c_str(), length);
-
-      // HANDLE is just typedef'd to void *
-      auto uintptr_handle =
-          reinterpret_cast<std::uintptr_t>(shm->handle().GetHandle());
-      std::string field_trial_handle =
-          std::to_string(uintptr_handle) + "," + std::to_string(length);
-
-      cmd_line->AppendSwitchASCII(field_trial_handle_switch,
-                                  field_trial_handle);
-      return shm;
-    }
-#endif
     cmd_line->AppendSwitchASCII(switches::kForceFieldTrials,
                                 field_trial_states);
   }
-  return std::unique_ptr<base::SharedMemory>(nullptr);
+}
+
+// static
+base::SharedPersistentMemoryAllocator*
+FieldTrialList::GetFieldTrialAllocatorForTesting() {
+  return g_field_trial_allocator;
 }
 
 // static
@@ -694,12 +770,87 @@ void FieldTrialList::NotifyFieldTrialGroupSelection(FieldTrial* field_trial) {
   if (!field_trial->enable_field_trial_)
     return;
 
+  if (kUseSharedMemoryForFieldTrials) {
+    FieldTrialList::UpdateFieldTrialAllocator(field_trial);
+  }
+
   global_->observer_list_->Notify(
       FROM_HERE, &FieldTrialList::Observer::OnFieldTrialGroupFinalized,
       field_trial->trial_name(), field_trial->group_name_internal());
 }
 
 // static
+void FieldTrialList::InstantiateFieldTrialAllocatorIfNeeded() {
+  AutoLock auto_lock(global_->lock_);
+  // Create the allocator if not already created and add all existing trials.
+  if (g_field_trial_allocator == nullptr) {
+    std::unique_ptr<base::SharedMemory> shm(new base::SharedMemory());
+    shm->CreateAndMapAnonymous(kFieldTrialAllocationSize);
+
+    // TODO(lawrencewu): call UpdateTrackingHistograms() when all field trials
+    // have been registered (perhaps in the destructor?)
+    g_field_trial_allocator = new base::SharedPersistentMemoryAllocator(
+        std::move(shm), 0, kAllocatorName, false);
+    g_field_trial_allocator->CreateTrackingHistograms(kAllocatorName);
+
+    // Add all existing field trials.
+    for (const auto& registered : global_->registered_) {
+      FieldTrialList::UpdateFieldTrialAllocator(registered.second);
+    }
+
+#if defined(OS_WIN)
+    // Duplicate and set |g_readonly_allocator_handle| so we can pass it to be
+    // inherited and via the command line.
+    HANDLE src = g_field_trial_allocator->shared_memory()->handle().GetHandle();
+    ProcessHandle process = GetCurrentProcess();
+    DWORD access = SECTION_MAP_READ | SECTION_QUERY;
+    ::DuplicateHandle(process, src, process, &g_readonly_allocator_handle,
+                      access, true, 0);
+#endif
+  }
+}
+
+// static
+void FieldTrialList::UpdateFieldTrialAllocator(FieldTrial* field_trial) {
+  // Don't do anything if the allocator hasn't been instantiated yet.
+  if (g_field_trial_allocator == nullptr)

[bcwhite, 2016/10/21 13:48:29]

Since we've established that there could be parallel calls to the method that
does the original construction of this global object then there is also a
thread-safety issue here.  You're reading a non-atomic variable without a lock
when that variable could be written on other threads.

It's a write-once of a machine word so although there are theoretical problems,
it's not really a practical one... with THIS variable.  However, there is
another potential problem:  Without atomic release-store and acquire-load
operations, there is no ordering of memory accesses.  That means that this
pointer could be seen with a valid value but the actual object being pointed to
has not yet been committed to RAM.  Crash. Boom. Bang.  Good luck reproducing
that under test.

The TSAN bot may not complain because I don't think there are any multi-threaded
tests to trigger this condition, but that could change in the future.

Choices are:

1) Acquire the global lock each time before reading this variable.

2) Use an AtomicWord and reinterpret_cast the pointer to/from it.  See
histogram_macros.h for an example.  You could (but shouldn't) take this several
steps further and avoid the global lock completely -- see lazy_instance.h if
you're curious how.

3) Create a new class that has an Allocator member, takes no ctor parameters,
and initializes that member as desired when constructed.  Use a
base::LazyInstance<MyType>::Leaky as the type for your global variable and
access the Allocator member within.

I'd go with #3 if practical, especially if it means you can lose the global
lock.

[lawrencewu, 2016/10/23 20:01:56]

Done.

+    return;
+
+  // We allocate just enough memory to fit the FTE struct, trial name, and group
+  // name. If there's no more memory available, quit early.
+  FieldTrial::State trial_state;
+  field_trial->GetState(&trial_state);
+
+  std::string trial_name;
+  trial_state.trial_name.CopyToString(&trial_name);
+  size_t trial_name_size = trial_name.size() + 1;
+
+  std::string group_name;
+  trial_state.group_name.CopyToString(&group_name);
+  size_t group_name_size = group_name.size() + 1;
+
+  uint32_t trial_name_offset = sizeof(FieldTrialEntry);
+  uint32_t group_name_offset = trial_name_offset + trial_name_size;
+  size_t size = sizeof(FieldTrialEntry) + trial_name_size + group_name_size;
+
+  base::SharedPersistentMemoryAllocator::Reference ref =
+      g_field_trial_allocator->Allocate(size, kFieldTrialType);
+  if (ref == base::SharedPersistentMemoryAllocator::kReferenceNull)
+    return;
+
+  FieldTrialEntry* dst = g_field_trial_allocator->GetAsObject<FieldTrialEntry>(
+      ref, kFieldTrialType);
+  FieldTrialEntry entry = {trial_state.activated, group_name_offset};
+  *dst = entry;
+  memcpy(reinterpret_cast<char*>(dst) + trial_name_offset, trial_name.c_str(),
+         trial_name_size);
+  memcpy(reinterpret_cast<char*>(dst) + group_name_offset, group_name.c_str(),
+         group_name_size);
+
+  g_field_trial_allocator->MakeIterable(ref);
+}
+
+// static
 size_t FieldTrialList::GetFieldTrialCount() {
   if (!global_)
     return 0;