Revert 64070 - Revert 64065 - testserver.py listens on ephemeral ports by default.

If --port is specified on the command line, testserver.py will listen on that port, otherwise it will listen on an ephemeral port. If --startup_pipe is specified, the port number is written to the pipe as a 2 byte unsigned int in host order.

TestServer by default spawns testserver.py to listen on an ephemeral port and reads the port value from the pipe. If necessary, the port can still be fixed using TestServer::ForcePort, but that will hopefully get deprecated quickly.

BUG=56814
TEST=try bots pass

Review URL: http://codereview.chromium.org/3549003

TBR=cbentzel@chromium.org
Review URL: http://codereview.chromium.org/4165004

TBR=cbentzel@chromium.org
Review URL: http://codereview.chromium.org/4146008

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@64118 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 75 insertions, 69 deletions

diff --git a/net/test/test_server.cc b/net/test/test_server.cc
index 0b1cd085..99cb4a6 100644
--- a/net/test/test_server.cc
+++ b/net/test/test_server.cc
@@ -38,46 +38,6 @@ const int kServerConnectionAttempts = 10;
 // Connection timeout in milliseconds for tests.
 const int kServerConnectionTimeoutMs = 1000;
 
-const char kTestServerShardFlag[] = "test-server-shard";
-
-int GetPortBase(net::TestServer::Type type) {
-  switch (type) {
-    case net::TestServer::TYPE_FTP:
-      return 3117;
-    case net::TestServer::TYPE_HTTP:
-      return 1337;
-    case net::TestServer::TYPE_HTTPS:
-      return 9443;
-    case net::TestServer::TYPE_HTTPS_CLIENT_AUTH:
-      return 9543;
-    case net::TestServer::TYPE_HTTPS_EXPIRED_CERTIFICATE:
-      // TODO(phajdan.jr): Some tests rely on this hardcoded value.
-      // Some uses of this are actually in .html/.js files.
-      return 9666;
-    case net::TestServer::TYPE_HTTPS_MISMATCHED_HOSTNAME:
-      return 9643;
-    default:
-      NOTREACHED();
-  }
-  return -1;
-}
-
-int GetPort(net::TestServer::Type type) {
-  int port = GetPortBase(type);
-  if (CommandLine::ForCurrentProcess()->HasSwitch(kTestServerShardFlag)) {
-    std::string shard_str(CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
-                              kTestServerShardFlag));
-    int shard = -1;
-    if (base::StringToInt(shard_str, &shard)) {
-      port += shard;
-    } else {
-      LOG(FATAL) << "Got invalid " << kTestServerShardFlag << " flag value. "
-                 << "An integer is expected.";
-    }
-  }
-  return port;
-}
-
 std::string GetHostname(net::TestServer::Type type) {
   if (type == net::TestServer::TYPE_HTTPS_MISMATCHED_HOSTNAME) {
     // Return a different hostname string that resolves to the same hostname.
@@ -96,9 +56,10 @@ void SetMacTestCertificate(X509Certificate* cert);
 #endif
 
 TestServer::TestServer(Type type, const FilePath& document_root)
-    : host_port_pair_(GetHostname(type), GetPort(type)),
+    : host_port_pair_(GetHostname(type), 0),
       process_handle_(base::kNullProcessHandle),
-      type_(type) {
+      type_(type),
+      started_(false) {
   FilePath src_dir;
   PathService::Get(base::DIR_SOURCE_ROOT, &src_dir);
 
@@ -148,6 +109,7 @@ bool TestServer::Start() {
     return false;
   }
 
+  started_ = true;
   return true;
 }
 
@@ -155,6 +117,8 @@ bool TestServer::Stop() {
   if (!process_handle_)
     return true;
 
+  started_ = false;
+
   // First check if the process has already terminated.
   bool ret = base::WaitForSingleProcess(process_handle_, 0);
   if (!ret)
@@ -170,6 +134,11 @@ bool TestServer::Stop() {
   return ret;
 }
 
+const HostPortPair& TestServer::host_port_pair() const {
+  DCHECK(started_);
+  return host_port_pair_;
+}
+
 std::string TestServer::GetScheme() const {
   switch (type_) {
     case TYPE_FTP:
diff --git a/net/test/test_server.h b/net/test/test_server.h
index 4e68fd9..2ee801e 100644
--- a/net/test/test_server.h
+++ b/net/test/test_server.h
@@ -53,7 +53,7 @@ class TestServer {
   bool Stop();
 
   const FilePath& document_root() const { return document_root_; }
-  const HostPortPair& host_port_pair() const { return host_port_pair_; }
+  const HostPortPair& host_port_pair() const;
   std::string GetScheme() const;
   bool GetAddressList(AddressList* address_list) const WARN_UNUSED_RESULT;
 
@@ -121,6 +121,9 @@ class TestServer {
 
   Type type_;
 
+  // Has the server been started?
+  bool started_;
+
   DISALLOW_COPY_AND_ASSIGN(TestServer);
 };
 
diff --git a/net/test/test_server_posix.cc b/net/test/test_server_posix.cc
index 1456ac8..17b2631 100644
--- a/net/test/test_server_posix.cc
+++ b/net/test/test_server_posix.cc
@@ -110,24 +110,37 @@ bool TestServer::LaunchPython(const FilePath& testserver_path) {
 }
 
 bool TestServer::WaitToStart() {
-  struct pollfd poll_fds[1];
-
-  poll_fds[0].fd = child_fd_;
-  poll_fds[0].events = POLLIN | POLLPRI;
-  poll_fds[0].revents = 0;
+  uint16 port;
+  uint8* buffer = reinterpret_cast<uint8*>(&port);
+  ssize_t bytes_read = 0;
+  ssize_t bytes_max = sizeof(port);
+  while (bytes_read < bytes_max) {
+    struct pollfd poll_fds[1];
+
+    poll_fds[0].fd = child_fd_;
+    poll_fds[0].events = POLLIN | POLLPRI;
+    poll_fds[0].revents = 0;
+
+    int rv = HANDLE_EINTR(poll(poll_fds, 1,
+                               TestTimeouts::action_max_timeout_ms()));
+    if (rv != 1) {
+      LOG(ERROR) << "Failed to poll for the child file descriptor.";
+      return false;
+    }
 
-  int rv = HANDLE_EINTR(poll(poll_fds, 1,
-                             TestTimeouts::action_max_timeout_ms()));
-  if (rv != 1) {
-    LOG(ERROR) << "Failed to poll for the child file descriptor.";
-    return false;
+    ssize_t num_bytes = HANDLE_EINTR(read(child_fd_, buffer + bytes_read,
+                                          bytes_max - bytes_read));
+    if (num_bytes <= 0)
+      break;
+    bytes_read += num_bytes;
   }
 
-  char buf[8];
-  ssize_t n = HANDLE_EINTR(read(child_fd_, buf, sizeof(buf)));
   // We don't need the FD anymore.
   child_fd_closer_.reset(NULL);
-  return n > 0;
+  if (bytes_read < bytes_max)
+    return false;
+  host_port_pair_.set_port(port);
+  return true;
 }
 
 bool TestServer::CheckCATrusted() {
diff --git a/net/test/test_server_win.cc b/net/test/test_server_win.cc
index a8b3678..cfc7178 100644
--- a/net/test/test_server_win.cc
+++ b/net/test/test_server_win.cc
@@ -149,11 +149,24 @@ bool TestServer::LaunchPython(const FilePath& testserver_path) {
 }
 
 bool TestServer::WaitToStart() {
-  char buf[8];
-  DWORD bytes_read;
-  BOOL result = ReadFile(child_fd_, buf, sizeof(buf), &bytes_read, NULL);
+  uint16 port;
+  uint8* buffer = reinterpret_cast<uint8*>(&port);
+  DWORD bytes_read = 0;
+  DWORD bytes_max = sizeof(port);
+  while (bytes_read < bytes_max) {
+    DWORD num_bytes;
+    if (!ReadFile(child_fd_, buffer + bytes_read, bytes_max - bytes_read,
+                  &num_bytes, NULL))
+      break;
+    if (num_bytes <= 0)
+      break;
+    bytes_read += num_bytes;
+  }
   child_fd_.Close();
-  return result && bytes_read > 0;
+  if (bytes_read < bytes_max)
+    return false;
+  host_port_pair_.set_port(port);
+  return true;
 }
 
 bool TestServer::CheckCATrusted() {
diff --git a/net/tools/testserver/testserver.py b/net/tools/testserver/testserver.py
index c3fe86b..7604de6 100644
--- a/net/tools/testserver/testserver.py
+++ b/net/tools/testserver/testserver.py
@@ -6,7 +6,9 @@
 """This is a simple HTTP server used for testing Chrome.
 
 It supports several test URLs, as specified by the handlers in TestPageHandler.
-It defaults to living on localhost:8888.
+By default, it listens on an ephemeral port and sends the port number back to
+the originating process over a pipe. The originating process can specify an
+explicit port if necessary.
 It can use https if you specify the flag --https=CERT where CERT is the path
 to a pem file containing the certificate and private key that should be used.
 """
@@ -20,6 +22,7 @@ import re
 import shutil
 import SocketServer
 import sys
+import struct
 import time
 import urllib2
 import warnings
@@ -496,7 +499,7 @@ class TestPageHandler(BaseHTTPServer.BaseHTTPRequestHandler):
       'pre { border: 1px solid black; margin: 5px; padding: 5px }'
       '</style></head><body>'
       '<div style="float: right">'
-      '<a href="http://localhost:8888/echo">back to referring page</a></div>'
+      '<a href="/echo">back to referring page</a></div>'
       '<h1>Request Body:</h1><pre>')
 
     if self.command == 'POST' or self.command == 'PUT':
@@ -1170,15 +1173,15 @@ def main(options, args):
           return
       server = HTTPSServer(('127.0.0.1', port), TestPageHandler, options.cert,
                            options.ssl_client_auth, options.ssl_client_ca)
-      print 'HTTPS server started on port %d...' % port
+      print 'HTTPS server started on port %d...' % server.server_port
     else:
       server = StoppableHTTPServer(('127.0.0.1', port), TestPageHandler)
-      print 'HTTP server started on port %d...' % port
+      print 'HTTP server started on port %d...' % server.server_port
 
     server.data_dir = MakeDataDir()
     server.file_root_url = options.file_root_url
     server._sync_handler = None
-
+    listen_port = server.server_port
   # means FTP Server
   else:
     my_data_dir = MakeDataDir()
@@ -1203,7 +1206,8 @@ def main(options, args):
     # Instantiate FTP server class and listen to 127.0.0.1:port
     address = ('127.0.0.1', port)
     server = pyftpdlib.ftpserver.FTPServer(address, ftp_handler)
-    print 'FTP server started on port %d...' % port
+    listen_port = server.socket.getsockname()[1]
+    print 'FTP server started on port %d...' % listen_port
 
   # Notify the parent that we've started. (BaseServer subclasses
   # bind their sockets on construction.)
@@ -1213,7 +1217,10 @@ def main(options, args):
     else:
       fd = options.startup_pipe
     startup_pipe = os.fdopen(fd, "w")
-    startup_pipe.write("READY")
+    # Write the listening port as a 2 byte value. This is _not_ using
+    # network byte ordering since the other end of the pipe is on the same
+    # machine.
+    startup_pipe.write(struct.pack('@H', listen_port))
     startup_pipe.close()
 
   try:
@@ -1228,8 +1235,9 @@ if __name__ == '__main__':
                            const=SERVER_FTP, default=SERVER_HTTP,
                            dest='server_type',
                            help='FTP or HTTP server: default is HTTP.')
-  option_parser.add_option('', '--port', default='8888', type='int',
-                           help='Port used by the server.')
+  option_parser.add_option('', '--port', default='0', type='int',
+                           help='Port used by the server. If unspecified, the '
+                           'server will listen on an ephemeral port.')
   option_parser.add_option('', '--data-dir', dest='data_dir',
                            help='Directory from which to read the files.')
   option_parser.add_option('', '--https', dest='cert',