chrome: respect --child-clean-exit.

content/browser/child_process_launcher.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/child_process_launcher.h"
 
 #include <utility>  // For std::pair.
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 321 matching lines @@
       client_->OnProcessLaunched();
     } else {
       Terminate();
     }
   }
 
   void Terminate() {
     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(
         BrowserThread::PROCESS_LAUNCHER, FROM_HERE,
         base::Bind(
             &Context::TerminateInternal,
 #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);
   }
 
   static void SetProcessBackgrounded(base::ProcessHandle handle,
                                      bool background) {
     base::Process process(handle);
     process.SetProcessBackgrounded(background);
   }
 
   static void TerminateInternal(
 #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);
 #else
     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)
     if (zygote) {
       // If the renderer was created via a zygote, we have to proxy the reaping
       // through the zygote process.
       ZygoteHostImpl::GetInstance()->EnsureProcessTerminated(handle);
     } 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();
 #endif  // defined(OS_ANDROID)
   }
 
   Client* client_;
   BrowserThread::ID client_thread_id_;
   base::Process process_;
   base::TerminationStatus termination_status_;
@@ skipping 95 matching lines @@
           GetHandle(), background));
 }
 
 void ChildProcessLauncher::SetTerminateChildOnShutdown(
   bool terminate_on_shutdown) {
   if (context_.get())
     context_->set_terminate_child_on_shutdown(terminate_on_shutdown);
 }
 
 }  // namespace content