Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(229)

Issue 248903002: Fix LocalTestServer shutdown on Windows (Closed)

Created:
6 years, 8 months ago by Stanislav Albreht
Modified:
6 years, 7 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, erikwright+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

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=

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+25 lines, -1 line) Patch
M base/process/kill.h View 1 chunk +8 lines, -0 lines 0 comments Download
M base/process/kill_win.cc View 1 chunk +12 lines, -0 lines 2 comments Download
M net/test/spawned_test_server/local_test_server.cc View 1 chunk +5 lines, -1 line 0 comments Download

Messages

Total messages: 5 (0 generated)
Stanislav Albreht
6 years, 8 months ago (2014-04-23 06:08:14 UTC) #1
Stanislav Albreht
BTW, KillProcess() just below job killing in net/test/spawned_test_server/local_test_server.cc doesn't seem to have much sense on ...
6 years, 8 months ago (2014-04-23 06:12:42 UTC) #2
Paweł Hajdan Jr.
Thank you for the patch, really appreciated. Adding Carlos (cpu) for Windows expertise. Stanislav, could ...
6 years, 8 months ago (2014-04-23 10:49:22 UTC) #3
Stanislav Albreht
Paweł, thank you for so prompt reply! > Stanislav, could you explain more what's the ...
6 years, 8 months ago (2014-04-23 12:09:33 UTC) #4
cpu_(ooo_6.6-7.5)
6 years, 8 months ago (2014-04-24 03:47:37 UTC) #5
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.

Powered by Google App Engine
This is Rietveld 408576698