New issue
Advanced search Search tips

Issue 775843 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug

Blocked on:
issue 769381

Blocking:
issue 782851



Sign in to add a comment

OSCrypt for KWallet should use dbus' runner

Project Member Reported by cfroussios@chromium.org, Oct 18 2017

Issue description

DBus 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 
 
Blockedon: 769381
Blocking: 782851
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?
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.
Cc: thestig@chromium.org
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.
Labels: -Pri-3 Pri-2
Owner: cfroussios@chromium.org
Status: Assigned (was: Available)
Project Member

Comment 7 by bugdroid1@chromium.org, 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