Prototype implementation of zygotes

Prototype implementation of zygotes.  
Limitations that need addressing still:
- Doesn't forcibly terminate children that should have exited but haven't

Enable with env var ENABLE_ZYGOTE_MANAGER=1.

BUG=11841
TEST=
start the browser, then make chrome and all .pak files unreadable; or alternately, start an installed browser, and uninstall the browser while it's running.  Then create a new tab and browse to two new sites. 

Here's an example script to hide and unhide the .pak files (note: do not move the directory they're in, that doesn't work):

#!/bin/sh
chmod_all() {
   chmod $1 sconsbuild/Debug/chrome
   for path in . locales obj/chrome/app/intermediate/repack obj/global_intermediate/* themes
   do
     chmod $1 sconsbuild/Debug/$path/*.pak
   done
}
case $1 in
hide) chmod_all 000 ;;
show) chmod_all 755 ;;
esac



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=17840

[agl, 2009-05-27 00:41:55]

Only dealing with high level stuff:

There should be two socketpairs in play. Both should be UNIX
DGRAM sockets to deal with framing issues.

The first should be exclusively between the zygote and the
browser process.  Thus, no locking is needed.

The second should be installed in each child for dealing
with the OpenFile.  Since it's a DGRAM socket, the child can
all write to it without any danger of mixing messages. The
OpenFile request should pass one end of a UNIX DGRAM to the
browser. The browser should then write the result to this
descriptor. That way, each child gets the right reply
without having to lock.


AGL

http://codereview.chromium.org/115773/diff/1/8
File base/zygote_manager.cc (right):

http://codereview.chromium.org/115773/diff/1/8#newcode22
Line 22: const char ZygoteManager::kZMagic[] = "zygo";
static?

[dank, 2009-05-29 18:18:00]

OK, this is a lot closer to real.  It still uses the single socketpair,
and locking, but you indicated in email you might be ok with that.

[dank, 2009-05-29 19:54:51]

Now works even if you start the app with a relative path.

[agl, 2009-06-01 18:26:32]

I got through most of zygote_manager.cc, but this code is not ready for review.
Mostly you need to do a cleanup pass and fix the logging at least.

http://codereview.chromium.org/115773/diff/93/1066
File base/command_line.cc (right):

http://codereview.chromium.org/115773/diff/93/1066#newcode129
Line 129: FilePath program(argv_[0]);
argv[0] is just the name of the current binary. Here's an example of bash
running ls:
  execve("/bin/ls", ["ls"], [/* 47 vars */]) = 0

argv[0] is "ls", which doesn't include the path to the binary, nor is it
relative to the current directory (which was '/tmp' in the example above). One
can get the path to the current binary via readlink'ing "/proc/self/exe".

http://codereview.chromium.org/115773/diff/93/1068
File base/file_util_posix.cc (right):

http://codereview.chromium.org/115773/diff/93/1068#newcode631
Line 631: base::ZygoteManager *zm = base::ZygoteManager::Get();
* on the left.

http://codereview.chromium.org/115773/diff/93/1068#newcode649
Line 649: LOG(ERROR) << "Couldn't fstat " << file_name.value();
If you're going to log, you should include errno or strerror(errno).

http://codereview.chromium.org/115773/diff/93/1068#newcode658
Line 658: if (data_ == NULL)
Merge into the conditional above and include errno or strerror(errno).

http://codereview.chromium.org/115773/diff/93/1069
File base/process_util_linux.cc (right):

http://codereview.chromium.org/115773/diff/93/1069#newcode54
Line 54: LOG(INFO) << "Can use zygote if " << argv[0] << " == " << argv0;
Needs to be removed before landing.

