Restricting lifetime of python sync server on Windows via a JobObject.

Restricting lifetime of python sync server on Windows via a JobObject.

If a sync integration test runs for 30 seconds or more, OutOfProcTestRunner
forcibly kills the test case executable. This leaves an orphaned python
server instance in memory. When a subsequent test case is run, it
attempts to kick off a new python server and immediately checks for the
existence of a running server, following which it pumps messages to the
server instance. If the server detected by the test case happens to be
the old orphaned instance (that goes on to die when a new server
instance is started), test cases can fail.

This change list restricts the lifetime of the python.exe process started by
a test case such that if the test case is killed by OutOfProcTestRunner,
the python server instance it created dies with it.

In order to restrict the lifetime of a test server, it needs to be
started as a job. When a test server is spawned as a child process by a
test case that is running under a debugger, the child process needs to
be created using the CREATE_BREAKAWAY_FROM_JOB flag to first
disassociate it from the JobObject created by the debugger.

TestServerLauncher::Start() used to invoke base::LaunchApp() in order to
create the child process. This changelist implements a new method in
class TestServerLauncher called LaunchTestServerAsJob(). The call to
base::LaunchApp() in TestServerLauncher::Start() is replaced with a call
to TestServerLauncher::LaunchTestServerAsJob().

BUG=43777
TEST=sync_integration_tests

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

[Raghu Simha, 2010-05-28 00:13:16]

This is the starting point of this checkin. The "access denied" error message
that appears when you try to associate python with a JobObject needs to be
handled by creating a special manifest file. Nick and I will be collaborating on
this fix.

[Raghu Simha, 2010-05-28 04:53:38]

Patch set 4 addresses the ACCESS_DENIED errors that were seen when the code was
run under a debugger. I was able to get it to work under Windows 7 with UAC
fully turned on, without the use of any manifest file for python.exe.

[ncarter (slow), 2010-05-28 18:01:48]

http://codereview.chromium.org/2344001/diff/11001/12001
File net/socket/ssl_test_util.cc (right):

http://codereview.chromium.org/2344001/diff/11001/12001#newcode356
net/socket/ssl_test_util.cc:356: bool
TestServerLauncher::LaunchTestServerAsJob(const std::wstring& cmdline,
Better to make this a helper function, not a method, and to return a possibly
null handle.  Make the declaration Windows-only.  This will make it easier to
pull this into base, if it happens that somebody else wants to do this.

http://codereview.chromium.org/2344001/diff/11001/12001#newcode380
net/socket/ssl_test_util.cc:380: HANDLE hJob = CreateJobObject(NULL, NULL);
Give it a google-style name, like job_handle

http://codereview.chromium.org/2344001/diff/11001/12001#newcode385
net/socket/ssl_test_util.cc:385: JOBOBJECT_EXTENDED_LIMIT_INFORMATION jeli =
{0};
limit_info

http://codereview.chromium.org/2344001/diff/11001/12001#newcode397
net/socket/ssl_test_util.cc:397: }
We're basically leaking the job handle here (closing the job will kill the child
process).  It might be better to store it in a scoped_handle.  The net_unittests
are not in-proc browser tests; they'll start and stop several testservers during
the course of execution.  I don't know if there's a limit to the number of job
objects we can create, but it seems prudent to free what we allocate.

[M-A Ruel, 2010-05-28 19:21:43]

It'd be nice if you could reuse code, like src/sandbox/src/job.h

but I don't mind too much.

[Raghu Simha, 2010-05-28 22:39:44]

On 2010/05/28 19:21:43, Marc-Antoine Ruel wrote:
> It'd be nice if you could reuse code, like src/sandbox/src/job.h
> 
> but I don't mind too much.

Marc-Antoine,

I investigated the reuse of code within sandbox::Job in
net::TestServerLauncher::Start(), but this required making net dependent on
sandbox, and I take it this is undesirable.

I can see 3 options of how to solve this, and was wondering which one you
recommend:

