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..8dda7e03fde7b0dd5f1f82e58bef30b2ba02feb0 100644
--- a/chrome/browser/password_manager/password_store_mac_unittest.cc
+++ b/chrome/browser/password_manager/password_store_mac_unittest.cc
@@ -19,6 +19,7 @@
 #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_consumer.h"
+#include "components/password_manager/core/browser/password_store_origin_unittest.h"
 #include "content/public/test/test_browser_thread.h"
 #include "content/public/test/test_utils.h"
 #include "crypto/mock_apple_keychain.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:
@@ -172,8 +167,112 @@ PasswordStoreChangeList AddChangeForForm(const PasswordForm& form) {
       1, PasswordStoreChange(PasswordStoreChange::ADD, form));
 }
 
+class PasswordStoreMacTestDelegate {
+ public:
+  PasswordStoreMacTestDelegate();
+  ~PasswordStoreMacTestDelegate();
+
+  PasswordStoreMac* store() { return store_.get(); }
+
+  base::Thread* thread() { return thread_.get(); }
+
+  void FinishAsyncProcessing();
+
+ private:
+  static void InitLoginDatabase(LoginDatabase* login_db);
+  void Initialize();
+  base::FilePath test_login_db_file_path() const;
+
+  void ClosePasswordStore();
+
+  base::MessageLoopForUI message_loop_;
+  content::TestBrowserThread ui_thread_;

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

It's deprecated. Use TestBrowserThreadBundle. It can also be used to get rid of
|thread_ and |message_loop_|

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

Okay, done. Apart from adding TestBrowserThreadBundle, I also replaced all
references to threads by ThreadTaskRunnerHandle. Please have a look and let me
know if the approach is correct.

[vasilii, 2015/11/23 15:04:56]

In the implementation you made the test single-threaded because you don't set up
a real background thread while constructing TestBrowserThreadBundle. As I
already said I'm fine with that if you don't touch PasswordStoreMacTest.
Otherwise, you should set up a second thread.
FYI, this class is deprecated and will be ultimately removed.

[Timo Reimann, 2015/11/24 02:36:09]

This part was and still is somewhat unclear to me: As I needed to refactor
PasswordStoreMacTest in order to break out PasswordStoreMacTestDelegate, it
seems impossible to keep the former completely unchanged. So as I understand, I
have no other option but to set up a second thread if I want to stick to the
delegate (which is a requirement for the type-parameterized tests in
PasswordStoreOriginTest).

What I did now was enable the IO_MAINLOOP option on the TestBrowserThreadBundle
instance and post all tasks to the IO thread. (I tried doing the same on the
REAL_DB_THREAD but that seg-faulted miserably.) This was my best guess on how to
use TestBrowserThreadBundle in order to create a separate thread. Please let me
know if this is the right way or not.

Alternatively, will the deprecation of this class simplify things somehow? My
understanding is that SimplePasswordStoreMac (which is implemented in terms of
PasswordStoreDefault) will take over in the future. Does this mean we could
possibly dump this CL altogether (assuming my feature won't need to ship before
PasswordStoreMac's deprecation)?

(In case the latter is true, please still let me know how to create a second
thread properly . I'd like to understand how things work.)

[vasilii, 2015/11/24 18:14:54]

You don't have to refactor PasswordStoreMacTest. You could just write the
delegate and instantiate your tests. That's it.
If you want to refactor then you need to create a TestBrowserThreadBundle with
REAL_FILE_THREAD (for example) flag. You can post to that thread
BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, callback); or you can
get a task runner GetMessageLoopProxyForThread(BrowserThread::FILE).
We don't know when the class will be deleted. It's not happening soon.  Thus we
need to test your feature. Ideally without putting a lot of efforts. That's why
I prefer to repeat basically what you did for PasswordStoreDefault.

[Timo Reimann, 2015/11/24 23:06:02]

Without any refactoring permitted, wouldn't that prevent usage of the Mac
delegate as a typed test in PasswordStoreOriginTest? As of now, all
parameterized test delegates need to adhere to a certain interface and
encapsulate the store life cycle. For instance, they must support a
FinishAsyncProcessing() method. I couldn't provide that for
PasswordStoreMacTestDelegate if I wasn't allowed to move (refactor) it from
PasswordStoreMacTest into the delegate.
I'm using REAL_FILE_THREAD now. I think it's not the worst choice after all as
we've made a few improvements to PasswordStoreMacTest along the way. These might
come handy even if the class will be removed at some remote point in the future.

[vasilii, 2015/11/25 18:18:39]

I don't understand your concern. You have PasswordStoreMacTest which works fine.
Then you wrote PasswordStoreOriginTest and PasswordStoreMacTestDelegate as a
delegate. Why do you feel like you have to change PasswordStoreMacTest? You
don't have to move, you can copy what you need.
I am suggesting the single-threaded PasswordStoreMacTestDelegate because of
simplicity. The current patch set contains quite a few mistakes related to
multi-threading.

[Timo Reimann, 2015/11/25 18:47:32]

I believe I'm starting to understand where you're heading: Create the delegate
and re-implement/copy whatever functionality is necessary to support store
management. I didn't consider this because it will create a bit of duplicated
code which I tried to avoid (such as the Initialize, Terminate, and
FinishAsyncProcessing methods). Is this what you mean?

[Timo Reimann, 2015/11/26 01:12:51]

I followed your advice and build a simplified PasswordStoreMacDelegate. Turned
out it contains much less duplicate code than I thought due to its
single-threaded nature -- apologies for not realizing this earlier. The
multi-threading-related problems you addressed in the other comments should all
be obsolete now.

The changes to PasswordStoreMacTest have been reverted (except for the minor
MockPasswordStoreObserver update). Let me know what you think.

+  // 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_;
+
+  DISALLOW_COPY_AND_ASSIGN(PasswordStoreMacTestDelegate);
+};
+
+PasswordStoreMacTestDelegate::PasswordStoreMacTestDelegate()
+    : ui_thread_(BrowserThread::UI, &message_loop_) {
+  Initialize();
+}
+
+PasswordStoreMacTestDelegate::~PasswordStoreMacTestDelegate() {
+  ClosePasswordStore();
+  thread_.reset();
+  login_db_.reset();
+}
+
+void PasswordStoreMacTestDelegate::Initialize() {
+  ASSERT_TRUE(db_dir_.CreateUniqueTempDir());
+
+  // Ensure that LoginDatabase will use the mock keychain if it needs to
+  // encrypt/decrypt a password.
+  OSCrypt::UseMockKeychain(true);
+  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()))));
+
+  // Creat and initialize the password store.
+  store_ = new PasswordStoreMac(
+      base::ThreadTaskRunnerHandle::Get(), nullptr,
+      make_scoped_ptr<AppleKeychain>(new MockAppleKeychain));

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

