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

Issue 1915003: Kernel zombie death race! (Closed)

Created:
10 years, 7 months ago by Mark Mentovai
Modified:
9 years, 7 months ago
Reviewers:
viettrungluu, jeremy
CC:
chromium-reviews, John Grabowski, pam+watch_chromium.org
Visibility:
Public.

Description

Kernel zombie death race! Harden process_watcher_mac against zombie-admitting holes. A race between the application and the kernel exists in that a process stops being kqueueable before it becomes waitable. If ProcessWatcher::EnsureProcessTerminated was called within this window, it would assume that the process had already been reaped. The process would then become waitable, but by then, the application forgot about it, and it would be leaked as a zombie until the application as a whole was quit. In order to kill the undead, this code now detects not-kqueueable not-waitable processes and does a blocking wait, with a kill thrown in for good measure. Other opportunities for this code to return early without making the best effort to kill the process have been plugged up too. If any of the kqueue operations fail, this will now fall through to kill and wait for the process. BUG=43150, 43244 TEST=A. Steps from bug 43150 comment 8: 1. Clean profile. 2. Launch. The home page, http://www.google.com/, will load. 3. Type (or paste) about:blank into the omnibox. Expect: no zombies. Observe: original renderer process almost always becomes undead. Note: Perform the test multiple times. It's timing-sensitive. I experienced varying degrees of success reproducing zombies when launching the app by double-clicking it, running it from a terminal window with stdout and stderr on the terminal, and running it from a terminal window with stdout and stderr redirected to a file. With this fix, no zombies should be condition. B. zombies.py test from bug 43244. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=46466

Patch Set 1 : '' #

Total comments: 5

Patch Set 2 : '' #

Patch Set 3 : '' #

Patch Set 4 : '' #

Total comments: 2

Patch Set 5 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+127 lines, -77 lines) Patch
M chrome/common/process_watcher_mac.cc View 1 2 3 4 1 chunk +127 lines, -77 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
Mark Mentovai
10 years, 7 months ago (2010-05-04 22:20:48 UTC) #1
viettrungluu
http://codereview.chromium.org/1915003/diff/3001/4001 File chrome/common/process_watcher_mac.cc (right): http://codereview.chromium.org/1915003/diff/3001/4001#newcode31 chrome/common/process_watcher_mac.cc:31: void WaitForChildToDie(pid_t child, int timeout) { Could you add ...
10 years, 7 months ago (2010-05-04 22:53:55 UTC) #2
Mark Mentovai
New version up. http://codereview.chromium.org/1915003/diff/3001/4001 File chrome/common/process_watcher_mac.cc (right): http://codereview.chromium.org/1915003/diff/3001/4001#newcode48 chrome/common/process_watcher_mac.cc:48: PLOG(ERROR) << "kevent (setup " << ...
10 years, 7 months ago (2010-05-05 01:04:12 UTC) #3
jeremy
LGTM http://codereview.chromium.org/1915003/diff/15001/16001 File chrome/common/process_watcher_mac.cc (right): http://codereview.chromium.org/1915003/diff/15001/16001#newcode70 chrome/common/process_watcher_mac.cc:70: int result; nit: defensively set = -1; ? ...
10 years, 7 months ago (2010-05-05 07:09:21 UTC) #4
viettrungluu
LGTM. (I assume that you've tested it against the current zombie repro, right?)
10 years, 7 months ago (2010-05-05 14:30:23 UTC) #5
Mark Mentovai
jeremy@chromium.org wrote: > http://codereview.chromium.org/1915003/diff/15001/16001#newcode70 > chrome/common/process_watcher_mac.cc:70: int result; > nit: defensively set > = -1; ...
10 years, 7 months ago (2010-05-05 16:33:15 UTC) #6
Mark Mentovai
10 years, 7 months ago (2010-05-05 16:33:49 UTC) #7
viettrungluu@chromium.org wrote:
> LGTM.
>
> (I assume that you've tested it against the current zombie repro, right?)

Yup, I’ve tested it with GREAT SUCCESS against the steps in the bug
(and the changelist), and against Nirnimesh’s pyauto-driven
zombiemaker.

Powered by Google App Engine
This is Rietveld 408576698