chrome: respect --child-clean-exit.

chrome: control how chrome orchestrates its exit.

Without this CL, the zygote process sends a SIGTERM after 2 seconds of waiting
for the renderer process to exit. The renderer process can take longer than 2
seconds if it is instrumented to collect profile data, for example.

Another issue with the current exit process is that the browser process can exit
before the children exit. This causes issues on ChromeOS where if the browser
process exits, the session_manager kills all children of the browser process
with a SIGKILL.

The goal of this CL is 2-fold:
1. Have --child-clean-exit ensure all child processes exit cleanly.
2. Have --wait-for-children-before-exiting ensure the browser process is the
last one to exit.

These objectives are achieved through the following measures (when
--child-clean-exit and --wait-for-children-to-exit are passed):
1. --child-clean-exit is propagated to the zygote process.
2. The zygote process reaps its child processes instead of sending a SIGTERM to
them.
3. The zygote process waits indefinitely until all its child processes have
exited.
4. The browser process waits indefinitely until all its child processes have
exited.
5. The browser process waits for the zygote and reaps it before exiting.
6. The renderer process does not send a SIGALRM to itself after 30 seconds.
Instead it waits indefinitely to cleanly exit. 30 seconds is not enough for a
clean exit when Chrome is instrumented and writing profile data.

BUG=259824
TEST=python test_clean_exit.py (pyauto unittest already checked in) works.

[asharif1, 2013-07-18 22:52:55]

Please take a look.

I tested it by doing the following:

1. Built Chrome under PGO mode so the renderer would take a long time to exit
(cause it dumps 11k profile files).
2. Launched the PGO testsuite on ChromeOS.
3. While it was running, straced all Chrome processes.
4. Waited for test to finish.
5. Examined all strace log files and ensured everything had exit_group(0) at the
end of the file.

I also tried to write a unittest which slowed down the render process using
SIGSTOP and SIGCONT, but that one causes other issues (renderer process waits a
indefinitely on reading a pipe from the browser process). Not sure if a good
unittest can be written here. I am open to ideas.

[jln (very slow on Chromium), 2013-07-18 23:26:05]

I took a very quick look in a rush, but I thought I would already send this
piece of feedback since something seems very odd.

https://chromiumcodereview.appspot.com/19231006/diff/3001/content/zygote/zygo...
File content/zygote/zygote_linux.cc (right):

https://chromiumcodereview.appspot.com/19231006/diff/3001/content/zygote/zygo...
content/zygote/zygote_linux.cc:118: pthread_exit(NULL);
What are you trying to do here ?

You're exiting the current thread and definitely not waiting for children.

This is the same as _exit() since this process is monothreaded.

[Markus (顧孟勤), 2013-07-18 23:26:47]

https://chromiumcodereview.appspot.com/19231006/diff/3001/content/zygote/zygo...
File content/zygote/zygote_linux.cc (right):

https://chromiumcodereview.appspot.com/19231006/diff/3001/content/zygote/zygo...
content/zygote/zygote_linux.cc:117: // Wait for all children to exit first.
This comment is super confusing. This might very well be correct code -- I don't
understand things well enough to judge that.

But the comment doesn't match the code. pthread_exec() is essentially the same
as syscall(__NR_exit). And _exit() is essentially the same as
syscall(__NR_exit_group).

The difference is that the former terminates the current thread only. The latter
terminates all threads in the current process.

Neither one of them has any impact on any child processes. The difference is
strictly limited to threads.

So, a comment talking about children has me even more confused than no comment
at all.

[asharif1, 2013-07-19 03:01:31]

Please review my comments and take another look at the code.

https://codereview.chromium.org/19231006/diff/3001/content/zygote/zygote_linu...
File content/zygote/zygote_linux.cc (right):

https://codereview.chromium.org/19231006/diff/3001/content/zygote/zygote_linu...
content/zygote/zygote_linux.cc:117: // Wait for all children to exit first.
On 2013/07/18 23:26:47, Markus (顧孟勤) wrote:
> This comment is super confusing. This might very well be correct code -- I
don't
> understand things well enough to judge that.
> 
> But the comment doesn't match the code. pthread_exec() is essentially the same
> as syscall(__NR_exit). And _exit() is essentially the same as
> syscall(__NR_exit_group).
> 
> The difference is that the former terminates the current thread only. The
latter
> terminates all threads in the current process.
> 
> Neither one of them has any impact on any child processes. The difference is
> strictly limited to threads.
> 
> So, a comment talking about children has me even more confused than no comment
> at all.

