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

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 78602f1e613d39f00020dea6f1f18f093bcfa72f..b4196d8c1f55acf674588d57692020f534f0a934 100644
--- a/base/metrics/field_trial.cc
+++ b/base/metrics/field_trial.cc
@@ -208,25 +208,6 @@ void AddFeatureAndFieldTrialFlags(const char* enable_features_switch,
   }
 }
 
-#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();
@@ -775,15 +756,15 @@ bool FieldTrialList::CreateTrialsFromString(
 // 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
@@ -792,8 +773,10 @@ void FieldTrialList::CreateTrialsFromCommandLine(
   // 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
@@ -832,27 +815,27 @@ void FieldTrialList::AppendFieldTrialHandleIfNeeded(
     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) {
@@ -873,37 +856,16 @@ void FieldTrialList::CopyFieldTrialStateToFlags(
     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;
   }
 
@@ -1132,22 +1094,83 @@ FieldTrialList::GetAllFieldTrialsFromPersistentAllocator(
   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;
 
@@ -1158,12 +1181,12 @@ bool FieldTrialList::CreateTrialsFromDescriptor(int 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;
 }
@@ -1254,7 +1277,7 @@ void FieldTrialList::InstantiateFieldTrialAllocatorIfNeeded() {
   // 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
 }