Linux: delete the unencrypted login database file after a successful password migration.

Linux: delete the unencrypted login database file after a successful password store migration, since otherwise fragments of password data may be left in it.
BUG=25404
TEST=the "Login Data" file should get deleted and recreated from scratch after migration

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=51094

[Evan Martin, 2010-06-25 02:31:57]

http://codereview.chromium.org/2866023/diff/1/2
File app/sql/meta_table.cc (right):

http://codereview.chromium.org/2866023/diff/1/2#newcode49
app/sql/meta_table.cc:49: db_ = NULL;
don't you need to free this?  (db_ isn't a smart poitner)

http://codereview.chromium.org/2866023/diff/1/5
File chrome/browser/password_manager/login_database.h (right):

http://codereview.chromium.org/2866023/diff/1/5#newcode72
chrome/browser/password_manager/login_database.h:72: // Deletes the login
database file on disk, and creates a new, empty database.
Why create the new one?

[Mike Mammarella, 2010-06-25 03:59:12]

http://codereview.chromium.org/2866023/diff/1/2
File app/sql/meta_table.cc (right):

http://codereview.chromium.org/2866023/diff/1/2#newcode49
app/sql/meta_table.cc:49: db_ = NULL;
On 2010/06/25 02:31:57, Evan Martin wrote:
> don't you need to free this?  (db_ isn't a smart poitner)

No, MetaTable doesn't own it. It's actually a member of LoginDatabase; freeing
it would be quite bad.

http://codereview.chromium.org/2866023/diff/1/5
File chrome/browser/password_manager/login_database.h (right):

http://codereview.chromium.org/2866023/diff/1/5#newcode72
chrome/browser/password_manager/login_database.h:72: // Deletes the login
database file on disk, and creates a new, empty database.
On 2010/06/25 02:31:57, Evan Martin wrote:
> Why create the new one?

Just in case the LoginDatabase gets used again somehow, mostly. It shouldn't,
although in fact it is in unit tests (but as far as I know, only in unit tests).
This API just seemed "safer."

[Paweł Hajdan Jr., 2010-06-25 06:36:09]

Drive-by with a test question. Please let me take another look before you
commit.

http://codereview.chromium.org/2866023/diff/1/8
File chrome/browser/password_manager/password_store_x_unittest.cc (right):

http://codereview.chromium.org/2866023/diff/1/8#newcode295
chrome/browser/password_manager/password_store_x_unittest.cc:295: if
(temp_dir_.IsValid())
This is weird. Did you hit a case where temp_dir_ was valid at that point? It
shouldn't be. I wonder that maybe we should fix ScopedTempDir. Or does gtest
instantiate TestWithParam in some other way?

[Mike Mammarella, 2010-06-25 16:45:19]

http://codereview.chromium.org/2866023/diff/1/8
File chrome/browser/password_manager/password_store_x_unittest.cc (right):

http://codereview.chromium.org/2866023/diff/1/8#newcode295
chrome/browser/password_manager/password_store_x_unittest.cc:295: if
(temp_dir_.IsValid())
On 2010/06/25 06:36:09, Paweł Hajdan Jr. wrote:
> This is weird. Did you hit a case where temp_dir_ was valid at that point? It
> shouldn't be. I wonder that maybe we should fix ScopedTempDir. Or does gtest
> instantiate TestWithParam in some other way?

It's my understanding that SetUp() gets called once per test, so since the temp
dir is scoped to the fixture, it will get reinitialized here. Unless the fixture
itself is instantiated once per test? (I haven't actually checked that this is
necessary, but it looked like it was a problem to me.)

[Paweł Hajdan Jr., 2010-06-25 16:51:13]

http://codereview.chromium.org/2866023/diff/1/8
File chrome/browser/password_manager/password_store_x_unittest.cc (right):

