New issue
Advanced search Search tips

Issue 806451 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Simplify & cleanup //base process termination APIs

Project Member Reported by w...@chromium.org, Jan 27 2018

Issue description

There are a surprising number of base::Process::Current().Terminate() call-sites. Since we also have base::Process::TerminateCurrentProcessImmediately(), we should switch to using one or other version consistently everywhere.
 

Comment 1 by w...@chromium.org, Jan 27 2018

Cc: haraken@chromium.org
It looks like you added TerminateCurrentProcessImmediately in https://codereview.chromium.org/2629623003.

Comment 2 by w...@chromium.org, Jan 27 2018

Cc: primiano@chromium.org thakis@chromium.org rsesek@chromium.org
Reviewing the set of process-termination calls we have in //base, to diagnose  issue 750756 , we have:

base::Process::Terminate()
- Documented as "terminate with extreme prejudice", but on POSIX actually sends SIGTERM first.
- Has an |option| to wait for the process, essentially in-built WaitForExit(), but with a separate implementation.
- Under Windows and Fuchsia, |wait| can block for up to 60s.
- Under POSIX, |wait| blocks for <60s, at the end of which process is SIGKILL'd but _not_ waited-for.
- On all platforms can terminate the current process, though POSIX & Fuchsia impls differ from TerminateCurrentProcessImmediately.

base::Process::WaitForExit[WithTimeout]()
- Different implementation(s) from the waiting inside Terminate() and EnsureProcessTerminated().

base::Process::TerminateCurrentProcessImmediately()
- Immediately exits the current process (POSIX & Win variants)

base::EnsureProcessTerminated()
- On Mac+Fuchsia, blocks for up to 2s, then kills process forcibly.
- On Linux, spawns a thread that waits for up to 2s, then reaps.
- On Windows, posts a task that kills the process after 2s.
- Used in ChildProcessLauncherHelper, which runs it on the launcher thread because "On Posix, EnsureProcessTerminated can lead to 2 seconds of sleep!"


To reduce the amount of code duplication and subtle differences, how about:
1. Provide both Terminate() and a TerminateImmediately().
  - These really just distinguish the POSIX SIGTERM vs SIGKILL.
2. Have all Terminate() implementations use WaitForExitWithTimeout() to implement |wait|.
3. Replace TerminateCurrentProcessImmediately with special-case handling of Process::Current() in TerminateImmediately().
4. Remove EnsureProcessTerminated() call-sites, or reimplement using TerminateImmediately()+WaitForExit().

Comment 3 by w...@chromium.org, Jan 29 2018

Cc: scottmg@chromium.org

Comment 4 by w...@chromium.org, Jan 29 2018

Labels: -Pri-3 M-66 Pri-2

Comment 5 by w...@chromium.org, Jan 29 2018

Components: Internals
Project Member

Comment 6 by bugdroid1@chromium.org, Jan 29 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/996812e1c9b72d29bac5cf688d9f89a7b9ffa535

commit 996812e1c9b72d29bac5cf688d9f89a7b9ffa535
Author: Wez <wez@chromium.org>
Date: Mon Jan 29 20:00:49 2018

Workaround for EnsureProcessTerminated to address ProcessUtilTest failures.

Bug:  750756 , 806451
Change-Id: I60ab7ca8a99bd1845492d7486e811f68105116f2
Reviewed-on: https://chromium-review.googlesource.com/890109
Reviewed-by: Scott Graham <scottmg@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532558}
[modify] https://crrev.com/996812e1c9b72d29bac5cf688d9f89a7b9ffa535/base/process/kill_fuchsia.cc

Comment 7 by w...@chromium.org, Feb 7 2018

Labels: -Pri-2 -M-66 Pri-3

Comment 8 by w...@chromium.org, Feb 7 2018

Status: Started (was: Assigned)
Project Member

Comment 9 by bugdroid1@chromium.org, Feb 8 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9a0f72327d0be98c7d8360f4bfced1a427fd4f56

commit 9a0f72327d0be98c7d8360f4bfced1a427fd4f56
Author: Wez <wez@chromium.org>
Date: Thu Feb 08 17:14:29 2018

Replace some Process::Current().Terminate() calls.

We provide Process::TerminateCurrentProcessImmediately() for the special
case of a process wishing to immediately terminate itself, without
running the normal exit() code-paths. This allows the callers to benefit
from potential for [[noreturn]] optimizations.

