New issue
Advanced search Search tips

Issue 795019 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug



Sign in to add a comment

Deadlock initializing the password manager on Linux

Project Member Reported by brettw@chromium.org, Dec 14 2017

Issue description

I'm seeing a deadlock on Trunk @ commit 523828.

The main thread is waiting on initializing the libsecret key storage on a background thread:

0  pthread_cond_wait@@GLIBC_2.3.2 ()
    at ../nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S:185
#1  0x00007ffff7823970 in base::ConditionVariable::Wait (this=0x7fffffff4ae8)
    at ../../base/synchronization/condition_variable_posix.cc:71
#2  0x00007ffff78255d9 in base::WaitableEvent::TimedWaitUntil (
    this=0x7fffffff4d80, end_time=106751991 days, 4:00:54.775807)
    at ../../base/synchronization/waitable_event_posix.cc:223
#3  0x00007ffff782526c in base::WaitableEvent::Wait (this=0x7fffffff4d80)
    at ../../base/synchronization/waitable_event_posix.cc:154
#4  0x00005555597c01a9 in KeyStorageLinux::WaitForInitOnTaskRunner (
    this=0x2bd27f5004e0) at ../../components/os_crypt/key_storage_linux.cc:120
#5  0x00005555597bed7c in KeyStorageLinux::CreateService (config=...)
    at ../../components/os_crypt/key_storage_linux.cc:60
#6  0x00005555597bde1f in (anonymous namespace)::CreateKeyStorage ()
    at ../../components/os_crypt/os_crypt_linux.cc:74
#7  0x00005555597be2ad in (anonymous namespace)::GetPasswordV11 ()
    at ../../components/os_crypt/os_crypt_linux.cc:98
#8  0x00005555597bd11c in (anonymous namespace)::GetEncryptionKey (
    version=(anonymous namespace)::V11)
    at ../../components/os_crypt/os_crypt_linux.cc:118
#9  0x00005555597bccf7 in OSCrypt::DecryptString (ciphertext=..., 
    plaintext=0x7fffffff6638)
    at ../../components/os_crypt/os_crypt_linux.cc:211
#10 0x000055555b47e561 in password_manager::HashPasswordManager::RetrivedDecryptedStringFromPrefs (this=0x2bd27fe9adf8, pref_name=...)
    at ../../components/password_manager/core/browser/hash_password_manager.cc:131

#11 0x000055555b47d614 in password_manager::HashPasswordManager::RetrievePasswordHash (this=0x2bd27fe9adf8)
    at ../../components/password_manager/core/browser/hash_password_manager.cc:59
#12 0x000055555b402408 in password_manager::PasswordStore::Init(base::RepeatingCallback<void (syncer::ModelType)> const&, PrefService*) (this=0x2bd27fe9ada0, 
    flare=..., prefs=0x2bd27ec27920)
    at ../../components/password_manager/core/browser/password_store.cc:129

#13 0x00005555587ec8b3 in PasswordStoreFactory::BuildServiceInstanceFor (
    this=0x2bd27e341020, context=0x2bd27e0dede0)
    at ../../chrome/browser/password_manager/password_store_factory.cc:255
#14 0x00007fffeb3f0a66 in RefcountedBrowserContextKeyedServiceFactory::BuildServiceInstanceFor (this=0x2bd27e341020, context=0x2bd27e0dede0)
    at ../../components/keyed_service/content/refcounted_browser_context_keyed_service_factory.cc:91
#15 0x00007fffec17389a in RefcountedKeyedServiceFactory::GetServiceForContext (
    this=0x2bd27e341020, context=0x2bd27e0dede0, create=true)
    at ../../components/keyed_service/core/refcounted_keyed_service_factory.cc:88
#16 0x00007fffeb3f083e in RefcountedBrowserContextKeyedServiceFactory::GetServiceForBrowserContext (this=0x2bd27e341020, context=0x2bd27e0dede0, create=true)
    at ../../components/keyed_service/content/refcounted_browser_context_keyed_service_factory.cc:52
