[Feature] Atomic shutdown |
|||||||||||
Issue descriptionCurrently, on some platforms, we go alllllll the way to the end of main() and beyond through static uninitialization. This is a problem because it can cause use-after-free on worker threads in code that uses globals which are eventually freed on the main thread. This is the reason to require TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN semantics in the first place, which people anecdotally find confusing. Step #1 (trivial): TerminateProcess() at the end of main() instead of letting it unwind and perform static uninitialization (which is pointless). Step #2 (requires thought): TerminateProcess() before AtExitManager kicks in. Prevents uninitialization of things like LazyInstances/Singletons. Nothing should really depend on destructor of such constructs (AFAIK things that use DestructorAtExit traits do so for unit testing reasons not to *actually* perform cleanup wayyyyy too late on shutdown -- FWIW Windows/Android never get there anyways). Step #3 Atomic Shutdown : Notify services that care that shutdown is happening, call TaskScheduler::Shutdown() which will terminate all the BLOCK_SHUTDOWN async work, then TerminateProcess() immediately :). *Nothing* runs after the shutdown phase, woohooooo! Bonus : Rename CONTINUE_ON_SHUTDOWN to IGNORE_ON_SHUTDOWN as it won't actually continue but will be terminated midflight if shutdown happens to complete while it's running :)
,
Jan 10 2018
Re #1: How have you verified that there is nothing in the static uninitialization that we need?
,
Jan 10 2018
@wez: e already don't go through it on Android (never shutdown) and Windows end-session (TerminateProcess even earlier then end of main). Anything that doesn't save state can't be useful. And anything that does *really* shouldn't wait until static uninitialization... I'm assuming the only done in this step is reclaiming resources which is useless when process is about to terminate (don't clean the carpets before burning down the building).
,
Jan 11 2018
I believe the renderers already do this [1], so this is "just" extending it to the other process types as well. [1] https://codereview.chromium.org/2309153002/
,
Jan 11 2018
I am broadly in favor of this, but we definitely need to proceed with care. Also, we still have a vested interest in avoiding static destructors because (unless we hack clang, which we certainly could do) any global variables with destructors will still generate code to call those destructors, which is wasteful. Note that on some machines (with third-party software) ::TerminateProcess(GetCurrentProcess(), exit_code); will actually return. This is insane, and there is nothing that we can do about it but I thought you should know.
,
Jan 12 2018
Noted about desire to avoid codegen despite prior termination. In general though I'd tend to assume that code for destructors of constexpr types (i.e. simple members) is simple as well. Noted about TerminateProcess possibly returning... Wow, reeeeallly!!!! At that point I guess the machine is probably undergoing a worse time than just wasteful static destruction... Le ven. 12 janv. 2018 00 h 30, brucedawson via monorail < monorail+v2.1410982004@chromium.org> a écrit :
,
Jan 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/19a74a4e72ee85c2c24336a7bb5f2024523ca8bd commit 19a74a4e72ee85c2c24336a7bb5f2024523ca8bd Author: Gabriel Charette <gab@chromium.org> Date: Mon Jan 15 09:33:53 2018 TerminateProcess instead of letting main unwind. Avoids going through wasteful and often problematic static uninitialization phase. Unfortunately can't do this in ContentMain because of embedders (namely test frameworks). Bug: 800808 Change-Id: I768b096a8dbea4804a94d0d50e7345d46c9ef781 Reviewed-on: https://chromium-review.googlesource.com/860617 Commit-Queue: Gabriel Charette <gab@chromium.org> Reviewed-by: Jochen Eisinger <jochen@chromium.org> Cr-Commit-Position: refs/heads/master@{#529233} [modify] https://crrev.com/19a74a4e72ee85c2c24336a7bb5f2024523ca8bd/chrome/app/chrome_main.cc
,
Jan 16 2018
I have a question regarding iOS. On iOS, it is forbidden for an application to terminate itself (either by returning from main or by calling exit() or similar methods). See https://developer.apple.com/library/content/qa/qa1561/_index.html for details. It is possible for the user to ask the application to terminate. The application will receive the -applicationWillTerminate: notification. If the method return in a given time interval (currently around 5 seconds), then the application is cleanly terminated -- the main method does not return, but global destructor are called and function registered with atexit() are invoked. If the method does not return, the application is killed by the OS. Currently, iOS application does go through some shutdown when terminated by the user while in foreground. See MainController -applicationWillTerminate:applicationNavigation:. I am not sure whether it is possible to have the application terminate properly without having global destructors invoked in real production application. In a test application, it is possible to call _exit() from -applicationWillTerminate: but I don' know whether this is allowed or not by the review process for application in the store.
,
Jan 16 2018
Are you saying that iOS invokes global destructors despite not unwinding main's stack? That sounds surprising to me. I'd assume they also TerminateProcess after notifying the app.
,
Jan 16 2018
I think the runtime invokes exit() after calling -applicationWillTerminate:, and not _exit(). You can find attached a sample iOS application that just create some globals with phony destructors (print on stdout) and that register some code to be invoked with atexit(). All the global destructors are run, but not the code after UIApplicationMain(). Output is (when running on a device and swiping the app while in the foreground, i.e. initiating the terminate via user gesture): Foo[0x102658d40]::Foo() Foo[0x102658d41]::Foo() -applicationWillTerminate:1 OnExit... Foo[0x102658d41]::~Foo() Foo[0x102658d40]::~Foo() If I insert a call to _exit() in -applicationWillTerminate:, then neither the atexit() registered code nor the global destructor are run. However, I don't know whether calling _exit() is allowed per app store policy (and the FAQ linked in #8 make me think it may not).
,
Jan 16 2018
Ah, hmmm, well that's interesting indeed. Good news is we're also trying to get rid of global destructors (i.e. support -Wexit-time-destructors). But atomic shutdown ideally goes beyond that and would help iOS too. The goal is to be able to just TaskScheduler::Shutdown() and terminate process. Which on iOS would translate to running TaskScheduler::Shutdown() before returning from -applicationWillTerminate() and knowing that all of chrome's state was properly shutdown (not teared down -- the tear down / cleanup / deletion phase is what is wasteful and we're trying to avoid).
,
Jan 24 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/00d4c5682096bf92e821637a697628fc40d9915c commit 00d4c5682096bf92e821637a697628fc40d9915c Author: Qiang(Joe) Xu <warx@chromium.org> Date: Wed Jan 24 18:36:13 2018 Revert "TerminateProcess instead of letting main unwind." This reverts commit 19a74a4e72ee85c2c24336a7bb5f2024523ca8bd. Reason for revert: crbug.com/805443 Original change's description: > TerminateProcess instead of letting main unwind. > > Avoids going through wasteful and often problematic static > uninitialization phase. > > Unfortunately can't do this in ContentMain because of embedders (namely > test frameworks). > > Bug: 800808 > Change-Id: I768b096a8dbea4804a94d0d50e7345d46c9ef781 > Reviewed-on: https://chromium-review.googlesource.com/860617 > Commit-Queue: Gabriel Charette <gab@chromium.org> > Reviewed-by: Jochen Eisinger <jochen@chromium.org> > Cr-Commit-Position: refs/heads/master@{#529233} TBR=gab@chromium.org,jochen@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 800808, 805443 Change-Id: Icb7b027db65ee5e39f240d52f13e9a025edc76cf Reviewed-on: https://chromium-review.googlesource.com/883603 Commit-Queue: Qiang(Joe) Xu <warx@chromium.org> Reviewed-by: Qiang(Joe) Xu <warx@chromium.org> Cr-Commit-Position: refs/heads/master@{#531616} [modify] https://crrev.com/00d4c5682096bf92e821637a697628fc40d9915c/chrome/app/chrome_main.cc
,
Jan 25 2018
,
Feb 13 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/23c73d55a5e5dc485a7bb02fece033d1d6987063 commit 23c73d55a5e5dc485a7bb02fece033d1d6987063 Author: Qiang(Joe) Xu <warx@chromium.org> Date: Tue Feb 13 07:34:32 2018 [Merge M65] Revert "TerminateProcess instead of letting main unwind." This reverts commit 19a74a4e72ee85c2c24336a7bb5f2024523ca8bd. Reason for revert: crbug.com/805443 Original change's description: > TerminateProcess instead of letting main unwind. > > Avoids going through wasteful and often problematic static > uninitialization phase. > > Unfortunately can't do this in ContentMain because of embedders (namely > test frameworks). > > Bug: 800808 > Change-Id: I768b096a8dbea4804a94d0d50e7345d46c9ef781 > Reviewed-on: https://chromium-review.googlesource.com/860617 > Commit-Queue: Gabriel Charette <gab@chromium.org> > Reviewed-by: Jochen Eisinger <jochen@chromium.org> > Cr-Commit-Position: refs/heads/master@{#529233} TBR=gab@chromium.org,jochen@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 800808, 805443 Change-Id: Icb7b027db65ee5e39f240d52f13e9a025edc76cf Reviewed-on: https://chromium-review.googlesource.com/883603 Commit-Queue: Qiang(Joe) Xu <warx@chromium.org> Reviewed-by: Qiang(Joe) Xu <warx@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#531616}(cherry picked from commit 00d4c5682096bf92e821637a697628fc40d9915c) Reviewed-on: https://chromium-review.googlesource.com/915063 Reviewed-by: Gabriel Charette <gab@chromium.org> Cr-Commit-Position: refs/branch-heads/3325@{#443} Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369} [modify] https://crrev.com/23c73d55a5e5dc485a7bb02fece033d1d6987063/chrome/app/chrome_main.cc
,
Aug 23
,
Nov 12
,
Dec 27
Reland "[PlatformThread] Join() is a wait, update assertions as such." This is a reland of 97120bd710f798ee92a93ed48816b07fd6dbed1b It was reverted @ https://chromium-review.googlesource.com/c/chromium/src/+/1384626 because it caused failures on an optional bot ( crbug.com/916544 ). Realizing my initial CL didn't have full coverage : in this reland I also did a code search for "ScopedAllow(Blocking|IO)\b (Stop|reset)\(\)" and migrated everything I found to ScopedAllowSyncPrimitivesOutsideBlockingScope. Original change's description: > [PlatformThread] Join() is a wait, update assertions as such. > > This was using ScopedBlockingCall (which was a 1:1 migration from > AssertIOAllowed()) for legacy reasons. > > CL continued from https://codereview.chromium.org/2790473006/ > > Some of these allowances aren't desired but this CL merely highlights > existing use cases and as such will not block on fixing them. > > TBR=thestig@chromium.org (trivial side-effects chrome/browser/printing/printer_query.cc) > > Bug: 707362 > Change-Id: I01a9ae13888ab8602369ddf2d12907388bdc908b > Reviewed-on: https://chromium-review.googlesource.com/c/1283010 > Commit-Queue: Gabriel Charette <gab@chromium.org> > Reviewed-by: Michael Wasserman <msw@chromium.org> > Reviewed-by: François Doray <fdoray@chromium.org> > Reviewed-by: Julien Tinnes <jln@chromium.org> > Reviewed-by: Matt Menke <mmenke@chromium.org> > Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> > Reviewed-by: Toni Baržić <tbarzic@chromium.org> > Reviewed-by: Andrey Kosyakov <caseq@chromium.org> > Cr-Commit-Position: refs/heads/master@{#617724} TBR=fdoray@chromium.org (bypassing reviewers from files identical to previous CL) CQ_INCLUDE_TRYBOTS=luci.chromium.try:linux-blink-rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;luci.chromium.try:android_optional_gpu_tests_rel Bug: 707362 Change-Id: Iad5bdb5ba27c7a277dce0072905ddb8c353af881 Reviewed-on: https://chromium-review.googlesource.com/c/1384646 Commit-Queue: Gabriel Charette <gab@chromium.org> Reviewed-by: Dale Curtis <dalecurtis@chromium.org> Reviewed-by: Sergey Ulanov <sergeyu@chromium.org> Reviewed-by: Matt Menke <mmenke@chromium.org> Reviewed-by: Michael Wasserman <msw@chromium.org> Reviewed-by: François Doray <fdoray@chromium.org> Cr-Commit-Position: refs/heads/master@{#618109}
,
Dec 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/be8e5da750961cdc67572a9c53ead47c5aa943d7 commit be8e5da750961cdc67572a9c53ead47c5aa943d7 Author: Rohit Rao <rohitrao@chromium.org> Date: Thu Dec 27 22:28:41 2018 [ios] Fixes an AssertBaseSyncPrimitivesAllowed() crash at shutdown. Adds calls to SetWaitAllowed() when shutdown is initiated, similar to the existing code in BrowserMainLoop. BUG= 917583 ,800808 Change-Id: I53ad0df9c28a0e14348f6c9ad5be97446732505b Reviewed-on: https://chromium-review.googlesource.com/c/1390912 Reviewed-by: Gabriel Charette <gab@chromium.org> Commit-Queue: Gabriel Charette <gab@chromium.org> Cr-Commit-Position: refs/heads/master@{#619077} [modify] https://crrev.com/be8e5da750961cdc67572a9c53ead47c5aa943d7/base/threading/thread_restrictions.h [modify] https://crrev.com/be8e5da750961cdc67572a9c53ead47c5aa943d7/ios/web/app/web_main_loop.mm |
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by gab@chromium.org
, Jan 10 2018