Start the migration of passwords from the Keychain.

Start the migration of passwords from the Keychain.

Design doc: https://docs.google.com/a/google.com/document/d/1A8ZG16bLuUH1u21K0GoABKz_wpz1kchXMnMlpmq_ecA/edit?usp=sharing

BUG=466638
Committed: https://crrev.com/b312da5608ac316b7e49459c906b5a5d72332776
Cr-Commit-Position: refs/heads/master@{#337842}

[vasilii, 2015-07-02 19:53:27]

vasilii@chromium.org changed reviewers:
+ gcasto@chromium.org, isherman@chromium.org, stuartmorgan@chromium.org

[vasilii, 2015-07-02 19:54:04]

Hi guys, 

please review the CL.

[vabr (Chromium), 2015-07-03 08:40:47]

vabr@chromium.org changed reviewers:
+ vabr@chromium.org

[vabr (Chromium), 2015-07-03 08:40:48]

Hi Vasilii,

This LGTM with some minor comments.

Having said that, this seems risky enough that we should roll it out behind
Finch -- what do you think? We could ramp up slowly, and if something goes
wrong, stop it. For that we would need also to make sure that stopping this
would result in some consistent state (i.e., while not doing migration, the code
would still need to understand whether the migration happened or not). What do
you think?

Also, one thing I was looking at: if encryption fails (after migration) in the
login database, it is a silent failure (e.g.,
PasswordStoreDefault::FillMatchingLogins just returns nothing). So users who
have issues with their keychains would see breakages with no hints to the
(decrypting-key-holding) keychain as the cuplrit. I guess that these users are
unlikely to go through the migration anyway, and also they probably have the
same silent failures now when trying to get their passwords from the keychain,
so that's not anything to block this CL. But please double-check in case I
confused something.

Cheers,
Vaclav

https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/password_store_mac.cc (left):

https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_store_mac.cc:963:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
Just out of curiosity, why is this removed?

https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/password_store_proxy_mac.cc (right):

https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_store_proxy_mac.cc:31:
scoped_ptr<password_manager::LoginDatabase>());
optional: I believe nullptr should work here as well. But if you want to keep
the more verbose empty scoped_ptr, because it makes it clear that it is a null
database pointer, that's fine as well.

https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/password_store_proxy_mac_unittest.cc
(right):

