Implement origin-based deletion for passwords in PasswordDefaultMac.

chrome/browser/password_manager/password_store_mac_unittest.cc

Index: chrome/browser/password_manager/password_store_mac_unittest.cc
diff --git a/chrome/browser/password_manager/password_store_mac_unittest.cc b/chrome/browser/password_manager/password_store_mac_unittest.cc
index 36cc5073332854000851a375b4e420e0ff552398..aa8c62d121964c94b972370146405d467eb35d15 100644
--- a/chrome/browser/password_manager/password_store_mac_unittest.cc
+++ b/chrome/browser/password_manager/password_store_mac_unittest.cc
@@ -18,6 +18,7 @@
 #include "components/os_crypt/os_crypt.h"
 #include "components/password_manager/core/browser/login_database.h"
 #include "components/password_manager/core/browser/password_manager_test_utils.h"
+#include "components/password_manager/core/browser/password_store_common_unittest.h"
 #include "components/password_manager/core/browser/password_store_consumer.h"
 #include "content/public/test/test_browser_thread.h"
 #include "content/public/test/test_utils.h"
@@ -74,12 +75,6 @@ class MockPasswordStoreConsumer : public PasswordStoreConsumer {
   }
 };
 
-class MockPasswordStoreObserver : public PasswordStore::Observer {
- public:
-  MOCK_METHOD1(OnLoginsChanged,
-               void(const password_manager::PasswordStoreChangeList& changes));
-};
-
 // A LoginDatabase that simulates an Init() method that takes a long time.
 class SlowToInitLoginDatabase : public password_manager::LoginDatabase {
  public:
@@ -174,6 +169,96 @@ PasswordStoreChangeList AddChangeForForm(const PasswordForm& form) {
 
 }  // namespace
 
+namespace password_manager {

[vasilii, 2015/10/21 16:47:56]

You don't need this namespace. Use the anonymous one.

[Timo Reimann, 2015/11/17 23:10:55]

Done.

+
+class PasswordStoreMacTestDelegate {
+ public:
+  PasswordStoreMacTestDelegate()
+      : ui_thread_(BrowserThread::UI, &message_loop_) {
+    initialize();

[vasilii, 2015/10/21 16:47:56]

Method's name should start with a capital letter.

[Timo Reimann, 2015/11/17 23:10:55]

Done.

+  }
+  ~PasswordStoreMacTestDelegate() {
+    ClosePasswordStore();
+    thread_.reset();
+    login_db_.reset();
+  }
+
+  PasswordStoreMac* store() { return store_.get(); }
+
+  void FinishAsyncProcessing() {
+    scoped_refptr<content::MessageLoopRunner> runner =
+        new content::MessageLoopRunner;
+    ASSERT_TRUE(thread_->task_runner()->PostTaskAndReply(
+        FROM_HERE, base::Bind(&Noop), runner->QuitClosure()));
+    runner->Run();
+  }
+
+  base::Thread* thread() { return thread_.get(); }

[vasilii, 2015/10/21 16:47:56]

Why is it needed?

[Timo Reimann, 2015/11/17 23:10:55]

PasswordStoreMacTest.ImportFromKeychain,
PasswordStoreMacTest.ImportFederatedFromLockedKeychain, and
PasswordStoreMacTest.ImportFromLockedKeychainError all post
PasswordStoreMac::ImportFromKeychain to the thread's task runner during the
respective tests.

[vasilii, 2015/11/18 16:27:42]

You can expose a task runner, not a thread.

[Timo Reimann, 2015/11/18 23:42:14]

I use ThreadTaskRunnerHandle now. Hope that's the right approach.

+
+ private:
+  base::MessageLoopForUI message_loop_;

[vasilii, 2015/10/21 16:47:56]

Methods should be above. They are complex enough so don't make them inline.

[Timo Reimann, 2015/11/17 23:10:55]

Done (for PasswordStoreDefaultTest too).

+  content::TestBrowserThread ui_thread_;
+  // Thread that the synchronous methods are run on.
+  scoped_ptr<base::Thread> thread_;
+
+  base::ScopedTempDir db_dir_;
+  scoped_ptr<LoginDatabase> login_db_;
+  scoped_refptr<PasswordStoreMac> store_;
+
+  static void InitLoginDatabase(LoginDatabase* login_db) {
+    ASSERT_TRUE(login_db->Init());
+  }
+
+  void initialize() {

[Timo Reimann, 2015/10/20 20:47:09]

The initialization of PasswordStoreMac for testing purposes seems more
complicated that the equivalent for PasswordStoreDefault. For example, the Mac
version manages at least one more thread.

I am wondering if we can (and should) try to align the two approaches, either by
making one more sophisticated (and thus supposedly more correct) or streamlining
the other (and thus remove supposedly extra complexity).

[vasilii, 2015/10/21 16:47:56]

For your tests you can definitely simplify PasswordStoreMac creation by using
just one thread as for PasswordStoreDefault.
However, it should not change PasswordStoreMacTest logic. So choose one
approach.

+    ASSERT_TRUE(db_dir_.CreateUniqueTempDir());
+
+    // Ensure that LoginDatabase will use the mock keychain if it needs to
+    // encrypt/decrypt a password.
+    OSCrypt::UseMockKeychain(true);

[vasilii, 2015/10/21 16:47:56]

You just pass the Keychain instance into constructor.

[Timo Reimann, 2015/11/17 23:10:55]

Which constructor do you mean? The test already passes an instance of
MockAppleKeychain into PasswordStoreMac's constructor. However, that does not
seem to suffice: Commenting out the call to OSCrypt::UseMockKeychain(true) leads
to failing tests.

+    login_db_.reset(new LoginDatabase(test_login_db_file_path()));
+    thread_.reset(new base::Thread("Chrome_PasswordStore_Thread"));
+    ASSERT_TRUE(thread_->Start());
+    ASSERT_TRUE(thread_->task_runner()->PostTask(
+        FROM_HERE, base::Bind(&PasswordStoreMacTestDelegate::InitLoginDatabase,
+                              base::Unretained(login_db_.get()))));
+    CreateAndInitPasswordStore(login_db_.get());
+    // Make sure deferred initialization is performed before some tests start
+    // accessing the |login_db| directly.
+    FinishAsyncProcessing();
+  }
+
+  base::FilePath test_login_db_file_path() const {
+    return db_dir_.path().Append(FILE_PATH_LITERAL("login.db"));
+  }
+
+  void CreateAndInitPasswordStore(LoginDatabase* login_db) {

[vasilii, 2015/10/21 16:47:56]

merge the method into initialize()

[Timo Reimann, 2015/11/17 23:10:55]

Done.

+    store_ = new PasswordStoreMac(
+        base::ThreadTaskRunnerHandle::Get(), nullptr,
+        make_scoped_ptr<AppleKeychain>(new MockAppleKeychain));
+    ASSERT_TRUE(thread_->task_runner()->PostTask(
+        FROM_HERE, base::Bind(&PasswordStoreMac::InitWithTaskRunner, store_,
+                              thread_->task_runner())));
+
+    ASSERT_TRUE(thread_->task_runner()->PostTask(
+        FROM_HERE, base::Bind(&PasswordStoreMac::set_login_metadata_db, store_,
+                              base::Unretained(login_db))));
+  }
+
+  void ClosePasswordStore() {
+    if (!store_)

[vasilii, 2015/10/21 16:47:56]

It can't be true.

[Timo Reimann, 2015/11/17 23:10:55]

I agree, unless there's a bug where store is attempted to be deleted twice.

Should we completely drop the if-clause or replace it by a DCHECK, just to be
safe and catch potential bugs early on?

[vasilii, 2015/11/18 16:27:42]

Just drop.

[Timo Reimann, 2015/11/18 23:42:14]

Done.

+      return;
+
+    store_->Shutdown();
+    store_ = nullptr;
+  }
+};
+
+INSTANTIATE_TYPED_TEST_CASE_P(Mac,
+                              PasswordStoreCommonTest,
+                              PasswordStoreMacTestDelegate);
+
+}  // namespace password_manager
+
 #pragma mark -
 
 class PasswordStoreMacInternalsTest : public testing::Test {
@@ -1173,32 +1258,11 @@ TEST_F(PasswordStoreMacInternalsTest, TestPasswordGetAll) {
 
 class PasswordStoreMacTest : public testing::Test {
  public:
-  PasswordStoreMacTest() : ui_thread_(BrowserThread::UI, &message_loop_) {}
-
-  void SetUp() override {
-    ASSERT_TRUE(db_dir_.CreateUniqueTempDir());
-    histogram_tester_.reset(new base::HistogramTester);
+  PasswordStoreMacTest()
+      : histogram_tester_(new base::HistogramTester),
+        store_(delegate_.store()) {}

[vasilii, 2015/10/21 16:47:56]

I'm generally against changing this class. Instead you can simplify the delegate
code a lot.

[Timo Reimann, 2015/11/17 23:10:55]

I'm not sure I understand: The delegate was created by moving parts of
PasswordStoreMacTest into it in order to provide a self-contained class to the
new type-parameterized test template. So it seems we need to do at least a
certain amount of modifications to PasswordStoreMacTest.

Could you please elaborate on how the delegate should be simplified from your
perspective?

 
-    // Ensure that LoginDatabase will use the mock keychain if it needs to
-    // encrypt/decrypt a password.
-    OSCrypt::UseMockKeychain(true);
-    login_db_.reset(
-        new password_manager::LoginDatabase(test_login_db_file_path()));
-    thread_.reset(new base::Thread("Chrome_PasswordStore_Thread"));
-    ASSERT_TRUE(thread_->Start());
-    ASSERT_TRUE(thread_->task_runner()->PostTask(
-        FROM_HERE, base::Bind(&PasswordStoreMacTest::InitLoginDatabase,
-                              base::Unretained(login_db_.get()))));
-    CreateAndInitPasswordStore(login_db_.get());
-    // Make sure deferred initialization is performed before some tests start
-    // accessing the |login_db| directly.
-    FinishAsyncProcessing();
-  }
-
-  void TearDown() override {
-    ClosePasswordStore();
-    thread_.reset();
-    login_db_.reset();
+  ~PasswordStoreMacTest() override {
     // Whatever a test did, PasswordStoreMac stores only empty password values
     // in LoginDatabase. The empty valus do not require encryption and therefore
     // OSCrypt shouldn't call the Keychain. The histogram doesn't cover the
@@ -1208,31 +1272,6 @@ class PasswordStoreMacTest : public testing::Test {
     }
   }
 
-  static void InitLoginDatabase(password_manager::LoginDatabase* login_db) {
-    ASSERT_TRUE(login_db->Init());
-  }
-
-  void CreateAndInitPasswordStore(password_manager::LoginDatabase* login_db) {
-    store_ = new PasswordStoreMac(
-        base::ThreadTaskRunnerHandle::Get(), nullptr,
-        make_scoped_ptr<AppleKeychain>(new MockAppleKeychain));
-    ASSERT_TRUE(thread_->task_runner()->PostTask(
-        FROM_HERE, base::Bind(&PasswordStoreMac::InitWithTaskRunner, store_,
-                              thread_->task_runner())));
-
-    ASSERT_TRUE(thread_->task_runner()->PostTask(
-        FROM_HERE, base::Bind(&PasswordStoreMac::set_login_metadata_db, store_,
-                              base::Unretained(login_db))));
-  }
-
-  void ClosePasswordStore() {
-    if (!store_)
-      return;
-
-    store_->Shutdown();
-    store_ = nullptr;
-  }
-
   // Verifies that the given |form| can be properly stored so that it can be
   // retrieved by FillMatchingLogins() and GetAutofillableLogins(), and then it
   // can be properly removed.
@@ -1250,7 +1289,7 @@ class PasswordStoreMacTest : public testing::Test {
       ::testing::Mock::VerifyAndClearExpectations(&mock_consumer);
 
       store()->AddLogin(form);
-      FinishAsyncProcessing();
+      delegate_.FinishAsyncProcessing();
 
       PasswordForm returned_form;
       EXPECT_CALL(mock_consumer, OnGetPasswordStoreResultsConstRef(SizeIs(1u)))
@@ -1283,10 +1322,6 @@ class PasswordStoreMacTest : public testing::Test {
     }
   }
 
-  base::FilePath test_login_db_file_path() const {
-    return db_dir_.path().Append(FILE_PATH_LITERAL("login.db"));
-  }
-
   password_manager::LoginDatabase* login_db() const {
     return store_->login_metadata_db();
   }
@@ -1295,26 +1330,16 @@ class PasswordStoreMacTest : public testing::Test {
     return static_cast<MockAppleKeychain*>(store_->keychain());
   }
 
-  void FinishAsyncProcessing() {
-    scoped_refptr<content::MessageLoopRunner> runner =
-        new content::MessageLoopRunner;
-    ASSERT_TRUE(thread_->task_runner()->PostTaskAndReply(
-        FROM_HERE, base::Bind(&Noop), runner->QuitClosure()));
-    runner->Run();
-  }
+  PasswordStoreMac* store() { return store_; }
 
-  PasswordStoreMac* store() { return store_.get(); }
+  password_manager::PasswordStoreMacTestDelegate& delegate() {
+    return delegate_;
+  }
 
  protected:
-  base::MessageLoopForUI message_loop_;
-  content::TestBrowserThread ui_thread_;
-  // Thread that the synchronous methods are run on.
-  scoped_ptr<base::Thread> thread_;
-
-  base::ScopedTempDir db_dir_;
-  scoped_ptr<password_manager::LoginDatabase> login_db_;
-  scoped_refptr<PasswordStoreMac> store_;
   scoped_ptr<base::HistogramTester> histogram_tester_;
+  password_manager::PasswordStoreMacTestDelegate delegate_;
+  PasswordStoreMac* store_;
 };
 
 TEST_F(PasswordStoreMacTest, TestStoreUpdate) {
@@ -1383,7 +1408,7 @@ TEST_F(PasswordStoreMacTest, TestStoreUpdate) {
     store_->UpdateLogin(*form);
   }
 
-  FinishAsyncProcessing();
+  delegate_.FinishAsyncProcessing();
 
   MacKeychainPasswordFormAdapter keychain_adapter(keychain());
   for (unsigned int i = 0; i < arraysize(updates); ++i) {
@@ -1460,7 +1485,7 @@ TEST_F(PasswordStoreMacTest, TestDBKeychainAssociation) {
   // 4. Remove both passwords.
   store_->RemoveLogin(*www_form);
   store_->RemoveLogin(m_form);
-  FinishAsyncProcessing();
+  delegate_.FinishAsyncProcessing();
 
   // No trace of www.facebook.com.
   ScopedVector<autofill::PasswordForm> matching_items =
@@ -1487,7 +1512,7 @@ public:
   }
 
   void WaitAndVerify(PasswordStoreMacTest* test) {
-    test->FinishAsyncProcessing();
+    test->delegate().FinishAsyncProcessing();
     ::testing::Mock::VerifyAndClearExpectations(this);
   }
 
@@ -1643,7 +1668,7 @@ TEST_F(PasswordStoreMacTest, TestRemoveLoginsMultiProfile) {
       L"submit", L"not_joe_user", L"12345", true, false, 1 };
   www_form = CreatePasswordFormFromDataForTesting(www_form_data2);
   store_->AddLogin(*www_form);
-  FinishAsyncProcessing();
+  delegate_.FinishAsyncProcessing();
 
   ScopedVector<PasswordForm> matching_items;
   EXPECT_TRUE(login_db()->GetLogins(*www_form, &matching_items));
@@ -1651,7 +1676,7 @@ TEST_F(PasswordStoreMacTest, TestRemoveLoginsMultiProfile) {
 
   store_->RemoveLoginsCreatedBetween(base::Time(), base::Time(),
                                      base::Closure());
-  FinishAsyncProcessing();
+  delegate_.FinishAsyncProcessing();
 
   // Check the second facebook form is gone.
   EXPECT_TRUE(login_db()->GetLogins(*www_form, &matching_items));
@@ -1674,7 +1699,8 @@ TEST_F(PasswordStoreMacTest, TestRemoveLoginsMultiProfile) {
 // implicitly deleted. However, the observers shouldn't get notified about
 // deletion of non-existent forms like m.facebook.com.
 TEST_F(PasswordStoreMacTest, SilentlyRemoveOrphanedForm) {
-  testing::StrictMock<MockPasswordStoreObserver> mock_observer;
+  testing::StrictMock<password_manager::MockPasswordStoreObserver>
+      mock_observer;
   store()->AddObserver(&mock_observer);
 
   // 1. Add a password for www.facebook.com to the LoginDatabase.
@@ -1770,13 +1796,13 @@ TEST_F(PasswordStoreMacTest, ImportFromKeychain) {
   store()->AddLogin(form1);
   store()->AddLogin(form2);
   store()->AddLogin(blacklisted_form);
-  FinishAsyncProcessing();
+  delegate_.FinishAsyncProcessing();
 
   ASSERT_TRUE(base::PostTaskAndReplyWithResult(
-      thread_->task_runner().get(), FROM_HERE,
+      delegate_.thread()->task_runner().get(), FROM_HERE,
       base::Bind(&PasswordStoreMac::ImportFromKeychain, store()),
       base::Bind(&CheckMigrationResult, PasswordStoreMac::MIGRATION_OK)));
-  FinishAsyncProcessing();
+  delegate_.FinishAsyncProcessing();
 
   // The password should be stored in the database by now.
   ScopedVector<PasswordForm> matching_items;
@@ -1808,12 +1834,12 @@ TEST_F(PasswordStoreMacTest, ImportFederatedFromLockedKeychain) {
   form1.federation_url = GURL("https://accounts.google.com/");
 
   store()->AddLogin(form1);
-  FinishAsyncProcessing();
+  delegate_.FinishAsyncProcessing();
   ASSERT_TRUE(base::PostTaskAndReplyWithResult(
-      thread_->task_runner().get(), FROM_HERE,
+      delegate_.thread()->task_runner().get(), FROM_HERE,
       base::Bind(&PasswordStoreMac::ImportFromKeychain, store()),
       base::Bind(&CheckMigrationResult, PasswordStoreMac::MIGRATION_OK)));
-  FinishAsyncProcessing();
+  delegate_.FinishAsyncProcessing();
 
   ScopedVector<PasswordForm> matching_items;
   EXPECT_TRUE(login_db()->GetLogins(form1, &matching_items));
@@ -1830,7 +1856,7 @@ TEST_F(PasswordStoreMacTest, ImportFromLockedKeychainError) {
   form1.username_value = ASCIIToUTF16("my_username");
   form1.password_value = ASCIIToUTF16("my_password");
   store()->AddLogin(form1);
-  FinishAsyncProcessing();
+  delegate_.FinishAsyncProcessing();
 
   // Add a second keychain item matching the Database entry.
   PasswordForm form2 = form1;
@@ -1841,10 +1867,10 @@ TEST_F(PasswordStoreMacTest, ImportFromLockedKeychainError) {
 
   keychain()->set_locked(true);
   ASSERT_TRUE(base::PostTaskAndReplyWithResult(
-      thread_->task_runner().get(), FROM_HERE,
+      delegate_.thread()->task_runner().get(), FROM_HERE,
       base::Bind(&PasswordStoreMac::ImportFromKeychain, store()),
       base::Bind(&CheckMigrationResult, PasswordStoreMac::KEYCHAIN_BLOCKED)));
-  FinishAsyncProcessing();
+  delegate_.FinishAsyncProcessing();
 
   ScopedVector<PasswordForm> matching_items;
   EXPECT_TRUE(login_db()->GetLogins(form1, &matching_items));