New issue
Advanced search Search tips

Issue 741660 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Task



Sign in to add a comment

Simplify background task runner handling in PasswordStore

Project Member Reported by vabr@chromium.org, Jul 12 2017

Issue description

Various implementations of PasswordStore use different types of background task runners:
  * Most of them (Win, CrOS, Linux without Gnome Keyring, Android and iOS) can just use base::CreateSequencedTaskRunnerWithTraits({base::MayBlock(), base::TaskPriority::USER_VISIBLE}).
  * Linux with Gnome Keyring needs Wait() allowed ( bug 739897 ), resulting in the additional trait base::WithBaseSyncPrimitives(). This task runner is already available before constructing the base PasswordStore.
  * Mac creates a special base::Thread to run background operations on. This happens during PasswordStoreMac::Init, and is not available during construction of the base PasswordStore.

Because the base PasswordStore also needs the background task runner, it needs it to be provided by the inheriting class. This happens currently by overriding PasswordStore::GetBackgroundTaskRunner, or alternatively by passing the task runner to PasswordStore through PasswordStoreDefault as a constructor argument (obviously does not work for Mac). The base implementation of PasswordStore::GetBackgroundTaskRunner returns whatever was passed to the constructor.

Based on the recommendation in https://chromium.googlesource.com/chromium/src/+/master/docs/task_scheduler_migration.md#BrowserThreads, we should move TaskRunner creation as much inside (near the base PasswordStore) as possible. So the steps to take are:

(1) Verify whether PasswordStoreMac really needs a dedicated thread.
(2) Change the API from allowing two ways of repeatedly changing the background task runner (overriding GetBackgroundTaskRunner any time + passing the constructor argument once) to a single, one-off way: likely passing creating options to PasswordStore::Init and letting it construct the correct task runner; further, PasswordStore::GetBackgroundTaskRunner should become non-virtual (and likely just a light background_task_runner() accessor).

This would be better done once the migration of PasswordStore off the DB thread is done.
 

Comment 1 by vabr@chromium.org, Jul 12 2017

Hi Vasilii, do you know why PasswordStoreMac needs the special thread and whether it could be replaced by an appropriate CreateSequencedTaskRunnerWithTraits call?
It can block the thread indefinitely because of the UI prompt. I didn't want to block the DB thread.
I guess we can use the same task runner now (base::MayBlock(), base::TaskPriority::USER_VISIBLE})

Comment 3 by vabr@chromium.org, Jul 12 2017

Thanks, Vasilii!

Indeed, the documentation for MayBlock() suggests that it should be enough to cover the case of waiting for UI. I will make sure to mention the blocking on UI prompt in a comment once I do the change.
I thought you were on vacation :)

Comment 5 by jochen@chromium.org, Jul 24 2017

Cc: -vasi...@chromium.org vabr@chromium.org
Owner: vasi...@chromium.org
Thanks Vaclav for working this out! Vasilii, can you implement this, so we hit he July 31st deadline?

Comment 6 by vabr@chromium.org, Jul 25 2017

Thanks for taking this!
Just wanted to clarify that this is not blocking the TaskScheduler migration, it's a non-blocking polish, so no need to rush (that's why it's P3).

The blocking piece in our code is only password_store_factory.cc, and as noted in https://docs.google.com/spreadsheets/d/18x9PGMlfgWcBr4fDz2SEEtIwTpSjcBFT2Puib47ZF1w/edit#gid=0, that one is blocked on pkasting's changes to the related WebDataService in https://chromium-review.googlesource.com/c/566181/ (which should land soon).

If any of you are taking the password_store_factory.cc work, please make sure to note that in the above linked spreadsheet, so that people know who to talk to about it if needed.

Thanks!
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 2 2017

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

commit 998e37023320444ead677d6f95b2ae3d86a7003a
Author: Vasilii Sukhanov <vasilii@chromium.org>
Date: Wed Aug 02 17:29:39 2017

Handle correctly the situation when PasswordStore is destroyed on the UI thread.

PasswordStore uses SKIP_ON_SHUTDOWN for background sequence. That means that PasswordStore::DestroyOnBackgroundThread() can be skipped. In this case PasswordStore::* may be destroyed on the UI thread.
It's not a problem for PasswordSyncableService. We just need to remove the check in the destructor.
PasswordReuseDetector has this check implicitly via base::CancelableTaskTracker. Thus, we can just leak the object.

