From 3131a4bc3ef3b6f545b7dea6ea55d8419099b7b2 Mon Sep 17 00:00:00 2001 From: "kkania@chromium.org" Date: Tue, 4 May 2010 20:23:36 +0000 Subject: Fix race in zygote_host_linux where socket was being read from and written to on different threads. Made ZygoteHost methods all run on PROCESS_LAUNCHER thread. PostTask in linux from UI thread to PROCESS_LAUNCHER thread for DidProcessCrash. Changed DidProcessCrash to answer via a callback, which occurs asynchronously on linux. Rework cases in nacl_host, browser_render_process_host, and child_process_host where this method was being called to fit the callback model. BUG=31737 TEST=none Review URL: http://codereview.chromium.org/1695026 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@46384 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/browser/child_process_host.cc | 38 +++++++--- chrome/browser/child_process_host.h | 7 +- chrome/browser/child_process_launcher.cc | 81 ++++++++++++++++------ chrome/browser/child_process_launcher.h | 11 ++- chrome/browser/metrics/metrics_service.cc | 18 ++--- chrome/browser/nacl_host/nacl_process_host.cc | 17 +++-- chrome/browser/nacl_host/nacl_process_host.h | 3 +- .../renderer_host/browser_render_process_host.cc | 39 +++++++---- .../renderer_host/browser_render_process_host.h | 1 + chrome/browser/renderer_host/render_process_host.h | 10 --- chrome/browser/zygote_host_linux.cc | 4 ++ chrome/browser/zygote_host_linux.h | 13 +++- 12 files changed, 165 insertions(+), 77 deletions(-) (limited to 'chrome/browser') diff --git a/chrome/browser/child_process_host.cc b/chrome/browser/child_process_host.cc index 0c87021..2a828e0 100644 --- a/chrome/browser/child_process_host.cc +++ b/chrome/browser/child_process_host.cc @@ -179,23 +179,36 @@ void ChildProcessHost::Notify(NotificationType type) { ChromeThread::UI, FROM_HERE, new ChildNotificationTask(type, this)); } -bool ChildProcessHost::DidChildCrash() { - return child_process_->DidProcessCrash(); +void ChildProcessHost::DetermineDidChildCrash() { + child_process_->DetermineDidProcessCrash(); } void ChildProcessHost::OnChildDied() { + // Either of these paths will lead to deleting this object in + // OnDidProcessCrashDetermined, so they should be the last calls in this + // method. + if (handle() != base::kNullProcessHandle) { + // Determine whether the process crashed or not. This method will invoke + // OnDidProcessCrashDetermined with the result. This may occur + // asynchronously. + DetermineDidChildCrash(); + } else { + // We do not have a process, so it couldn't have crashed. + OnDidProcessCrashDetermined(false); + } +} + +void ChildProcessHost::OnDidProcessCrashDetermined(bool did_crash) { + if (did_crash) { + OnProcessCrashed(); + // Report that this child process crashed. + Notify(NotificationType::CHILD_PROCESS_CRASHED); + UMA_HISTOGRAM_COUNTS("ChildProcess.Crashes", this->type()); + } if (handle() != base::kNullProcessHandle) { - bool did_crash = DidChildCrash(); - if (did_crash) { - OnProcessCrashed(); - // Report that this child process crashed. - Notify(NotificationType::CHILD_PROCESS_CRASHED); - UMA_HISTOGRAM_COUNTS("ChildProcess.Crashes", this->type()); - } // Notify in the main loop of the disconnection. Notify(NotificationType::CHILD_PROCESS_HOST_DISCONNECTED); } - delete this; } @@ -278,6 +291,11 @@ void ChildProcessHost::ListenerHook::OnProcessLaunched() { host_->OnProcessLaunched(); } +void ChildProcessHost::ListenerHook::OnDidProcessCrashDetermined( + bool did_crash) { + host_->OnDidProcessCrashDetermined(did_crash); +} + ChildProcessHost::Iterator::Iterator() : all_(true), type_(UNKNOWN_PROCESS) { diff --git a/chrome/browser/child_process_host.h b/chrome/browser/child_process_host.h index e735940..7f0509a 100644 --- a/chrome/browser/child_process_host.h +++ b/chrome/browser/child_process_host.h @@ -113,6 +113,7 @@ class ChildProcessHost : public ResourceDispatcherHost::Receiver, // ChildProcessLauncher::Client implementation. virtual void OnProcessLaunched() {} + virtual void OnDidProcessCrashDetermined(bool did_crash); // Derived classes can override this to know if the process crashed. virtual void OnProcessCrashed() {} @@ -120,7 +121,10 @@ class ChildProcessHost : public ResourceDispatcherHost::Receiver, bool opening_channel() { return opening_channel_; } const std::string& channel_id() { return channel_id_; } - virtual bool DidChildCrash(); + // Determines whether the exited process crashed or exited normally. + // OnDidProcessCrashDetermined method will be called with the answer, + // possibly asynchronously. + virtual void DetermineDidChildCrash(); // Called when the child process goes away. virtual void OnChildDied(); @@ -140,6 +144,7 @@ class ChildProcessHost : public ResourceDispatcherHost::Receiver, virtual void OnChannelConnected(int32 peer_pid); virtual void OnChannelError(); virtual void OnProcessLaunched(); + virtual void OnDidProcessCrashDetermined(bool did_crash); private: ChildProcessHost* host_; }; diff --git a/chrome/browser/child_process_launcher.cc b/chrome/browser/child_process_launcher.cc index 206d8a7..c60f766 100644 --- a/chrome/browser/child_process_launcher.cc +++ b/chrome/browser/child_process_launcher.cc @@ -182,14 +182,14 @@ class ChildProcessLauncher::Context client_thread_id_, FROM_HERE, NewRunnableMethod( this, - &ChildProcessLauncher::Context::Notify, + &ChildProcessLauncher::Context::NotifyProcessLaunched, #if defined(OS_LINUX) use_zygote, #endif handle)); } - void Notify( + void NotifyProcessLaunched( #if defined(OS_LINUX) bool zygote, #endif @@ -206,7 +206,61 @@ class ChildProcessLauncher::Context } } + void DetermineDidProcessCrash() { + DCHECK(ChromeThread::CurrentlyOn(client_thread_id_)); + base::ProcessHandle handle = process_.handle(); +#if defined(OS_LINUX) + if (zygote_) { + ChromeThread::PostTask( + ChromeThread::PROCESS_LAUNCHER, FROM_HERE, + NewRunnableMethod(this, + &Context::DetermineDidProcessCrashInternal, + handle)); + } else +#endif + { + DetermineDidProcessCrashInternal(handle); + } + } + + void DetermineDidProcessCrashInternal(base::ProcessHandle handle) { + bool did_crash, child_exited; +#if defined(OS_LINUX) + if (zygote_) { + did_crash = Singleton()->DidProcessCrash(handle, + &child_exited); + ChromeThread::PostTask( + client_thread_id_, FROM_HERE, + NewRunnableMethod(this, + &Context::OnDidProcessCrashDetermined, + child_exited, did_crash)); + } else +#endif + { + did_crash = base::DidProcessCrash(&child_exited, handle); + OnDidProcessCrashDetermined(child_exited, did_crash); + } + } + + void OnDidProcessCrashDetermined(bool child_exited, bool did_crash) { + DCHECK(ChromeThread::CurrentlyOn(client_thread_id_)); + + // POSIX: If the process crashed, then the kernel closed the socket for it + // and so the child has already died by the time we get here. Since + // DidProcessCrash called waitpid with WNOHANG, it'll reap the process. + // However, if DidProcessCrash didn't reap the child, we'll need to in + // Terminate via ProcessWatcher. So we can't close the handle here. + if (child_exited) + process_.Close(); + + if (client_) + client_->OnDidProcessCrashDetermined(did_crash); + // The client may have deleted us in the callback. Do not add anything + // after this point. + } + void Terminate() { + DCHECK(ChromeThread::CurrentlyOn(client_thread_id_)); if (!process_.handle()) return; @@ -295,27 +349,8 @@ base::ProcessHandle ChildProcessLauncher::GetHandle() { return context_->process_.handle(); } -bool ChildProcessLauncher::DidProcessCrash() { - bool did_crash, child_exited; - base::ProcessHandle handle = context_->process_.handle(); -#if defined(OS_LINUX) - if (context_->zygote_) { - did_crash = Singleton()->DidProcessCrash(handle, &child_exited); - } else -#endif - { - did_crash = base::DidProcessCrash(&child_exited, handle); - } - - // POSIX: If the process crashed, then the kernel closed the socket for it - // and so the child has already died by the time we get here. Since - // DidProcessCrash called waitpid with WNOHANG, it'll reap the process. - // However, if DidProcessCrash didn't reap the child, we'll need to in - // Terminate via ProcessWatcher. So we can't close the handle here. - if (child_exited) - context_->process_.Close(); - - return did_crash; +void ChildProcessLauncher::DetermineDidProcessCrash() { + context_->DetermineDidProcessCrash(); } void ChildProcessLauncher::SetProcessBackgrounded(bool background) { diff --git a/chrome/browser/child_process_launcher.h b/chrome/browser/child_process_launcher.h index ce1c04d..01c5d16 100644 --- a/chrome/browser/child_process_launcher.h +++ b/chrome/browser/child_process_launcher.h @@ -21,6 +21,11 @@ class ChildProcessLauncher { // Will be called on the thread that the ChildProcessLauncher was // constructed on. virtual void OnProcessLaunched() = 0; + + // Called as a response to DetermineDidProcessCrash on the thread that + // the ChildProcessLauncher was constructed on. This may be called + // during DetermineDidProcessCrash or asynchronously. + virtual void OnDidProcessCrashDetermined(bool did_crash) = 0; }; // Launches the process asynchronously, calling the client when the result is @@ -46,8 +51,10 @@ class ChildProcessLauncher { // Getter for the process handle. Only call after the process has started. base::ProcessHandle GetHandle(); - // Call this when the process exits to know if a process crashed or not. - bool DidProcessCrash(); + // Determines whether the exited process crashed or exited normally. + // The Client's OnDidProcessCrashDetermined method will be called with the + // answer. + void DetermineDidProcessCrash(); // Changes whether the process runs in the background or not. Only call // this after the process has started. diff --git a/chrome/browser/metrics/metrics_service.cc b/chrome/browser/metrics/metrics_service.cc index ad24a11..38833a0 100644 --- a/chrome/browser/metrics/metrics_service.cc +++ b/chrome/browser/metrics/metrics_service.cc @@ -481,7 +481,7 @@ void MetricsService::SetRecording(bool enabled) { NotificationService::AllSources()); registrar_.Add(this, NotificationType::LOAD_STOP, NotificationService::AllSources()); - registrar_.Add(this, NotificationType::RENDERER_PROCESS_CLOSED, + registrar_.Add(this, NotificationType::RENDERER_PROCESS_CRASHED, NotificationService::AllSources()); registrar_.Add(this, NotificationType::RENDERER_PROCESS_HANG, NotificationService::AllSources()); @@ -557,17 +557,13 @@ void MetricsService::Observe(NotificationType type, LogLoadStarted(); break; - case NotificationType::RENDERER_PROCESS_CLOSED: + case NotificationType::RENDERER_PROCESS_CRASHED: { - RenderProcessHost::RendererClosedDetails* process_details = - Details(details).ptr(); - if (process_details->did_crash) { - if (process_details->was_extension_renderer) { - LogExtensionRendererCrash(); - } else { - LogRendererCrash(); - } - } + // The details are whether it is an extension process. + if (*Details(details).ptr()) + LogExtensionRendererCrash(); + else + LogRendererCrash(); } break; diff --git a/chrome/browser/nacl_host/nacl_process_host.cc b/chrome/browser/nacl_host/nacl_process_host.cc index 2648b3e..0310e39 100644 --- a/chrome/browser/nacl_host/nacl_process_host.cc +++ b/chrome/browser/nacl_host/nacl_process_host.cc @@ -117,10 +117,19 @@ void NaClProcessHost::OnProcessLaunchedByBroker(base::ProcessHandle handle) { OnProcessLaunched(); } -bool NaClProcessHost::DidChildCrash() { - if (running_on_wow64_) - return base::DidProcessCrash(NULL, handle()); - return ChildProcessHost::DidChildCrash(); +void NaClProcessHost::DetermineDidChildCrash() { + // Either of these paths will lead to deleting this object in + // OnDidProcessCrashDetermined in ChildProcessHost, so they should be the + // last calls in this method. + if (running_on_wow64_) { + bool did_crash = base::DidProcessCrash(NULL, handle()); + OnDidProcessCrashDetermined(did_crash); + } else { + // Determine whether the process crashed or not. This method will invoke + // OnDidProcessCrashDetermined with the result. This may occur + // asynchronously. + ChildProcessHost::DetermineDidChildCrash(); + } } void NaClProcessHost::OnChildDied() { diff --git a/chrome/browser/nacl_host/nacl_process_host.h b/chrome/browser/nacl_host/nacl_process_host.h index 28893fc..b6c3313 100644 --- a/chrome/browser/nacl_host/nacl_process_host.h +++ b/chrome/browser/nacl_host/nacl_process_host.h @@ -36,7 +36,8 @@ class NaClProcessHost : public ChildProcessHost { void OnProcessLaunchedByBroker(base::ProcessHandle handle); protected: - virtual bool DidChildCrash(); + // Override ChildProcessHost methods. + virtual void DetermineDidChildCrash(); virtual void OnChildDied(); private: diff --git a/chrome/browser/renderer_host/browser_render_process_host.cc b/chrome/browser/renderer_host/browser_render_process_host.cc index fff6622..f7fb092 100644 --- a/chrome/browser/renderer_host/browser_render_process_host.cc +++ b/chrome/browser/renderer_host/browser_render_process_host.cc @@ -860,23 +860,12 @@ void BrowserRenderProcessHost::OnChannelError() { if (!channel_.get()) return; - // NULL in single process mode or if fast termination happened. - bool did_crash = - child_process_.get() ? child_process_->DidProcessCrash() : false; - - if (did_crash) { - UMA_HISTOGRAM_PERCENTAGE("BrowserRenderProcessHost.ChildCrashes", - extension_process_ ? 2 : 1); - } - - RendererClosedDetails details(did_crash, extension_process_); NotificationService::current()->Notify( NotificationType::RENDERER_PROCESS_CLOSED, Source(this), - Details(&details)); + NotificationService::NoDetails()); WebCacheManager::GetInstance()->Remove(id()); - child_process_.reset(); channel_.reset(); IDMap::iterator iter(&listeners_); @@ -888,6 +877,32 @@ void BrowserRenderProcessHost::OnChannelError() { ClearTransportDIBCache(); + if (child_process_.get()) { + // Determine whether the process crashed or not. This method will invoke + // OnDidProcessCrashDetermined with the result. This may occur + // asynchronously. + child_process_->DetermineDidProcessCrash(); + } else { + // This occurs in single process mode or if fast termination happened. + // Manually trigger OnDidProcessCrashDetermined, because we know this is + // not a crash. + OnDidProcessCrashDetermined(false); + } +} + +void BrowserRenderProcessHost::OnDidProcessCrashDetermined(bool did_crash) { + child_process_.reset(); + + if (did_crash) { + UMA_HISTOGRAM_PERCENTAGE("BrowserRenderProcessHost.ChildCrashes", + extension_process_ ? 2 : 1); + + NotificationService::current()->Notify( + NotificationType::RENDERER_PROCESS_CRASHED, + Source(this), + Details(&extension_process_)); + } + // this object is not deleted at this point and may be reused later. // TODO(darin): clean this up } diff --git a/chrome/browser/renderer_host/browser_render_process_host.h b/chrome/browser/renderer_host/browser_render_process_host.h index 2a7f8b6..00c2a02 100644 --- a/chrome/browser/renderer_host/browser_render_process_host.h +++ b/chrome/browser/renderer_host/browser_render_process_host.h @@ -100,6 +100,7 @@ class BrowserRenderProcessHost : public RenderProcessHost, // ChildProcessLauncher::Client implementation. virtual void OnProcessLaunched(); + virtual void OnDidProcessCrashDetermined(bool did_crash); private: friend class VisitRelayingRenderProcessHost; diff --git a/chrome/browser/renderer_host/render_process_host.h b/chrome/browser/renderer_host/render_process_host.h index 1ceeb42..1eb9804 100644 --- a/chrome/browser/renderer_host/render_process_host.h +++ b/chrome/browser/renderer_host/render_process_host.h @@ -46,16 +46,6 @@ class RenderProcessHost : public IPC::Channel::Sender, TYPE_EXTENSION, // Renderer with extension privileges. }; - // Details for RENDERER_PROCESS_CLOSED notifications. - struct RendererClosedDetails { - RendererClosedDetails(bool did_crash, bool was_extension_renderer) { - this->did_crash = did_crash; - this->was_extension_renderer = was_extension_renderer; - } - bool did_crash; - bool was_extension_renderer; - }; - explicit RenderProcessHost(Profile* profile); virtual ~RenderProcessHost(); diff --git a/chrome/browser/zygote_host_linux.cc b/chrome/browser/zygote_host_linux.cc index 9885234..f28e2ad 100644 --- a/chrome/browser/zygote_host_linux.cc +++ b/chrome/browser/zygote_host_linux.cc @@ -19,6 +19,7 @@ #include "base/string_util.h" #include "base/unix_domain_socket_posix.h" +#include "chrome/browser/chrome_thread.h" #include "chrome/browser/renderer_host/render_sandbox_host_linux.h" #include "chrome/common/chrome_constants.h" #include "chrome/common/chrome_switches.h" @@ -193,6 +194,7 @@ void ZygoteHost::Init(const std::string& sandbox_cmd) { pid_t ZygoteHost::ForkRenderer( const std::vector& argv, const base::GlobalDescriptors::Mapping& mapping) { + DCHECK(ChromeThread::CurrentlyOn(ChromeThread::PROCESS_LAUNCHER)); DCHECK(init_); Pickle pickle; @@ -241,6 +243,7 @@ pid_t ZygoteHost::ForkRenderer( } void ZygoteHost::EnsureProcessTerminated(pid_t process) { + DCHECK(ChromeThread::CurrentlyOn(ChromeThread::PROCESS_LAUNCHER)); DCHECK(init_); Pickle pickle; @@ -253,6 +256,7 @@ void ZygoteHost::EnsureProcessTerminated(pid_t process) { bool ZygoteHost::DidProcessCrash(base::ProcessHandle handle, bool* child_exited) { + DCHECK(ChromeThread::CurrentlyOn(ChromeThread::PROCESS_LAUNCHER)); DCHECK(init_); Pickle pickle; pickle.WriteInt(kCmdDidProcessCrash); diff --git a/chrome/browser/zygote_host_linux.h b/chrome/browser/zygote_host_linux.h index 53485e0..21118d5 100644 --- a/chrome/browser/zygote_host_linux.h +++ b/chrome/browser/zygote_host_linux.h @@ -26,15 +26,19 @@ class ZygoteHost { public: void Init(const std::string& sandbox_cmd); - // Tries to start a renderer process. Returns its pid on success, otherwise - // base::kNullProcessHandle; + // Tries to start a renderer process. Returns its pid on success, otherwise + // base::kNullProcessHandle. Must be called on the PROCESS_LAUNCHER + // thread. pid_t ForkRenderer(const std::vector& command_line, const base::GlobalDescriptors::Mapping& mapping); + + // Must be called on the PROCESS_LAUNCHER thread. void EnsureProcessTerminated(pid_t process); // Get the termination status (exit code) of the process and return true if // the status indicates the process crashed. |child_exited| is set to true // iff the child process has terminated. (|child_exited| may be NULL.) + // Must be called on the PROCESS_LAUNCHER thread. bool DidProcessCrash(base::ProcessHandle handle, bool* child_exited); // These are the command codes used on the wire between the browser and the @@ -52,7 +56,10 @@ class ZygoteHost { ZygoteHost(); ~ZygoteHost(); - int control_fd_; // the socket to the zygote + // The socket to the zygote, which should only be accessed on the + // PROCESS_LAUNCHER thread. + int control_fd_; + pid_t pid_; bool init_; bool using_suid_sandbox_; -- cgit v1.1