Open the LoginDatabase on the DB thread, not the UI thread.

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 a3e8810e9e14243f48c1180ef72c323d3d453e76..c720e4a998daa020cc4287ec45f7360805778595 100644
--- a/chrome/browser/password_manager/password_store_mac_unittest.cc
+++ b/chrome/browser/password_manager/password_store_mac_unittest.cc
@@ -13,6 +13,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/password_manager/password_store_mac_internal.h"
 #include "chrome/common/chrome_paths.h"
+#include "components/password_manager/core/browser/login_database.h"
 #include "components/password_manager/core/browser/password_store_consumer.h"
 #include "content/public/test/test_browser_thread.h"
 #include "crypto/mock_apple_keychain.h"
@@ -60,18 +61,36 @@ ACTION(QuitUIMessageLoop) {
   base::MessageLoop::current()->Quit();
 }
 
+class MockPasswordStoreObserver : public PasswordStore::Observer {
+ public:
+  MOCK_METHOD1(OnLoginsChanged,
+               void(const password_manager::PasswordStoreChangeList& changes));
+};
+
+// A mock LoginDatabase that simulates a failing Init() method.
+class BadLoginDatabase : public password_manager::LoginDatabase {
+ public:
+  BadLoginDatabase() : password_manager::LoginDatabase(base::FilePath()) {}
+  ~BadLoginDatabase() override {}
+
+  // LoginDatabase:
+  bool Init() override { return false; }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(BadLoginDatabase);
+};
+
 class TestPasswordStoreMac : public PasswordStoreMac {
  public:
   TestPasswordStoreMac(
       scoped_refptr<base::SingleThreadTaskRunner> main_thread_runner,
       scoped_refptr<base::SingleThreadTaskRunner> db_thread_runner,
-      crypto::AppleKeychain* keychain,
-      LoginDatabase* login_db)
+      scoped_ptr<crypto::AppleKeychain> keychain,
+      scoped_ptr<password_manager::LoginDatabase> login_db)
       : PasswordStoreMac(main_thread_runner,
                          db_thread_runner,
-                         keychain,
-                         login_db) {
-  }
+                         keychain.Pass(),
+                         login_db.Pass()) {}
 
   using PasswordStoreMac::GetBackgroundTaskRunner;
 
