New issue
Advanced search Search tips

Issue 818244 link

Starred by 1 user

Issue metadata

Status: Untriaged
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Chrome , Mac
Pri: 3
Type: Bug



Sign in to add a comment

ChildProcessLauncherHelper::Terminate() does not ensure termination, nor reap the process

Project Member Reported by w...@chromium.org, Mar 2 2018

Issue description

ChildProcessLauncherHelper provides two process-termination APIs: Terminate() and ForceNormalProcessTerminationAsync().

Under Linux/ChromeOS and Mac, the latter invokes Process::Terminate() on the process, then uses the POSIX-specific EnsureProcessTerminated() call to have the process monitored for exit for a few seconds, and then forcibly terminated if necessary, and the process-id properly reaped.

ChildProcessLauncherHelper::Terminate() currently only invokes Process::Terminate(), which only dispatches a SIGTERM to the child process, rather than SIGKILL, meaning the child will exit teardown code which may take a long time, or may even fail to exit entirely.  There is also no explicit attempt in the CPLH::Terminate() call to reap the process-id; IIUC we are relying on a later call to ForceNormalProcessTerminationAsync() from the ChildProcessLauncher destructor to do that.

We should consider making the two APIs consistent wrt ensuring termination, and pid-reaping.
 

Comment 1 by wfh@chromium.org, Mar 2 2018

Cc: wfh@chromium.org
hmm I thought that 'alraedy_dead' changed this call from SIGTERM to SIGKILL to ensure termination...?

Comment 2 by w...@chromium.org, Mar 2 2018

Re #1: If you mean the |already_dead| parameter to RPHI::ProcessDied(), then IIUC that only affects (1) whether we block waiting for the exit status of the child, and (2) whether we adjust the reported exit |status| (based on https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_process_host_impl.cc?sq=package:chromium&l=3648).

There are only two places I see SIGKILL explicitly mentioned, one of which is Mac-specific WaitForChildToDie, and the other is Process::Terminate(wait=true). The latter is not something we typically use in production code, AFAICT.
Cc: ellyjo...@chromium.org
+ellyjones as Macspert.

Sign in to add a comment