[Sync] Ensure we don't hold a transaction when we access password store.

chrome/browser/sync/glue/password_model_associator.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/sync/glue/password_model_associator.h"
 
 #include <set>
 
 #include "base/stl_util-inl.h"
 #include "base/utf_string_conversions.h"
@@ skipping 27 matching lines @@
 
 PasswordModelAssociator::~PasswordModelAssociator() {}
 
 bool PasswordModelAssociator::AssociateModels() {
   DCHECK(expected_loop_ == MessageLoop::current());
   {
     base::AutoLock lock(abort_association_pending_lock_);
     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)) {
     STLDeleteElements(&passwords);
     LOG(ERROR) << "Could not get the password entries.";
     return false;
   }
 
   std::set<std::string> current_passwords;
   PasswordVector new_passwords;
   PasswordVector updated_passwords;
+  {
+    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;
+    }
 
-  for (std::vector<webkit_glue::PasswordForm*>::iterator ix = passwords.begin();
-       ix != passwords.end(); ++ix) {
-    if (IsAbortPending())
-      return false;
-    std::string tag = MakeTag(**ix);
+    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));
+      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;
+        webkit_glue::PasswordForm new_password;
 
-      if (MergePasswords(password, **ix, &new_password)) {
-        sync_api::WriteNode write_node(&trans);
-        if (!write_node.InitByClientTagLookup(syncable::PASSWORDS, tag)) {
+        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);
+        }
+
+        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);
+
+        WriteToSyncNode(**ix, &node);
+
+        Associate(&tag, node.GetId());
       }
 
-      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.";
+      current_passwords.insert(tag);
+    }
+
+    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);
 
-      WriteToSyncNode(**ix, &node);
+      // 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;
 
-      Associate(&tag, node.GetId());
+        CopyPassword(password, &new_password);
+        Associate(&tag, sync_child_node.GetId());
+        new_passwords.push_back(new_password);
+      }
+
+      sync_child_id = sync_child_node.GetSuccessorId();
     }
-
-    current_passwords.insert(tag);
   }
 
-  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);
-
-    // 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);
-    }
-
-    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;
   }
 
   return true;
 }
 
 bool PasswordModelAssociator::DeleteAllNodes(
     sync_api::WriteTransaction* trans) {
@@ skipping 245 matching lines @@
     const std::string& password_element,
     const std::string& signon_realm) {
   return EscapePath(origin_url) + "|" +
          EscapePath(username_element) + "|" +
          EscapePath(username_value) + "|" +
          EscapePath(password_element) + "|" +
          EscapePath(signon_realm);
 }
 
 }  // namespace browser_sync