Handle duplicate password sync nodes

If the PasswordStore is asked to add an entry that already exists it
will overwrite the existing entry.  When this happens, it will report a
PasswordStoreChange::ADD event, much like it would had the original
entry not existed.  This confuses the PasswordChangeProcessor which
assumes that an ADD event implies that the added item did not previously
exist.

This change works around the issue by allowing the password change
processor to try to update an existing node in the event that the
creation of a new node fails.  This allows it to handle the case where a
it receives an ADD event for an existing node.

A better solution would be to have the PasswordStore to always return an
UPDATE event in cases where the node being updated already exists.  If
that solution were implemented the PasswordChangeProcessor would be
right to report an UnrecoverableError if it is unable to create a new
sync node in response to an ADD event.

BUG=87855
TEST=See repro steps in crbug.com/87855, crbug.com/103455


Review URL: http://codereview.chromium.org/8862005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@113888 0039d316-1c4b-4281-b951-d872f2087c98

1 files changed, 29 insertions, 9 deletions

diff --git a/chrome/browser/sync/glue/password_change_processor.cc b/chrome/browser/sync/glue/password_change_processor.cc
index 180a796..432b293 100644
--- a/chrome/browser/sync/glue/password_change_processor.cc
+++ b/chrome/browser/sync/glue/password_change_processor.cc
@@ -81,16 +81,36 @@ void PasswordChangeProcessor::Observe(
     switch (change->type()) {
       case PasswordStoreChange::ADD: {
         sync_api::WriteNode sync_node(&trans);
-        if (!sync_node.InitUniqueByCreation(syncable::PASSWORDS,
-                                            password_root, tag)) {
-          error_handler()->OnUnrecoverableError(FROM_HERE,
-              "Failed to create password sync node.");
-          return;
+        if (sync_node.InitUniqueByCreation(syncable::PASSWORDS,
+                                           password_root, tag)) {
+          PasswordModelAssociator::WriteToSyncNode(change->form(), &sync_node);
+          model_associator_->Associate(&tag, sync_node.GetId());
+          break;
+        } else {
+          // Maybe this node already exists and we should update it.
+          //
+          // If the PasswordStore is told to add an entry but an entry with the
+          // same name already exists, it will overwrite it.  It will report
+          // this change as an ADD rather than an UPDATE.  Ideally, it would be
+          // able to tell us what action was actually taken, rather than what
+          // action was requested.  If it did so, we wouldn't need to fall back
+          // to trying to update an existing password node here.
+          //
+          // TODO: Remove this.  See crbug.com/87855.
+          int64 sync_id = model_associator_->GetSyncIdFromChromeId(tag);
+          if (sync_api::kInvalidId == sync_id) {
+            error_handler()->OnUnrecoverableError(FROM_HERE,
+                "Unable to create or retrieve password node");
+            return;
+          }
+          if (!sync_node.InitByIdLookup(sync_id)) {
+            error_handler()->OnUnrecoverableError(FROM_HERE,
+                "Unable to create or retrieve password node");
+            return;
+          }
+          PasswordModelAssociator::WriteToSyncNode(change->form(), &sync_node);
+          break;
         }
-
-        PasswordModelAssociator::WriteToSyncNode(change->form(), &sync_node);
-        model_associator_->Associate(&tag, sync_node.GetId());
-        break;
       }
       case PasswordStoreChange::UPDATE: {
         sync_api::WriteNode sync_node(&trans);