[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

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;