Pass the GUID for the SharedMemoryHandle used by base::FieldTrialList.

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/debug/activity_tracker.h"
 #include "base/logging.h"
 #include "base/metrics/field_trial_param_associator.h"
 #include "base/process/memory.h"
 #include "base/rand_util.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/unguessable_token.h"
 
 // On POSIX, the fd is shared using the mapping in GlobalDescriptors.
 #if defined(OS_POSIX) && !defined(OS_NACL)
 #include "base/posix/global_descriptors.h"
 #endif
 
@@ skipping 172 matching lines @@
     cmd_line->AppendSwitchASCII(disable_features_switch, disabled_features);
 
   std::string field_trial_states;
   FieldTrialList::AllStatesToString(&field_trial_states);
   if (!field_trial_states.empty()) {
     cmd_line->AppendSwitchASCII(switches::kForceFieldTrials,
                                 field_trial_states);
   }
 }
 
-#if defined(OS_WIN)
-HANDLE CreateReadOnlyHandle(FieldTrialList::FieldTrialAllocator* allocator) {
-  HANDLE src = allocator->shared_memory()->handle().GetHandle();
-  ProcessHandle process = GetCurrentProcess();
-  DWORD access = SECTION_MAP_READ | SECTION_QUERY;
-  HANDLE dst;
-  if (!::DuplicateHandle(process, src, process, &dst, access, true, 0))
-    return kInvalidPlatformFile;
-  return dst;
-}
-#endif
-
-#if defined(OS_POSIX) && !defined(OS_NACL)
-int CreateReadOnlyHandle(FieldTrialList::FieldTrialAllocator* allocator) {
-  SharedMemoryHandle handle = allocator->shared_memory()->GetReadOnlyHandle();
-  return SharedMemory::GetFdFromSharedMemoryHandle(handle);
-}
-#endif
-
 void OnOutOfMemory(size_t size) {
 #if defined(OS_NACL)
   NOTREACHED();
 #else
   TerminateBecauseOutOfMemory(size);
 #endif
 }
 
 }  // namespace
 