I will try to clarify this comment in the next patchset (another 14 hours or
so).

But for an explanation, here it is:

The old code calls _exit(), which exits the zygote process immediately (without
waiting for renderer processes to exit).

The goal of my CL is to have the zygote wait on all the renderer processes to
exit, and have the browser wait on the zygote processes to exit. In this way, I
will ensure that the browser process is the last one to exit with
--child-clean-exit. Currently such a situation does not happen, and it causes
problems on ChromeOS because the ChromeOS session_manager sends a SIGKILL to
-browser_process_pid as soon as it terminates. See this thread for details:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-os-dev/mGxMV8...

Now that the goal is understood, let's see what this code does.

When the browser process wants to exit (user clicks close or sends SIGINT to the
browser process), it closes the channel to the renderer process and asks the
zygote to reap the renderer. The zygote calls EnsureProcessGetsReaped() around
like 185. If you look at the implementation of this, it actually reaps the
renderer in a separate, non-joinable thread.

The browser also closes the channel to the zygote so this code waits until the
renderer(s) are reaped on the background thread.

https://codereview.chromium.org/19231006/diff/3001/content/zygote/zygote_linu...
content/zygote/zygote_linux.cc:118: pthread_exit(NULL);
On 2013/07/18 23:26:05, Julien Tinnes wrote:
> What are you trying to do here ?
> 
> You're exiting the current thread and definitely not waiting for children.
> 
> This is the same as _exit() since this process is monothreaded.

Although this process seems single-threaded, close to the browser exiting, it is
not. This is because we get a reap request to reap the renderer close to the EOF
from the browser. The reap request is handled using a detatched thread (see
kill_posix.cc for details) which calls waitpid(). Specifically look at
backgroundreaper.

The goal of my code is to ensure that reaper actually reaps the renderer before
the zygote quits. So we must not call _exit() here as that will exit
immediately.

Please also read my other comment above. I will update the comment tomorrow.

[Jói, 2013-07-19 09:14:41]

I'm not an appropriate reviewer for this; removing myself. Perhaps you should
ask jam@ to review.

Cheers,
Jói

[asharif1, 2013-07-19 16:05:23]

I have updated this patchset. Please take another look.

[asharif1, 2013-07-19 16:12:29]

On 2013/07/19 16:05:23, asharif1 wrote:
> I have updated this patchset. Please take another look.

jam@ and other reviewers, for background please read this:

https://groups.google.com/a/chromium.org/forum/#!topic/chromium-os-dev/mGxMV8...

[asharif1, 2013-07-19 16:13:27]

Adding mnissler and cmasone to reviewers as they suggested this approach.

Adding scottz to cc in case he's interested.

[jln (very slow on Chromium), 2013-07-19 16:20:44]

On 2013/07/19 16:13:27, asharif1 wrote:
> Adding mnissler and cmasone to reviewers as they suggested this approach.
> 
> Adding scottz to cc in case he's interested.

Who do you want as a reviewer for this? Please, select one reviewer and then add
owners as appropriate, after the review is done.

There are now 7 reviewers on your review line. Markus or myself are the right
main owners for this, but feel free to get someone to do a first pass review
first. Or I can do both.

[asharif1, 2013-07-19 16:33:51]

On 2013/07/19 16:20:44, Julien Tinnes wrote:
> On 2013/07/19 16:13:27, asharif1 wrote:
> > Adding mnissler and cmasone to reviewers as they suggested this approach.
> > 
> > Adding scottz to cc in case he's interested.
> 
> Who do you want as a reviewer for this? Please, select one reviewer and then
add
> owners as appropriate, after the review is done.
> 
> There are now 7 reviewers on your review line. Markus or myself are the right
> main owners for this, but feel free to get someone to do a first pass review
> first. Or I can do both.

I see. To select reviewers, I added people suggested by the git cl upload tool
and OWNERS. You're right... 7 is too many.

I cut it down to jln and jam. Once you guys do a first pass on this, I can add
markus to take a final look.

[jln (very slow on Chromium), 2013-07-19 16:39:36]

