New issue
Advanced search Search tips

Issue 782851 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 3
Type: Task

Blocked on: View detail
issue 571003
issue 775843



Sign in to add a comment

Refactor OSCrypt to accomodate thread affinities

Project Member Reported by cfroussios@chromium.org, Nov 8 2017

Issue description

Out of the 3 supported backends
kwallet and keyring have thread affinities
libsecret needs to be mutually exclusive with any other client of libsecret

The result is that every backend depends on a thread. It's time to put this in the architecture.
 
Blockedon: 775843
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 9 2017

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

commit 985d1aacf636e0b59b3cc78f1b7afbf4e332357c
Author: Christos Froussios <cfroussios@chromium.org>
Date: Thu Nov 09 11:01:07 2017

[OSCrypt] Introduce GetKeyImpl in KeyStorageLinux

Instead of implementing the Getter itself, subclasses will be called by
the parent, KeyStorageLinux. This will allow us to control how the task
will be scheduled in a centralised place.

Bug: 782851
Change-Id: I0f57d622c0416f56df3c60d99aaf33ed41c6f6b5
Reviewed-on: https://chromium-review.googlesource.com/758778
Commit-Queue: Christos Froussios <cfroussios@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515135}
[modify] https://crrev.com/985d1aacf636e0b59b3cc78f1b7afbf4e332357c/components/os_crypt/key_storage_keyring.cc
[modify] https://crrev.com/985d1aacf636e0b59b3cc78f1b7afbf4e332357c/components/os_crypt/key_storage_keyring.h
[modify] https://crrev.com/985d1aacf636e0b59b3cc78f1b7afbf4e332357c/components/os_crypt/key_storage_kwallet.cc
[modify] https://crrev.com/985d1aacf636e0b59b3cc78f1b7afbf4e332357c/components/os_crypt/key_storage_kwallet.h
[modify] https://crrev.com/985d1aacf636e0b59b3cc78f1b7afbf4e332357c/components/os_crypt/key_storage_libsecret.cc
[modify] https://crrev.com/985d1aacf636e0b59b3cc78f1b7afbf4e332357c/components/os_crypt/key_storage_libsecret.h
[modify] https://crrev.com/985d1aacf636e0b59b3cc78f1b7afbf4e332357c/components/os_crypt/key_storage_linux.cc
[modify] https://crrev.com/985d1aacf636e0b59b3cc78f1b7afbf4e332357c/components/os_crypt/key_storage_linux.h
[modify] https://crrev.com/985d1aacf636e0b59b3cc78f1b7afbf4e332357c/components/os_crypt/os_crypt_mocker_linux.cc
[modify] https://crrev.com/985d1aacf636e0b59b3cc78f1b7afbf4e332357c/components/os_crypt/os_crypt_mocker_linux.h

Project Member

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

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

commit 4e170cb0f5a9ae46c82099cbed85b3241cf8b158
Author: Christos Froussios <cfroussios@chromium.org>
Date: Fri Dec 01 09:42:33 2017

[OSCrypt] KeyStorageLinux supports provided TaskRunners

While we need to jump to a specific thread and back, OSCrypt remains a
synchronous API, therefore we need to wait on the result. An exception
for KeyStorageLinux is made to allow for blocking.

We wrap the Init() and GetKey() methods, such that they are called on
preferred TaskRunner for the targeted backend.

OSCrypt itself has no expectation for which thread it's going to be called
on. It might even already be the thread that the backend requires (e.g. UI).
We avoid a deadlock by only posting the task if the TaskRunner is
different than the current one.

Bug: 782851
Change-Id: I48ae623ce04da23a5e79d0c1e52890df8a8ef79e
Reviewed-on: https://chromium-review.googlesource.com/758854
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Commit-Queue: Christos Froussios <cfroussios@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520898}
[modify] https://crrev.com/4e170cb0f5a9ae46c82099cbed85b3241cf8b158/base/threading/thread_restrictions.h
[modify] https://crrev.com/4e170cb0f5a9ae46c82099cbed85b3241cf8b158/components/os_crypt/BUILD.gn
[modify] https://crrev.com/4e170cb0f5a9ae46c82099cbed85b3241cf8b158/components/os_crypt/key_storage_linux.cc
[modify] https://crrev.com/4e170cb0f5a9ae46c82099cbed85b3241cf8b158/components/os_crypt/key_storage_linux.h
[add] https://crrev.com/4e170cb0f5a9ae46c82099cbed85b3241cf8b158/components/os_crypt/key_storage_linux_unittest.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Dec 1 2017

