diff options
author | zea@chromium.org <zea@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-04-19 22:29:27 +0000 |
---|---|---|
committer | zea@chromium.org <zea@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-04-19 22:29:27 +0000 |
commit | 8e31fe6500e63e0300ca026abcd0a4fec1b896b0 (patch) | |
tree | f695942d27d27514eb6d2ddd07242c1c8d4ce228 /chrome | |
parent | de54e7dffb52e0bd8d81a17d2d64377cbcbc5b3f (diff) | |
download | chromium_src-8e31fe6500e63e0300ca026abcd0a4fec1b896b0.zip chromium_src-8e31fe6500e63e0300ca026abcd0a4fec1b896b0.tar.gz chromium_src-8e31fe6500e63e0300ca026abcd0a4fec1b896b0.tar.bz2 |
[Sync] Ensure we don't hold a transaction when we access password store.
BUG=70658
TEST=ProfileSyncSerivcePasswordTest.EnsureNoTransactions
Review URL: http://codereview.chromium.org/6878038
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@82174 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'chrome')
4 files changed, 183 insertions, 77 deletions
diff --git a/chrome/browser/sync/glue/password_change_processor.cc b/chrome/browser/sync/glue/password_change_processor.cc index a4d7805..de47d0a 100644 --- a/chrome/browser/sync/glue/password_change_processor.cc +++ b/chrome/browser/sync/glue/password_change_processor.cc @@ -130,7 +130,6 @@ void PasswordChangeProcessor::ApplyChangesFromSyncModel( DCHECK(expected_loop_ == MessageLoop::current()); if (!running()) return; - StopObserving(); sync_api::ReadNode password_root(trans); if (!password_root.InitByTagLookup(kPasswordTag)) { @@ -139,9 +138,8 @@ void PasswordChangeProcessor::ApplyChangesFromSyncModel( return; } - PasswordModelAssociator::PasswordVector new_passwords; - PasswordModelAssociator::PasswordVector updated_passwords; - PasswordModelAssociator::PasswordVector deleted_passwords; + DCHECK(deleted_passwords_.empty() && new_passwords_.empty() && + updated_passwords_.empty()); for (int i = 0; i < change_count; ++i) { if (sync_api::SyncManager::ChangeRecord::ACTION_DELETE == @@ -154,7 +152,7 @@ void PasswordChangeProcessor::ApplyChangesFromSyncModel( const sync_pb::PasswordSpecificsData& password = extra->unencrypted(); webkit_glue::PasswordForm form; PasswordModelAssociator::CopyPassword(password, &form); - deleted_passwords.push_back(form); + deleted_passwords_.push_back(form); model_associator_->Disassociate(changes[i].id); continue; } @@ -178,21 +176,32 @@ void PasswordChangeProcessor::ApplyChangesFromSyncModel( if (sync_api::SyncManager::ChangeRecord::ACTION_ADD == changes[i].action) { std::string tag(PasswordModelAssociator::MakeTag(password)); model_associator_->Associate(&tag, sync_node.GetId()); - new_passwords.push_back(password); + new_passwords_.push_back(password); } else { DCHECK(sync_api::SyncManager::ChangeRecord::ACTION_UPDATE == changes[i].action); - updated_passwords.push_back(password); + updated_passwords_.push_back(password); } } +} + +void PasswordChangeProcessor::CommitChangesFromSyncModel() { + DCHECK(expected_loop_ == MessageLoop::current()); + if (!running()) + return; + StopObserving(); - if (!model_associator_->WriteToPasswordStore(&new_passwords, - &updated_passwords, - &deleted_passwords)) { + if (!model_associator_->WriteToPasswordStore(&new_passwords_, + &updated_passwords_, + &deleted_passwords_)) { error_handler()->OnUnrecoverableError(FROM_HERE, "Error writing passwords"); return; } + deleted_passwords_.clear(); + new_passwords_.clear(); + updated_passwords_.clear(); + StartObserving(); } diff --git a/chrome/browser/sync/glue/password_change_processor.h b/chrome/browser/sync/glue/password_change_processor.h index ab6b89d..155a1b3 100644 --- a/chrome/browser/sync/glue/password_change_processor.h +++ b/chrome/browser/sync/glue/password_change_processor.h @@ -9,6 +9,8 @@ #include "chrome/browser/sync/glue/change_processor.h" #include "base/basictypes.h" +#include "base/compiler_specific.h" +#include "chrome/browser/sync/glue/password_model_associator.h" #include "chrome/browser/sync/glue/sync_backend_host.h" #include "content/common/notification_observer.h" #include "content/common/notification_registrar.h" @@ -20,7 +22,6 @@ class NotificationService; namespace browser_sync { -class PasswordModelAssociator; class UnrecoverableErrorHandler; // This class is responsible for taking changes from the password backend and @@ -39,17 +40,23 @@ class PasswordChangeProcessor : public ChangeProcessor, // Passwords -> sync_api model change application. virtual void Observe(NotificationType type, const NotificationSource& source, - const NotificationDetails& details); + const NotificationDetails& details) OVERRIDE; // sync_api model -> WebDataService change application. virtual void ApplyChangesFromSyncModel( const sync_api::BaseTransaction* trans, const sync_api::SyncManager::ChangeRecord* changes, - int change_count); + int change_count) OVERRIDE; + + // Commit changes buffered during ApplyChanges. We must commit them to the + // password store only after the sync_api transaction is released, else there + // is risk of deadlock due to the password store posting tasks to the UI + // thread (http://crbug.com/70658). + virtual void CommitChangesFromSyncModel() OVERRIDE; protected: - virtual void StartImpl(Profile* profile); - virtual void StopImpl(); + virtual void StartImpl(Profile* profile) OVERRIDE; + virtual void StopImpl() OVERRIDE; private: void StartObserving(); @@ -63,6 +70,12 @@ class PasswordChangeProcessor : public ChangeProcessor, // holding a reference. PasswordStore* password_store_; + // Buffers used between ApplyChangesFromSyncModel and + // CommitChangesFromSyncModel. + PasswordModelAssociator::PasswordVector new_passwords_; + PasswordModelAssociator::PasswordVector updated_passwords_; + PasswordModelAssociator::PasswordVector deleted_passwords_; + NotificationRegistrar notification_registrar_; bool observing_; diff --git a/chrome/browser/sync/glue/password_model_associator.cc b/chrome/browser/sync/glue/password_model_associator.cc index 3551be5..fd4daa1 100644 --- a/chrome/browser/sync/glue/password_model_associator.cc +++ b/chrome/browser/sync/glue/password_model_associator.cc @@ -45,14 +45,9 @@ bool PasswordModelAssociator::AssociateModels() { abort_association_pending_ = false; } - sync_api::WriteTransaction trans(sync_service_->GetUserShare()); - sync_api::ReadNode password_root(&trans); - if (!password_root.InitByTagLookup(kPasswordTag)) { - LOG(ERROR) << "Server did not create the top-level password node. We " - << "might be running against an out-of-date server."; - return false; - } - + // We must not be holding a transaction when we interact with the password + // store, as it can post tasks to the UI thread which can itself be blocked + // on our transaction, resulting in deadlock. (http://crbug.com/70658) std::vector<webkit_glue::PasswordForm*> passwords; if (!password_store_->FillAutofillableLogins(&passwords) || !password_store_->FillBlacklistLogins(&passwords)) { @@ -64,76 +59,89 @@ bool PasswordModelAssociator::AssociateModels() { std::set<std::string> current_passwords; PasswordVector new_passwords; PasswordVector updated_passwords; - - for (std::vector<webkit_glue::PasswordForm*>::iterator ix = passwords.begin(); - ix != passwords.end(); ++ix) { - if (IsAbortPending()) + { + sync_api::WriteTransaction trans(sync_service_->GetUserShare()); + sync_api::ReadNode password_root(&trans); + if (!password_root.InitByTagLookup(kPasswordTag)) { + LOG(ERROR) << "Server did not create the top-level password node. We " + << "might be running against an out-of-date server."; return false; - std::string tag = MakeTag(**ix); - - sync_api::ReadNode node(&trans); - if (node.InitByClientTagLookup(syncable::PASSWORDS, tag)) { - const sync_pb::PasswordSpecificsData& password = - node.GetPasswordSpecifics(); - DCHECK_EQ(tag, MakeTag(password)); + } - webkit_glue::PasswordForm new_password; + for (std::vector<webkit_glue::PasswordForm*>::iterator ix = + passwords.begin(); + ix != passwords.end(); ++ix) { + if (IsAbortPending()) + return false; + std::string tag = MakeTag(**ix); + + sync_api::ReadNode node(&trans); + if (node.InitByClientTagLookup(syncable::PASSWORDS, tag)) { + const sync_pb::PasswordSpecificsData& password = + node.GetPasswordSpecifics(); + DCHECK_EQ(tag, MakeTag(password)); + + webkit_glue::PasswordForm new_password; + + if (MergePasswords(password, **ix, &new_password)) { + sync_api::WriteNode write_node(&trans); + if (!write_node.InitByClientTagLookup(syncable::PASSWORDS, tag)) { + STLDeleteElements(&passwords); + LOG(ERROR) << "Failed to edit password sync node."; + return false; + } + WriteToSyncNode(new_password, &write_node); + updated_passwords.push_back(new_password); + } - if (MergePasswords(password, **ix, &new_password)) { - sync_api::WriteNode write_node(&trans); - if (!write_node.InitByClientTagLookup(syncable::PASSWORDS, tag)) { + Associate(&tag, node.GetId()); + } else { + sync_api::WriteNode node(&trans); + if (!node.InitUniqueByCreation(syncable::PASSWORDS, + password_root, tag)) { STLDeleteElements(&passwords); - LOG(ERROR) << "Failed to edit password sync node."; + LOG(ERROR) << "Failed to create password sync node."; return false; } - WriteToSyncNode(new_password, &write_node); - updated_passwords.push_back(new_password); - } - Associate(&tag, node.GetId()); - } else { - sync_api::WriteNode node(&trans); - if (!node.InitUniqueByCreation(syncable::PASSWORDS, - password_root, tag)) { - STLDeleteElements(&passwords); - LOG(ERROR) << "Failed to create password sync node."; - return false; - } + WriteToSyncNode(**ix, &node); - WriteToSyncNode(**ix, &node); + Associate(&tag, node.GetId()); + } - Associate(&tag, node.GetId()); + current_passwords.insert(tag); } - current_passwords.insert(tag); - } + STLDeleteElements(&passwords); - STLDeleteElements(&passwords); + int64 sync_child_id = password_root.GetFirstChildId(); + while (sync_child_id != sync_api::kInvalidId) { + sync_api::ReadNode sync_child_node(&trans); + if (!sync_child_node.InitByIdLookup(sync_child_id)) { + LOG(ERROR) << "Failed to fetch child node."; + return false; + } + const sync_pb::PasswordSpecificsData& password = + sync_child_node.GetPasswordSpecifics(); + std::string tag = MakeTag(password); - int64 sync_child_id = password_root.GetFirstChildId(); - while (sync_child_id != sync_api::kInvalidId) { - sync_api::ReadNode sync_child_node(&trans); - if (!sync_child_node.InitByIdLookup(sync_child_id)) { - LOG(ERROR) << "Failed to fetch child node."; - return false; - } - const sync_pb::PasswordSpecificsData& password = - sync_child_node.GetPasswordSpecifics(); - std::string tag = MakeTag(password); - - // The password only exists on the server. Add it to the local - // model. - if (current_passwords.find(tag) == current_passwords.end()) { - webkit_glue::PasswordForm new_password; - - CopyPassword(password, &new_password); - Associate(&tag, sync_child_node.GetId()); - new_passwords.push_back(new_password); - } + // The password only exists on the server. Add it to the local + // model. + if (current_passwords.find(tag) == current_passwords.end()) { + webkit_glue::PasswordForm new_password; - sync_child_id = sync_child_node.GetSuccessorId(); + CopyPassword(password, &new_password); + Associate(&tag, sync_child_node.GetId()); + new_passwords.push_back(new_password); + } + + sync_child_id = sync_child_node.GetSuccessorId(); + } } + // We must not be holding a transaction when we interact with the password + // store, as it can post tasks to the UI thread which can itself be blocked + // on our transaction, resulting in deadlock. (http://crbug.com/70658) if (!WriteToPasswordStore(&new_passwords, &updated_passwords, NULL)) { LOG(ERROR) << "Failed to write passwords."; return false; diff --git a/chrome/browser/sync/profile_sync_service_password_unittest.cc b/chrome/browser/sync/profile_sync_service_password_unittest.cc index 22005ce..16cb99e 100644 --- a/chrome/browser/sync/profile_sync_service_password_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_password_unittest.cc @@ -83,6 +83,13 @@ ACTION_P3(MakePasswordSyncComponents, service, ps, dtc) { change_processor); } +ACTION_P(AcquireSyncTransaction, password_test_service) { + // Check to make sure we can aquire a transaction (will crash if a transaction + // is already held by this thread, deadlock if held by another thread). + sync_api::WriteTransaction trans(password_test_service->GetUserShare()); + VLOG(1) << "Sync transaction acquired."; +} + static void QuitMessageLoop() { MessageLoop::current()->Quit(); } @@ -137,6 +144,10 @@ class PasswordTestProfileSyncService : public TestProfileSyncService { }; class ProfileSyncServicePasswordTest : public AbstractProfileSyncServiceTest { + public: + sync_api::UserShare* GetUserShare() { + return service_->GetUserShare(); + } protected: ProfileSyncServicePasswordTest() : db_thread_(BrowserThread::DB) { @@ -476,6 +487,71 @@ TEST_F(ProfileSyncServicePasswordTest, HasNativeHasSyncNoMerge) { EXPECT_TRUE(ComparePasswords(expected_forms[1], new_sync_forms[1])); } +// Same as HasNativeHasEmptyNoMerge, but we attempt to aquire a sync transaction +// every time the password store is accessed. +TEST_F(ProfileSyncServicePasswordTest, EnsureNoTransactions) { + std::vector<PasswordForm*> native_forms; + std::vector<PasswordForm> sync_forms; + std::vector<PasswordForm> expected_forms; + { + PasswordForm* new_form = new PasswordForm; + new_form->scheme = PasswordForm::SCHEME_HTML; + new_form->signon_realm = "pie"; + new_form->origin = GURL("http://pie.com"); + new_form->action = GURL("http://pie.com/submit"); + new_form->username_element = UTF8ToUTF16("name"); + new_form->username_value = UTF8ToUTF16("tom"); + new_form->password_element = UTF8ToUTF16("cork"); + new_form->password_value = UTF8ToUTF16("password1"); + new_form->ssl_valid = true; + new_form->preferred = false; + new_form->date_created = base::Time::FromInternalValue(1234); + new_form->blacklisted_by_user = false; + + native_forms.push_back(new_form); + expected_forms.push_back(*new_form); + } + + { + PasswordForm new_form; + new_form.scheme = PasswordForm::SCHEME_HTML; + new_form.signon_realm = "pie2"; + new_form.origin = GURL("http://pie2.com"); + new_form.action = GURL("http://pie2.com/submit"); + new_form.username_element = UTF8ToUTF16("name2"); + new_form.username_value = UTF8ToUTF16("tom2"); + new_form.password_element = UTF8ToUTF16("cork2"); + new_form.password_value = UTF8ToUTF16("password12"); + new_form.ssl_valid = false; + new_form.preferred = true; + new_form.date_created = base::Time::FromInternalValue(12345); + new_form.blacklisted_by_user = false; + sync_forms.push_back(new_form); + expected_forms.push_back(new_form); + } + + EXPECT_CALL(*password_store_, FillAutofillableLogins(_)) + .WillOnce(DoAll(SetArgumentPointee<0>(native_forms), + AcquireSyncTransaction(this), + Return(true))); + EXPECT_CALL(*password_store_, FillBlacklistLogins(_)) + .WillOnce(DoAll(AcquireSyncTransaction(this), + Return(true))); + EXPECT_CALL(*password_store_, AddLoginImpl(_)) + .WillOnce(AcquireSyncTransaction(this)); + + CreateRootTask root_task(this, syncable::PASSWORDS); + AddPasswordEntriesTask node_task(this, sync_forms); + StartSyncService(&root_task, &node_task); + + std::vector<PasswordForm> new_sync_forms; + GetPasswordEntriesFromSyncDB(&new_sync_forms); + + EXPECT_EQ(2U, new_sync_forms.size()); + EXPECT_TRUE(ComparePasswords(expected_forms[0], new_sync_forms[0])); + EXPECT_TRUE(ComparePasswords(expected_forms[1], new_sync_forms[1])); +} + TEST_F(ProfileSyncServicePasswordTest, HasNativeHasSyncMergeEntry) { std::vector<PasswordForm*> native_forms; std::vector<PasswordForm> sync_forms; |