On 2013/07/19 16:33:51, asharif1 wrote:
> On 2013/07/19 16:20:44, Julien Tinnes wrote:
> > On 2013/07/19 16:13:27, asharif1 wrote:
> > > Adding mnissler and cmasone to reviewers as they suggested this approach.
> > > 
> > > Adding scottz to cc in case he's interested.
> > 
> > Who do you want as a reviewer for this? Please, select one reviewer and then
> add
> > owners as appropriate, after the review is done.
> > 
> > There are now 7 reviewers on your review line. Markus or myself are the
right
> > main owners for this, but feel free to get someone to do a first pass review
> > first. Or I can do both.
> 
> I see. To select reviewers, I added people suggested by the git cl upload tool
> and OWNERS. You're right... 7 is too many.
> 
> I cut it down to jln and jam. Once you guys do a first pass on this, I can add
> markus to take a final look.

No, just one reviewer (ideally who knows what you're doing already, if not, pick
me). And then you can select an owner (either Markus or myself).

So from what I understand, you could either have cmasone do a first pass review
and then pick me or Markus as owners. Or just pick me directly to do both.

The former is the more usual procedure as it's a bit more lightweight on owners.

[asharif1, 2013-07-19 16:54:35]

jln@, please do a first-pass. Later on I can ask you or markus to do a 2nd pass
and approval.

[Markus (顧孟勤), 2013-07-22 22:50:13]

Other than the fact that this is horribly ugly and complicated code (not your
fault!), your change is making things only minimally worse. I am not thrilled,
but I can live with this CL.

https://chromiumcodereview.appspot.com/19231006/diff/15001/content/app/conten...
File content/app/content_main_runner.cc (right):

https://chromiumcodereview.appspot.com/19231006/diff/15001/content/app/conten...
content/app/content_main_runner.cc:794: while (wait(NULL) != -1);
Minor nit-pick (feel free to avoid).

This looks very much like an anti-pattern:
http://c-faq.com/strangeprob/emptyloop.html

I'd prefer if you were more verbose, just to make sure nobody "fixes" this code
at some point in the future.

How about:

for (;;) {
  if (wait(NULL) < 0 && errno == ECHILD) {
    break;
  }
}

This also has the advantage of looping on EINTR.

Otherwise, at least consider adding HANDLE_EINTR(wait(NULL));

https://chromiumcodereview.appspot.com/19231006/diff/15001/content/browser/ch...
File content/browser/child_process_launcher.cc (right):

https://chromiumcodereview.appspot.com/19231006/diff/15001/content/browser/ch...
content/browser/child_process_launcher.cc:389:
base::EnsureProcessGetsReaped(handle);
I am really sad that we need to launch another thread here. It makes it very
hard to reason about the correctness of the code.

I'd be much happier if we could keep the zygote completely single-threaded.

But I understand that this is very difficult to change. So, I am reluctantly OK
with your change.

If you are willing to take on the annoying task of fixing this code in a way
that doesn't introduce threads though, I'd applaud you and do my best to help.

[asharif1, 2013-07-22 23:43:54]

This CL seems to be working fine for our purposes (profiling an instrumented
Chrome) on ChromeOS. The profile that is dumped seems to be clean and free from
SIGKILLs.

However, I encountered some issues while writing a unittest for it. The unittest
I wrote was the following:

1. Starts up chrome with --no-sandbox --child-clean-exit
2. Waits till it navigates to a page.
3. Sends a SIGSTOP to the renderer.
4. Sends a SIGINT to the browser.
5. Now, waits for 20 seconds or so.
6. Ensures the browser is still running and the renderer too (this behavior is
enforced by this patch, to ensure the browser is the last one to exit).
7. Sends a SIGCONT to the renderer.
8. Waits for a while (20 seconds).
9. Ensures the browser and renderer are both gone.

This revealed a problem with this patch. The browser apparently forks a suid
sandbox process even under --no-sandbox, which handles stuff like getting the
time for the renderer process (this is used for even trivial things like LOG()
messages, etc.).

The way this patch is written, the order of events at shutdown are:

exit_manager_.reset(NULL); // This runs all destructors
// Wait for children to exit.

When the destructor for SandboxIPCProcess runs, it closes a lifeline fd, which
causes the suid sandbox process to _exit() immediately. Consequently, the
browser process can no longer get the local time for log messages, etc. and
hangs. The zygote also hangs because it is doing a wait() to reap the renderer,
and the browser also hangs because it's doing a wait() to reap the zygote.