Bug: 806451
Change-Id: I0b49f3572d06591b5cee50f04dc4f5a79e1d5dc6
Reviewed-on: https://chromium-review.googlesource.com/907675
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535416}
[modify] https://crrev.com/9a0f72327d0be98c7d8360f4bfced1a427fd4f56/chrome/browser/lifetime/application_lifetime.cc
[modify] https://crrev.com/9a0f72327d0be98c7d8360f4bfced1a427fd4f56/content/child/child_thread_impl.cc
[modify] https://crrev.com/9a0f72327d0be98c7d8360f4bfced1a427fd4f56/content/public/test/network_service_test_helper.cc
[modify] https://crrev.com/9a0f72327d0be98c7d8360f4bfced1a427fd4f56/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/9a0f72327d0be98c7d8360f4bfced1a427fd4f56/content/shell/utility/shell_content_utility_client.cc
[modify] https://crrev.com/9a0f72327d0be98c7d8360f4bfced1a427fd4f56/net/tools/stress_cache/stress_cache.cc

Comment 10 by w...@chromium.org, Feb 25 2018

Summary: Simplify & cleanup //base process termination APIs (was: Replace base::Process::Current().Terminate() calls with base::Process::TerminateCurrentProcessImmediately())
Project Member

Comment 11 by bugdroid1@chromium.org, Feb 26 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/20d3636958565e7ae0327b7317d1ae6909592ee7

commit 20d3636958565e7ae0327b7317d1ae6909592ee7
Author: Wez <wez@chromium.org>
Date: Mon Feb 26 22:10:22 2018

Simplify POSIX Process::Terminate and process-reaping helper APIs.

Remove some duplication of code responsible for waiting for processes
to exit on POSIX:
- Use Process::WaitForExit[WithTimeout]() inside
  Process::Terminate(wait=true), to wait for the process to exit in
  response to SIGTERM, before giving up and issuing a SIGKILL.
- Process::Terminate() will no longer record the process as Exited()
  except when wait=true and the process did actually exit.
- The EnsureProcessTerminated() and EnsureProcessReaped() helpers are
  also migrated to use Process::WaitForExit[WithTimeout]() internally.

Bug: 806451
Change-Id: I69b4b18ab040adcb1339a55036b679e6cd6809d2
Reviewed-on: https://chromium-review.googlesource.com/920922
Commit-Queue: Wez <wez@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Brian White <bcwhite@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539282}
[modify] https://crrev.com/20d3636958565e7ae0327b7317d1ae6909592ee7/base/process/kill_posix.cc
[modify] https://crrev.com/20d3636958565e7ae0327b7317d1ae6909592ee7/base/process/process_posix.cc
[modify] https://crrev.com/20d3636958565e7ae0327b7317d1ae6909592ee7/base/test/launcher/test_launcher.cc
[modify] https://crrev.com/20d3636958565e7ae0327b7317d1ae6909592ee7/chromeos/process_proxy/process_proxy.cc
[modify] https://crrev.com/20d3636958565e7ae0327b7317d1ae6909592ee7/chromeos/process_proxy/process_proxy.h
[modify] https://crrev.com/20d3636958565e7ae0327b7317d1ae6909592ee7/chromeos/process_proxy/process_proxy_unittest.cc
[modify] https://crrev.com/20d3636958565e7ae0327b7317d1ae6909592ee7/content/browser/frame_host/frame_tree_browsertest.cc
[modify] https://crrev.com/20d3636958565e7ae0327b7317d1ae6909592ee7/net/test/spawned_test_server/local_test_server.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Mar 3 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0abfbf51fbbeacf29e3c8dc2870b7c845590971e

commit 0abfbf51fbbeacf29e3c8dc2870b7c845590971e
Author: Wez <wez@chromium.org>
Date: Sat Mar 03 01:54:45 2018

Remove |wait| parameter from RenderProcessHost::Shutdown() API.

The Shutdown() API was added to RenderProcessHost with parameters
reflecting those of the underlying base::Process::Terminate() API,
which include a synchronous |wait| option.

We never use Shutdown(.., wait=true) in production code, and in tests
we can instead use a RenderProcessHostObserver to wait for a process
to crash or exit, so we can remove the |wait| parameter and simplify
both callers and the implementation.

