Fix LocalTestServer shutdown on Windows

Fix LocalTestServer shutdown on Windows

Proper job shutdown for local test server (not just handle closing
which doesn't work on newer Windows).

R=phajdan.jr@chromium.org
BUG=

[Stanislav Albreht, 2014-04-23 06:12:42]

BTW, KillProcess() just below job killing in
net/test/spawned_test_server/local_test_server.cc doesn't seem to have much
sense on Windows because it kills cmd.exe but not python.exe, and I've seen it
staying alive after cmd.exe was killed (in case job termination failed).

[Paweł Hajdan Jr., 2014-04-23 10:49:22]

Thank you for the patch, really appreciated.

Adding Carlos (cpu) for Windows expertise.

Stanislav, could you explain more what's the bug? It can also be very useful for
future reference.

https://codereview.chromium.org/248903002/diff/1/base/process/kill_win.cc
File base/process/kill_win.cc (right):

https://codereview.chromium.org/248903002/diff/1/base/process/kill_win.cc#new...
base/process/kill_win.cc:109: // The processes may not end immediately due to
pending I/O
nit: Please end all comments with a dot.

https://codereview.chromium.org/248903002/diff/1/base/process/kill_win.cc#new...
base/process/kill_win.cc:111: DLOG_GETLASTERROR(ERROR) << "Error waiting for job
exit";
With this wait=true is effectively a no-op in case of timeouts.

How about this (and I see it mirrors KillProcess above, so maybe let's also
change KillProcess): actually return false in this case, and also allow callers
to pass a different timeout?

Feel free to maybe add another named function for KillProcess and deprecate
KillProcess (and finish the transition in follow-up CLs if possible).

[Stanislav Albreht, 2014-04-23 12:09:33]

Paweł, thank you for so prompt reply!

> Stanislav, could you explain more what's the bug? It can also be very useful
for
> future reference.

Well, I tried to run tests on Win8 and noticed that
ProxyScriptFetcherImplTest.NoCache from net_unittests failed (what it didn't do
on our Win2008s and 7). It seemed to be one of the tests where result of Stop()
is actually checked, and Stop() seemed to fail each time because KillProcess
failed ("Unable to terminate process: Access denied"). Same was with my other
Win8 machine. If I gave the test a couple of seconds after job handle closing,
it was able to kill both cmd.exe & python.exe and didn't enter "if (!ret)"
branch.

It looks like a race involving process termination (which is run by closing of
the job handle), checking that process is alive at one point
(WaitForSingleProcess()) and trying to kill it at another if it _was_ alive, so
I added a wait similar to what's done in KillProcess() but for a job. Perhaps
job closing became more time consuming on newer Wins or maybe behaves slightly
differently concerning killing processes inside the closing job, I don't know
for sure. Still, this approach looks like a more correct one disregarding the
platform.

>
https://codereview.chromium.org/248903002/diff/1/base/process/kill_win.cc#new...
> base/process/kill_win.cc:111: DLOG_GETLASTERROR(ERROR) << "Error waiting for
job
> exit";
> With this wait=true is effectively a no-op in case of timeouts.
> 
> How about this (and I see it mirrors KillProcess above, so maybe let's also
> change KillProcess): actually return false in this case, and also allow
callers
> to pass a different timeout?
> 
> Feel free to maybe add another named function for KillProcess and deprecate
> KillProcess (and finish the transition in follow-up CLs if possible).

If I deprecate the old KillProcess(), should I grep for its usages and change
them to the new version at once? Would overload of this function work for the
new version as well, I mean maybe use the same name?

[cpu_(ooo_6.6-7.5), 2014-04-24 03:47:37]

looks ok to me. I would have to grep the whole thing to have a full opinion but
bear in mind:

1- Win8 introduces nested job objects. Pre-win8 job objects don't nest so if the
os for some reason (like compat shim) puts a process in a job object you (we)
are out of luck. Win8 allows nesting so it is probably used a lot.

2- What we use in some other places is a job object with terminate-on-close set,
so basically closing the handle to the job object has the same effect as this
code. In fact we just leak the job object handle and when the parent process
dies the os closes all its handles including the job object handle which causes
the childs to be killed.

That approach is more robust in case the process just faults or somebody kills
it.