Issue metadata
Sign in to add a comment
|
mash: Shutdown crash in PrefChangeRegistrar from NightLightController |
||||||||||||||||||||||||
Issue descriptionChrome ToT r472870, Chrome OS ToT self-built, peach_pit * stop ui * Edit /etc/chrome_dev.conf to add --mash * start ui * Sign in. * System tray > sign out /var/log/ui/ui.LATEST shows chrome crash: [8923:8923:0518/122829.259991:505478546:WARNING:pref_notifier_impl.cc(23)] Pref observer found at shutdown. [8923:8923:0518/122829.260086:505478636:WARNING:pref_notifier_impl.cc(23)] Pref observer found at shutdown. Received signal 11 SEGV_MAPERR 000047231a1f #0 0x0000b2d2b0b8 base::debug::StackTrace::StackTrace() #1 0x0000b2d2afa4 base::debug::(anonymous namespace)::StackDumpSignalHandler() #2 0x0000b0d18c40 <unknown> #3 0x0000b35e2132 PrefChangeRegistrar::~PrefChangeRegistrar() #4 0x0000b438acb0 ash::NightLightController::~NightLightController() #5 0x0000b437e6d2 ash::Shell::~Shell() #6 0x0000b437e9d4 ash::Shell::~Shell() #7 0x0000b230bce0 ash::mus::WindowManager::~WindowManager() #8 0x0000b230be02 ash::mus::WindowManager::~WindowManager() #9 0x0000b230d34c ash::mus::WindowManagerApplication::~WindowManagerApplication() #10 0x0000b230d436 ash::mus::WindowManagerApplication::~WindowManagerApplication() #11 0x0000b368b7c6 service_manager::ServiceContext::~ServiceContext() #12 0x0000b2a60f66 _ZN4base8internal7InvokerINS0_9BindStateIZN15service_manager12_GLOBAL__N_110RunServiceEPNS3_12MainDelegateEE3$_0JS6_PiEEEFvN4mojo16InterfaceRequestINS3_5mojom7ServiceEEEEE3RunEPNS0_13BindStateBaseEOSE_ #13 0x0000b1a25882 service_manager::RunStandaloneService() #14 0x0000b2a60900 service_manager::Main() #15 0x0000b2a492f4 content::ContentMain() #16 0x0000b17bb74a ChromeMain #17 0x0000b0d088b8 __libc_start_main [end of stack trace] Calling _exit(1). Core file will not be generated. Doesn't repro on linux target_os=chromeos.
,
May 18 2017
This happens because PrefChangeRegistrar::~PrefChangeRegistrar() calls RemoveAll() which assumes that the |service_| is still alive [https://cs.chromium.org/chromium/src/components/prefs/pref_change_registrar.cc?q=PrefChangeRegistrar::~PrefChangeRegistrar+package:%5Echromium$&l=54]. I'm not sure why the PrefService doesn't support notifying the registrar that it's going to be destroyed. battre@ Should know more.
,
May 18 2017
I think this also means that SessionObserver::OnActiveUserSessionChanged() is not called when user signs out in --mash.
,
May 18 2017
,
May 18 2017
Re #3: We don't have a session state for sign-out. And there is no active user change during sign-out either. That is why neither OnSessionStateChanged nor OnActiveuserSessionChanged is called. It seems Shell's |pref_service_| is released in dtor [1]. Explicitly release NightLightController before that might fix the issue. [1]: https://cs.chromium.org/chromium/src/ash/shell.cc?rcl=ba5159d871bb21b4b151262dd9234d0261b81331&l=788
,
May 18 2017
+bauerb - this code is years old. I think that the motivation was that the PrefService was only owned by the Profile (consumers typically stored a pointer to the Profile) and this DCHECK was an indication of an incorrect shutdown order. Bernhard, do you recall another reason? I wonder whether we should have auto-deregistration if the PrefService is destroyed.
,
May 19 2017
That's correct. The PrefChangeRegistrar depends on the PrefService, so it's not supposed to outlive it. If we added a destruction notification, it would just paper over the problem that your shutdown order is ill-defined or messed up. (Similarly to how we prefer KeyedServices to shut down profile-related objects rather than every single object listening to a notification about profile destruction.)
,
May 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/21501560f5074b5e07c397fedc09d47a0a6c5b3f commit 21501560f5074b5e07c397fedc09d47a0a6c5b3f Author: afakhry <afakhry@chromium.org> Date: Fri May 19 22:52:40 2017 Destroy NightLightController before PrefService NightLightController depends on the PrefService to read and write user prefs related to NightLight. It uses a PrefChangeRegistrar to observe changes to those prefs. PrefChangeRegistrar is not expected to outlive the PrefService, and hence must be destroyed first. BUG= 724231 TEST=Start chrome with --mash and expects to see no crashes NightLightController when Shell is destroyed. Review-Url: https://codereview.chromium.org/2892983002 Cr-Commit-Position: refs/heads/master@{#473375} [modify] https://crrev.com/21501560f5074b5e07c397fedc09d47a0a6c5b3f/ash/shell.cc
,
May 23 2017
,
Aug 1 2017
,
Jan 22 2018
,
Feb 26 2018
|
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by jamescook@chromium.org
, May 18 2017