@@ skipping 528 matching lines @@
       // processes are properly reported in crash reports.
       trial->group();
     }
   }
   return true;
 }
 
 // static
 void FieldTrialList::CreateTrialsFromCommandLine(
     const CommandLine& cmd_line,
-    const char* field_trial_handle_switch,
+    const char* field_trial_switch_value,

[Alexei Svitkine (slow), 2017/05/05 22:37:50]

Nit: You still have a sed error here :P

[erikchen, 2017/05/08 23:37:49]

Done.

     int fd_key) {
   global_->create_trials_from_command_line_called_ = true;
 
 #if defined(OS_WIN)
-  if (cmd_line.HasSwitch(field_trial_handle_switch)) {
-    std::string handle_switch =
-        cmd_line.GetSwitchValueASCII(field_trial_handle_switch);
-    bool result = CreateTrialsFromHandleSwitch(handle_switch);
+  if (cmd_line.HasSwitch(field_trial_switch_value)) {
+    std::string switch_value =
+        cmd_line.GetSwitchValueASCII(field_trial_switch_value);
+    bool result = CreateTrialsFromSwitchValue(switch_value);
     DCHECK(result);
   }
 #endif
 
 #if defined(OS_POSIX) && !defined(OS_NACL)
   // On POSIX, we check if the handle is valid by seeing if the browser process
   // sent over the switch (we don't care about the value). Invalid handles
   // occur in some browser tests which don't initialize the allocator.
-  if (cmd_line.HasSwitch(field_trial_handle_switch)) {
-    bool result = CreateTrialsFromDescriptor(fd_key);
+  if (cmd_line.HasSwitch(field_trial_switch_value)) {
+    std::string switch_value =
+        cmd_line.GetSwitchValueASCII(field_trial_switch_value);
+    bool result = CreateTrialsFromDescriptor(fd_key, switch_value);
     DCHECK(result);
   }
 #endif
 
   if (cmd_line.HasSwitch(switches::kForceFieldTrials)) {
     bool result = FieldTrialList::CreateTrialsFromString(
         cmd_line.GetSwitchValueASCII(switches::kForceFieldTrials),
         std::set<std::string>());
     DCHECK(result);
   }
@@ skipping 18 matching lines @@
 }
 
 #if defined(OS_WIN)
 // static
 void FieldTrialList::AppendFieldTrialHandleIfNeeded(
     HandlesToInheritVector* handles) {
   if (!global_)
     return;
   if (kUseSharedMemoryForFieldTrials) {
     InstantiateFieldTrialAllocatorIfNeeded();
-    if (global_->readonly_allocator_handle_)
-      handles->push_back(global_->readonly_allocator_handle_);
+    if (global_->readonly_allocator_handle_.IsValid())
+      handles->push_back(global_->readonly_allocator_handle_.GetHandle());
   }
 }
 #endif
 
 #if defined(OS_POSIX) && !defined(OS_NACL)
 // static
-int FieldTrialList::GetFieldTrialHandle() {
+SharedMemoryHandle FieldTrialList::GetFieldTrialHandle() {
   if (global_ && kUseSharedMemoryForFieldTrials) {
     InstantiateFieldTrialAllocatorIfNeeded();
     // We check for an invalid handle where this gets called.
     return global_->readonly_allocator_handle_;
   }
-  return kInvalidPlatformFile;
+  return SharedMemoryHandle();
 }
 #endif
 
 // static
 void FieldTrialList::CopyFieldTrialStateToFlags(
     const char* field_trial_handle_switch,
     const char* enable_features_switch,
     const char* disable_features_switch,
     CommandLine* cmd_line) {
   // TODO(lawrencewu): Ideally, having the global would be guaranteed. However,
   // content browser tests currently don't create a FieldTrialList because they
   // don't run ChromeBrowserMainParts code where it's done for Chrome.
   // Some tests depend on the enable and disable features flag switch, though,
   // so we can still add those even though AllStatesToString() will be a no-op.
   if (!global_) {
     AddFeatureAndFieldTrialFlags(enable_features_switch,
                                  disable_features_switch, cmd_line);
     return;
   }
 
   // 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) {
     InstantiateFieldTrialAllocatorIfNeeded();
     // If the readonly handle didn't get duplicated properly, then fallback to
     // original behavior.
-    if (global_->readonly_allocator_handle_ == kInvalidPlatformFile) {
+    if (!global_->readonly_allocator_handle_.IsValid()) {
       AddFeatureAndFieldTrialFlags(enable_features_switch,
                                    disable_features_switch, cmd_line);
       return;
     }
 
     global_->field_trial_allocator_->UpdateTrackingHistograms();
-
-#if defined(OS_WIN)
-    // We need to pass a named anonymous handle to shared memory over the
-    // command line on Windows, since the child doesn't know which of the
-    // handles it inherited it should open.
-    // PlatformFile is typedef'd to HANDLE which is 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>(global_->readonly_allocator_handle_);
-    std::string field_trial_handle = std::to_string(uintptr_handle);
-    cmd_line->AppendSwitchASCII(field_trial_handle_switch, field_trial_handle);
-#elif defined(OS_POSIX)
-    // On POSIX, we dup the fd into a fixed fd kFieldTrialDescriptor, so we
-    // don't have to pass over the handle (it's not even the right handle
-    // anyways). But some browser tests don't create the allocator, so we need
-    // to be able to distinguish valid and invalid handles. We do that by just
-    // checking that the flag is set with a dummy value.
-    cmd_line->AppendSwitchASCII(field_trial_handle_switch, "1");
-#else
-#error Unsupported OS
-#endif
+    std::string switch_value = SerializeSharedMemoryHandleMetadata(
+        global_->readonly_allocator_handle_);
+    cmd_line->AppendSwitchASCII(field_trial_handle_switch, switch_value);
     return;
   }
 
   AddFeatureAndFieldTrialFlags(enable_features_switch, disable_features_switch,
                                cmd_line);
 }
 
 // static
 FieldTrial* FieldTrialList::CreateFieldTrial(
     const std::string& name,
@@ skipping 208 matching lines @@
   std::vector<const FieldTrial::FieldTrialEntry*> entries;
   FieldTrialAllocator::Iterator iter(&allocator);
   const FieldTrial::FieldTrialEntry* entry;
   while ((entry = iter.GetNextOfObject<FieldTrial::FieldTrialEntry>()) !=
          nullptr) {
     entries.push_back(entry);
   }
   return entries;
 }
 
+// static
+std::string FieldTrialList::SerializeSharedMemoryHandleMetadata(
+    const SharedMemoryHandle& shm) {
+  std::stringstream ss;
+#if defined(OS_WIN)
+  // Tell the child process the name of the inherited HANDLE.
+  uintptr_t uintptr_handle = reinterpret_cast<uintptr_t>(shm.GetHandle());
+  ss << uintptr_handle << ",";
+#elif !defined(OS_POSIX)
+#error Unsupported OS
+#endif
+
+  base::UnguessableToken guid = shm.GetGUID();
+  ss << guid.GetHighForSerialization() << "," << guid.GetLowForSerialization();
+  return ss.str();
+}
+
 #if defined(OS_WIN)
 // static
-bool FieldTrialList::CreateTrialsFromHandleSwitch(
-    const std::string& handle_switch) {
-  int field_trial_handle = std::stoi(handle_switch);
+SharedMemoryHandle FieldTrialList::DeserializeSharedMemoryHandleMetadata(
+    const std::string& switch_value) {
+  std::vector<std::string> tokens = base::SplitString(

[Alexei Svitkine (slow), 2017/05/05 22:37:50]

Nit: Use StringSplitPiece() version

[erikchen, 2017/05/08 23:37:49]

Done.

+      switch_value, ",", base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL);
+
+  if (tokens.size() != 3)
+    return SharedMemoryHandle();
+
+  int field_trial_handle = 0;
+  if (!base::StringToInt(tokens[0], &field_trial_handle))
+    return SharedMemoryHandle();
   HANDLE handle = reinterpret_cast<HANDLE>(field_trial_handle);
-  // TODO(erikchen): Plumb a GUID for this SharedMemoryHandle.
-  // https://crbug.com/713763.
-  SharedMemoryHandle shm_handle(handle, base::UnguessableToken::Create());
-  return FieldTrialList::CreateTrialsFromSharedMemoryHandle(shm_handle);
+
+  uint64_t high = 0;
+  if (!base::StringToUint64(tokens[1], &high))
+    return SharedMemoryHandle();
+  uint64_t low = 0;
+  if (!base::StringToUint64(tokens[2], &low))
+    return SharedMemoryHandle();
+
+  base::UnguessableToken guid = base::UnguessableToken::Deserialize(high, low);
+  return SharedMemoryHandle(handle, guid);
+}
+
+// static
+bool FieldTrialList::CreateTrialsFromSwitchValue(
+    const std::string& switch_value) {

[Alexei Svitkine (slow), 2017/05/05 22:37:50]

Please match the same order in .cc as in .h file - specifically have the two
Deserialize functions one after the other, rather than trying to group things
into the same ifdef.

[erikchen, 2017/05/08 23:37:49]

Done.

+  SharedMemoryHandle shm = DeserializeSharedMemoryHandleMetadata(switch_value);
+  if (!shm.IsValid())
+    return false;
+  return FieldTrialList::CreateTrialsFromSharedMemoryHandle(shm);
 }
 #endif
 
 #if defined(OS_POSIX) && !defined(OS_NACL)
 // static
-bool FieldTrialList::CreateTrialsFromDescriptor(int fd_key) {
+SharedMemoryHandle FieldTrialList::DeserializeSharedMemoryHandleMetadata(
+    int fd,
+    const std::string& switch_value) {
+  std::vector<std::string> tokens = base::SplitString(
+      switch_value, ",", base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL);
+
+  if (tokens.size() != 2)
+    return SharedMemoryHandle();
+  uint64_t high = 0;
+  if (!base::StringToUint64(tokens[0], &high))
+    return SharedMemoryHandle();
+  uint64_t low = 0;
+  if (!base::StringToUint64(tokens[1], &low))
+    return SharedMemoryHandle();

[Alexei Svitkine (slow), 2017/05/05 22:37:50]

Nit: I find the following a bit cleaner:

uint64_t high = 0;
uint64_t low = 0;
if (!base::StringToUint64(tokens[0], &high) ||
    !base::StringToUint64(tokens[1], &low)) {
  return SharedMemoryHandle();
}

[erikchen, 2017/05/08 23:37:49]

Done.

+
+  base::UnguessableToken guid = base::UnguessableToken::Deserialize(high, low);

[Alexei Svitkine (slow), 2017/05/05 22:37:51]

Suggest making a helper anon function to share this logic.

If you make the guid always first (i.e. tokens[0] and tokens[1]) then you can
call this helper from both the Windows and Posix impls and it can return the
guid.

This should avoid some of the duplication.

[erikchen, 2017/05/08 23:37:49]

Done.

+  return SharedMemoryHandle(FileDescriptor(fd, true), guid);
+}
+
+// static
+bool FieldTrialList::CreateTrialsFromDescriptor(
+    int fd_key,
+    const std::string& switch_value) {
   if (!kUseSharedMemoryForFieldTrials)
     return false;
 
   if (fd_key == -1)
     return false;
 
   int fd = GlobalDescriptors::GetInstance()->MaybeGet(fd_key);
   if (fd == -1)
     return false;
 
-  // TODO(erikchen): Plumb a GUID for this SharedMemoryHandle.
-  // https://crbug.com/713763.
-  SharedMemoryHandle shm_handle(FileDescriptor(fd, true),
-                                base::UnguessableToken::Create());
+  SharedMemoryHandle shm =
+      DeserializeSharedMemoryHandleMetadata(fd, switch_value);
+  if (!shm.IsValid())
+    return false;
 
-  bool result = FieldTrialList::CreateTrialsFromSharedMemoryHandle(shm_handle);
+  bool result = FieldTrialList::CreateTrialsFromSharedMemoryHandle(shm);
   DCHECK(result);
   return true;
 }
 #endif
 
 // static
 bool FieldTrialList::CreateTrialsFromSharedMemoryHandle(
     SharedMemoryHandle shm_handle) {
   // shm gets deleted when it gets out of scope, but that's OK because we need
   // it only for the duration of this method.
@@ skipping 70 matching lines @@
   }
 
   // Add all existing features.
   FeatureList::GetInstance()->AddFeaturesToAllocator(
       global_->field_trial_allocator_.get());
 
 #if !defined(OS_NACL)
   // Set |readonly_allocator_handle_| so we can pass it to be inherited and
   // via the command line.
   global_->readonly_allocator_handle_ =
-      CreateReadOnlyHandle(global_->field_trial_allocator_.get());
+      global_->field_trial_allocator_->shared_memory()->GetReadOnlyHandle();
 #endif
 }
 
 // static
 void FieldTrialList::AddToAllocatorWhileLocked(
     PersistentMemoryAllocator* allocator,
     FieldTrial* field_trial) {
   // Don't do anything if the allocator hasn't been instantiated yet.
   if (allocator == nullptr)
     return;
@@ skipping 91 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