Bug: 806451
Change-Id: I2aeae32a3d20488b4970cf96959eaa287642b7ed
Reviewed-on: https://chromium-review.googlesource.com/936450
Commit-Queue: Wez <wez@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540713}
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/chrome/browser/android/hung_renderer_infobar_delegate.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/chrome/browser/apps/guest_view/web_view_browsertest.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/chrome/browser/chrome_navigation_browsertest.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/chrome/browser/extensions/extension_crash_recovery_browsertest.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/chrome/browser/policy/policy_browsertest.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/chrome/browser/printing/print_browsertest.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/chrome/browser/renderer_host/site_per_process_text_input_browsertest.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/chrome/browser/ui/browser_browsertest.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/chrome/browser/ui/cocoa/hung_renderer_controller.mm
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/chrome/browser/ui/views/hung_renderer_view.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/chrome/browser/ui/views/sad_tab_view_interactive_uitest.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/chrome/browser/ui/zoom/zoom_controller_browsertest.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/chrome/browser/web_bluetooth_browsertest.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/content/browser/child_process_launcher.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/content/browser/child_process_launcher.h
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/content/browser/child_process_launcher_helper.h
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/content/browser/child_process_launcher_helper_android.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/content/browser/child_process_launcher_helper_fuchsia.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/content/browser/child_process_launcher_helper_linux.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/content/browser/child_process_launcher_helper_mac.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/content/browser/child_process_launcher_helper_win.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/content/browser/child_process_security_policy_browsertest.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/content/browser/frame_host/frame_tree_browsertest.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/content/browser/frame_host/render_frame_host_impl_browsertest.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/content/browser/frame_host/render_frame_host_manager_browsertest.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/content/browser/indexed_db/indexed_db_browsertest.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/content/browser/service_worker/service_worker_browsertest.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/content/browser/site_per_process_hit_test_browsertest.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/content/browser/wake_lock/wake_lock_browsertest.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/content/public/browser/browser_message_filter.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/content/public/browser/render_process_host.h
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/content/public/test/browser_test_utils.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/content/public/test/mock_render_process_host.cc
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/content/public/test/mock_render_process_host.h
[modify] https://crrev.com/0abfbf51fbbeacf29e3c8dc2870b7c845590971e/extensions/browser/guest_view/web_view/web_view_guest.cc

Comment 13 by w...@chromium.org, Mar 6 2018

Landing CL https://chromium-review.googlesource.com/920922 causes us to hit the NOTIMPLEMENTED() warning from https://cs.chromium.org/chromium/src/base/process/process_posix.cc?sq=package:chromium&l=199 during tests like SitePerProcessPrintBrowserTest.SubframeUnavailableDuringPrint.

This appears to originate from RenderProcessHostImpl::Shutdown(wait=true) calling Terminate(), which in the new implementation calls on to WaitForExitWithTimeout() if |wait=true|.  Because the process is actually child of the zygote process it cannot be waited-on by the browser process via waitpid(), hence the NOTIMPLEMENTED().

The NOTIMPLEMENTED() should probably be made a NOTREACHED(), but we should also be able to handle the special-case of a zero-length timeout, i.e. polling to determine process exit, without actually calling waitpid() - e.g. see the GetParentProcessId() call at the top of WaitForExitWithTimeoutImpl().

However, the wait=true parameter was removed from the Shutdown() API by https://chromium-review.googlesource.com/936450, so at this point I'd be inclined to just make the NOTIMPLEMENTED()->NOTREACHED() change.

Project Member

Comment 14 by bugdroid1@chromium.org, Apr 2 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c18a57c2b9c89230c5f67d2a06ec527499db0cfd

commit c18a57c2b9c89230c5f67d2a06ec527499db0cfd
Author: Wez <wez@chromium.org>
Date: Mon Apr 02 20:20:14 2018

Simplify EnsureProcessTerminated() implementations.

EnsureProcessTerminated() is used by a parent process to ensure that a
child process it expects should exit "soon" actually does so. The child
process is asynchronously monitored, and forcibly terminated if it
fails to exit in a timely manner. Under POSIX platforms the child
process Id is also reaped with waitpid(), to release its exited
"zombie" process structure.

We had ended up with separate implementations for each platform, each
with different properties.

This CL makes the following changes:
- Reimplements POSIX EnsureProcessTerminated using base::Process.
- Provides a common implementation for platforms on which there is no
  need to waitpid() to cleanup zombie processes.

The Mac specialization will be replaced with an asynchronous
implementation in a subsequent patch.