Bug:  741660 
Change-Id: Ic49cb068733c159bca55e8d76eb58f70793f5fbe
Reviewed-on: https://chromium-review.googlesource.com/598008
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491422}
[modify] https://crrev.com/998e37023320444ead677d6f95b2ae3d86a7003a/components/password_manager/core/browser/password_store.cc
[modify] https://crrev.com/998e37023320444ead677d6f95b2ae3d86a7003a/components/password_manager/core/browser/password_store.h
[modify] https://crrev.com/998e37023320444ead677d6f95b2ae3d86a7003a/components/password_manager/core/browser/password_syncable_service.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Aug 3 2017

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

commit d95936bdef4d430e20500a14d778fa0c8ff0ac7a
Author: Vasilii Sukhanov <vasilii@chromium.org>
Date: Thu Aug 03 10:22:07 2017

Move the task runner creation to password store from the factory.

In addition the CL changes some tests to use ScopedTaskEnvironment where appropriate instead of MessageLoop.

Bug:  741660 
Change-Id: I9993eaba6bd92e30dfe4a39a5fb89518d5ba4a3d
Reviewed-on: https://chromium-review.googlesource.com/584908
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Christos Froussios <cfroussios@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491693}
[modify] https://crrev.com/d95936bdef4d430e20500a14d778fa0c8ff0ac7a/chrome/browser/password_manager/password_store_factory.cc
[modify] https://crrev.com/d95936bdef4d430e20500a14d778fa0c8ff0ac7a/chrome/browser/password_manager/password_store_mac.cc
[modify] https://crrev.com/d95936bdef4d430e20500a14d778fa0c8ff0ac7a/chrome/browser/password_manager/password_store_mac.h
[modify] https://crrev.com/d95936bdef4d430e20500a14d778fa0c8ff0ac7a/chrome/browser/password_manager/password_store_mac_unittest.cc
[modify] https://crrev.com/d95936bdef4d430e20500a14d778fa0c8ff0ac7a/chrome/browser/password_manager/password_store_win.cc
[modify] https://crrev.com/d95936bdef4d430e20500a14d778fa0c8ff0ac7a/chrome/browser/password_manager/password_store_win.h
[modify] https://crrev.com/d95936bdef4d430e20500a14d778fa0c8ff0ac7a/chrome/browser/password_manager/password_store_win_unittest.cc
[modify] https://crrev.com/d95936bdef4d430e20500a14d778fa0c8ff0ac7a/chrome/browser/password_manager/password_store_x.cc
[modify] https://crrev.com/d95936bdef4d430e20500a14d778fa0c8ff0ac7a/chrome/browser/password_manager/password_store_x.h
[modify] https://crrev.com/d95936bdef4d430e20500a14d778fa0c8ff0ac7a/chrome/browser/password_manager/password_store_x_unittest.cc
[modify] https://crrev.com/d95936bdef4d430e20500a14d778fa0c8ff0ac7a/components/password_manager/content/browser/credential_manager_impl_unittest.cc
[modify] https://crrev.com/d95936bdef4d430e20500a14d778fa0c8ff0ac7a/components/password_manager/core/browser/android_affiliation/affiliated_match_helper_unittest.cc
[modify] https://crrev.com/d95936bdef4d430e20500a14d778fa0c8ff0ac7a/components/password_manager/core/browser/form_fetcher_impl_unittest.cc
[modify] https://crrev.com/d95936bdef4d430e20500a14d778fa0c8ff0ac7a/components/password_manager/core/browser/http_data_cleaner_unittest.cc
[modify] https://crrev.com/d95936bdef4d430e20500a14d778fa0c8ff0ac7a/components/password_manager/core/browser/http_password_store_migrator_unittest.cc
[modify] https://crrev.com/d95936bdef4d430e20500a14d778fa0c8ff0ac7a/components/password_manager/core/browser/mock_password_store.cc
[modify] https://crrev.com/d95936bdef4d430e20500a14d778fa0c8ff0ac7a/components/password_manager/core/browser/mock_password_store.h
[modify] https://crrev.com/d95936bdef4d430e20500a14d778fa0c8ff0ac7a/components/password_manager/core/browser/password_store.cc
[modify] https://crrev.com/d95936bdef4d430e20500a14d778fa0c8ff0ac7a/components/password_manager/core/browser/password_store.h
[modify] https://crrev.com/d95936bdef4d430e20500a14d778fa0c8ff0ac7a/components/password_manager/core/browser/password_store_default.cc
[modify] https://crrev.com/d95936bdef4d430e20500a14d778fa0c8ff0ac7a/components/password_manager/core/browser/password_store_default.h
[modify] https://crrev.com/d95936bdef4d430e20500a14d778fa0c8ff0ac7a/components/password_manager/core/browser/password_store_default_unittest.cc
[modify] https://crrev.com/d95936bdef4d430e20500a14d778fa0c8ff0ac7a/components/password_manager/core/browser/password_store_unittest.cc
[modify] https://crrev.com/d95936bdef4d430e20500a14d778fa0c8ff0ac7a/components/password_manager/core/browser/password_syncable_service_unittest.cc
[modify] https://crrev.com/d95936bdef4d430e20500a14d778fa0c8ff0ac7a/components/password_manager/core/browser/test_password_store.cc
[modify] https://crrev.com/d95936bdef4d430e20500a14d778fa0c8ff0ac7a/components/password_manager/core/browser/test_password_store.h
[modify] https://crrev.com/d95936bdef4d430e20500a14d778fa0c8ff0ac7a/ios/chrome/browser/passwords/ios_chrome_password_store_factory.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Aug 3 2017

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

