summaryrefslogtreecommitdiffstats
path: root/chrome
diff options
context:
space:
mode:
authorzea@chromium.org <zea@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>2011-04-19 22:29:27 +0000
committerzea@chromium.org <zea@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>2011-04-19 22:29:27 +0000
commit8e31fe6500e63e0300ca026abcd0a4fec1b896b0 (patch)
treef695942d27d27514eb6d2ddd07242c1c8d4ce228 /chrome
parentde54e7dffb52e0bd8d81a17d2d64377cbcbc5b3f (diff)
downloadchromium_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')
-rw-r--r--chrome/browser/sync/glue/password_change_processor.cc29
-rw-r--r--chrome/browser/sync/glue/password_change_processor.h23
-rw-r--r--chrome/browser/sync/glue/password_model_associator.cc132
-rw-r--r--chrome/browser/sync/profile_sync_service_password_unittest.cc76
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;