Bug: 806451,  750756 
Change-Id: If251dc13e7ad0a0cffb4f1921897a89305d6cb21
Reviewed-on: https://chromium-review.googlesource.com/920799
Commit-Queue: Wez <wez@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547508}
[modify] https://crrev.com/c18a57c2b9c89230c5f67d2a06ec527499db0cfd/base/process/kill.cc
[modify] https://crrev.com/c18a57c2b9c89230c5f67d2a06ec527499db0cfd/base/process/kill.h
[modify] https://crrev.com/c18a57c2b9c89230c5f67d2a06ec527499db0cfd/base/process/kill_fuchsia.cc
[modify] https://crrev.com/c18a57c2b9c89230c5f67d2a06ec527499db0cfd/base/process/kill_posix.cc
[modify] https://crrev.com/c18a57c2b9c89230c5f67d2a06ec527499db0cfd/base/process/kill_win.cc
[modify] https://crrev.com/c18a57c2b9c89230c5f67d2a06ec527499db0cfd/base/process/process_posix.cc
[modify] https://crrev.com/c18a57c2b9c89230c5f67d2a06ec527499db0cfd/base/process/process_util_unittest.cc
[modify] https://crrev.com/c18a57c2b9c89230c5f67d2a06ec527499db0cfd/chrome/browser/extensions/api/messaging/native_message_process_host.cc
[modify] https://crrev.com/c18a57c2b9c89230c5f67d2a06ec527499db0cfd/chrome/browser/platform_util_linux.cc
[modify] https://crrev.com/c18a57c2b9c89230c5f67d2a06ec527499db0cfd/chrome/browser/printing/printer_manager_dialog_linux.cc
[modify] https://crrev.com/c18a57c2b9c89230c5f67d2a06ec527499db0cfd/chrome/browser/ui/webui/settings_utils_linux.cc
[modify] https://crrev.com/c18a57c2b9c89230c5f67d2a06ec527499db0cfd/content/browser/child_process_launcher_helper_linux.cc
[modify] https://crrev.com/c18a57c2b9c89230c5f67d2a06ec527499db0cfd/content/browser/zygote_host/zygote_host_impl_linux.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Jun 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c5ee6b94056e4cb7fa55b2d2bcd9652829673ba1

commit c5ee6b94056e4cb7fa55b2d2bcd9652829673ba1
Author: Wez <wez@chromium.org>
Date: Tue Jun 12 01:58:25 2018

Replace custom TerminateSelf with TerminateCurrentProcessImmediately.

Since TerminateCurrentProcessImmediately is not supported on iOS,
and this functionality is only used by the crash_cache test tool, which
we never run on that platform, GenerateCrash() is switched to ignore
iOS.

Bug: 806451
Change-Id: I768723a935b494218bf466da4ba395101ad0addd
Reviewed-on: https://chromium-review.googlesource.com/1089480
Commit-Queue: Wez <wez@chromium.org>
Reviewed-by: Josh Karlin <jkarlin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566264}
[modify] https://crrev.com/c5ee6b94056e4cb7fa55b2d2bcd9652829673ba1/net/disk_cache/blockfile/rankings.cc

Comment 16 by w...@chromium.org, Jun 12 2018

Status: Assigned (was: Started)
Project Member

Comment 17 by bugdroid1@chromium.org, Jun 27 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b63f00de4179518540f06dacb6108d7e21091e45

commit b63f00de4179518540f06dacb6108d7e21091e45
Author: Wez <wez@chromium.org>
Date: Wed Jun 27 16:46:36 2018

Fix chrome://kill debug URL to simulate process kills correctly.

The implementation of this debug URL was changed in
https://chromium-review.googlesource.com/c/chromium/src/+/907675 to one
which _exit()s the process under POSIX, rather than self-terminating via
kill().  Since we detect process kills (versus self-termination, crashes
etc) by checking for SIGTERM, this broke chrome://kill.

Fix chrome://kill by explicitly kill()ing the process, and structure the
implementation to make clear that we need to simulate termination by a
peer process, specifically.

Bug: 806451,  854926 
Change-Id: I0408dd1d248e04b3045f37d46051c220b33ab081
Reviewed-on: https://chromium-review.googlesource.com/1116076
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570801}
[modify] https://crrev.com/b63f00de4179518540f06dacb6108d7e21091e45/content/renderer/render_frame_impl.cc

Cc: ellyjo...@chromium.org
Owner: ----
Status: Available (was: Assigned)
+ellyjones@ because Macspertize.

As part of https://chromium-review.googlesource.com/920799 I hoped to switch Mac to use the same EnsureProcessTerminated() as other POSIX platforms, but it was felt that that was a potentially substantial performance regression, so we skipped that improvement.

If we want EnsureProcessTerminated() to work better under Mac then I'd suggest that a Macspert take over. :)

Sign in to add a comment