commit 6b2367e5917f0e363cdb6b62a087c0eb90b3588c
Author: Guido Urdaneta <guidou@chromium.org>
Date: Thu Aug 03 11:47:41 2017

Revert "Move the task runner creation to password store from the factory."

This reverts commit d95936bdef4d430e20500a14d778fa0c8ff0ac7a.

Reason for revert: Breaks component_tests on some bots.
See:
https://build.chromium.org/p/chromium.win/builders/Win10%20Tests%20x64/builds/14332

Original change's description:
> Move the task runner creation to password store from the factory.
> 
> In addition the CL changes some tests to use ScopedTaskEnvironment where appropriate instead of MessageLoop.
> 
> Bug:  741660 
> Change-Id: I9993eaba6bd92e30dfe4a39a5fb89518d5ba4a3d
> Reviewed-on: https://chromium-review.googlesource.com/584908
> Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
> Reviewed-by: Christos Froussios <cfroussios@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#491693}

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

Change-Id: I03224c0b0eeb33a18daa021046394d34062e242d
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  741660 
Reviewed-on: https://chromium-review.googlesource.com/599829
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491702}
[modify] https://crrev.com/6b2367e5917f0e363cdb6b62a087c0eb90b3588c/chrome/browser/password_manager/password_store_factory.cc
[modify] https://crrev.com/6b2367e5917f0e363cdb6b62a087c0eb90b3588c/chrome/browser/password_manager/password_store_mac.cc
[modify] https://crrev.com/6b2367e5917f0e363cdb6b62a087c0eb90b3588c/chrome/browser/password_manager/password_store_mac.h
[modify] https://crrev.com/6b2367e5917f0e363cdb6b62a087c0eb90b3588c/chrome/browser/password_manager/password_store_mac_unittest.cc
[modify] https://crrev.com/6b2367e5917f0e363cdb6b62a087c0eb90b3588c/chrome/browser/password_manager/password_store_win.cc
[modify] https://crrev.com/6b2367e5917f0e363cdb6b62a087c0eb90b3588c/chrome/browser/password_manager/password_store_win.h
[modify] https://crrev.com/6b2367e5917f0e363cdb6b62a087c0eb90b3588c/chrome/browser/password_manager/password_store_win_unittest.cc
[modify] https://crrev.com/6b2367e5917f0e363cdb6b62a087c0eb90b3588c/chrome/browser/password_manager/password_store_x.cc
[modify] https://crrev.com/6b2367e5917f0e363cdb6b62a087c0eb90b3588c/chrome/browser/password_manager/password_store_x.h
[modify] https://crrev.com/6b2367e5917f0e363cdb6b62a087c0eb90b3588c/chrome/browser/password_manager/password_store_x_unittest.cc
[modify] https://crrev.com/6b2367e5917f0e363cdb6b62a087c0eb90b3588c/components/password_manager/content/browser/credential_manager_impl_unittest.cc
[modify] https://crrev.com/6b2367e5917f0e363cdb6b62a087c0eb90b3588c/components/password_manager/core/browser/android_affiliation/affiliated_match_helper_unittest.cc
[modify] https://crrev.com/6b2367e5917f0e363cdb6b62a087c0eb90b3588c/components/password_manager/core/browser/form_fetcher_impl_unittest.cc
[modify] https://crrev.com/6b2367e5917f0e363cdb6b62a087c0eb90b3588c/components/password_manager/core/browser/http_data_cleaner_unittest.cc
[modify] https://crrev.com/6b2367e5917f0e363cdb6b62a087c0eb90b3588c/components/password_manager/core/browser/http_password_store_migrator_unittest.cc
[modify] https://crrev.com/6b2367e5917f0e363cdb6b62a087c0eb90b3588c/components/password_manager/core/browser/mock_password_store.cc
[modify] https://crrev.com/6b2367e5917f0e363cdb6b62a087c0eb90b3588c/components/password_manager/core/browser/mock_password_store.h
[modify] https://crrev.com/6b2367e5917f0e363cdb6b62a087c0eb90b3588c/components/password_manager/core/browser/password_store.cc
[modify] https://crrev.com/6b2367e5917f0e363cdb6b62a087c0eb90b3588c/components/password_manager/core/browser/password_store.h
[modify] https://crrev.com/6b2367e5917f0e363cdb6b62a087c0eb90b3588c/components/password_manager/core/browser/password_store_default.cc
[modify] https://crrev.com/6b2367e5917f0e363cdb6b62a087c0eb90b3588c/components/password_manager/core/browser/password_store_default.h
[modify] https://crrev.com/6b2367e5917f0e363cdb6b62a087c0eb90b3588c/components/password_manager/core/browser/password_store_default_unittest.cc
[modify] https://crrev.com/6b2367e5917f0e363cdb6b62a087c0eb90b3588c/components/password_manager/core/browser/password_store_unittest.cc
[modify] https://crrev.com/6b2367e5917f0e363cdb6b62a087c0eb90b3588c/components/password_manager/core/browser/password_syncable_service_unittest.cc
[modify] https://crrev.com/6b2367e5917f0e363cdb6b62a087c0eb90b3588c/components/password_manager/core/browser/test_password_store.cc
[modify] https://crrev.com/6b2367e5917f0e363cdb6b62a087c0eb90b3588c/components/password_manager/core/browser/test_password_store.h
[modify] https://crrev.com/6b2367e5917f0e363cdb6b62a087c0eb90b3588c/ios/chrome/browser/passwords/ios_chrome_password_store_factory.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Aug 3 2017

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