http://codereview.chromium.org/115773/diff/93/1069#newcode55
Line 55: if (argv[0] == argv0) {
I don't really like this magic exception here. Why should argv[0] be absolute?
Where is this requirement documented? It would appear that we need an other
function (Relaunch?) which explicitly runs the current app and falls back to the
original LaunchApp on OSX and Windows.

http://codereview.chromium.org/115773/diff/93/1069#newcode82
Line 82: LOG(ERROR) << "LaunchApp: exec failed!, argv_cstr0 " << argv_cstr[0];
errno

http://codereview.chromium.org/115773/diff/93/1062
File base/zygote_manager.cc (right):

http://codereview.chromium.org/115773/diff/93/1062#newcode1
Line 1: #include "base/zygote_manager.h"
The functions in this file are called from a variety of contexts. Each needs a
comment above it saying where it's called from.

http://codereview.chromium.org/115773/diff/93/1062#newcode31
Line 31: LOG(INFO) << __FUNCTION__;
Needs to be removed before landing.

http://codereview.chromium.org/115773/diff/93/1062#newcode45
Line 45: // Posix doesn't wake poll() when a peer does close().
Posix -> POSIX.

This is only in the case of DGRAM or SEQPACKET sockets. Use a STREAM socket and
it'll clean up a bunch of code around the poll() below too.

http://codereview.chromium.org/115773/diff/93/1062#newcode49
Line 49: shutdown(canary_fd_, 2);
2 -> SHUT_RDWR

http://codereview.chromium.org/115773/diff/93/1062#newcode71
Line 71: zm = Singleton<ZygoteManager>::get();
merge with previous line.

http://codereview.chromium.org/115773/diff/93/1062#newcode75
Line 75: #define Lock(fd) do { \
These should be functions.

http://codereview.chromium.org/115773/diff/93/1062#newcode91
Line 91: pid_t clientpid = (pid_t) -1;
This initialisation seems wasted.

http://codereview.chromium.org/115773/diff/93/1062#newcode101
Line 101: int kind = -1;
likewise.

http://codereview.chromium.org/115773/diff/93/1062#newcode158
Line 158: Pickle* reply, std::vector<std::string>** newargv) {
indentation.

http://codereview.chromium.org/115773/diff/93/1062#newcode163
Line 163: const file_handle_mapping_vector& fds_to_remap) {
indentation.

http://codereview.chromium.org/115773/diff/93/1062#newcode250
Line 250: request.ReadInt(&iter, &argc);
You catch all the other errors, but not here.

http://codereview.chromium.org/115773/diff/93/1062#newcode255
Line 255: reply->WriteInt(-1);
You should move this error behaviour to the bottom of the function and use a
goto.

http://codereview.chromium.org/115773/diff/93/1062#newcode264
Line 264: request.ReadInt(&iter, &numfds);
and here

http://codereview.chromium.org/115773/diff/93/1062#newcode265
Line 265: DCHECK(numfds == num_wire_fds);
DCHECK_EQ

http://codereview.chromium.org/115773/diff/93/1062#newcode290
Line 290: it = fds_to_remap.begin(); it != fds_to_remap.end(); ++it) {
indentation.

http://codereview.chromium.org/115773/diff/93/1062#newcode322
Line 322: // Hope there are no clashes.
DCHECK that there are no clashes. I expect there probably will be at some point.

http://codereview.chromium.org/115773/diff/93/1062#newcode372
Line 372: if (filename.compare(0, strlen(allowed_prefix), allowed_prefix) != 0)
{
need to check that filename.size() >= 1

http://codereview.chromium.org/115773/diff/93/1062#newcode376
Line 376: static const char* allowed_suffix = ".pak";
need to check that filename.size() >= 4

http://codereview.chromium.org/115773/diff/93/1062#newcode390
Line 390: if (filename.compare(0, strlen(*p), *p) == 0) {
need to check the size first.

http://codereview.chromium.org/115773/diff/93/1062#newcode398
Line 398: int ZygoteManager::OpenFile(std::string filename) {
const &

http://codereview.chromium.org/115773/diff/93/1062#newcode473
Line 473: LOG(ERROR) << "reply wrong kind " << kind;
In these error cases you need to close the descriptors from the cmsg. Probably
use a goto.

http://codereview.chromium.org/115773/diff/93/1062#newcode520
Line 520: LOG(ERROR) << "can't stat";
filename, errno.

http://codereview.chromium.org/115773/diff/93/1062#newcode524
Line 524: LOG(ERROR) << "not regular file";
filename

http://codereview.chromium.org/115773/diff/93/1063
File base/zygote_manager.h (right):

http://codereview.chromium.org/115773/diff/93/1063#newcode22
Line 22: #if defined(OS_LINUX)
Might as well move the #if up to save compile time on other platforms. (Not to
mention that Windows will probably fail to include some of those files.)

http://codereview.chromium.org/115773/diff/93/1063#newcode42
Line 42: // Returns once in original process, and
line width issues.

http://codereview.chromium.org/115773/diff/93/1063#newcode53
Line 53: const file_handle_mapping_vector& fds_to_remap);
indentation

http://codereview.chromium.org/115773/diff/93/1063#newcode68
Line 68: int OpenFile(std::string filename);
const &

[dank, 2009-06-04 05:15:21]

Thanks for the good partial review.  I've addressed some of the
interesting bits.   I've been out sick a lot since last Thursday,
but I hope to have the rest of your issues taken care of tomorrow.

[dank, 2009-06-05 06:00:32]

I think I've addressed your comments, except for the bit about
simplifying poll() result handling; I think I made it more correct,
but didn't see how to simplify it.

Other changes since last try include:
- now reaps promptly, thanks to sigchld
- preserves logging file descriptors

[agl, 2009-06-05 20:53:52]

I didn't see reversed_file_descriptors.h in the CL.

This looks landable at this state, although there are obviously several TODOs
left in there.

LGTM.

http://codereview.chromium.org/115773/diff/150/157
File base/file_descriptor_shuffle.cc (right):

http://codereview.chromium.org/115773/diff/150/157#newcode26
Line 26: LOG(ERROR) << "Uh-oh.  Both fd " << i->source << " and " << j->source
<< " want to map to " << i->dest;
Hmm. If this actually happens, it's a bug in other parts of the code, which is
why I made it a DCHECK. I feel that either it should stay a DCHECK (in which
case you can DCHECK(x) << "Message here") or you should make it a check?

http://codereview.chromium.org/115773/diff/150/159
File base/logging.cc (right):

http://codereview.chromium.org/115773/diff/150/159#newcode215
Line 215: static const int kReservedFds = 5;
The idea of reserved file descriptor number doesn't seem to work very well with
the zygote model. Every time someone adds some code to open a file, they'll need
these sorts of tricks.

You can totally punt this to a followup CL if you like, but I think the way
we'll want it to work is that we have a indirection table somewhere:

enum GlobalFDTypes {
  GLOBAL_FD_IPC,
  GLOBAL_FD_SANDBOX,
  GLOBAL_FD_CRASH_SIGNAL,
}

class SingletonThing {
  map<GlobalFDType, int> global_fds;
  SetGlobalFD(GlobalFDType, int);
  GetGlobalFD(GlobalFDType);
}

Then the spawn message to the zygote process contains a list of GlobalFDType
values which match up with the array of fds from the control message.

Of course, this object needs to be read from the crash dumping code so it'll
have to be pretty carefully written. (e.g., one probably cannot actually use STL
objects).

http://codereview.chromium.org/115773/diff/150/159#newcode236
Line 236: if (log_file)
Just a reminder to thing about locking etc. I don't know what the semantics of
this global are.

http://codereview.chromium.org/115773/diff/150/161
File base/process_util.h (right):

http://codereview.chromium.org/115773/diff/150/161#newcode140
Line 140: const file_handle_mapping_vector& fds_to_remap,
Indentation

http://codereview.chromium.org/115773/diff/150/162
File base/process_util_linux.cc (right):

http://codereview.chromium.org/115773/diff/150/162#newcode47
Line 47: const file_handle_mapping_vector& fds_to_remap,
indentation. (These might just be Rietveld issues)

http://codereview.chromium.org/115773/diff/150/162#newcode49
Line 49: pid_t pid = -1;
remove line.

http://codereview.chromium.org/115773/diff/150/162#newcode54
Line 54: pid = zm->LongFork(argv, fds_to_remap);
const pid_t pid = ...

http://codereview.chromium.org/115773/diff/150/151
File base/zygote_manager.cc (right):

http://codereview.chromium.org/115773/diff/150/151#newcode1
Line 1: #include "base/zygote_manager.h"
Needs copyright gubbins

http://codereview.chromium.org/115773/diff/150/151#newcode26
Line 26: namespace {
You can save five lines if you make the function static :)

http://codereview.chromium.org/115773/diff/150/151#newcode63
Line 63: // If, when we later try to fork a chrome, and for some reason the path
Is this comment still true?

http://codereview.chromium.org/115773/diff/150/151#newcode100
Line 100: // Returns true on success, places round trip time in delta
If would be very helpful to include comments above these functions saying in
which progress they're run.

http://codereview.chromium.org/115773/diff/150/151#newcode375
Line 375: if (filename.compare(0, strlen(allowed_prefix), allowed_prefix) != 0)
{
Need to check that filename is long enough.

http://codereview.chromium.org/115773/diff/150/151#newcode394
Line 394: if (filename.compare(0, strlen(*p), *p) == 0) {
Still need to check that the filename is long enough.

http://codereview.chromium.org/115773/diff/150/151#newcode644
Line 644: LOG(ERROR) << "msg didn't start with " << kZMagic << ", got " <<
magic;
In the error cases here you need to close the descriptors from the control
message.

http://codereview.chromium.org/115773/diff/150/152
File base/zygote_manager.h (right):

http://codereview.chromium.org/115773/diff/150/152#newcode32
Line 32: ZygoteManager() : server_fd_(-1), client_fd_(-1), canary_fd_(-1),
style:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Class_Format

http://codereview.chromium.org/115773/diff/150/163
File chrome/app/breakpad_linux.cc (right):

http://codereview.chromium.org/115773/diff/150/163#newcode24
Line 24: #include "chrome/common/reserved_file_descriptors.h"
What's this used for? There are no other changes in the file.

[dank, 2009-06-06 00:13:05]

OK, took care of all those issues, I think.
Changed the environment variable check to a DCHECK.

[TVL, 2009-06-08 03:04:15]

I noticed a small bump in the Mac executable size this weekend, so I was looking
at what landed.

Should some more of the code with POSIX ifdefs really be within LINUX checks
since I don't think any of this was intended for the Mac builds? (number of
reserved channels, etc.)

[dank, 2009-06-08 03:35:28]

I'm at a loss to explain the size change.  40KB seems excessive for the
bits that were not in #ifdef OS_LINUX.  I'll fix logging.cc, but I can't
imagine those seven lines of code could explain 30-40KB.  Is debugging
info included in those sizes?

Maybe our daily builds should include size info for each of our .o's...

[TVL, 2009-06-08 12:22:12]

On 2009/06/08 03:35:28, dank wrote:
> I'm at a loss to explain the size change.  40KB seems excessive for the
> bits that were not in #ifdef OS_LINUX.  I'll fix logging.cc, but I can't
> imagine those seven lines of code could explain 30-40KB.  Is debugging
> info included in those sizes?
> 
> Maybe our daily builds should include size info for each of our .o's...

I thought it was only 4k, i'll take a peek again today.