1) Move sandbox::Job to base, and make net::TestServerLauncher::Start() reuse
the code in base.
2) Add a new (Windows only) function called LaunchAppAsJob() to namespace base
(in src\base\process_util.h) and replace calls to LaunchApp() with a call to
LaunchAppAsJob().
3) For the most part, stick with patch set 4, but make LaunchTestServerAsJob() a
helper function within namespace net (in src\net\socket\ssl_test_util.cc).

Let me know what you think.

Thanks.

[M-A Ruel, 2010-05-31 14:46:04]

You still haven't done Nick's comments which are relevant. Other than that lgtm.

[Raghu Simha, 2010-06-02 01:32:22]

On 2010/05/31 14:46:04, Marc-Antoine Ruel wrote:
> You still haven't done Nick's comments which are relevant. Other than that
lgtm.

Done. Patch set 5 has all of Nick's suggestions. Thanks.

[Raghu Simha, 2010-06-02 01:32:33]

http://codereview.chromium.org/2344001/diff/11001/12001
File net/socket/ssl_test_util.cc (right):

http://codereview.chromium.org/2344001/diff/11001/12001#newcode356
net/socket/ssl_test_util.cc:356: bool
TestServerLauncher::LaunchTestServerAsJob(const std::wstring& cmdline,
On 2010/05/28 18:01:48, nick wrote:
> Better to make this a helper function, not a method, and to return a possibly
> null handle.  Make the declaration Windows-only.  This will make it easier to
> pull this into base, if it happens that somebody else wants to do this.

Done.

I have stuck to the format in base::LaunchApp() and am returning the process
handle via a pointer argument. LaunchTestServerAsJob() returns a boolean, making
it consistent with base::LaunchApp() (and its variants).

http://codereview.chromium.org/2344001/diff/11001/12001#newcode380
net/socket/ssl_test_util.cc:380: HANDLE hJob = CreateJobObject(NULL, NULL);
On 2010/05/28 18:01:48, nick wrote:
> Give it a google-style name, like job_handle

Done.

http://codereview.chromium.org/2344001/diff/11001/12001#newcode385
net/socket/ssl_test_util.cc:385: JOBOBJECT_EXTENDED_LIMIT_INFORMATION jeli =
{0};
On 2010/05/28 18:01:48, nick wrote:
> limit_info

Done.

http://codereview.chromium.org/2344001/diff/11001/12001#newcode397
net/socket/ssl_test_util.cc:397: }
On 2010/05/28 18:01:48, nick wrote:
> We're basically leaking the job handle here (closing the job will kill the
child
> process).  It might be better to store it in a scoped_handle.  The
net_unittests
> are not in-proc browser tests; they'll start and stop several testservers
during
> the course of execution.  I don't know if there's a limit to the number of job
> objects we can create, but it seems prudent to free what we allocate.

Done.

[M-A Ruel, 2010-06-02 01:42:13]

lgtm with a small nit

http://codereview.chromium.org/2344001/diff/21001/22002
File net/socket/ssl_test_util.h (right):

http://codereview.chromium.org/2344001/diff/21001/22002#newcode143
net/socket/ssl_test_util.h:143: }
}  // namespace net

[Raghu Simha, 2010-06-02 01:48:32]

Done fixing small nit. Thanks :)

http://codereview.chromium.org/2344001/diff/21001/22002
File net/socket/ssl_test_util.h (right):

http://codereview.chromium.org/2344001/diff/21001/22002#newcode143
net/socket/ssl_test_util.h:143: }
On 2010/06/02 01:42:13, Marc-Antoine Ruel wrote:
> }  // namespace net

Done.

[ncarter (slow), 2010-06-02 18:42:03]

On 2010/06/02 01:48:32, rsimha wrote:
> Done fixing small nit. Thanks :)
> 
> http://codereview.chromium.org/2344001/diff/21001/22002
> File net/socket/ssl_test_util.h (right):
> 
> http://codereview.chromium.org/2344001/diff/21001/22002#newcode143
> net/socket/ssl_test_util.h:143: }
> On 2010/06/02 01:42:13, Marc-Antoine Ruel wrote:
> > }  // namespace net
> 
> Done.

LGTM