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"
@@ skipping 190 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,
     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* field_trial_switch_value,

[Alexei Svitkine (slow), 2017/05/05 18:02:16]

It's not the switch value - it's the switch name.... If you're just trying to
clarify this, suggest using switch_name instead. Or if it's part of an earlier
iteration that you decided to change your mind on, please revert. Either way,
please make sure the .cc changes are consistent with what's in the .h - which is
currently not the case with your CL.

[erikchen, 2017/05/05 19:41:51]

sed error.

     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_switch_value, 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::istringstream iss(switch_value);
+  std::string token;
+
+  std::getline(iss, token, ',');

[Avi (use Gerrit), 2017/05/05 04:30:12]

You use base::StringToInt, why not base::SplitString?

[Alexei Svitkine (slow), 2017/05/05 18:02:16]

+1 - let's use SplitString()

[erikchen, 2017/05/05 19:41:51]

Done.

+  int field_trial_handle = 0;
+  if (!base::StringToInt(token, &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);
+
+  std::getline(iss, token, ',');
+  uint64_t high = 0;
+  if (!base::StringToUint64(token, &high))
+    return SharedMemoryHandle();
+  std::getline(iss, token, ',');
+  uint64_t low = 0;
+  if (!base::StringToUint64(token, &low))
+    return SharedMemoryHandle();
+
+  base::UnguessableToken guid = base::UnguessableToken::Deserialize(high, low);
+  return SharedMemoryHandle(handle, guid);
+}
+
+// static
+bool FieldTrialList::CreateTrialsFromSwitchValue(
+    const std::string& switch_value) {
+  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) {

[Alexei Svitkine (slow), 2017/05/05 18:02:16]

How about unifying the code for the windows and posix versions? You can pass the
int fd params to both and just mention that it's not used on Windows. Then, I
think the only ifdef you need to have for win/non-win is the return line.

[erikchen, 2017/05/05 19:41:51]

I thought about this - there needs to be another ifdef for processing the handle
on windows, and now that we use split-string, some more changes w.r.t. counting
tokens. overall I think combining causes more confusion for not-that-more code
reuse and decided against it.

+  std::istringstream iss(switch_value);
+  std::string token;
+  std::getline(iss, token, ',');
+  uint64_t high = 0;
+  if (!base::StringToUint64(token, &high))
+    return SharedMemoryHandle();
+  std::getline(iss, token, ',');
+  uint64_t low = 0;
+  if (!base::StringToUint64(token, &low))
+    return SharedMemoryHandle();
+
+  base::UnguessableToken guid = base::UnguessableToken::Deserialize(high, low);
+  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