Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(318)

Unified Diff: chrome/browser/sync/glue/password_model_associator.cc

Issue 6878038: [Sync] Ensure we don't hold a transaction when we access password store. (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src
Patch Set: Rearrange Created 9 years, 8 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: chrome/browser/sync/glue/password_model_associator.cc
diff --git a/chrome/browser/sync/glue/password_model_associator.cc b/chrome/browser/sync/glue/password_model_associator.cc
index 3551be56260c8d8478d40e242431c3e5757e9ab6..fd4daa1f2d63b8bb21a120424d279f2bcabe180f 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;
« no previous file with comments | « chrome/browser/sync/glue/password_change_processor.cc ('k') | chrome/browser/sync/profile_sync_service_password_unittest.cc » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698