@@ -1052,24 +1071,39 @@ class PasswordStoreMacTest : public testing::Test {
   PasswordStoreMacTest() : ui_thread_(BrowserThread::UI, &message_loop_) {}
 
   void SetUp() override {
-    login_db_ = new LoginDatabase();
     ASSERT_TRUE(db_dir_.CreateUniqueTempDir());
-    base::FilePath db_file = db_dir_.path().AppendASCII("login.db");
-    ASSERT_TRUE(login_db_->Init(db_file));
-
-    keychain_ = new MockAppleKeychain();
 
+    scoped_ptr<password_manager::LoginDatabase> login_db(
+        new password_manager::LoginDatabase(test_login_db_file_path()));
     store_ = new TestPasswordStoreMac(
-        base::MessageLoopProxy::current(),
-        base::MessageLoopProxy::current(),
-        keychain_,
-        login_db_);
+        base::MessageLoopProxy::current(), base::MessageLoopProxy::current(),
+        make_scoped_ptr<AppleKeychain>(new MockAppleKeychain), login_db.Pass());
     ASSERT_TRUE(store_->Init(syncer::SyncableService::StartSyncFlare()));
+    // Make sure deferred initialization is performed before some tests start
+    // accessing the |login_db| directly.
+    base::MessageLoop::current()->Run();
   }
 
   void TearDown() override {
+    ClosePasswordStore();
+  }
+
+  void ClosePasswordStore() {
     store_->Shutdown();
     EXPECT_FALSE(store_->GetBackgroundTaskRunner().get());

[vabr (Chromium), 2015/01/20 15:41:35]

optional nit: .get() should be unnecessary and is usually omitted.

[engedy, 2015/01/20 15:48:50]

Done.

+    store_ = nullptr;
+  }
+
+  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();
+  }
+
+  MockAppleKeychain* keychain() {
+    return static_cast<MockAppleKeychain*>(store_->keychain());
   }
 
   void WaitForStoreUpdate() {
@@ -1083,16 +1117,12 @@ class PasswordStoreMacTest : public testing::Test {
 
   TestPasswordStoreMac* store() { return store_.get(); }
 
-  MockAppleKeychain* keychain() { return keychain_; }
-
  protected:
   base::MessageLoopForUI message_loop_;
   content::TestBrowserThread ui_thread_;
 
-  MockAppleKeychain* keychain_;  // Owned by store_.
-  LoginDatabase* login_db_;  // Owned by store_.
-  scoped_refptr<TestPasswordStoreMac> store_;
   base::ScopedTempDir db_dir_;
+  scoped_refptr<TestPasswordStoreMac> store_;
 };
 
 TEST_F(PasswordStoreMacTest, TestStoreUpdate) {
@@ -1107,12 +1137,12 @@ TEST_F(PasswordStoreMacTest, TestStoreUpdate) {
     L"username", L"password", L"submit", L"joe_user", L"sekrit", true, false, 1
   };
   scoped_ptr<PasswordForm> joint_form(CreatePasswordFormFromData(joint_data));
-  login_db_->AddLogin(*joint_form);
+  login_db()->AddLogin(*joint_form);
   MockAppleKeychain::KeychainTestData joint_keychain_data = {
     kSecAuthenticationTypeHTMLForm, "some.domain.com",
     kSecProtocolTypeHTTP, "/insecure.html", 0, NULL, "20020601171500Z",
     "joe_user", "sekrit", false };
-  keychain_->AddTestItem(joint_keychain_data);
+  keychain()->AddTestItem(joint_keychain_data);
 
   // Insert a password into the keychain only.
   MockAppleKeychain::KeychainTestData keychain_only_data = {
@@ -1120,7 +1150,7 @@ TEST_F(PasswordStoreMacTest, TestStoreUpdate) {
     kSecProtocolTypeHTTP, NULL, 0, NULL, "20020601171500Z",
     "keychain", "only", false
   };
-  keychain_->AddTestItem(keychain_only_data);
+  keychain()->AddTestItem(keychain_only_data);
 
   struct UpdateData {
     PasswordFormData form_data;
@@ -1162,7 +1192,7 @@ TEST_F(PasswordStoreMacTest, TestStoreUpdate) {
 
   WaitForStoreUpdate();
 
-  MacKeychainPasswordFormAdapter keychain_adapter(keychain_);
+  MacKeychainPasswordFormAdapter keychain_adapter(keychain());
   for (unsigned int i = 0; i < arraysize(updates); ++i) {
     scoped_ptr<PasswordForm> query_form(
         CreatePasswordFormFromData(updates[i].form_data));
@@ -1180,7 +1210,7 @@ TEST_F(PasswordStoreMacTest, TestStoreUpdate) {
     }
     STLDeleteElements(&matching_items);
 
-    login_db_->GetLogins(*query_form, &matching_items);
+    login_db()->GetLogins(*query_form, &matching_items);
     EXPECT_EQ(updates[i].password ? 1U : 0U, matching_items.size())
         << "iteration " << i;
     STLDeleteElements(&matching_items);
@@ -1210,8 +1240,8 @@ TEST_F(PasswordStoreMacTest, TestDBKeychainAssociation) {
     L"username", L"password", L"submit", L"joe_user", L"sekrit", true, false, 1
   };
   scoped_ptr<PasswordForm> www_form(CreatePasswordFormFromData(www_form_data));
-  login_db_->AddLogin(*www_form);
-  MacKeychainPasswordFormAdapter owned_keychain_adapter(keychain_);
+  login_db()->AddLogin(*www_form);
+  MacKeychainPasswordFormAdapter owned_keychain_adapter(keychain());
   owned_keychain_adapter.SetFindsOnlyOwnedItems(true);
   owned_keychain_adapter.AddPassword(*www_form);
 
@@ -1229,7 +1259,7 @@ TEST_F(PasswordStoreMacTest, TestDBKeychainAssociation) {
   EXPECT_EQ(1u, consumer.last_result.size());
 
   // 3. Add the returned password for m.facebook.com.
-  login_db_->AddLogin(consumer.last_result[0]);
+  login_db()->AddLogin(consumer.last_result[0]);
   owned_keychain_adapter.AddPassword(m_form);
 
   // 4. Remove both passwords.
@@ -1242,13 +1272,13 @@ TEST_F(PasswordStoreMacTest, TestDBKeychainAssociation) {
   matching_items = owned_keychain_adapter.PasswordsFillingForm(
       www_form->signon_realm, www_form->scheme);
   EXPECT_EQ(0u, matching_items.size());
-  login_db_->GetLogins(*www_form, &matching_items);
+  login_db()->GetLogins(*www_form, &matching_items);
   EXPECT_EQ(0u, matching_items.size());
   // No trace of m.facebook.com.
   matching_items = owned_keychain_adapter.PasswordsFillingForm(
       m_form.signon_realm, m_form.scheme);
   EXPECT_EQ(0u, matching_items.size());
-  login_db_->GetLogins(m_form, &matching_items);
+  login_db()->GetLogins(m_form, &matching_items);
   EXPECT_EQ(0u, matching_items.size());
 }
 
@@ -1395,11 +1425,11 @@ TEST_F(PasswordStoreMacTest, TestRemoveLoginsMultiProfile) {
       kSecAuthenticationTypeHTMLForm, "some.domain.com",
       kSecProtocolTypeHTTP, "/insecure.html", 0, NULL, "20020601171500Z",
       "joe_user", "sekrit", false };
-  keychain_->AddTestItem(keychain_data);
+  keychain()->AddTestItem(keychain_data);
 
   // Add a password through the adapter. It has the "Chrome" creator tag.
   // However, it's not referenced by the password database.
-  MacKeychainPasswordFormAdapter owned_keychain_adapter(keychain_);
+  MacKeychainPasswordFormAdapter owned_keychain_adapter(keychain());
   owned_keychain_adapter.SetFindsOnlyOwnedItems(true);
   PasswordFormData www_form_data1 = {
       PasswordForm::SCHEME_HTML, "http://www.facebook.com/",
@@ -1418,7 +1448,7 @@ TEST_F(PasswordStoreMacTest, TestRemoveLoginsMultiProfile) {
   WaitForStoreUpdate();
 
   ScopedVector<PasswordForm> matching_items;
-  login_db_->GetLogins(*www_form, &matching_items.get());
+  login_db()->GetLogins(*www_form, &matching_items.get());
   EXPECT_EQ(1u, matching_items.size());
   matching_items.clear();
 
@@ -1426,7 +1456,7 @@ TEST_F(PasswordStoreMacTest, TestRemoveLoginsMultiProfile) {
   WaitForStoreUpdate();
 
   // Check the second facebook form is gone.
-  login_db_->GetLogins(*www_form, &matching_items.get());
+  login_db()->GetLogins(*www_form, &matching_items.get());
   EXPECT_EQ(0u, matching_items.size());
 
   // Check the first facebook form is still there.
@@ -1442,3 +1472,109 @@ TEST_F(PasswordStoreMacTest, TestRemoveLoginsMultiProfile) {
       "http://some.domain.com/insecure.html", PasswordForm::SCHEME_HTML);
   ASSERT_EQ(1u, matching_items.size());
 }
+
+TEST_F(PasswordStoreMacTest, StoreIsUsableImmediatelyAfterConstruction) {
+  PasswordFormData password_form_data = {
+    PasswordForm::SCHEME_HTML, "http://example.com/",
+    "http://example.com/login.html", "login.cgi",
+    L"username", L"password", L"submit", L"joe_user", L"sekrit", true, false, 1
+  };
+  scoped_ptr<PasswordForm> password_form(
+      CreatePasswordFormFromData(password_form_data));
+  store()->AddLogin(*password_form);
+
+  // Close the store.
+  ClosePasswordStore();
+  base::MessageLoop::current()->RunUntilIdle();
+
+  // Reopen the store and immediately try to read its contents without running
+  // the message loop in-between. This simulates the scenario where the DB
+  // thread is busy during initialization so the GetLogins task gets scheduled
+  // to the DB thread while the store is still uninitialized. If tasks are
+  // correctly processed in order, this should of no concern.

[vabr (Chromium), 2015/01/20 15:41:35]

typo: should of -> should be of

[engedy, 2015/01/20 15:48:50]

Done.

+  scoped_ptr<password_manager::LoginDatabase> login_db(
+      new password_manager::LoginDatabase(test_login_db_file_path()));
+  scoped_refptr<TestPasswordStoreMac> reopened_store(new TestPasswordStoreMac(
+      base::MessageLoopProxy::current(), base::MessageLoopProxy::current(),
+      make_scoped_ptr<crypto::AppleKeychain>(new MockAppleKeychain),
+      login_db.Pass()));
+  reopened_store->Init(syncer::SyncableService::StartSyncFlare());
+
+  MockPasswordStoreConsumer consumer;
+  EXPECT_CALL(consumer, OnGetPasswordStoreResults(_))
+      .WillOnce(DoAll(WithArg<0>(Invoke(
+                          &consumer, &MockPasswordStoreConsumer::CopyElements)),
+                      WithArg<0>(STLDeleteElements0())));
+  reopened_store->GetLogins(*password_form, PasswordStore::ALLOW_PROMPT,
+                            &consumer);
+  base::MessageLoop::current()->RunUntilIdle();
+  EXPECT_EQ(1u, consumer.last_result.size());
+}
+
+// Verify that operations on a PasswordStore with a bad database cause no
+// explosions, but fail without side effect, return no data and trigger no
+// notifications.
+TEST_F(PasswordStoreMacTest, OperationsOnABadDatabaseSilentlyFail) {
+  scoped_refptr<TestPasswordStoreMac> bad_store(new TestPasswordStoreMac(
+      base::MessageLoopProxy::current(), base::MessageLoopProxy::current(),
+      make_scoped_ptr<crypto::AppleKeychain>(new MockAppleKeychain),
+      make_scoped_ptr<password_manager::LoginDatabase>(new BadLoginDatabase)));
+
+  bad_store->Init(syncer::SyncableService::StartSyncFlare());
+  base::MessageLoop::current()->RunUntilIdle();
+  ASSERT_EQ(nullptr, bad_store->login_metadata_db());
+
+  testing::StrictMock<MockPasswordStoreObserver> mock_observer;
+  bad_store->AddObserver(&mock_observer);
+
+  // Add a new autofillable login + a blacklisted login.
+  PasswordFormData www_form_data = {
+      PasswordForm::SCHEME_HTML, "http://www.facebook.com/",
+      "http://www.facebook.com/index.html", "login", L"username", L"password",
+      L"submit", L"not_joe_user", L"12345", true, false, 1};
+  scoped_ptr<PasswordForm> form(CreatePasswordFormFromData(www_form_data));
+  scoped_ptr<PasswordForm> blacklisted_form(new PasswordForm(*form));
+  blacklisted_form->signon_realm = "http://foo.example.com";
+  blacklisted_form->origin = GURL("http://foo.example.com/origin");
+  blacklisted_form->action = GURL("http://foo.example.com/action");
+  blacklisted_form->blacklisted_by_user = true;
+  bad_store->AddLogin(*form);
+  bad_store->AddLogin(*blacklisted_form);
+  base::MessageLoop::current()->RunUntilIdle();
+
+  // Get all logins; autofillable logins; blacklisted logins.
+  testing::StrictMock<MockPasswordStoreConsumer> mock_consumer;
+  EXPECT_CALL(mock_consumer, OnGetPasswordStoreResults(testing::ElementsAre()));
+  bad_store->GetLogins(*form, PasswordStore::DISALLOW_PROMPT, &mock_consumer);
+  base::MessageLoop::current()->RunUntilIdle();
+  testing::Mock::VerifyAndClearExpectations(&mock_consumer);
+  EXPECT_CALL(mock_consumer, OnGetPasswordStoreResults(testing::ElementsAre()));
+  bad_store->GetAutofillableLogins(&mock_consumer);
+  base::MessageLoop::current()->RunUntilIdle();
+  testing::Mock::VerifyAndClearExpectations(&mock_consumer);
+  EXPECT_CALL(mock_consumer, OnGetPasswordStoreResults(testing::ElementsAre()));
+  bad_store->GetBlacklistLogins(&mock_consumer);
+  base::MessageLoop::current()->RunUntilIdle();
+
+  // Report metrics.
+  bad_store->ReportMetrics("Test Username", true);
+  base::MessageLoop::current()->RunUntilIdle();
+
+  // Change the login.
+  form->password_value = base::ASCIIToUTF16("a different password");
+  bad_store->UpdateLogin(*form);
+  base::MessageLoop::current()->RunUntilIdle();
+
+  // Delete one login; a range of logins.
+  bad_store->RemoveLogin(*form);
+  base::MessageLoop::current()->RunUntilIdle();
+  bad_store->RemoveLoginsCreatedBetween(base::Time(), base::Time::Max());
+  base::MessageLoop::current()->RunUntilIdle();
+  bad_store->RemoveLoginsSyncedBetween(base::Time(), base::Time::Max());
+  base::MessageLoop::current()->RunUntilIdle();
+
+  // Ensure no notifications and no explosions during shutdown either.
+  bad_store->RemoveObserver(&mock_observer);
+  bad_store->Shutdown();
+  base::MessageLoop::current()->RunUntilIdle();
+}