Store and retrieve features from shared memory

base/feature_list.cc

 // Copyright 2015 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/feature_list.h"
 
 #include <stddef.h>
 
 #include <utility>
 #include <vector>
 
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/field_trial.h"
+#include "base/pickle.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 
 namespace base {
 
 namespace {
 
 // Pointer to the FeatureList instance singleton that was set via
 // FeatureList::SetInstance(). Does not use base/memory/singleton.h in order to
 // have more control over initialization timing. Leaky.
 FeatureList* g_instance = nullptr;
 
 // Tracks whether the FeatureList instance was initialized via an accessor.
 bool g_initialized_from_accessor = false;
 
+const uint32_t kFeatureType = 0x06567CA6 + 1;  // SHA1(FeatureEntry) v1
+
+// An allocator entry for a feature in shared memory. The FeatureEntry is
+// followed by a base::Pickle object that contains the feature and trial name.
+// Any changes to this structure requires a bump in kFeatureType defined above.
+struct FeatureEntry {
+  // Expected size for 32/64-bit check.
+  static constexpr size_t kExpectedInstanceSize = 8;
+
+  // Specifies whether a feature override enables or disables the feature. Same
+  // values as the OverrideState enum in feature_list.h
+  uint32_t override_state;

[piman, 2016/12/02 22:29:26]

Do you intend to modify this field (or other contents in shared memory) after
allocator->MakeIterable(ref) is called? If so, this needs to be atomic, or some
form of higher level mutual exclusion needs to exist, otherwise the code in
InitializeFromSharedMemory is unsafe.

[lawrencewu, 2016/12/02 22:51:48]

I'm pretty sure we don't need to modify any fields after MakeIterable is called.
cc: asvitkine, can you confirm this?

[piman, 2016/12/02 23:03:24]

Either way, please add comments somewhere as to the safety protocol for
accessing this shared data. Assuming you never change this, it is safe because
MakeIterable is a "release" atomic operation, and
PersistentMemoryAllocator::Iterator::GetNextOfType is an "acquire" operation (so
memory writes before MakeIterable happen-before memory reads after
GetNextOfType). Also please mention somewhere that the only process that has
write access to this is the (trusted) browser process.

+
+  // Size of the pickled structure, NOT the total size of this entry.
+  uint32_t pickle_size;
+
+  // Reads the feature and trial name from the pickle. Calling this is only
+  // valid on an initialized entry that's in shared memory.
+  bool GetFeatureAndTrialName(StringPiece* feature_name,
+                              StringPiece* trial_name) const {
+    char* src = reinterpret_cast<char*>(const_cast<FeatureEntry*>(this)) +

[piman, 2016/12/02 22:29:26]

nit: why const_cast? The memory is read-only, so we should probably keep
everything const to be explicit. So src should be a const char* etc.

[lawrencewu, 2016/12/02 22:51:48]

Done.

+                sizeof(FeatureEntry);
+
+    Pickle pickle(src, pickle_size);
+    PickleIterator pickle_iter(pickle);
+
+    if (!pickle_iter.ReadStringPiece(feature_name))
+      return false;
+
+    // Return true because we are not guaranteed to have a trial name anyways.
+    auto sink = pickle_iter.ReadStringPiece(trial_name);
+    (void)sink;

[piman, 2016/12/02 22:29:26]

nit: ALLOW_UNUSED_LOCAL(sink)

[lawrencewu, 2016/12/02 22:51:48]

Done.

+    return true;
+  }
+};
+
 // Some characters are not allowed to appear in feature names or the associated
 // field trial names, as they are used as special characters for command-line
 // serialization. This function checks that the strings are ASCII (since they
 // are used in command-line API functions that require ASCII) and whether there
 // are any reserved characters present, returning true if the string is valid.
 // Only called in DCHECKs.
 bool IsValidFeatureOrFieldTrialName(const std::string& name) {
   return IsStringASCII(name) && name.find_first_of(",<*") == std::string::npos;
 }
 
 }  // namespace
 
 FeatureList::FeatureList() {}
 
 FeatureList::~FeatureList() {}
 
 void FeatureList::InitializeFromCommandLine(
     const std::string& enable_features,
     const std::string& disable_features) {
   DCHECK(!initialized_);
 
   // Process disabled features first, so that disabled ones take precedence over
   // enabled ones (since RegisterOverride() uses insert()).
   RegisterOverridesFromCommandLine(disable_features, OVERRIDE_DISABLE_FEATURE);
   RegisterOverridesFromCommandLine(enable_features, OVERRIDE_ENABLE_FEATURE);
 
   initialized_from_command_line_ = true;
 }
 
+void FeatureList::InitializeFromSharedMemory(
+    PersistentMemoryAllocator* allocator) {
+  DCHECK(!initialized_);
+
+  PersistentMemoryAllocator::Iterator iter(allocator);
+
+  PersistentMemoryAllocator::Reference ref;
+  while ((ref = iter.GetNextOfType(kFeatureType)) !=
+         PersistentMemoryAllocator::kReferenceNull) {
+    const FeatureEntry* entry =
+        allocator->GetAsObject<const FeatureEntry>(ref, kFeatureType);
+
+    OverrideState override_state =
+        static_cast<OverrideState>(entry->override_state);
+
+    StringPiece feature_name;
+    StringPiece trial_name;
+    if (!entry->GetFeatureAndTrialName(&feature_name, &trial_name))
+      continue;
+
+    FieldTrial* trial = FieldTrialList::Find(trial_name.as_string());
+    RegisterOverride(feature_name, override_state, trial);
+  }
+}
+
 bool FeatureList::IsFeatureOverriddenFromCommandLine(
     const std::string& feature_name,
     OverrideState state) const {
   auto it = overrides_.find(feature_name);
   return it != overrides_.end() && it->second.overridden_state == state &&
          !it->second.overridden_by_field_trial;
 }
 
 void FeatureList::AssociateReportingFieldTrial(
     const std::string& feature_name,
@@ skipping 22 matching lines @@
   DCHECK(!ContainsKey(overrides_, feature_name) ||
          !overrides_.find(feature_name)->second.field_trial)
       << "Feature " << feature_name
       << " has conflicting field trial overrides: "
       << overrides_.find(feature_name)->second.field_trial->trial_name()
       << " / " << field_trial->trial_name();
 
   RegisterOverride(feature_name, override_state, field_trial);
 }
 
+void FeatureList::AddFeaturesToAllocator(PersistentMemoryAllocator* allocator) {
+  DCHECK(initialized_);
+
+  for (const auto& override : overrides_) {
+    Pickle pickle;
+    pickle.WriteString(override.first);
+    if (override.second.field_trial)
+      pickle.WriteString(override.second.field_trial->trial_name());
+
+    size_t total_size = sizeof(FeatureEntry) + pickle.size();
+    PersistentMemoryAllocator::Reference ref =
+        allocator->Allocate(total_size, kFeatureType);
+    if (!ref)
+      return;
+
+    FeatureEntry* entry =
+        allocator->GetAsObject<FeatureEntry>(ref, kFeatureType);
+    entry->override_state = override.second.overridden_state;
+    entry->pickle_size = pickle.size();
+
+    char* dst = reinterpret_cast<char*>(entry) + sizeof(FeatureEntry);
+    memcpy(dst, pickle.data(), pickle.size());
+
+    allocator->MakeIterable(ref);
+  }
+}
+
 void FeatureList::GetFeatureOverrides(std::string* enable_overrides,
                                       std::string* disable_overrides) {
   DCHECK(initialized_);
 
   enable_overrides->clear();
   disable_overrides->clear();
 
   // Note: Since |overrides_| is a std::map, iteration will be in alphabetical
   // order. This not guaranteed to users of this function, but is useful for
   // tests to assume the order.
@@ skipping 196 matching lines @@
   return it->second == &feature;
 }
 
 FeatureList::OverrideEntry::OverrideEntry(OverrideState overridden_state,
                                           FieldTrial* field_trial)
     : overridden_state(overridden_state),
       field_trial(field_trial),
       overridden_by_field_trial(field_trial != nullptr) {}
 
 }  // namespace base