#17 0x00005555587eb0dd in PasswordStoreFactory::GetForProfile (profile=
    0x2bd27e0dede0, access_type=ServiceAccessType::IMPLICIT_ACCESS)
    at ../../chrome/browser/password_manager/password_store_factory.cc:83
#18 0x0000555558af7e18 in browser_sync::ChromeSyncClient::Initialize (
    this=0x2bd27fe8cb80) at ../../chrome/browser/sync/chrome_sync_client.cc:223
#19 0x000055555b71707b in browser_sync::ProfileSyncService::Initialize (
    this=0x2bd27eb46020)
    at ../../components/browser_sync/profile_sync_service.cc:218
#20 0x0000555558b014cc in ProfileSyncServiceFactory::BuildServiceInstanceFor (
    this=0x2bd27e341290, context=0x2bd27e0dede0)
    at ../../chrome/browser/sync/profile_sync_service_factory.cc:251
#21 0x00007fffeb3ee02a in BrowserContextKeyedServiceFactory::BuildServiceInstanceFor (this=0x2bd27e341290, context=0x2bd27e0dede0)
    at ../../components/keyed_service/content/browser_context_keyed_service_factory.cc:98
#22 0x00007fffec16c627 in KeyedServiceFactory::GetServiceForContext (
    this=0x2bd27e341290, context=0x2bd27e0dede0, create=true)
    at ../../components/keyed_service/core/keyed_service_factory.cc:89
#23 0x00007fffeb3eddd7 in BrowserContextKeyedServiceFactory::GetServiceForBrowserContext (this=0x2bd27e341290, context=0x2bd27e0dede0, create=true)
    at ../../components/keyed_service/content/browser_context_keyed_service_factory.cc:51
#24 0x0000555558b000fc in ProfileSyncServiceFactory::GetSyncServiceForBrowserContext (context=0x2bd27e0dede0)
    at ../../chrome/browser/sync/profile_sync_service_factory.cc:117
#25 0x0000555558b000b5 in ProfileSyncServiceFactory::GetForProfile (
    profile=0x2bd27e0dede0)
    at ../../chrome/browser/sync/profile_sync_service_factory.cc:106
#26 0x0000555558f00e91 in SupervisedUserService::Init (this=0x2bd27ec22020)
    at ../../chrome/browser/supervised_user/supervised_user_service.cc:196
#27 0x0000555558a248ef in ProfileManager::DoFinalInitForServices (
    this=0x2bd27e75c620, profile=0x2bd27e0dede0, go_off_the_record=false)
    at ../../chrome/browser/profiles/profile_manager.cc:1257
#28 0x0000555558a24653 in ProfileManager::DoFinalInit (this=0x2bd27e75c620, 
    profile=0x2bd27e0dede0, go_off_the_record=false)
    at ../../chrome/browser/profiles/profile_manager.cc:1209
#29 0x0000555558a263db in ProfileManager::AddProfile (this=0x2bd27e75c620, 
    profile=0x2bd27e0dede0)


The libsecret code does not run. The background thread it wants seems to run on is blocked on previous work. I believe it's this one:

#0  __lll_lock_wait ()
    at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
#1  0x00007ffff7bc6649 in _L_lock_909 ()
   from /lib/x86_64-linux-gnu/libpthread.so.0
#2  0x00007ffff7bc6470 in __GI___pthread_mutex_lock (
    mutex=0x55555da61960 <(anonymous namespace)::g_cache+48>)
    at ../nptl/pthread_mutex_lock.c:79
#3  0x00007ffff7824b21 in base::internal::LockImpl::Lock (
    this=0x55555da61960 <(anonymous namespace)::g_cache+48>)
    at ../../base/synchronization/lock_impl_posix.cc:73
#4  0x00005555570012b3 in base::Lock::Acquire (
    this=0x55555da61958 <(anonymous namespace)::g_cache+40>)
    at ../../base/synchronization/lock.h:45
#5  0x0000555557000e33 in base::AutoLock::AutoLock (this=0x7fffbad56e90, 
    lock=Unknown state) at ../../base/synchronization/lock.h:115
#6  0x00005555597be28a in (anonymous namespace)::GetPasswordV11 ()
    at ../../components/os_crypt/os_crypt_linux.cc:96
