Store and retrieve features from shared memory

base/feature_list.cc

Index: base/feature_list.cc
diff --git a/base/feature_list.cc b/base/feature_list.cc
index 89b105defc6df0119688cb3b0e708f1b905f36df..38e632252c3b895f811cd440f390a50ff2e5600d 100644
--- a/base/feature_list.cc
+++ b/base/feature_list.cc
@@ -12,6 +12,7 @@
 #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"
 
@@ -27,6 +28,40 @@ 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 future. Same

[Alexei Svitkine (slow), 2016/12/01 20:05:52]

future -> feature

[bcwhite, 2016/12/01 20:15:14]

future => feature

[lawrencewu, 2016/12/02 19:24:46]

Done.

+  // values as the OverrideState enum in feature_list.h
+  uint32_t override_state;
+
+  // Size of the pickled structure, NOT the total size of this entry.

[Alexei Svitkine (slow), 2016/12/01 20:05:52]

Nit: Why not name it pickle_size then?

[lawrencewu, 2016/12/02 19:24:46]

Done.

+  uint32_t size;
+
+  bool GetFeatureAndTrialName(StringPiece* feature_name,

[Alexei Svitkine (slow), 2016/12/01 20:05:52]

Add a comment and document that it's only valid to call this on a FeatureEntry
that's in shared memory.

[lawrencewu, 2016/12/02 19:24:46]

Done.

+                              StringPiece* trial_name) const {
+    char* src = reinterpret_cast<char*>(const_cast<FeatureEntry*>(this)) +
+                sizeof(FeatureEntry);
+
+    Pickle pickle(src, 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.
+    if (!pickle_iter.ReadStringPiece(trial_name))

[bcwhite, 2016/12/01 20:15:15]

Just remove the if() and keep the comment as to why you're ignoring the return
code from pickle_iter.

[lawrencewu, 2016/12/02 19:24:46]

The function has warn_unused_result so I think I have to do something with the
return value.

[bcwhite, 2016/12/02 19:34:57]

// Need to return true because... so just ignore the return value.
auto sink = pickle_iter...

If that isn't sufficient, you can add another line:
(void)sink;

+      return true;

[Alexei Svitkine (slow), 2016/12/01 20:05:52]

Seems you can just simplify this to not have the if since you return true below
anyway.

[lawrencewu, 2016/12/02 19:24:46]

The function has warn_unused_result so I think I have to do something with the
return value.

+    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
@@ -56,6 +91,30 @@ void FeatureList::InitializeFromCommandLine(
   initialized_from_command_line_ = true;
 }
 
+void FeatureList::InitializeFromSharedMemory(
+    SharedPersistentMemoryAllocator* allocator) {
+  DCHECK(!initialized_);
+
+  SharedPersistentMemoryAllocator::Iterator iter(allocator);
+
+  SharedPersistentMemoryAllocator::Reference ref;
+  while ((ref = iter.GetNextOfType(kFeatureType)) !=
+         SharedPersistentMemoryAllocator::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;
+    entry->GetFeatureAndTrialName(&feature_name, &trial_name);

[Alexei Svitkine (slow), 2016/12/01 20:05:52]

Check return value?

[lawrencewu, 2016/12/02 19:24:46]

Not sure if we should return or continue on failure. I put a 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 {
@@ -98,6 +157,34 @@ void FeatureList::RegisterFieldTrialOverride(const std::string& feature_name,
   RegisterOverride(feature_name, override_state, field_trial);
 }
 
+void FeatureList::AddFeaturesToAllocator(
+    FieldTrialList::FieldTrialAllocator* 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();
+    SharedPersistentMemoryAllocator::Reference ref =
+        allocator->Allocate(total_size, kFeatureType);
+    if (!ref)
+      return;

[Alexei Svitkine (slow), 2016/12/01 20:05:52]

Seems like a bad thing to silently fail on.

[lawrencewu, 2016/12/02 19:24:46]

We do the same thing in field_trial.cc. Maybe TerminateOutOfMemory here and
CHECK(false) for NACL?

+
+    FeatureEntry* entry =
+        allocator->GetAsObject<FeatureEntry>(ref, kFeatureType);
+    entry->override_state = override.second.overridden_state;
+    entry->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_);