I think it should work without the type specifier.

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

It does. Removed.

+  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_.get()))));
+
+  // Make sure deferred initialization is performed before some tests start
+  // accessing the |login_db| directly.
+  FinishAsyncProcessing();
+}
+
+void PasswordStoreMacTestDelegate::FinishAsyncProcessing() {
+  scoped_refptr<content::MessageLoopRunner> runner =
+      new content::MessageLoopRunner;
+  ASSERT_TRUE(thread_->task_runner()->PostTaskAndReply(
+      FROM_HERE, base::Bind(&Noop), runner->QuitClosure()));
+  runner->Run();
+}
+

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

// static

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

Done.

+void PasswordStoreMacTestDelegate::InitLoginDatabase(LoginDatabase* login_db) {
+  ASSERT_TRUE(login_db->Init());
+}
+
+base::FilePath PasswordStoreMacTestDelegate::test_login_db_file_path() const {
+  return db_dir_.path().Append(FILE_PATH_LITERAL("login.db"));
+}
+
+void PasswordStoreMacTestDelegate::ClosePasswordStore() {
+  if (!store_)
+    return;
+
+  store_->Shutdown();
+  FinishAsyncProcessing();
+  store_ = nullptr;
+}
+
 }  // namespace
 
+namespace password_manager {
+
+INSTANTIATE_TYPED_TEST_CASE_P(Mac,
+                              PasswordStoreOriginTest,
+                              PasswordStoreMacTestDelegate);
+
+}  // namespace password_manager
+
 #pragma mark -
 
 class PasswordStoreMacInternalsTest : public testing::Test {
@@ -1173,32 +1272,11 @@ TEST_F(PasswordStoreMacInternalsTest, TestPasswordGetAll) {
 
 class PasswordStoreMacTest : public testing::Test {
  public:
-  PasswordStoreMacTest() : ui_thread_(BrowserThread::UI, &message_loop_) {}
+  PasswordStoreMacTest()
+      : histogram_tester_(new base::HistogramTester),
+        store_(delegate_.store()) {}
 
-  void SetUp() override {
-    ASSERT_TRUE(db_dir_.CreateUniqueTempDir());
-    histogram_tester_.reset(new base::HistogramTester);
-
-    // 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 +1286,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 +1303,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 +1336,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 +1344,14 @@ 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(); }
+  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_;
+  PasswordStoreMacTestDelegate delegate_;
+  PasswordStoreMac* store_;

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

Remove

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

Done (and replaced all references to store_ by delegate_.store()).

I also removed |store_| and store() from PasswordStoreMacTest and redirected
calls through delegate_. However, I had to add a const overload for
PasswordStoreMacTestDelegate.store() in order to satisfy login_db()'s constness.

 };
 
 TEST_F(PasswordStoreMacTest, TestStoreUpdate) {
@@ -1383,7 +1420,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 +1497,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 =
@@ -1481,13 +1518,13 @@ namespace {
 
 class PasswordsChangeObserver :
     public password_manager::PasswordStore::Observer {
-public:
- PasswordsChangeObserver(PasswordStoreMac* store) : observer_(this) {
+ public:
+  explicit PasswordsChangeObserver(PasswordStoreMac* store) : observer_(this) {
     observer_.Add(store);
   }
 
   void WaitAndVerify(PasswordStoreMacTest* test) {
-    test->FinishAsyncProcessing();
+    test->delegate().FinishAsyncProcessing();
     ::testing::Mock::VerifyAndClearExpectations(this);
   }
 
@@ -1495,7 +1532,7 @@ public:
   MOCK_METHOD1(OnLoginsChanged,
                void(const password_manager::PasswordStoreChangeList& changes));
 
-private:
+ private:
   ScopedObserver<password_manager::PasswordStore,
                 PasswordsChangeObserver> observer_;
 };
@@ -1643,7 +1680,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 +1688,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 +1711,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 +1808,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 +1846,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 +1868,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 +1879,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));