chrome: respect --child-clean-exit.

content/browser/child_process_launcher.cc

Index: content/browser/child_process_launcher.cc
diff --git a/content/browser/child_process_launcher.cc b/content/browser/child_process_launcher.cc
index e497762aff8ebbd651e8467c6c0409ff7c8f8fe2..cbf13e97adf4a2fdb50233c895227e03485828cc 100644
--- a/content/browser/child_process_launcher.cc
+++ b/content/browser/child_process_launcher.cc
@@ -339,9 +339,6 @@ class ChildProcessLauncher::Context
     if (!process_.handle())
       return;
 
-    if (!terminate_child_on_shutdown_)
-      return;
-
     // On Posix, EnsureProcessTerminated can lead to 2 seconds of sleep!  So
     // don't this on the UI/IO threads.
     BrowserThread::PostTask(
@@ -351,7 +348,8 @@ class ChildProcessLauncher::Context
 #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_ANDROID)
             zygote_,
 #endif
-            process_.handle()));
+            process_.handle(),
+            !terminate_child_on_shutdown_));
     process_.set_handle(base::kNullProcessHandle);
   }
 
@@ -365,7 +363,8 @@ class ChildProcessLauncher::Context
 #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_ANDROID)
       bool zygote,
 #endif
-      base::ProcessHandle handle) {
+      base::ProcessHandle handle,
+      bool wait_for_child_to_exit_cleanly) {
 #if defined(OS_ANDROID)
     LOG(INFO) << "ChromeProcess: Stopping process with handle " << handle;
     StopChildProcess(handle);
@@ -373,7 +372,9 @@ class ChildProcessLauncher::Context
     base::Process process(handle);
      // Client has gone away, so just kill the process.  Using exit code 0
     // means that UMA won't treat this as a crash.
-    process.Terminate(RESULT_CODE_NORMAL_EXIT);
+    if (!wait_for_child_to_exit_cleanly)
+      process.Terminate(RESULT_CODE_NORMAL_EXIT);
+
     // On POSIX, we must additionally reap the child.
 #if defined(OS_POSIX)
 #if !defined(OS_MACOSX)
@@ -384,7 +385,10 @@ class ChildProcessLauncher::Context
     } else
 #endif  // !OS_MACOSX
     {
-      base::EnsureProcessTerminated(handle);
+      if (wait_for_child_to_exit_cleanly)
+        base::EnsureProcessGetsReaped(handle);

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

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]

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]

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).

+      else
+        base::EnsureProcessTerminated(handle);
     }
 #endif  // OS_POSIX
     process.Close();