#7  0x00005555597bd11c in (anonymous namespace)::GetEncryptionKey (
    version=(anonymous namespace)::V11)
    at ../../components/os_crypt/os_crypt_linux.cc:118
#8  0x00005555597bccf7 in OSCrypt::DecryptString (ciphertext=..., 
    plaintext=0x7fffbad57688)
    at ../../components/os_crypt/os_crypt_linux.cc:211
#9  0x0000555559847ec9 in TokenServiceTable::GetAllTokens (
    this=0x3194467fd930, tokens=0x7fffbad57e28)
    at ../../components/signin/core/browser/webdata/token_service_table.cc:124
#10 0x00005555598457cb in TokenWebDataBackend::GetAllTokens (
    this=0x3194467f8a70, db=0x31944671f5c0)
    at ../../components/signin/core/browser/webdata/token_web_data.cc:53

This background thread is blocked waiting for the os_crypt_linux.cc g_cache.lock.

This lock is held by the main thread GetPasswordV11 (frame #7).

Possibly related to https://chromium-review.googlesource.com/c/chromium/src/+/815055
 
Cc: vabr@chromium.org
I didn't know that the initialisation of password manager depends on oscrypt. That's a problem.

I will revert r520980 and r523409.

+vabr
We need to yet again discuss how to remove concurrency between password manager and oscrypt.




Project Member

Comment 2 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

Project Member

Comment 3 by bugdroid1@chromium.org, Dec 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/90879ade4ca34e83e7467abdb61a47f8ac09187e

commit 90879ade4ca34e83e7467abdb61a47f8ac09187e
Author: Christos Froussios <cfroussios@chromium.org>
Date: Thu Dec 14 21:14:51 2017

Revert "[OSCrypt][Password Manager] Introduce TaskRunner for libsecret"

This reverts commit a22b9873c5fa353ad274e5408b0f8d8f5c54781d.

Reason for revert: Password Manager initialisation depends on oscrypt,
which creates the possibility of deadlock. See crbug/795019

Original change's description:
> [OSCrypt][Password Manager] Introduce TaskRunner for libsecret
> 
> A SequencedTaskRunner will be used to guarantee no race conditions
> between the two clients of libsecret.
> 
> Bug: 782851
> Change-Id: I6549f867a14378d019404429bab42008fca548a7
> Reviewed-on: https://chromium-review.googlesource.com/803954
> Commit-Queue: Christos Froussios <cfroussios@chromium.org>
> Reviewed-by: Vaclav Brozek <vabr@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#523409}

TBR=vabr@chromium.org,cfroussios@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 782851, 795019 
Change-Id: Ie435a623de544767189571b80f929777af881012
Reviewed-on: https://chromium-review.googlesource.com/826070
Reviewed-by: Christos Froussios <cfroussios@chromium.org>
Commit-Queue: Christos Froussios <cfroussios@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524178}
[modify] https://crrev.com/90879ade4ca34e83e7467abdb61a47f8ac09187e/chrome/browser/password_manager/native_backend_libsecret.cc
[modify] https://crrev.com/90879ade4ca34e83e7467abdb61a47f8ac09187e/components/os_crypt/BUILD.gn
[modify] https://crrev.com/90879ade4ca34e83e7467abdb61a47f8ac09187e/components/os_crypt/key_storage_libsecret.cc
[modify] https://crrev.com/90879ade4ca34e83e7467abdb61a47f8ac09187e/components/os_crypt/key_storage_libsecret.h
[modify] https://crrev.com/90879ade4ca34e83e7467abdb61a47f8ac09187e/components/os_crypt/key_storage_libsecret_unittest.cc
[delete] https://crrev.com/bb29325c1aedbb530110644fd38430a905af1b96/components/os_crypt/libsecret_task_runner_linux.cc
[delete] https://crrev.com/bb29325c1aedbb530110644fd38430a905af1b96/components/os_crypt/libsecret_task_runner_linux.h

Components: UI>Browser>Passwords Internals>LocalDataEncryption
Status: Fixed (was: Assigned)
The deadlock on initialisation should not appear anymore.
Cc: -vabr@chromium.org

Sign in to add a comment