Multiprocess test client: Android child process launcher rework.

base/posix/global_descriptors.h

Index: base/posix/global_descriptors.h
diff --git a/base/posix/global_descriptors.h b/base/posix/global_descriptors.h
index edb299de5c957cc36f5fbd0ae85a056051012ba9..198bf468524c6964963edff20f81c22a0ae13265 100644
--- a/base/posix/global_descriptors.h
+++ b/base/posix/global_descriptors.h
@@ -52,18 +52,7 @@ class BASE_EXPORT GlobalDescriptors {
 
   // Often we want a canonical descriptor for a given Key. In this case, we add
   // the following constant to the key value:
-#if !defined(OS_ANDROID)
   static const int kBaseDescriptor = 3;  // 0, 1, 2 are already taken.
-#else
-  // 3 used by __android_log_write().
-  // 4 used by... something important on Android M.
-  // 5 used by... something important on Android L... on low-end devices.
-  // TODO(amistry): An Android, this mechanism is only used for tests since the
-  // content child launcher spawns a process by creating a new Activity using
-  // the Android APIs. For tests, come up with a way that doesn't require using
-  // a pre-defined fd.
-  static const int kBaseDescriptor = 6;
-#endif
 
   // Return the singleton instance of GlobalDescriptors.
   static GlobalDescriptors* GetInstance();
@@ -84,6 +73,9 @@ class BASE_EXPORT GlobalDescriptors {
   // Set the descriptor and |region| for the given |key|.
   void Set(Key key, int fd, base::MemoryMappedFile::Region region);
 
+  // Generates a random key and uses it to set the descriptor.

[Robert Sesek, 2016/12/20 20:44:34]

This seems like it could be confusing unless Register is always the last of the
Set operations that is performed, especially since Set does not return whether
or not a replacement happened. Random may very well return 4, 5, or 6 some
times, which could break things that use kBaseDescriptor + value. I also think
that it could make debugging hard if it ever did occur, since it would be
non-deterministic.

Is there a reason why this needs to be done this way, as opposed to just
choosing a kBaseDescriptor+N like the other users of this class?

[Jay Civelli, 2016/12/20 23:07:26]

What I don't like about the kBaseDescriptor+N approach is that it becomes hard
to maintain with several modules, which then need to all agree on their base
descriptor. (and with servicification we'll have to address it some generic way)
Here I am doing some of what is suggested in the comment at the top of the class
by choosing randomly. But I agree this can cause problems with collision with
calls to Set().
Would it work for you if I gave the generated keys a high range and check if set
is called with anything in that range?

[Robert Sesek, 2016/12/21 19:26:07]

I agree that this could start to become a problem with servicification, but I
think tackling that problem is probably best done on its own, rather than ad-hoc
in this CL. For example, it may be possible, with a little refactoring, to have
GlobalDescriptors assign the keys and return them from Set, rather than letting
the clients specify.
I think what the comment is getting at something like this (a static random
value as opposed to dynamically choosing one:
https://cs.chromium.org/chromium/src/sandbox/linux/seccomp-bpf/sandbox_bpf.cc...).
I'd prefer to not add this new method and to continue using GlobalDescriptors
the way it is used elsewhere.

[Jay Civelli, 2016/12/21 22:26:22]

Sounds good, now using a fixed key.
I will try to look at what you suggested (having Set() return the key) in a
future CL.

+  Key Register(int fd);
+
   void Reset(const Mapping& mapping);
 
  private: