Linux: tear down Zygote at browser shutdown.

Linux: tear down Zygote at browser shutdown.

In BrowserMainLoop::ShutdownThreadsAndCleanUp(), we explicitly call
a new ZygoteHostImpl::TearDownAfterLastChild API to notify the Zygote
to exit after its last child died.

The notification is to simply close the IPC channel to the Zygote.
Before this patch, this would happen implicitly when the browser process
died.

To implement this, we add a lightweight child tracking mechanism to ZygoteHostImpl.

BUG=334345
R=piman@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247572

[jln (very slow on Chromium), 2014-01-28 00:18:45]

Antoine, PTAL!

I need to watch NaCl in the try bots. I wonder if some failures could happen if
the Zygote just dies while the NaCl Zygote still runs. AFAIR, the NaCl Zygote
should die automatically if the main Zygote disappears.

[jln (very slow on Chromium), 2014-01-28 00:59:37]

It looks like this simplistic approach won't work.

Out of process plugins hosts can still try and make use of
ZygoteHostImpl::EnsureProcessTerminated() at this stage.

So, as you suspected we may need to "tag" the Zygote as ready to exit.

It would require sending a new 'Die ASAP' message to the Zygote that could then
oblige once all processes it knows about have been waited for. But I'm unsure
that will fix the current issue. This area is so messy I would be surprised if
"something" really tries and gets exit status for all processes.

Another approach would be to simply tag the ZygoteHost with an "everything is
exiting now" status and in that case simply ignore the
ZygoteHostImpl::EnsureProcessTerminated() /
ZygoteHostImpl::GetTerminationStatus() requests. We'd need to be sure that all
processes would eventually terminate, even without EnsureProcessTerminated().
(We'd consider it a bug if processes don't die by themselves when loosing their
IPC channel to the browser).

I'm not sure what to do. It looks like this has never been properly thought-out
and I wonder how much we can do in a merge-able patch.

[jln (very slow on Chromium), 2014-01-28 02:13:01]

Still looking into this:

I did implement process tracking in ZygoteHostImpl and there are a number of
children that are never waited for.

I can look into it more and see how convoluted it would be to have an "owner"
for them, but it doesn't look good so far.

[piman, 2014-01-28 02:20:44]

https://codereview.chromium.org/148443006/diff/1/content/browser/zygote_host/...
File content/browser/zygote_host/zygote_host_impl_linux.cc (right):

https://codereview.chromium.org/148443006/diff/1/content/browser/zygote_host/...
content/browser/zygote_host/zygote_host_impl_linux.cc:226: }
nit: maybe reset control_fd_ to -1 for added defense against races / bad usage.

Speaking of races, this is called on the main (UI) thread, before the IO thread
is torn down. I think most of the other methods are called from the IO thread.
Isn't there a race risk? Should this be done under control_lock_? And have
SendMessage/ReadReply properly check the value of control_fd and/or init_ and
return failure?

[jln (very slow on Chromium), 2014-01-28 02:52:45]

So, it turns out that in the SIGTERM (or "browser close button") case child of
the Zygote seems to be properly waited on.

The problematic case is when trying to close all renderers. After the last
renderers is closed, the browser exits, but some renderers are never waited on.

If that's really what's happening, the current CL could potentially be enough to
fix the current bug (i.e. the SIGTERM case).

I've uploaded a new version with child tracking, let's see what the bots say.

https://codereview.chromium.org/148443006/diff/1/content/browser/zygote_host/...
File content/browser/zygote_host/zygote_host_impl_linux.cc (right):

https://codereview.chromium.org/148443006/diff/1/content/browser/zygote_host/...
content/browser/zygote_host/zygote_host_impl_linux.cc:226: }
On 2014/01/28 02:20:44, piman wrote:
> nit: maybe reset control_fd_ to -1 for added defense against races / bad
usage.

Done. I hadn't done it because "init_" is checked in almost all places anyways.

> Speaking of races, this is called on the main (UI) thread, before the IO
thread
> is torn down. I think most of the other methods are called from the IO thread.
> Isn't there a race risk? Should this be done under control_lock_? And have
> SendMessage/ReadReply properly check the value of control_fd and/or init_ and
> return failure?

That race existed before (in the destructor), but you're right that this could
make it worse.

That a lock could be problematic for fringe cases (for instance if Zygote is
SIGSTOP-ed), but otherwise we should be fine. Added!

[piman, 2014-01-28 03:46:49]

LGTM

[jln (very slow on Chromium), 2014-01-28 21:42:25]

Antoine: do you mind taking another quick look?

I added a lock around the child tracking variables.
I wanted to use a ThreadChecker instead, but destroying on another thread makes
this unnecessarily convoluted.

I don't think that the lock would ever be under contention in practice. In the
non contended case, the overhead is negligible: it's around fairly heavy
operations involving system calls.

[piman, 2014-01-28 22:38:31]

https://chromiumcodereview.appspot.com/148443006/diff/110001/content/browser/...
File content/browser/zygote_host/zygote_host_impl_linux.cc (right):

https://chromiumcodereview.appspot.com/148443006/diff/110001/content/browser/...
content/browser/zygote_host/zygote_host_impl_linux.cc:226: TearDown();
Can we release the lock before calling TearDown?
The lock would only protect the set:

bool has_no_children = false;
{
  base::AutoLock lock(child_tracking_lock_);
  should_teardown_after_last_child_exits_ = true;
  has_no_children = list_of_running_zygote_children_.empty();
}
if (has_no_children)
  TearDown();


And similar in ZygoteChildDied.

That way:
- it's even clearer that we won't have contention
- we avoid potential ABA issues between child_tracking_lock_ and control_lock_

[jln (very slow on Chromium), 2014-01-28 23:22:11]

Thanks, ptal!

https://chromiumcodereview.appspot.com/148443006/diff/110001/content/browser/...
File content/browser/zygote_host/zygote_host_impl_linux.cc (right):

https://chromiumcodereview.appspot.com/148443006/diff/110001/content/browser/...
content/browser/zygote_host/zygote_host_impl_linux.cc:226: TearDown();
On 2014/01/28 22:38:32, piman wrote:
[...]
> - it's even clearer that we won't have contention

Yes, that's much better, thanks!

> - we avoid potential ABA issues between child_tracking_lock_ and control_lock_

Do you really mean ABA or deadlock? The deadlock risk is that control_lock_
would be held when we call TearDown() (which acquires control_lock_ again).
I had manually convinced myself that it cannot happen as neither
ZygoteChildBorn() and ZygoteChildDied() are called with control_lock_ held.

But even with your suggested change, this remains fragile. The good news is that
this would immediately blow up in a DEBUG build without any need of a specific
race to happen.

[piman, 2014-01-28 23:25:21]

lgtm

[commit-bot: I haz the power, 2014-01-29 00:06:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jln@chromium.org/148443006/130001

[jln (very slow on Chromium), 2014-01-29 01:37:59]

Committed patchset #6 manually as r247572 (presubmit successful).

[commit-bot: I haz the power, 2014-01-29 01:38:49]

CQ bit was unchecked on CL. Ignoring.