How do we break this deadlock? I think we should prevent it from happening in
the first place. Specifically, the lifeline fd should not be closed before the
renderer has exited.

Possibly, we have to do a synchronous wait for the zygote to exit before closing
the lifeline fd for the suid process.

I am open to ideas here. markus and jln, your thoughts?

https://codereview.chromium.org/19231006/diff/15001/content/app/content_main_...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/19231006/diff/15001/content/app/content_main_...
content/app/content_main_runner.cc:794: while (wait(NULL) != -1);
On 2013/07/22 22:50:13, Markus (顧孟勤) wrote:
> Minor nit-pick (feel free to avoid).
> 
> This looks very much like an anti-pattern:
> http://c-faq.com/strangeprob/emptyloop.html
> 
> I'd prefer if you were more verbose, just to make sure nobody "fixes" this
code
> at some point in the future.
> 
> How about:
> 
> for (;;) {
>   if (wait(NULL) < 0 && errno == ECHILD) {
>     break;
>   }
> }
> 
> This also has the advantage of looping on EINTR.
> 
> Otherwise, at least consider adding HANDLE_EINTR(wait(NULL));

I can easily make this change, but please comment on the other issue I'm seeing,
which is described in a message along with my replies to your comments.

https://codereview.chromium.org/19231006/diff/15001/content/browser/child_pro...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/19231006/diff/15001/content/browser/child_pro...
content/browser/child_process_launcher.cc:389:
base::EnsureProcessGetsReaped(handle);
On 2013/07/22 22:50:13, Markus (顧孟勤) wrote:
> I am really sad that we need to launch another thread here. It makes it very
> hard to reason about the correctness of the code.
> 
> I'd be much happier if we could keep the zygote completely single-threaded.
> 
> But I understand that this is very difficult to change. So, I am reluctantly
OK
> with your change.
> 
> If you are willing to take on the annoying task of fixing this code in a way
> that doesn't introduce threads though, I'd applaud you and do my best to help.

Note that the zygote process isn't multithreaded because of this file, but
because of this function (EnsureProcessGetsReaped).

I could write a synchronous version of the reap using waitpid() if you want and
have it be called in --child-clean-exit mode. What do you think?

[Markus (顧孟勤), 2013-07-22 23:54:27]

https://codereview.chromium.org/19231006/diff/15001/content/browser/child_pro...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/19231006/diff/15001/content/browser/child_pro...
content/browser/child_process_launcher.cc:389:
base::EnsureProcessGetsReaped(handle);
If writing a synchronous version is possible without breaking anything else in
the process, then yes, that would be much preferred.

As you are discovering yourself, this code is insanely fragile -- and threads
add more insane complications on top of it. It's hard enough to reason what the
single-threaded code does.

In an ideal world, we would rewrite the entire Zygote and clean it up. In
practice, that's not going to happen, but we can at least strive to make things
better than what we found before.

I am still not convinced that your CL is actually doing that (again, not trying
to upset you; just commenting on the fact that you are inevitably adding
complexity).

[asharif1, 2013-07-24 15:17:23]

markus, please take a look again.

I have redone this CL a bit.

1. I addressed one of your initial comment. The other one
2. I added a new flag to ensure the browser is the last one to exit. This is
orthogonal from the original --child-clean-exit case.
3. To work around crbug.com/263594 I am calling exit() in the renderer.

https://codereview.chromium.org/19231006/diff/15001/content/app/content_main_...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/19231006/diff/15001/content/app/content_main_...
content/app/content_main_runner.cc:794: while (wait(NULL) != -1);
On 2013/07/22 22:50:13, Markus (顧孟勤) wrote:
> Minor nit-pick (feel free to avoid).
> 
> This looks very much like an anti-pattern:
> http://c-faq.com/strangeprob/emptyloop.html
> 
> I'd prefer if you were more verbose, just to make sure nobody "fixes" this
code
> at some point in the future.
> 
> How about:
> 
> for (;;) {
>   if (wait(NULL) < 0 && errno == ECHILD) {
>     break;
>   }
> }
> 
> This also has the advantage of looping on EINTR.
> 
> Otherwise, at least consider adding HANDLE_EINTR(wait(NULL));

Done.