commit 044b0572ba8059a64de4e940548e29e6f614af57
Author: John Mellor <johnme@chromium.org>
Date: Thu Aug 03 13:59:52 2017

Revert "Move the task runner creation to password store from the factory."

This reverts commit d95936bdef4d430e20500a14d778fa0c8ff0ac7a.

Reason for revert: this caused
PasswordStoreTest.GetLoginsWithAffiliationAndBrandingInformation
to fail on KitKat Tablet Tester with the following error:

[ERROR:connection.cc(1964)] Passwords sqlite error 5, errno 0: database is locked, sql: PRAGMA auto_vacuum
[FATAL:connection.cc(1979)] database is locked

https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.android%2FKitKat_Tablet_Tester%2F8506%2F%2B%2Frecipes%2Fsteps%2Fcomponents_unittests_on_Android%2F0%2Flogs%2FPasswordStoreTest.GetLoginsWithAffiliationAndBrandingInformation%2F0

Original change's description:
> Move the task runner creation to password store from the factory.
> 
> In addition the CL changes some tests to use ScopedTaskEnvironment where appropriate instead of MessageLoop.
> 
> Bug:  741660 
> Change-Id: I9993eaba6bd92e30dfe4a39a5fb89518d5ba4a3d
> Reviewed-on: https://chromium-review.googlesource.com/584908
> Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
> Reviewed-by: Christos Froussios <cfroussios@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#491693}

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

Change-Id: Ifa8c22c62bab7f0200437e3c2ab7661b5fddc78a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  741660 
Reviewed-on: https://chromium-review.googlesource.com/600211
Reviewed-by: John Mellor <johnme@chromium.org>
Commit-Queue: John Mellor <johnme@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491729}

Project Member

Comment 11 by bugdroid1@chromium.org, Aug 3 2017

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

commit 779fc22e3cc4487b316b4523e6275ecdfbe32764
Author: Vasilii Sukhanov <vasilii@chromium.org>
Date: Thu Aug 03 16:01:14 2017

Reland "Move the task runner creation to password store from the factory."

This is a reland of d95936bdef4d430e20500a14d778fa0c8ff0ac7a
Original change's description:
> Move the task runner creation to password store from the factory.
> 
> In addition the CL changes some tests to use ScopedTaskEnvironment where appropriate instead of MessageLoop.
> 
> Bug:  741660 
> Change-Id: I9993eaba6bd92e30dfe4a39a5fb89518d5ba4a3d
> Reviewed-on: https://chromium-review.googlesource.com/584908
> Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
> Reviewed-by: Christos Froussios <cfroussios@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#491693}

