Reef: chapsd crash in "RegisterCallback without an AtExitManage" |
|||||||
Issue descriptionChrome Version: 61.0.3116.0 OS: 9602.0.0-developer-build What steps will reproduce the problem? (1) ./build_packages --board=reef --withdev (2) ./build_image --board=reef --noenable_rootfs_verification test (3) cros flash (4) Boot and see /var/log/messages What is the expected result? What happens instead? 2017-05-30T17:06:20.609846+09:00 INFO chapsd[1879]: Starting PKCS #11 services. 2017-05-30T17:06:20.611802+09:00 CRIT chapsd[1879]: Check failed: false. Tried to RegisterCallback without an AtExitManager#012/usr/lib64/libbase-core-395517.so(base::debug::StackTrace::StackTrace()+0x13) [0x7dd545681ba3]#012 The change https://chromium-review.googlesource.com/#/c/508255/ has removed an base::AtExitManager instanciation: #if USE_TPM2 - base::AtExitManager at_exit_manager; base::Thread tpm_background_thread(kTpmThreadName); CHECK(tpm_background_thread.StartWithOptions( base::Thread::Options(base::MessageLoop::TYPE_IO, 0 /* use default stack size */))); chaps::TPM2UtilityImpl tpm(tpm_background_thread.task_runner()); #else ... LOG(INFO) << "Starting D-Bus dispatcher."; - RunDispatcher(&lock, &service, &slot_manager); + chaps::Daemon(&lock, &service, &slot_manager).Run(); but base::Thread::StartWithOptions looks still requiring it (via the use of base::LazyInstance) https://cs.chromium.org/chromium/src/base/threading/thread.cc?type=cs&q=startwithoptions+package:%5Echromium$&l=93 Just reviving the |at_exit_manager| variable did not work for me, since the chaps::Daemon instance also declares base::AtExitManager, which hits the DCHECK. https://cs.chromium.org/chromium/src/base/at_exit.cc?type=cs&q=at_exit+package:%5Echromium$&l=32 Eric, could you mind taking a look?
,
May 31 2017
Ah jeez. This is awkward because Daemon depends on SlotManagerImpl, which depends on TPMUtility, which depends on this thread (which needs an AtExitManager we can't instantiate until Daemon does it for us). I'll have to shuffle some stuff around.
,
Jun 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/465a258004302f05b8e4a92c1508426c9c0ac7e8 commit 465a258004302f05b8e4a92c1508426c9c0ac7e8 Author: Eric Caruso <ejcaruso@chromium.org> Date: Sat Jun 03 01:39:38 2017 chaps: rearrange initialization code The TPM2 base::Thread requires a base::AtExitManager, but we don't create one of those until construction of the DBusServiceDaemon, which comes later. Instantiating an AtExitManager before that will result in a DCHECK failure since AtExitManager is a singleton and we would be trying to make two. In this case, we instantiate the Daemon first, and then do the formerly main-function initialization inside of the OnInit method. This ensures an AtExitManager exists before trying to instantiate any threads. Additionally, the async init thread is changed to a base::Thread from a PlatformThread to reduce some boilerplate and also because the base::Thread destructor will ensure the thread is stopped before we destroy the Daemon. BUG= chromium:727987 TEST=deploy to cyan device and run (since thread construction is no longer behind USE_TPM2), use pkcs11-tool, build on reef Change-Id: I24584d640ea509cb19aa0358a3f7e304560d0a09 Reviewed-on: https://chromium-review.googlesource.com/520163 Commit-Ready: Eric Caruso <ejcaruso@chromium.org> Tested-by: Eric Caruso <ejcaruso@chromium.org> Reviewed-by: Andrey Pronin <apronin@chromium.org> [modify] https://crrev.com/465a258004302f05b8e4a92c1508426c9c0ac7e8/chaps/chapsd.cc
,
Jun 3 2017
,
Jun 3 2017
Does this require backporting to 60?
,
Jun 5 2017
The first change looks to be included in R60: https://chromium.googlesource.com/chromiumos/platform2/+log/release-R60-9592.B/chaps hence I believe we want to cherry-pick. +label:Merge-Request-60
,
Jun 6 2017
Your change meets the bar and is auto-approved for M60. Please go ahead and merge the CL to branch 3112 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/af505e4b8de6dd03ee9beca54240766677245d31 commit af505e4b8de6dd03ee9beca54240766677245d31 Author: Eric Caruso <ejcaruso@chromium.org> Date: Tue Jun 06 17:29:31 2017 chaps: rearrange initialization code The TPM2 base::Thread requires a base::AtExitManager, but we don't create one of those until construction of the DBusServiceDaemon, which comes later. Instantiating an AtExitManager before that will result in a DCHECK failure since AtExitManager is a singleton and we would be trying to make two. In this case, we instantiate the Daemon first, and then do the formerly main-function initialization inside of the OnInit method. This ensures an AtExitManager exists before trying to instantiate any threads. Additionally, the async init thread is changed to a base::Thread from a PlatformThread to reduce some boilerplate and also because the base::Thread destructor will ensure the thread is stopped before we destroy the Daemon. BUG= chromium:727987 TEST=deploy to cyan device and run (since thread construction is no longer behind USE_TPM2), use pkcs11-tool, build on reef Change-Id: I24584d640ea509cb19aa0358a3f7e304560d0a09 Reviewed-on: https://chromium-review.googlesource.com/520163 Commit-Ready: Eric Caruso <ejcaruso@chromium.org> Tested-by: Eric Caruso <ejcaruso@chromium.org> Reviewed-by: Andrey Pronin <apronin@chromium.org> (cherry picked from commit 465a258004302f05b8e4a92c1508426c9c0ac7e8) Reviewed-on: https://chromium-review.googlesource.com/526055 Reviewed-by: Eric Caruso <ejcaruso@chromium.org> Commit-Queue: Eric Caruso <ejcaruso@chromium.org> Trybot-Ready: Eric Caruso <ejcaruso@chromium.org> [modify] https://crrev.com/af505e4b8de6dd03ee9beca54240766677245d31/chaps/chapsd.cc
,
Jun 6 2017
,
Jan 22 2018
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by kinaba@chromium.org
, May 31 2017