[jln (very slow on Chromium), 2013-08-27 00:55:13]

On 2013/07/24 15:17:23, asharif1 wrote:
> markus, please take a look again.
> 
> I have redone this CL a bit.
> 
> 1. I addressed one of your initial comment. The other one
> 2. I added a new flag to ensure the browser is the last one to exit. This is
> orthogonal from the original --child-clean-exit case.
> 3. To work around crbug.com/263594 I am calling exit() in the renderer.
> 
>
https://codereview.chromium.org/19231006/diff/15001/content/app/content_main_...
> File content/app/content_main_runner.cc (right):
> 
>
https://codereview.chromium.org/19231006/diff/15001/content/app/content_main_...
> content/app/content_main_runner.cc:794: while (wait(NULL) != -1);
> On 2013/07/22 22:50:13, Markus (顧孟勤) wrote:
> > Minor nit-pick (feel free to avoid).
> > 
> > This looks very much like an anti-pattern:
> > http://c-faq.com/strangeprob/emptyloop.html
> > 
> > I'd prefer if you were more verbose, just to make sure nobody "fixes" this
> code
> > at some point in the future.
> > 
> > How about:
> > 
> > for (;;) {
> >   if (wait(NULL) < 0 && errno == ECHILD) {
> >     break;
> >   }
> > }
> > 
> > This also has the advantage of looping on EINTR.
> > 
> > Otherwise, at least consider adding HANDLE_EINTR(wait(NULL));
> 
> Done.

Is this CL still relevant ? Note that there is slightly related work in
crbug.com/274827 and crbug.com/133453.

[asharif1, 2013-08-27 22:38:03]

On 2013/08/27 00:55:13, Julien Tinnes wrote:
> On 2013/07/24 15:17:23, asharif1 wrote:
> > markus, please take a look again.
> > 
> > I have redone this CL a bit.
> > 
> > 1. I addressed one of your initial comment. The other one
> > 2. I added a new flag to ensure the browser is the last one to exit. This is
> > orthogonal from the original --child-clean-exit case.
> > 3. To work around crbug.com/263594 I am calling exit() in the renderer.
> > 
> >
>
https://codereview.chromium.org/19231006/diff/15001/content/app/content_main_...
> > File content/app/content_main_runner.cc (right):
> > 
> >
>
https://codereview.chromium.org/19231006/diff/15001/content/app/content_main_...
> > content/app/content_main_runner.cc:794: while (wait(NULL) != -1);
> > On 2013/07/22 22:50:13, Markus (顧孟勤) wrote:
> > > Minor nit-pick (feel free to avoid).
> > > 
> > > This looks very much like an anti-pattern:
> > > http://c-faq.com/strangeprob/emptyloop.html
> > > 
> > > I'd prefer if you were more verbose, just to make sure nobody "fixes" this
> > code
> > > at some point in the future.
> > > 
> > > How about:
> > > 
> > > for (;;) {
> > >   if (wait(NULL) < 0 && errno == ECHILD) {
> > >     break;
> > >   }
> > > }
> > > 
> > > This also has the advantage of looping on EINTR.
> > > 
> > > Otherwise, at least consider adding HANDLE_EINTR(wait(NULL));
> > 
> > Done.
> 
> Is this CL still relevant ? Note that there is slightly related work in
> crbug.com/274827 and crbug.com/133453.

This CL isn't needed at the moment, but may be needed in the future when we
deploy PGO. I did not get any review comments from anyone so I implemented a
workaround for this CL.

I can close this CL and someone else can own this CL if they want to implement
this solution into Chrome.

[jln (very slow on Chromium), 2013-08-28 01:28:54]

On 2013/08/27 22:38:03, asharif1 wrote:

> I can close this CL and someone else can own this CL if they want to implement
> this solution into Chrome.

Let me know what you prefer. If you feel like landing it, I'm happy to review!
Sorry this slipped through.

[asharif1, 2013-09-04 03:08:28]

On 2013/08/28 01:28:54, Julien Tinnes wrote:
> On 2013/08/27 22:38:03, asharif1 wrote:
> 
> > I can close this CL and someone else can own this CL if they want to
implement
> > this solution into Chrome.
> 
> Let me know what you prefer. If you feel like landing it, I'm happy to review!
> Sorry this slipped through.

Closing this now as I have to focus on other things. I'll revive it if I we want
this in.