Bug:  741660 
Change-Id: I9ef1b4304f69dbf6fb3dc98dcec5843e92fc6371
Reviewed-on: https://chromium-review.googlesource.com/600207
Reviewed-by: Christos Froussios <cfroussios@chromium.org>
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491755}
[modify] https://crrev.com/779fc22e3cc4487b316b4523e6275ecdfbe32764/chrome/browser/password_manager/password_store_factory.cc
[modify] https://crrev.com/779fc22e3cc4487b316b4523e6275ecdfbe32764/chrome/browser/password_manager/password_store_mac.cc
[modify] https://crrev.com/779fc22e3cc4487b316b4523e6275ecdfbe32764/chrome/browser/password_manager/password_store_mac.h
[modify] https://crrev.com/779fc22e3cc4487b316b4523e6275ecdfbe32764/chrome/browser/password_manager/password_store_mac_unittest.cc
[modify] https://crrev.com/779fc22e3cc4487b316b4523e6275ecdfbe32764/chrome/browser/password_manager/password_store_win.cc
[modify] https://crrev.com/779fc22e3cc4487b316b4523e6275ecdfbe32764/chrome/browser/password_manager/password_store_win.h
[modify] https://crrev.com/779fc22e3cc4487b316b4523e6275ecdfbe32764/chrome/browser/password_manager/password_store_win_unittest.cc
[modify] https://crrev.com/779fc22e3cc4487b316b4523e6275ecdfbe32764/chrome/browser/password_manager/password_store_x.cc
[modify] https://crrev.com/779fc22e3cc4487b316b4523e6275ecdfbe32764/chrome/browser/password_manager/password_store_x.h
[modify] https://crrev.com/779fc22e3cc4487b316b4523e6275ecdfbe32764/chrome/browser/password_manager/password_store_x_unittest.cc
[modify] https://crrev.com/779fc22e3cc4487b316b4523e6275ecdfbe32764/components/password_manager/content/browser/credential_manager_impl_unittest.cc
[modify] https://crrev.com/779fc22e3cc4487b316b4523e6275ecdfbe32764/components/password_manager/core/browser/android_affiliation/affiliated_match_helper_unittest.cc
[modify] https://crrev.com/779fc22e3cc4487b316b4523e6275ecdfbe32764/components/password_manager/core/browser/form_fetcher_impl_unittest.cc
[modify] https://crrev.com/779fc22e3cc4487b316b4523e6275ecdfbe32764/components/password_manager/core/browser/http_data_cleaner_unittest.cc
[modify] https://crrev.com/779fc22e3cc4487b316b4523e6275ecdfbe32764/components/password_manager/core/browser/http_password_store_migrator_unittest.cc
[modify] https://crrev.com/779fc22e3cc4487b316b4523e6275ecdfbe32764/components/password_manager/core/browser/mock_password_store.cc
[modify] https://crrev.com/779fc22e3cc4487b316b4523e6275ecdfbe32764/components/password_manager/core/browser/mock_password_store.h
[modify] https://crrev.com/779fc22e3cc4487b316b4523e6275ecdfbe32764/components/password_manager/core/browser/password_store.cc
[modify] https://crrev.com/779fc22e3cc4487b316b4523e6275ecdfbe32764/components/password_manager/core/browser/password_store.h
[modify] https://crrev.com/779fc22e3cc4487b316b4523e6275ecdfbe32764/components/password_manager/core/browser/password_store_default.cc
[modify] https://crrev.com/779fc22e3cc4487b316b4523e6275ecdfbe32764/components/password_manager/core/browser/password_store_default.h
[modify] https://crrev.com/779fc22e3cc4487b316b4523e6275ecdfbe32764/components/password_manager/core/browser/password_store_default_unittest.cc
[modify] https://crrev.com/779fc22e3cc4487b316b4523e6275ecdfbe32764/components/password_manager/core/browser/password_store_unittest.cc
[modify] https://crrev.com/779fc22e3cc4487b316b4523e6275ecdfbe32764/components/password_manager/core/browser/password_syncable_service_unittest.cc
[modify] https://crrev.com/779fc22e3cc4487b316b4523e6275ecdfbe32764/components/password_manager/core/browser/test_password_store.cc
[modify] https://crrev.com/779fc22e3cc4487b316b4523e6275ecdfbe32764/components/password_manager/core/browser/test_password_store.h
[modify] https://crrev.com/779fc22e3cc4487b316b4523e6275ecdfbe32764/ios/chrome/browser/passwords/ios_chrome_password_store_factory.cc

Status: Fixed (was: Assigned)
Cc: -vabr@chromium.org

Sign in to add a comment