Simplify & cleanup //base process termination APIs |
||||||||||
Issue descriptionThere 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.
,
Jan 27 2018
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().
,
Jan 29 2018
,
Jan 29 2018
,
Jan 29 2018
,
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
,
Feb 7 2018
,
Feb 7 2018
,
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
,
Feb 25 2018
,
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
,
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
,
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.
,
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
,
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
,
Jun 12 2018
,
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
,
Aug 29
+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 |
||||||||||
Comment 1 by w...@chromium.org
, Jan 27 2018