OSCrypt for KWallet should use dbus' runner |
||||
Issue descriptionDBus expects to be called on a dedicated runner. Currently the KWallet implementation of OSCrypt (which is backed by dbus) does not use any explicit threading scheme. OSCrypt should follow whatever scheme is implemented to solve issue 769381
,
Nov 8 2017
,
Dec 12 2017
I'm running KDE here without KWallet enabled. I'm building developer builds at r523250, and I'm seeing some weird kWallet related issues when running them. Last week, sometimes the browser hangs at shut down. I think r522545 may be have fixed it. Now, I get a DCHECK failure. [153167:153238:1211/170355.708705:ERROR:object_proxy.cc(626)] Failed to call method: org.kde.KWallet.isEnabled: object_path= /modules/kwalletd: org.freedesktop.DBus.Error.NoReply: Message did not receive a reply (timeout by message bus) [153167:153238:1211/170355.708790:ERROR:kwallet_dbus.cc(100)] Error contacting kwalletd (isEnabled) [153167:153214:1211/170355.708972:FATAL:bus.cc(804)] Check failed: origin_thread_id_ == base::PlatformThread::CurrentId() (153238 vs. 153214) #0 0x7f0287a86c0d base::debug::StackTrace::StackTrace() #1 0x7f0287a8503c base::debug::StackTrace::StackTrace() #2 0x7f0287b0cbba logging::LogMessage::~LogMessage() #3 0x7f027c50c6b0 dbus::Bus::AssertOnOriginThread() #4 0x7f027c50c7f7 dbus::Bus::AssertOnDBusThread() #5 0x7f027c565341 dbus::ObjectProxy::CallMethodAndBlockWithErrorDetails() #6 0x7f027c566074 dbus::ObjectProxy::CallMethodAndBlock() #7 0x5578e94dd81c KWalletDBus::Close() #8 0x5578e94d5e99 KeyStorageKWallet::~KeyStorageKWallet() #9 0x5578e94d5ff9 KeyStorageKWallet::~KeyStorageKWallet() #10 0x5578e94d17cc KeyStorageLinux::CreateService() #11 0x5578e94cf68b (anonymous namespace)::CreateKeyStorage() #12 0x5578e94cfb0d (anonymous namespace)::GetPasswordV11() #13 0x5578e94ce96c (anonymous namespace)::GetEncryptionKey() #14 0x5578e94cd736 OSCrypt::EncryptString() #15 0x5578ec8d64b1 cookie_config::(anonymous namespace)::CookieOSCryptoDelegate::EncryptString() #16 0x7f028477997f net::SQLitePersistentCookieStore::Backend::Commit() This this the right bug, or should I file a new one?
,
Dec 12 2017
OSCrypt should be doing the right thing since r520980. I forgot to close this bug. DBus locks itself to the first thread it sees. If the DCHECK is failing on the dbus task runner https://cs.chromium.org/chromium/src/chrome/browser/dbus/dbus_thread_linux.h, then probably someone who isn't using it is calling dbus first. We need to find who that is and inform them of the new dedicated task runner. You can reuse this bug if you want, but a new one might be more appropriate.
,
Dec 13 2017
My previous comment was wrong. The error happens during the destruction of KeyStorageKWallet. We make calls to dbus to close the wallet and the session from the destructor, but we don't schedule the destruction on the dedicated thread. I think a solution would be to move the threading logic into GetPasswordV11 https://cs.chromium.org/chromium/src/components/os_crypt/os_crypt_linux.cc?rcl=cf2f6b30521dca16eb0873a76ecc9b06f64fedb7&l=95 The entire life of a KeyStorage* happens within that method. Unfortunately, I will be OOO until next year and I don't want to land something like this and walk away. If it was working better before r520980 (I'm pretty sure the DCHECK will complain for the password manager, but at least it doesn't block all of Chrome), then feel free to revert it and I will work on the fix when I get back.
,
Dec 13 2017
,
Dec 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/70cc95bc9acc91ac19fc84b239ecf86b88156b2f commit 70cc95bc9acc91ac19fc84b239ecf86b88156b2f Author: Christos Froussios <cfroussios@chromium.org> Date: Thu Dec 14 21:13:23 2017 Revert "[OSCrypt] KWallet/dbus implementation should use dbus' thread" This reverts commit 96bca08b0c99372f8934429469837a526edb4fa3. Reason for revert: Initialisation of password manager depends on OSCrypt. This introduces a deadlock, as seen crbug/795019 Additionally, this CL failed to schedule the destruction of the KeyStorage on the dedicated thread. Original change's description: > [OSCrypt] KWallet/dbus implementation should use dbus' thread > > Dbus, which we use to communicate with KWallet, requires to be called > on a dedicated thread. > > Bug: 782851 > Change-Id: I7c55c0c6cec03d3cf688b6f1ad60326e4d5f417f > Reviewed-on: https://chromium-review.googlesource.com/803481 > Commit-Queue: Christos Froussios <cfroussios@chromium.org> > Reviewed-by: Lei Zhang <thestig@chromium.org> > Reviewed-by: Vaclav Brozek <vabr@chromium.org> > Cr-Commit-Position: refs/heads/master@{#520980} TBR=thestig@chromium.org,vabr@chromium.org,cfroussios@chromium.org Bug: 782851, 775843, 795019 Change-Id: I8b5d4c4ed8c2fe6949591336904befe80ef4fb42 Reviewed-on: https://chromium-review.googlesource.com/826069 Commit-Queue: Christos Froussios <cfroussios@chromium.org> Reviewed-by: Christos Froussios <cfroussios@chromium.org> Cr-Commit-Position: refs/heads/master@{#524176} [modify] https://crrev.com/70cc95bc9acc91ac19fc84b239ecf86b88156b2f/chrome/browser/chrome_browser_main_linux.cc [modify] https://crrev.com/70cc95bc9acc91ac19fc84b239ecf86b88156b2f/components/os_crypt/key_storage_config_linux.h [modify] https://crrev.com/70cc95bc9acc91ac19fc84b239ecf86b88156b2f/components/os_crypt/key_storage_kwallet.cc [modify] https://crrev.com/70cc95bc9acc91ac19fc84b239ecf86b88156b2f/components/os_crypt/key_storage_kwallet.h [modify] https://crrev.com/70cc95bc9acc91ac19fc84b239ecf86b88156b2f/components/os_crypt/key_storage_kwallet_unittest.cc [modify] https://crrev.com/70cc95bc9acc91ac19fc84b239ecf86b88156b2f/components/os_crypt/key_storage_linux.cc |
||||
►
Sign in to add a comment |
||||
Comment 1 by cfroussios@chromium.org
, Oct 18 2017