https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_store_proxy_mac_unittest.cc:423:
login_db_.reset(
Is this just to undo lines 420 and 421? Perhaps a code comment would help to
understand the purpose of this line in any case.

https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_store_proxy_mac_unittest.cc:470:
testing::Values(PasswordStoreProxyMac::NOT_STARTED,
Would it make sense to run this test also for the other migration statuses (with
appropriate changes to lines 451-453)?
While I understand that no migration should happen in those other cases, that
probably also needs to be tested (in the sense that whatever happens, the logins
are available from the proxy after it is created with a non-empty database and
keychain).

https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/simple_password_store_mac.h (right):

https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/simple_password_store_mac.h:16:
scoped_refptr<base::SingleThreadTaskRunner> background_thread_runner,
nit: Passing of background runners both in the constructor and in
InitWithTaskRunner deserves an explanation in a comment. Alternatively, if it
always passes nullptr to PasswordStoreDefault, then we should drop it as the
argument and just hard-code nullptr to the call to PasswordStoreDefault's
constructor.

https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/simple_password_store_mac.h:34: using
PasswordStoreDefault::Init;
Please remind me: where is this called from?

https://codereview.chromium.org/1213043003/diff/1/components/password_manager...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/1213043003/diff/1/components/password_manager...
components/password_manager/core/browser/password_manager.cc:146:
registry->RegisterIntegerPref(prefs::kKeychainMigrationStatus, 0);
While not detectable to the compiler, this is a kind of dependency violation --
the 0 here means the proxy's NOT_STARTED constant, which is defined in //chrome
and "used" here in //components.

There are multiple ways to address this, out of which I find the following the
easiest and sufficiently clean:

Move the definition of MigrationStatus to
components/password_manager/core/common/password_manager_pref_names.h. The pref
name is already there, so it makes sense to define the pref's values in the same
place. Then you can refer NOT_STARTED here directly, and there will be no
dependency violation.

[vasilii, 2015-07-03 12:59:11]

Hi Vaclav,

Thank you for the quick review. Regarding Finch. I'm reluctant to hide the
feature behind flags. This is not an experiment. We are going to drop the
Keychain. It may require some bug/crash fixes. Canary/Beta are here exactly for
the reason to identify and address the problems early. Also I don't think I'm
doing something dangerous because we are not cleaning the Keychain/database in
the process. If other reviewers push for Finch too, I may give in.

Encryption can't fail after migration. The key is looked up just once and cached
afterwards. Thus, if it's not available, the migration fails with
PasswordStoreMac::ENCRYPTOR_FAILURE.

Regards,
Vasily

https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/password_store_mac.cc (left):

https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_store_mac.cc:963:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
On 2015/07/03 08:40:47, vabr (Chromium) wrote:
> Just out of curiosity, why is this removed?

Because now it's called on the background thread where db_thread_runner_ is
mostly accessed.

https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/password_store_proxy_mac.cc (right):

https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_store_proxy_mac.cc:31:
scoped_ptr<password_manager::LoginDatabase>());
On 2015/07/03 08:40:48, vabr (Chromium) wrote:
> optional: I believe nullptr should work here as well. But if you want to keep
> the more verbose empty scoped_ptr, because it makes it clear that it is a null
> database pointer, that's fine as well.

Done.

https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/password_store_proxy_mac_unittest.cc
(right):

https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_store_proxy_mac_unittest.cc:423:
login_db_.reset(
On 2015/07/03 08:40:48, vabr (Chromium) wrote:
> Is this just to undo lines 420 and 421? Perhaps a code comment would help to
> understand the purpose of this line in any case.

I needed another instance because LoginDatabase doesn't tolerate double init.
The second init happened inside PasswordStoreProxyMac. The content on the other
hand is the same.

https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_store_proxy_mac_unittest.cc:470:
testing::Values(PasswordStoreProxyMac::NOT_STARTED,
On 2015/07/03 08:40:48, vabr (Chromium) wrote:
> Would it make sense to run this test also for the other migration statuses
(with
> appropriate changes to lines 451-453)?
> While I understand that no migration should happen in those other cases, that
> probably also needs to be tested (in the sense that whatever happens, the
logins
> are available from the proxy after it is created with a non-empty database and
> keychain).

Done.

https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/simple_password_store_mac.h (right):

https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/simple_password_store_mac.h:16:
scoped_refptr<base::SingleThreadTaskRunner> background_thread_runner,
On 2015/07/03 08:40:48, vabr (Chromium) wrote:
> nit: Passing of background runners both in the constructor and in
> InitWithTaskRunner deserves an explanation in a comment. Alternatively, if it
> always passes nullptr to PasswordStoreDefault, then we should drop it as the
> argument and just hard-code nullptr to the call to PasswordStoreDefault's
> constructor.

Done. I use both null and non-null calls.

https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/simple_password_store_mac.h:34: using
PasswordStoreDefault::Init;
On 2015/07/03 08:40:48, vabr (Chromium) wrote:
> Please remind me: where is this called from?

It's not called, that's why it's private.

https://codereview.chromium.org/1213043003/diff/1/components/password_manager...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/1213043003/diff/1/components/password_manager...
components/password_manager/core/browser/password_manager.cc:146:
registry->RegisterIntegerPref(prefs::kKeychainMigrationStatus, 0);
On 2015/07/03 08:40:48, vabr (Chromium) wrote:
> While not detectable to the compiler, this is a kind of dependency violation
--
> the 0 here means the proxy's NOT_STARTED constant, which is defined in
//chrome
> and "used" here in //components.
> 
> There are multiple ways to address this, out of which I find the following the
> easiest and sufficiently clean:
> 
> Move the definition of MigrationStatus to
> components/password_manager/core/common/password_manager_pref_names.h. The
pref
> name is already there, so it makes sense to define the pref's values in the
same
> place. Then you can refer NOT_STARTED here directly, and there will be no
> dependency violation.

Done.

[vabr (Chromium), 2015-07-03 13:14:14]

On 2015/07/03 12:59:11, vasilii wrote:
> Hi Vaclav,
> 
> Thank you for the quick review. Regarding Finch. I'm reluctant to hide the
> feature behind flags. This is not an experiment. We are going to drop the
> Keychain. It may require some bug/crash fixes. Canary/Beta are here exactly
for
> the reason to identify and address the problems early. Also I don't think I'm
> doing something dangerous because we are not cleaning the Keychain/database in
> the process. If other reviewers push for Finch too, I may give in.

Thanks, Vasilii, this sounds reasonable. I agree that this is not an experiment,
that unstable channels come with a disclaimer covering this, and that Keychain
should be keeping the passwords in the worst case.
I'll have a look at the rest of your comments and respond shortly.

Cheers,
Vaclav

> 
> Encryption can't fail after migration. The key is looked up just once and
cached
> afterwards. Thus, if it's not available, the migration fails with
> PasswordStoreMac::ENCRYPTOR_FAILURE.
> 
> Regards,
> Vasily
> 
>
https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
> File chrome/browser/password_manager/password_store_mac.cc (left):
> 
>
https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
> chrome/browser/password_manager/password_store_mac.cc:963:
> DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
> On 2015/07/03 08:40:47, vabr (Chromium) wrote:
> > Just out of curiosity, why is this removed?
> 
> Because now it's called on the background thread where db_thread_runner_ is
> mostly accessed.
> 
>
https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
> File chrome/browser/password_manager/password_store_proxy_mac.cc (right):
> 
>
https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
> chrome/browser/password_manager/password_store_proxy_mac.cc:31:
> scoped_ptr<password_manager::LoginDatabase>());
> On 2015/07/03 08:40:48, vabr (Chromium) wrote:
> > optional: I believe nullptr should work here as well. But if you want to
keep
> > the more verbose empty scoped_ptr, because it makes it clear that it is a
null
> > database pointer, that's fine as well.
> 
> Done.
> 
>
https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
> File chrome/browser/password_manager/password_store_proxy_mac_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
> chrome/browser/password_manager/password_store_proxy_mac_unittest.cc:423:
> login_db_.reset(
> On 2015/07/03 08:40:48, vabr (Chromium) wrote:
> > Is this just to undo lines 420 and 421? Perhaps a code comment would help to
> > understand the purpose of this line in any case.
> 
> I needed another instance because LoginDatabase doesn't tolerate double init.
> The second init happened inside PasswordStoreProxyMac. The content on the
other
> hand is the same.
> 
>
https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
> chrome/browser/password_manager/password_store_proxy_mac_unittest.cc:470:
> testing::Values(PasswordStoreProxyMac::NOT_STARTED,
> On 2015/07/03 08:40:48, vabr (Chromium) wrote:
> > Would it make sense to run this test also for the other migration statuses
> (with
> > appropriate changes to lines 451-453)?
> > While I understand that no migration should happen in those other cases,
that
> > probably also needs to be tested (in the sense that whatever happens, the
> logins
> > are available from the proxy after it is created with a non-empty database
and
> > keychain).
> 
> Done.
> 
>
https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
> File chrome/browser/password_manager/simple_password_store_mac.h (right):
> 
>
https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
> chrome/browser/password_manager/simple_password_store_mac.h:16:
> scoped_refptr<base::SingleThreadTaskRunner> background_thread_runner,
> On 2015/07/03 08:40:48, vabr (Chromium) wrote:
> > nit: Passing of background runners both in the constructor and in
> > InitWithTaskRunner deserves an explanation in a comment. Alternatively, if
it
> > always passes nullptr to PasswordStoreDefault, then we should drop it as the
> > argument and just hard-code nullptr to the call to PasswordStoreDefault's
> > constructor.
> 
> Done. I use both null and non-null calls.
> 
>
https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
> chrome/browser/password_manager/simple_password_store_mac.h:34: using
> PasswordStoreDefault::Init;
> On 2015/07/03 08:40:48, vabr (Chromium) wrote:
> > Please remind me: where is this called from?
> 
> It's not called, that's why it's private.
> 
>
https://codereview.chromium.org/1213043003/diff/1/components/password_manager...
> File components/password_manager/core/browser/password_manager.cc (right):
> 
>
https://codereview.chromium.org/1213043003/diff/1/components/password_manager...
> components/password_manager/core/browser/password_manager.cc:146:
> registry->RegisterIntegerPref(prefs::kKeychainMigrationStatus, 0);
> On 2015/07/03 08:40:48, vabr (Chromium) wrote:
> > While not detectable to the compiler, this is a kind of dependency violation
> --
> > the 0 here means the proxy's NOT_STARTED constant, which is defined in
> //chrome
> > and "used" here in //components.
> > 
> > There are multiple ways to address this, out of which I find the following
the
> > easiest and sufficiently clean:
> > 
> > Move the definition of MigrationStatus to
> > components/password_manager/core/common/password_manager_pref_names.h. The
> pref
> > name is already there, so it makes sense to define the pref's values in the
> same
> > place. Then you can refer NOT_STARTED here directly, and there will be no
> > dependency violation.
> 
> Done.

[vasilii, 2015-07-06 08:29:03]

The bots are green. Please review.

[vabr (Chromium), 2015-07-06 09:50:20]

Hi Vasilii,
LGTM + mostly nits.

Thanks,
Vaclav

https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/password_store_mac.cc (left):

https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_store_mac.cc:963:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
On 2015/07/03 12:59:10, vasilii wrote:
> On 2015/07/03 08:40:47, vabr (Chromium) wrote:
> > Just out of curiosity, why is this removed?
> 
> Because now it's called on the background thread where db_thread_runner_ is
> mostly accessed.

Acknowledged.

https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/password_store_proxy_mac_unittest.cc
(right):

https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/password_store_proxy_mac_unittest.cc:423:
login_db_.reset(
On 2015/07/03 12:59:11, vasilii wrote:
> On 2015/07/03 08:40:48, vabr (Chromium) wrote:
> > Is this just to undo lines 420 and 421? Perhaps a code comment would help to
> > understand the purpose of this line in any case.
> 
> I needed another instance because LoginDatabase doesn't tolerate double init.
> The second init happened inside PasswordStoreProxyMac. The content on the
other
> hand is the same.

Acknowledged.

https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/simple_password_store_mac.h (right):

https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/simple_password_store_mac.h:34: using
PasswordStoreDefault::Init;
On 2015/07/03 12:59:11, vasilii wrote:
> On 2015/07/03 08:40:48, vabr (Chromium) wrote:
> > Please remind me: where is this called from?
> 
> It's not called, that's why it's private.

Fair enough. But if you want to avoid people calling Init on
SimplePasswordStoreMac by accident, I would suggest actually providing an
implementation and putting NOTREACHED() in it. This way one can still call
Init() through down-casting the store's pointer. The private + using hint may be
too subtle and get missed, whereas a NOTREACHED() is a clear statement.

https://codereview.chromium.org/1213043003/diff/40001/chrome/browser/password...
File chrome/browser/password_manager/password_store_proxy_mac.cc (right):

https://codereview.chromium.org/1213043003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac.cc:70: // After the
thread has stopped, the backend can't change.
I'm a bit unclear about what the comment tries to say. Is it defending shutting
down the backend here? Or does it say that the backend cannot do anything after
line 62 above? The latter is a bit confusing (can it still shut down?), so
please explain.

https://codereview.chromium.org/1213043003/diff/40001/chrome/browser/password...
File chrome/browser/password_manager/password_store_proxy_mac.h (right):

https://codereview.chromium.org/1213043003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac.h:96: // The ownership
maybe transferred to |password_store_simple_|.
nit: maybe -> may be
(otherwise the verb is missing)

https://codereview.chromium.org/1213043003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac.h:105: // List of task
filled by InitOnBackgroundThread. They can't be just posted to
nit: task -> tasks

https://codereview.chromium.org/1213043003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac.h:105: // List of task
filled by InitOnBackgroundThread. They can't be just posted to
After clarifying the concurrency concerns with you off-line, Vasilii, I think
this comment needs to specifically point out that to avoid concurrent access to
the queue:
(1) after InitOnBackgroundThread is run once, the queue may not be modified on
the background thread any more
(2) Shutdown() flushes the queue _after stopping the background thread_

Mentioning (1) seems really important. Alternatively you can just add "only" to
"filled by InitOnBackgroundThread", but that's a stronger statement and could be
confusing in some future refactoring.

(2) is not crucial, but helps to understand the design.

https://codereview.chromium.org/1213043003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac.h:106: // the UI thread
because the message loop can shutdown before executing them.
nit: shutdown -> shut down
(the verb not the noun)

https://codereview.chromium.org/1213043003/diff/40001/chrome/browser/password...
File chrome/browser/password_manager/password_store_proxy_mac_unittest.cc
(right):

https://codereview.chromium.org/1213043003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac_unittest.cc:264:
EXPECT_EQ(static_cast<int>(MigrationStatus::MIGRATED), status);
How do we ensure that store's ShutDown() did not stop the background thread
before the migration happened?

https://codereview.chromium.org/1213043003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac_unittest.cc:425:
GetParam() == MigrationStatus::FAILED_ONCE)
Shouldn't the login DB also be clearing passwords in the FAILED_TWICE case?

https://codereview.chromium.org/1213043003/diff/40001/components/password_man...
File components/password_manager/core/common/password_manager_pref_names.h
(right):

https://codereview.chromium.org/1213043003/diff/40001/components/password_man...
components/password_manager/core/common/password_manager_pref_names.h:60: //
database. Stores the value from MigrationStatus.
nit: the value -> a value
(The type does not hold a concrete value, as a variable would, so there is no
already defined "the value" to speak about.)

[vasilii, 2015-07-06 13:57:58]

https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/simple_password_store_mac.h (right):

https://codereview.chromium.org/1213043003/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/simple_password_store_mac.h:34: using
PasswordStoreDefault::Init;
On 2015/07/06 09:50:19, vabr (Chromium) wrote:
> On 2015/07/03 12:59:11, vasilii wrote:
> > On 2015/07/03 08:40:48, vabr (Chromium) wrote:
> > > Please remind me: where is this called from?
> > 
> > It's not called, that's why it's private.
> 
> Fair enough. But if you want to avoid people calling Init on
> SimplePasswordStoreMac by accident, I would suggest actually providing an
> implementation and putting NOTREACHED() in it. This way one can still call
> Init() through down-casting the store's pointer. The private + using hint may
be
> too subtle and get missed, whereas a NOTREACHED() is a clear statement.

Done.

https://codereview.chromium.org/1213043003/diff/40001/chrome/browser/password...
File chrome/browser/password_manager/password_store_proxy_mac.cc (right):

https://codereview.chromium.org/1213043003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac.cc:70: // After the
thread has stopped, the backend can't change.
On 2015/07/06 09:50:19, vabr (Chromium) wrote:
> I'm a bit unclear about what the comment tries to say. Is it defending
shutting
> down the backend here? Or does it say that the backend cannot do anything
after
> line 62 above? The latter is a bit confusing (can it still shut down?), so
> please explain.

Done.

https://codereview.chromium.org/1213043003/diff/40001/chrome/browser/password...
File chrome/browser/password_manager/password_store_proxy_mac.h (right):

https://codereview.chromium.org/1213043003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac.h:96: // The ownership
maybe transferred to |password_store_simple_|.
On 2015/07/06 09:50:19, vabr (Chromium) wrote:
> nit: maybe -> may be
> (otherwise the verb is missing)

Done.

https://codereview.chromium.org/1213043003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac.h:105: // List of task
filled by InitOnBackgroundThread. They can't be just posted to
On 2015/07/06 09:50:19, vabr (Chromium) wrote:
> After clarifying the concurrency concerns with you off-line, Vasilii, I think
> this comment needs to specifically point out that to avoid concurrent access
to
> the queue:
> (1) after InitOnBackgroundThread is run once, the queue may not be modified on
> the background thread any more
> (2) Shutdown() flushes the queue _after stopping the background thread_
> 
> Mentioning (1) seems really important. Alternatively you can just add "only"
to
> "filled by InitOnBackgroundThread", but that's a stronger statement and could
be
> confusing in some future refactoring.
> 
> (2) is not crucial, but helps to understand the design.

Done.

https://codereview.chromium.org/1213043003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac.h:105: // List of task
filled by InitOnBackgroundThread. They can't be just posted to
On 2015/07/06 09:50:19, vabr (Chromium) wrote:
> nit: task -> tasks

Done.

https://codereview.chromium.org/1213043003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac.h:106: // the UI thread
because the message loop can shutdown before executing them.
On 2015/07/06 09:50:19, vabr (Chromium) wrote:
> nit: shutdown -> shut down
> (the verb not the noun)

Done.

https://codereview.chromium.org/1213043003/diff/40001/chrome/browser/password...
File chrome/browser/password_manager/password_store_proxy_mac_unittest.cc
(right):

https://codereview.chromium.org/1213043003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac_unittest.cc:264:
EXPECT_EQ(static_cast<int>(MigrationStatus::MIGRATED), status);
On 2015/07/06 09:50:19, vabr (Chromium) wrote:
> How do we ensure that store's ShutDown() did not stop the background thread
> before the migration happened?

Shutdown() calls Thread::Stop which runs the message loop first.

https://codereview.chromium.org/1213043003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac_unittest.cc:425:
GetParam() == MigrationStatus::FAILED_ONCE)
On 2015/07/06 09:50:19, vabr (Chromium) wrote:
> Shouldn't the login DB also be clearing passwords in the FAILED_TWICE case?

Done.

https://codereview.chromium.org/1213043003/diff/40001/components/password_man...
File components/password_manager/core/common/password_manager_pref_names.h
(right):

https://codereview.chromium.org/1213043003/diff/40001/components/password_man...
components/password_manager/core/common/password_manager_pref_names.h:60: //
database. Stores the value from MigrationStatus.
On 2015/07/06 09:50:19, vabr (Chromium) wrote:
> nit: the value -> a value
> (The type does not hold a concrete value, as a variable would, so there is no
> already defined "the value" to speak about.)

Done.

[Garrett Casto, 2015-07-06 21:38:54]

https://codereview.chromium.org/1213043003/diff/60001/chrome/browser/password...
File chrome/browser/password_manager/password_store_proxy_mac.cc (right):

https://codereview.chromium.org/1213043003/diff/60001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac.cc:62: thread_->Stop();
Stop should be called after GetBackend()->Shutdown(), or part of the shutdown
for the backend will be skipped. At the moment it doesn't matter because
PasswordSyncableService associated with the backend should never be accessed,
but it may cause problems in the future.

https://codereview.chromium.org/1213043003/diff/60001/chrome/browser/password...
File chrome/browser/password_manager/password_store_proxy_mac.h (right):

https://codereview.chromium.org/1213043003/diff/60001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac.h:111:
std::deque<base::Closure> pending_ui_tasks_;
Why is this a deque and not a vector?

https://codereview.chromium.org/1213043003/diff/60001/chrome/browser/password...
File chrome/browser/password_manager/simple_password_store_mac.h (right):

https://codereview.chromium.org/1213043003/diff/60001/chrome/browser/password...
chrome/browser/password_manager/simple_password_store_mac.h:28: void Shutdown()
override;
Do you still have plans to add extra functionality here eventually?

[Ilya Sherman, 2015-07-07 02:48:22]

I'm assuming you just wanted me to review the histograms code?

https://codereview.chromium.org/1213043003/diff/60001/components/password_man...
File components/password_manager/core/common/password_manager_pref_names.h
(right):

https://codereview.chromium.org/1213043003/diff/60001/components/password_man...
components/password_manager/core/common/password_manager_pref_names.h:14: enum
class MigrationStatus {
Please document that this enum is used to back a histogram, and should therefore
be treated as append-only.

https://codereview.chromium.org/1213043003/diff/60001/components/password_man...
components/password_manager/core/common/password_manager_pref_names.h:27:
NUM_MIGRATION_STATUS,
nit: either "NUM_MIGRATIN_STATUSES" or "MIGRATION_STATUS_COUNT" (or just
"NUM_STATUSES" or "COUNT", since this is an enum class).

https://codereview.chromium.org/1213043003/diff/60001/components/password_man...
components/password_manager/core/common/password_manager_pref_names.h:29: #endif
nit: These are not pref names.  Please declare them in a different header file.

https://codereview.chromium.org/1213043003/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1213043003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:27833: +  <summary>The status of
passwords migration from the Keychain.</summary>
Please document when this is recorded.

[Ilya Sherman, 2015-07-07 02:49:04]

https://codereview.chromium.org/1213043003/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1213043003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:58785: +  <int value="3"
label="Migration failed twice."/>
Optional nit: I'd omit the trailing periods, since these are shown as the
left-hand column labels of a table.

[vabr (Chromium), 2015-07-07 14:18:59]

Thanks, Vasilii.

LGTM, I have no more comments, but please do not land before clarifying the
other comments. In particular about the order of stopping the thread and calling
Shutdown.

Cheers,
Vaclav

[vasilii, 2015-07-07 15:45:54]

Ilya, I need an approval for histograms. Optionally you can review everything.

https://codereview.chromium.org/1213043003/diff/60001/chrome/browser/password...
File chrome/browser/password_manager/password_store_proxy_mac.cc (right):

https://codereview.chromium.org/1213043003/diff/60001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac.cc:62: thread_->Stop();
On 2015/07/06 21:38:54, Garrett Casto wrote:
> Stop should be called after GetBackend()->Shutdown(), or part of the shutdown
> for the backend will be skipped. At the moment it doesn't matter because
> PasswordSyncableService associated with the backend should never be accessed,
> but it may cause problems in the future.

PasswordSyncableService isn't even instantiated because I don't call Init().It
should be fine in the future too because I call only private implementation
methods. |observers_|, |syncable_service_|, |affiliated_match_helper_| should
all be empty.
I can't call GetBackend()->Shutdown() earlier because GetBackend() can change on
the background. Using WaitableEvent on the UI thread isn't allowed.

https://codereview.chromium.org/1213043003/diff/60001/chrome/browser/password...
File chrome/browser/password_manager/password_store_proxy_mac.h (right):

https://codereview.chromium.org/1213043003/diff/60001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac.h:111:
std::deque<base::Closure> pending_ui_tasks_;
On 2015/07/06 21:38:54, Garrett Casto wrote:
> Why is this a deque and not a vector?

Done.

https://codereview.chromium.org/1213043003/diff/60001/chrome/browser/password...
File chrome/browser/password_manager/simple_password_store_mac.h (right):

https://codereview.chromium.org/1213043003/diff/60001/chrome/browser/password...
chrome/browser/password_manager/simple_password_store_mac.h:28: void Shutdown()
override;
On 2015/07/06 21:38:54, Garrett Casto wrote:
> Do you still have plans to add extra functionality here eventually?

No, I removed the method.

https://codereview.chromium.org/1213043003/diff/60001/components/password_man...
File components/password_manager/core/common/password_manager_pref_names.h
(right):

https://codereview.chromium.org/1213043003/diff/60001/components/password_man...
components/password_manager/core/common/password_manager_pref_names.h:14: enum
class MigrationStatus {
On 2015/07/07 02:48:22, Ilya Sherman (Away July 8-13) wrote:
> Please document that this enum is used to back a histogram, and should
therefore
> be treated as append-only.

Done.

https://codereview.chromium.org/1213043003/diff/60001/components/password_man...
components/password_manager/core/common/password_manager_pref_names.h:27:
NUM_MIGRATION_STATUS,
On 2015/07/07 02:48:22, Ilya Sherman (Away July 8-13) wrote:
> nit: either "NUM_MIGRATIN_STATUSES" or "MIGRATION_STATUS_COUNT" (or just
> "NUM_STATUSES" or "COUNT", since this is an enum class).

Done.

https://codereview.chromium.org/1213043003/diff/60001/components/password_man...
components/password_manager/core/common/password_manager_pref_names.h:29: #endif
On 2015/07/07 02:48:22, Ilya Sherman (Away July 8-13) wrote:
> nit: These are not pref names.  Please declare them in a different header
file.

Done.

https://codereview.chromium.org/1213043003/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1213043003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:27833: +  <summary>The status of
passwords migration from the Keychain.</summary>
On 2015/07/07 02:48:22, Ilya Sherman (Away July 8-13) wrote:
> Please document when this is recorded.

Done.

https://codereview.chromium.org/1213043003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:58785: +  <int value="3"
label="Migration failed twice."/>
On 2015/07/07 02:49:04, Ilya Sherman (Away July 8-13) wrote:
> Optional nit: I'd omit the trailing periods, since these are shown as the
> left-hand column labels of a table.

Done.

[stuartmorgan, 2015-07-07 17:40:26]

https://codereview.chromium.org/1213043003/diff/80001/chrome/browser/password...
File chrome/browser/password_manager/password_store_proxy_mac.cc (right):

https://codereview.chromium.org/1213043003/diff/80001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac.cc:110:
password_store_simple_ = new SimplePasswordStoreMac(
Just to be safe (since the logic that creates this conditionally is elsewhere in
the file), maybe DCHECK(!password_store_simple) just before this.

https://codereview.chromium.org/1213043003/diff/80001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac.cc:119:
base::Bind(&PasswordStoreProxyMac::UpdateStatusPref, this, status));
Given that there's a race where the login DB could be updated and persistent
without the pref having been stored, perhaps the migration logic should be
updated to skip any entries in the DB that already have a non-empty password?

Although I can see the argument that it's so rare that just doing the work a
second time in that case isn't a big deal.

https://codereview.chromium.org/1213043003/diff/80001/chrome/browser/password...
File chrome/browser/password_manager/password_store_proxy_mac_unittest.cc
(right):

https://codereview.chromium.org/1213043003/diff/80001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac_unittest.cc:41:
PasswordStoreChangeList AddChangeForForm(const PasswordForm& form) {
Needs a comment.

https://codereview.chromium.org/1213043003/diff/80001/chrome/browser/password...
File chrome/browser/password_manager/simple_password_store_mac.cc (right):

https://codereview.chromium.org/1213043003/diff/80001/chrome/browser/password...
chrome/browser/password_manager/simple_password_store_mac.cc:14:
this->login_db()->set_clear_password_values(false);
Is it useful to keep this, now that it's done in InitWithTaskRunner?

[Ilya Sherman, 2015-07-07 22:15:45]

Thanks. Histograms lgtm % a final nit:

https://codereview.chromium.org/1213043003/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1213043003/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:27958: +    startup.
nit: To clarify, perhaps phrase this as "shortly after startup" -- otherwise,
"after startup" describes pretty much everything that Chrome does ;)

[Garrett Casto, 2015-07-07 22:22:26]

lgtm

https://codereview.chromium.org/1213043003/diff/60001/chrome/browser/password...
File chrome/browser/password_manager/password_store_proxy_mac.cc (right):

https://codereview.chromium.org/1213043003/diff/60001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac.cc:62: thread_->Stop();
On 2015/07/07 15:45:54, vasilii wrote:
> On 2015/07/06 21:38:54, Garrett Casto wrote:
> > Stop should be called after GetBackend()->Shutdown(), or part of the
shutdown
> > for the backend will be skipped. At the moment it doesn't matter because
> > PasswordSyncableService associated with the backend should never be
accessed,
> > but it may cause problems in the future.
> 
> PasswordSyncableService isn't even instantiated because I don't call Init().It
> should be fine in the future too because I call only private implementation
> methods. |observers_|, |syncable_service_|, |affiliated_match_helper_| should
> all be empty.
> I can't call GetBackend()->Shutdown() earlier because GetBackend() can change
on
> the background. Using WaitableEvent on the UI thread isn't allowed.

Can you then add a comment before GetBackend()->Shutdown() explaining why it's
okay that Shutdown() won't have background_task_runner_.

[vasilii, 2015-07-08 09:56:59]

https://codereview.chromium.org/1213043003/diff/60001/chrome/browser/password...
File chrome/browser/password_manager/password_store_proxy_mac.cc (right):

https://codereview.chromium.org/1213043003/diff/60001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac.cc:62: thread_->Stop();
On 2015/07/07 22:22:26, Garrett Casto wrote:
> On 2015/07/07 15:45:54, vasilii wrote:
> > On 2015/07/06 21:38:54, Garrett Casto wrote:
> > > Stop should be called after GetBackend()->Shutdown(), or part of the
> shutdown
> > > for the backend will be skipped. At the moment it doesn't matter because
> > > PasswordSyncableService associated with the backend should never be
> accessed,
> > > but it may cause problems in the future.
> > 
> > PasswordSyncableService isn't even instantiated because I don't call
Init().It
> > should be fine in the future too because I call only private implementation
> > methods. |observers_|, |syncable_service_|, |affiliated_match_helper_|
should
> > all be empty.
> > I can't call GetBackend()->Shutdown() earlier because GetBackend() can
change
> on
> > the background. Using WaitableEvent on the UI thread isn't allowed.
> 
> Can you then add a comment before GetBackend()->Shutdown() explaining why it's
> okay that Shutdown() won't have background_task_runner_.

Done.

https://codereview.chromium.org/1213043003/diff/80001/chrome/browser/password...
File chrome/browser/password_manager/password_store_proxy_mac.cc (right):

https://codereview.chromium.org/1213043003/diff/80001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac.cc:110:
password_store_simple_ = new SimplePasswordStoreMac(
On 2015/07/07 17:40:25, stuartmorgan wrote:
> Just to be safe (since the logic that creates this conditionally is elsewhere
in
> the file), maybe DCHECK(!password_store_simple) just before this.

Done.

https://codereview.chromium.org/1213043003/diff/80001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac.cc:119:
base::Bind(&PasswordStoreProxyMac::UpdateStatusPref, this, status));
On 2015/07/07 17:40:25, stuartmorgan wrote:
> Given that there's a race where the login DB could be updated and persistent
> without the pref having been stored, perhaps the migration logic should be
> updated to skip any entries in the DB that already have a non-empty password?
> 
> Although I can see the argument that it's so rare that just doing the work a
> second time in that case isn't a big deal.

There is no race. The preference is written in PasswordStoreProxyMac::Shutdown()
at latest. It's tested in PasswordStoreProxyMacTest.StartAndStop

https://codereview.chromium.org/1213043003/diff/80001/chrome/browser/password...
File chrome/browser/password_manager/password_store_proxy_mac_unittest.cc
(right):

https://codereview.chromium.org/1213043003/diff/80001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac_unittest.cc:41:
PasswordStoreChangeList AddChangeForForm(const PasswordForm& form) {
On 2015/07/07 17:40:25, stuartmorgan wrote:
> Needs a comment.

Done.

https://codereview.chromium.org/1213043003/diff/80001/chrome/browser/password...
File chrome/browser/password_manager/simple_password_store_mac.cc (right):

https://codereview.chromium.org/1213043003/diff/80001/chrome/browser/password...
chrome/browser/password_manager/simple_password_store_mac.cc:14:
this->login_db()->set_clear_password_values(false);
On 2015/07/07 17:40:25, stuartmorgan wrote:
> Is it useful to keep this, now that it's done in InitWithTaskRunner?

Yes, because InitWithTaskRunner isn't necessary to call if the constructor
received all the valid parameters.

https://codereview.chromium.org/1213043003/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1213043003/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:27958: +    startup.
On 2015/07/07 22:15:45, Ilya Sherman (Away July 8-13) wrote:
> nit: To clarify, perhaps phrase this as "shortly after startup" -- otherwise,
> "after startup" describes pretty much everything that Chrome does ;)

Done.

[vasilii, 2015-07-08 11:18:45]

Patchset #6 (id:100001) has been deleted

[stuartmorgan, 2015-07-08 14:18:40]

LGTM with nit

https://codereview.chromium.org/1213043003/diff/80001/chrome/browser/password...
File chrome/browser/password_manager/password_store_proxy_mac.cc (right):

https://codereview.chromium.org/1213043003/diff/80001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac.cc:119:
base::Bind(&PasswordStoreProxyMac::UpdateStatusPref, this, status));
On 2015/07/08 09:56:58, vasilii wrote:
> On 2015/07/07 17:40:25, stuartmorgan wrote:
> > Given that there's a race where the login DB could be updated and persistent
> > without the pref having been stored, perhaps the migration logic should be
> > updated to skip any entries in the DB that already have a non-empty
password?
> > 
> > Although I can see the argument that it's so rare that just doing the work a
> > second time in that case isn't a big deal.
> 
> There is no race. The preference is written in
PasswordStoreProxyMac::Shutdown()
> at latest.

I meant in case of a crash; there's a window of time where a crash could happen
after the DB has been written to disk, but before the pref.

https://codereview.chromium.org/1213043003/diff/80001/chrome/browser/password...
File chrome/browser/password_manager/simple_password_store_mac.cc (right):

https://codereview.chromium.org/1213043003/diff/80001/chrome/browser/password...
chrome/browser/password_manager/simple_password_store_mac.cc:14:
this->login_db()->set_clear_password_values(false);
On 2015/07/08 09:56:58, vasilii wrote:
> On 2015/07/07 17:40:25, stuartmorgan wrote:
> > Is it useful to keep this, now that it's done in InitWithTaskRunner?
> 
> Yes, because InitWithTaskRunner isn't necessary to call if the constructor
> received all the valid parameters.

Please clarify that in the header comment for IWTR then; I don't normally expect
an Init method on a class to be optional.

[vasilii, 2015-07-08 15:45:55]

https://codereview.chromium.org/1213043003/diff/80001/chrome/browser/password...
File chrome/browser/password_manager/password_store_proxy_mac.cc (right):

https://codereview.chromium.org/1213043003/diff/80001/chrome/browser/password...
chrome/browser/password_manager/password_store_proxy_mac.cc:119:
base::Bind(&PasswordStoreProxyMac::UpdateStatusPref, this, status));
On 2015/07/08 14:18:40, stuartmorgan wrote:
> On 2015/07/08 09:56:58, vasilii wrote:
> > On 2015/07/07 17:40:25, stuartmorgan wrote:
> > > Given that there's a race where the login DB could be updated and
persistent
> > > without the pref having been stored, perhaps the migration logic should be
> > > updated to skip any entries in the DB that already have a non-empty
> password?
> > > 
> > > Although I can see the argument that it's so rare that just doing the work
a
> > > second time in that case isn't a big deal.
> > 
> > There is no race. The preference is written in
> PasswordStoreProxyMac::Shutdown()
> > at latest.
> 
> I meant in case of a crash; there's a window of time where a crash could
happen
> after the DB has been written to disk, but before the pref.

I think it's rare and on the next run the passwords still should be accessible.
This optimisation makes possible another bug when the encryption key has changed
for some reason.

https://codereview.chromium.org/1213043003/diff/80001/chrome/browser/password...
File chrome/browser/password_manager/simple_password_store_mac.cc (right):

https://codereview.chromium.org/1213043003/diff/80001/chrome/browser/password...
chrome/browser/password_manager/simple_password_store_mac.cc:14:
this->login_db()->set_clear_password_values(false);
On 2015/07/08 14:18:40, stuartmorgan wrote:
> On 2015/07/08 09:56:58, vasilii wrote:
> > On 2015/07/07 17:40:25, stuartmorgan wrote:
> > > Is it useful to keep this, now that it's done in InitWithTaskRunner?
> > 
> > Yes, because InitWithTaskRunner isn't necessary to call if the constructor
> > received all the valid parameters.
> 
> Please clarify that in the header comment for IWTR then; I don't normally
expect
> an Init method on a class to be optional.

Done.

[vasilii, 2015-07-08 15:46:01]

The CQ bit was checked by vasilii@chromium.org

[vasilii, 2015-07-08 15:46:01]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
gcasto@chromium.org, isherman@chromium.org, stuartmorgan@chromium.org
Link to the patchset: https://codereview.chromium.org/1213043003/#ps140001
(title: "added a comment")

[commit-bot: I haz the power, 2015-07-08 15:46:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1213043003/140001

[commit-bot: I haz the power, 2015-07-08 16:54:48]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2015-07-08 16:55:44]

Patchset 7 (id:??) landed as
https://crrev.com/b312da5608ac316b7e49459c906b5a5d72332776
Cr-Commit-Position: refs/heads/master@{#337842}