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

Issue 14707006: Kill all chrome processes on linux when shutting down the ProxyLauncher. (Closed)

Created:
7 years, 7 months ago by tonyg
Modified:
7 years, 7 months ago
Reviewers:
Paweł Hajdan Jr.
CC:
chromium-reviews, robertshield, kkania
Visibility:
Public.

Description

Kill all chrome processes on linux when shutting down the ProxyLauncher. The startup performance_ui_tests have been hanging on the perf bots since they were upgraded to Precise. The hang is due to the performance_ui_tests binary not killing all of its child chrome processes which is due to bug 177218. When that happens, the buildbot step hangs until timeout waiting for the process group to end. The suggested workaround for the bug is to disable tcmalloc, which works, but is undesireable to do on the perf bots. So this patch works around the subprocess hang by always kill()ing all chrome processes on linux. BUG=235893 TEST=performance_ui_tests --gtest_filter=ShutdownTest.* Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198045

Patch Set 1 #

Total comments: 4

Patch Set 2 : Move TerminateAllChromeProcesses out of WaitForBrowserProcessToQuit #

Total comments: 1

Patch Set 3 : Remove stray include #

Unified diffs Side-by-side diffs Delta from patch set Stats (+21 lines, -10 lines) Patch
M chrome/test/automation/proxy_launcher.cc View 1 5 chunks +10 lines, -3 lines 0 comments Download
M chrome/test/base/chrome_process_util.h View 1 chunk +2 lines, -3 lines 0 comments Download
M chrome/test/base/chrome_process_util.cc View 1 chunk +1 line, -3 lines 0 comments Download
M chrome/test/ui/ui_test.cc View 1 2 chunks +5 lines, -0 lines 0 comments Download
M chrome/test/webdriver/webdriver_automation.cc View 1 chunk +3 lines, -1 line 0 comments Download

Messages

Total messages: 9 (0 generated)
tonyg
7 years, 7 months ago (2013-05-02 23:42:11 UTC) #1
Paweł Hajdan Jr.
Just minor comments. Indeed, the change is much better, thanks! https://codereview.chromium.org/14707006/diff/1/chrome/test/automation/proxy_launcher.cc File chrome/test/automation/proxy_launcher.cc (right): https://codereview.chromium.org/14707006/diff/1/chrome/test/automation/proxy_launcher.cc#newcode381 ...
7 years, 7 months ago (2013-05-02 23:47:01 UTC) #2
tonyg
https://codereview.chromium.org/14707006/diff/1/chrome/test/automation/proxy_launcher.cc File chrome/test/automation/proxy_launcher.cc (right): https://codereview.chromium.org/14707006/diff/1/chrome/test/automation/proxy_launcher.cc#newcode381 chrome/test/automation/proxy_launcher.cc:381: // On linux we always terminate all chrome processes ...
7 years, 7 months ago (2013-05-03 00:10:14 UTC) #3
Paweł Hajdan Jr.
LGTM! Looking forward to more solid tests. Thanks! https://codereview.chromium.org/14707006/diff/5001/chrome/test/automation/proxy_launcher.h File chrome/test/automation/proxy_launcher.h (right): https://codereview.chromium.org/14707006/diff/5001/chrome/test/automation/proxy_launcher.h#newcode18 chrome/test/automation/proxy_launcher.h:18: #include ...
7 years, 7 months ago (2013-05-03 00:12:54 UTC) #4
tonyg
On 2013/05/03 00:12:54, Paweł Hajdan Jr. wrote: > LGTM! Looking forward to more solid tests. ...
7 years, 7 months ago (2013-05-03 00:13:28 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tonyg@chromium.org/14707006/7001
7 years, 7 months ago (2013-05-03 00:13:47 UTC) #6
commit-bot: I haz the power
Retried try job too often on ios_dbg_simulator for step(s) googleurl_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_simulator&number=48738
7 years, 7 months ago (2013-05-03 00:36:09 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tonyg@chromium.org/14707006/7001
7 years, 7 months ago (2013-05-03 02:09:26 UTC) #8
commit-bot: I haz the power
7 years, 7 months ago (2013-05-03 03:37:08 UTC) #9
Message was sent while issue was closed.
Change committed as 198045

Powered by Google App Engine
This is Rietveld 408576698