http://codereview.chromium.org/2866023/diff/1/8#newcode295
chrome/browser/password_manager/password_store_x_unittest.cc:295: if
(temp_dir_.IsValid())
On 2010/06/25 16:45:19, Mike Mammarella wrote:
> It's my understanding that SetUp() gets called once per test, so since the
temp
> dir is scoped to the fixture, it will get reinitialized here. Unless the
fixture
> itself is instantiated once per test? (I haven't actually checked that this is
> necessary, but it looked like it was a problem to me.)

The whole test fixture is torn down and recreated for each test. So each test
does these things, in order:

1) Create a fresh instance of the test fixture.
2) Call SetUp.
3) Execute test body.
4) Call TearDown.
5) Destroy the test fixture instance.

I think you can safely delete the suspicious part of the test code.

[Mike Mammarella, 2010-06-25 18:40:45]

http://codereview.chromium.org/2866023/diff/1/8
File chrome/browser/password_manager/password_store_x_unittest.cc (right):

http://codereview.chromium.org/2866023/diff/1/8#newcode295
chrome/browser/password_manager/password_store_x_unittest.cc:295: if
(temp_dir_.IsValid())
On 2010/06/25 16:51:13, Paweł Hajdan Jr. wrote:
> On 2010/06/25 16:45:19, Mike Mammarella wrote:
> > It's my understanding that SetUp() gets called once per test, so since the
> temp
> > dir is scoped to the fixture, it will get reinitialized here. Unless the
> fixture
> > itself is instantiated once per test? (I haven't actually checked that this
is
> > necessary, but it looked like it was a problem to me.)
> 
> The whole test fixture is torn down and recreated for each test. So each test
> does these things, in order:
> 
> 1) Create a fresh instance of the test fixture.
> 2) Call SetUp.
> 3) Execute test body.
> 4) Call TearDown.
> 5) Destroy the test fixture instance.
> 
> I think you can safely delete the suspicious part of the test code.

Done.

[Paweł Hajdan Jr., 2010-06-25 19:35:03]

Code I commented in the drive-by LGTM.

[Mike Mammarella, 2010-06-25 22:15:30]

Evan: ping

[Evan Martin, 2010-06-28 23:18:37]

LGTM with one request

http://codereview.chromium.org/2866023/diff/14001/15004
File chrome/browser/password_manager/login_database.h (right):

http://codereview.chromium.org/2866023/diff/14001/15004#newcode72
chrome/browser/password_manager/login_database.h:72: // Deletes the login
database file on disk, and creates a new, empty database.
Can you rename the function to reflect the latter behavior as well?  (E.g.
"recreate" maybe.  I still think it's weird, but I understand your reasoning.)

http://codereview.chromium.org/2866023/diff/14001/15005
File chrome/browser/password_manager/password_store_default.h (right):

http://codereview.chromium.org/2866023/diff/14001/15005#newcode54
chrome/browser/password_manager/password_store_default.h:54: // Calls
DeleteDatabaseFile() on |login_db_|.
This comment is superfluous.

[Mike Mammarella, 2010-06-29 00:31:57]

http://codereview.chromium.org/2866023/diff/14001/15004
File chrome/browser/password_manager/login_database.h (right):

http://codereview.chromium.org/2866023/diff/14001/15004#newcode72
chrome/browser/password_manager/login_database.h:72: // Deletes the login
database file on disk, and creates a new, empty database.
On 2010/06/28 23:18:37, Evan Martin wrote:
> Can you rename the function to reflect the latter behavior as well?  (E.g.
> "recreate" maybe.  I still think it's weird, but I understand your reasoning.)

Done.

http://codereview.chromium.org/2866023/diff/14001/15005
File chrome/browser/password_manager/password_store_default.h (right):

http://codereview.chromium.org/2866023/diff/14001/15005#newcode54
chrome/browser/password_manager/password_store_default.h:54: // Calls
DeleteDatabaseFile() on |login_db_|.
On 2010/06/28 23:18:37, Evan Martin wrote:
> This comment is superfluous.

Done.