Multiprocess test client: Android child process launcher rework.

base/posix/global_descriptors.h

 // 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.
 
 #ifndef BASE_POSIX_GLOBAL_DESCRIPTORS_H_
 #define BASE_POSIX_GLOBAL_DESCRIPTORS_H_
 
 #include "build/build_config.h"
 
 #include <vector>
@@ skipping 34 matching lines @@
     Key key;
     // Actual FD.
     int fd;
     // Optional region, defaults to kWholeFile.
     base::MemoryMappedFile::Region region;
   };
   typedef std::vector<Descriptor> Mapping;
 
   // 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();
 
   // Get a descriptor given a key. It is a fatal error if the key is not known.
   int Get(Key key) const;
 
   // Get a descriptor given a key. Returns -1 on error.
   int MaybeGet(Key key) const;
 
   // Get a region given a key. It is a fatal error if the key is not known.
   base::MemoryMappedFile::Region GetRegion(Key key) const;
 
   // Set the descriptor for the given |key|. This sets the region associated
   // with |key| to kWholeFile.
   void Set(Key key, int fd);
 
   // 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:
   friend struct DefaultSingletonTraits<GlobalDescriptors>;
   GlobalDescriptors();
   ~GlobalDescriptors();
 
   Mapping descriptors_;
 };
 
 }  // namespace base
 
 #endif  // BASE_POSIX_GLOBAL_DESCRIPTORS_H_