Project Member

Comment 5 by bugdroid1@chromium.org, Dec 1 2017

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

commit d6cfe11e406c4fa71665ed8e33e9814bed7e1afe
Author: Christos Froussios <cfroussios@chromium.org>
Date: Fri Dec 01 19:28:26 2017

[OSCrypt] Use general threading implemention for KeyStorageKeyring

We now have a general implementation for dedicated TaskRunners for
OSCrypt's backends. The custom threading implementation for
KeyStorageKeyring can be removed and replaced with the general one.

Bug: 782851
Change-Id: I61b0df8568504d4160bbcce7301e99fbdb86609a
Reviewed-on: https://chromium-review.googlesource.com/804094
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Commit-Queue: Christos Froussios <cfroussios@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521014}
[modify] https://crrev.com/d6cfe11e406c4fa71665ed8e33e9814bed7e1afe/components/os_crypt/key_storage_keyring.cc
[modify] https://crrev.com/d6cfe11e406c4fa71665ed8e33e9814bed7e1afe/components/os_crypt/key_storage_keyring.h

Project Member

Comment 6 by bugdroid1@chromium.org, Dec 7 2017

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

commit 57fe57499d1944694f5dc39d70d0a921e35543ae
Author: Christos Froussios <cfroussios@chromium.org>
Date: Thu Dec 07 21:16:49 2017

[OSCrypt] Fix deadlock on init KeyStorageLinux

PostWithReply will post back to the calling environment, which is
waiting, thus the completion notification never arrives. I replaced
the PostTaskWithReply with a single task.

Bug: 782851
Change-Id: I7414a590aba39df77e48361460a625cea55e1f04
Reviewed-on: https://chromium-review.googlesource.com/815055
Commit-Queue: Christos Froussios <cfroussios@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522545}
[modify] https://crrev.com/57fe57499d1944694f5dc39d70d0a921e35543ae/components/os_crypt/key_storage_linux.cc
[modify] https://crrev.com/57fe57499d1944694f5dc39d70d0a921e35543ae/components/os_crypt/key_storage_linux.h

Components: Internals>LocalDataEncryption
Project Member

Comment 8 by bugdroid1@chromium.org, Dec 12 2017

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

commit a22b9873c5fa353ad274e5408b0f8d8f5c54781d
Author: Christos Froussios <cfroussios@chromium.org>
Date: Tue Dec 12 12:04:14 2017

[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}
[modify] https://crrev.com/a22b9873c5fa353ad274e5408b0f8d8f5c54781d/chrome/browser/password_manager/native_backend_libsecret.cc
[modify] https://crrev.com/a22b9873c5fa353ad274e5408b0f8d8f5c54781d/components/os_crypt/BUILD.gn
[modify] https://crrev.com/a22b9873c5fa353ad274e5408b0f8d8f5c54781d/components/os_crypt/key_storage_libsecret.cc
[modify] https://crrev.com/a22b9873c5fa353ad274e5408b0f8d8f5c54781d/components/os_crypt/key_storage_libsecret.h
[modify] https://crrev.com/a22b9873c5fa353ad274e5408b0f8d8f5c54781d/components/os_crypt/key_storage_libsecret_unittest.cc
[add] https://crrev.com/a22b9873c5fa353ad274e5408b0f8d8f5c54781d/components/os_crypt/libsecret_task_runner_linux.cc
[add] https://crrev.com/a22b9873c5fa353ad274e5408b0f8d8f5c54781d/components/os_crypt/libsecret_task_runner_linux.h

Project Member

Comment 9 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 10 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

Blockedon: 571003
We will instead drop the backends for Password Manager first. Once Password Manager becomes a client of OSCrypt (rather than both a client and parallel client of the backends), these particular deadlocks should disappear.

Sign in to add a comment