[Sync] Implementation of model association for passwords using sync API

chrome/browser/password_manager/password_syncable_service.cc

 // Copyright 2013 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/password_manager/password_syncable_service.h"
 
 #include "base/location.h"
+#include "base/memory/scoped_vector.h"
+#include "base/metrics/histogram.h"
+#include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/password_manager/password_store.h"
 #include "components/autofill/core/common/password_form.h"
 #include "net/base/escape.h"
 #include "sync/api/sync_error_factory.h"
 
 namespace {
 
-// Converts the |PasswordSpecifics| obtained from sync to an
-// object of type |PasswordForm|.
-void ExtractPasswordFromSpecifics(
-     const sync_pb::PasswordSpecificsData& password,
-     autofill::PasswordForm* new_password) {
-  new_password->scheme =
-      static_cast<autofill::PasswordForm::Scheme>(password.scheme());
-  new_password->signon_realm = password.signon_realm();
-  new_password->origin = GURL(password.origin());
-  new_password->action = GURL(password.action());
-  new_password->username_element =
-      UTF8ToUTF16(password.username_element());
-  new_password->password_element =
-      UTF8ToUTF16(password.password_element());
-  new_password->username_value =
-      UTF8ToUTF16(password.username_value());
-  new_password->password_value =
-      UTF8ToUTF16(password.password_value());
-  new_password->ssl_valid = password.ssl_valid();
-  new_password->preferred = password.preferred();
-  new_password->date_created =
-      base::Time::FromInternalValue(password.date_created());
-  new_password->blacklisted_by_user =
-      password.blacklisted();
-}
-
 // Merges the sync password (obtained from the password specifics) and
 // local password and stores the output in the |new_password_form| pointer.

[Ilya Sherman, 2013/11/05 08:39:21]

nit: Please document the semantics of the return value for this function.

[lipalani1, 2013/11/19 00:46:54]

Done. it is in the header file.

 bool MergeLocalAndSyncPasswords(
     const sync_pb::PasswordSpecificsData& password_specifics,
     const autofill::PasswordForm& password_form,
     autofill::PasswordForm* new_password_form) {
   if (password_specifics.scheme() == password_form.scheme &&
       password_form.signon_realm == password_specifics.signon_realm() &&
       password_form.origin.spec() == password_specifics.origin() &&
       password_form.action.spec() == password_specifics.action() &&
       UTF16ToUTF8(password_form.username_element) ==
           password_specifics.username_element() &&
@@ skipping 10 matching lines @@
       password_specifics.blacklisted() ==
           password_form.blacklisted_by_user) {
     return false;
   }
 
   // If the passwords differ, take the one that was created more recently.
   if (base::Time::FromInternalValue(password_specifics.date_created()) <=
           password_form.date_created) {
     *new_password_form = password_form;
   } else {
-    ExtractPasswordFromSpecifics(password_specifics, new_password_form);
+    PasswordSyncableService::ExtractPasswordFromSpecifics(
+        password_specifics,
+        new_password_form);
   }
 
   return true;
 }
 
 }  // namespace
 
 PasswordSyncableService::PasswordSyncableService(
     scoped_refptr<PasswordStore> password_store)
     : password_store_(password_store) {
 }
 
 PasswordSyncableService::~PasswordSyncableService() {}
 
+void PasswordSyncableService::CreateOrUpdateEntry(
+    const syncer::SyncData& data,
+    PasswordEntryMap* loaded_data,
+    ScopedVector<autofill::PasswordForm>* new_entries,
+    ScopedVector<autofill::PasswordForm>* updated_entries,
+    syncer::SyncChangeList* updated_db_entries) {
+  const sync_pb::EntitySpecifics& specifics = data.GetSpecifics();
+  const sync_pb::PasswordSpecificsData& password_specifics(
+      specifics.password().client_only_encrypted_data());
+

[Ilya Sherman, 2013/11/05 08:39:21]

Optional nit: Omit this newline IMO

[lipalani1, 2013/11/19 00:46:54]

Done.

+  std::string tag = MakeTag(password_specifics);
+
+  // Find if the data from sync is already in password store.
+  PasswordEntryMap::iterator it = loaded_data->find(tag);

[Ilya Sherman, 2013/11/05 08:39:21]

nit: Can you find a more descriptive name for this variable than "it"?

[lipalani1, 2013/11/19 00:46:54]

Done.

+

[Ilya Sherman, 2013/11/05 08:39:21]

Optional nit: Omit this newline IMO.

[lipalani1, 2013/11/19 00:46:54]

Done.

+  if (it == loaded_data->end()) {
+    // The sync data is not in password store, so we need to create it in

[Ilya Sherman, 2013/11/05 08:39:21]

nit: "in password store" -> "in the password store" (twice)

[lipalani1, 2013/11/19 00:46:54]

Done.

+    // password store. Add the entry to the new_entries list.
+    autofill::PasswordForm* new_password = new autofill::PasswordForm();

[Ilya Sherman, 2013/11/05 08:39:21]

Please store this in a scoped_ptr, and document ownership transfer by releasing
the scoped_ptr's hold on the memory on line 89 (at least, I'm pretty sure that's
where the ownership transfer happens).  Once you do this, line 88 likely becomes
unnecessary, as it's clear enough from the code itself.

[lipalani1, 2013/11/19 00:46:54]

Done.

+    ExtractPasswordFromSpecifics(password_specifics, new_password);
+    // The new_password pointer is owned by the caller.
+    new_entries->push_back(new_password);
+  } else {
+    // The entry is in password store. If the entries are not identical the
+    // entries need to be merged.
+    scoped_ptr<autofill::PasswordForm> new_password(
+        new autofill::PasswordForm());
+    if (MergeLocalAndSyncPasswords(password_specifics,
+                                   *(it->second),
+                                   new_password.get())) {
+      updated_db_entries->push_back(
+          syncer::SyncChange(FROM_HERE,
+                             syncer::SyncChange::ACTION_UPDATE,
+                             CreateSyncData(*(new_password.get()))));
+
+      updated_entries->push_back(new_password.release());

[Ilya Sherman, 2013/11/05 08:39:21]

Hmm, if I'm reading this code correctly, it looks like both the local DB and the
Sync model are always updated together.  That doesn't really make sense to me
though -- shouldn't only one of them -- the one less up-to-date -- be updated,
while the other remains constant?

[lipalani1, 2013/11/19 00:46:54]

In this case the entries are merged. there could be changes in both. so both
sync and passwords are updated.
On 2013/11/05 08:39:21, Ilya Sherman wrote:

+    }
+    // Remove the entry from the entry map to indicate a match has been found.
+    // Entries that remain in the map at the end of associating all sync entries
+    // will be treated as additions that needs to be propagated to sync.
+    loaded_data->erase(it);
+  }
+}
+
 syncer::SyncMergeResult
 PasswordSyncableService::MergeDataAndStartSyncing(
     syncer::ModelType type,
     const syncer::SyncDataList& initial_sync_data,
     scoped_ptr<syncer::SyncChangeProcessor> sync_processor,
     scoped_ptr<syncer::SyncErrorFactory> sync_error_factory) {
   syncer::SyncMergeResult merge_result(type);
   sync_error_factory_ = sync_error_factory.Pass();
   sync_processor_ = sync_processor.Pass();
 
-  merge_result.set_error(sync_error_factory->CreateAndUploadError(
-      FROM_HERE,
-      "Password Syncable Service Not Implemented."));
+  ScopedVector<autofill::PasswordForm> password_entries;
+  if (!password_store_->FillAutofillableLogins(&(password_entries.get())) ||
+      !password_store_->FillBlacklistLogins(&(password_entries.get()))) {
+    // Password store often fails to load passwords. Track failures with UMA.
+    // (http://crbug.com/249000)
+    UMA_HISTOGRAM_ENUMERATION("Sync.LocalDataFailedToLoad",
+                              ModelTypeToHistogramInt(syncer::PASSWORDS),
+                              syncer::MODEL_TYPE_COUNT);

[Ilya Sherman, 2013/11/05 08:39:21]

Please define a wrapper function for this histogram, so that the call sites look
more like sync::LogLocalDataFailedToLoad(syncer::PASSWORDS).  This reduces the
likelihood of typos creeping in and causing individual codepaths to fail, which
can be a hard bug to spot after the fact.

[lipalani1, 2013/11/19 00:46:54]

Done.

+    merge_result.set_error(sync_error_factory->CreateAndUploadError(
+        FROM_HERE,
+        "Failed to get passwords from store."));
+    return merge_result;;

[Ilya Sherman, 2013/11/05 08:39:21]

nit: Duplicate semicolon.

[lipalani1, 2013/11/19 00:46:54]

Done.

+  }
+
+  PasswordEntryMap new_db_entries;

[Ilya Sherman, 2013/11/05 08:39:21]

nit: "db" -> "database" or "local"

[lipalani1, 2013/11/19 00:46:54]

Done.

+  for (std::vector<autofill::PasswordForm*>::iterator

[Ilya Sherman, 2013/11/05 08:39:21]

nit: Since you do have a typedef for this type, why not use it here?

[lipalani1, 2013/11/19 00:46:54]

Done.

+           it = password_entries.begin();
+       it != password_entries.end();
+       ++it) {
+     // We add all the db entries as |new_db_entries| initially. While
+     // association entries with matching sync entries will be removed and this
+     // list will only contain entries that are not in sync.

[Ilya Sherman, 2013/11/05 08:39:21]

nit: This second sentence is worded a little bit strangely.  Please try to
reword it to be a little clearer.

[lipalani1, 2013/11/19 00:46:54]

Done.

+     new_db_entries[MakeTag(**it)] = *it;

[Ilya Sherman, 2013/11/05 08:39:21]

nit: This line might be easier to read if the previous line gave a name to "*it"
as e.g. "autofill::PasswordForm* password_entry = *it;"

[lipalani1, 2013/11/19 00:46:54]

Done.

+  }
+
+  merge_result.set_num_items_before_association(new_db_entries.size());
+
+  // List that contains the entries that are known only to sync.
+  ScopedVector<autofill::PasswordForm> new_sync_entries;
+
+  // List that contains the entries that are known to both sync and db but
+  // have to be updated in sync.

[Ilya Sherman, 2013/11/05 08:39:21]

Are these entries that need to be updated in Sync, or locally?  Reading through
the code, it kind of looks like these are being updated in the local DB rather
than in Sync.

[lipalani1, 2013/11/19 00:46:54]

Done.

+  ScopedVector<autofill::PasswordForm> updated_sync_entries;

[Ilya Sherman, 2013/11/05 08:39:21]

Thanks, the comments for both this variable and the one above it are very clear
and helpful :)

[lipalani1, 2013/11/19 00:46:54]

Done.

+
+  // Changes from password db that needs to be propagated to sync.
+  syncer::SyncChangeList db_changes;
+  for (syncer::SyncDataList::const_iterator sync_iter =
+           initial_sync_data.begin();
+       sync_iter != initial_sync_data.end(); ++sync_iter) {
+    CreateOrUpdateEntry(*sync_iter,
+                        &new_db_entries,
+                        &new_sync_entries,
+                        &updated_sync_entries,
+                        &db_changes);
+  }
+
+  WriteToPasswordStore(new_sync_entries.get(),
+                       updated_sync_entries.get());
+
+  merge_result.set_num_items_after_association(
+      merge_result.num_items_before_association() + new_sync_entries.size());
+
+  merge_result.set_num_items_added(new_sync_entries.size());

[Ilya Sherman, 2013/11/05 08:39:21]

Out of curiosity, why does the Sync API require clients to call both
set_num_items_after_association() and set_num_items_added()?  Shouldn't either
one be deducible from the other, given that set_num_items_before_association()
was already called?

[lipalani1, 2013/11/19 00:46:54]

these stats will get uploaded to the server. Having a separate field will help
us run queries easily.
On 2013/11/05 08:39:21, Ilya Sherman wrote:

+
+  merge_result.set_num_items_modified(updated_sync_entries.size());
+
+  for (PasswordEntryMap::iterator it = new_db_entries.begin();
+       it != new_db_entries.end();
+       ++it) {
+    db_changes.push_back(syncer::SyncChange(FROM_HERE,
+                              syncer::SyncChange::ACTION_ADD,
+                              CreateSyncData(*(it->second))));
+  }
+
+  merge_result.set_error(
+      sync_processor_->ProcessSyncChanges(FROM_HERE, db_changes));
   return merge_result;
 }
 
 void PasswordSyncableService::StopSyncing(syncer::ModelType type) {
 }
 
 syncer::SyncDataList PasswordSyncableService::GetAllSyncData(
     syncer::ModelType type) const {
   syncer::SyncDataList sync_data;
   return sync_data;
 }
 
 syncer::SyncError PasswordSyncableService::ProcessSyncChanges(
     const tracked_objects::Location& from_here,
     const syncer::SyncChangeList& change_list) {
   syncer::SyncError error(FROM_HERE,
                           syncer::SyncError::UNRECOVERABLE_ERROR,
                           "Password Syncable Service Not Implemented.",
                           syncer::PASSWORDS);
   return error;
 }
 
 void PasswordSyncableService::WriteToPasswordStore(
-    PasswordForms* new_entries,
-    PasswordForms* updated_entries) {
+    const PasswordForms& new_entries,
+    const PasswordForms& updated_entries) {
   for (std::vector<autofill::PasswordForm*>::const_iterator it =
-           new_entries->begin();
-       it != new_entries->end();
+           new_entries.begin();
+       it != new_entries.end();
        ++it) {
     password_store_->AddLoginImpl(**it);
   }
 
   for (std::vector<autofill::PasswordForm*>::const_iterator it =
-           updated_entries->begin();
-       it != updated_entries->end();
+           updated_entries.begin();
+       it != updated_entries.end();
        ++it) {
     password_store_->UpdateLoginImpl(**it);
   }
 
-  if (!new_entries->empty() || !updated_entries->empty()) {
+  if (!new_entries.empty() || !updated_entries.empty()) {
     // We have to notify password store observers of the change by hand since
     // we use internal password store interfaces to make changes synchronously.
-    password_store_->PostNotifyLoginsChanged();
+    NotifyPasswordStore();
   }
 }
 
+void PasswordSyncableService::NotifyPasswordStore() {
+  password_store_->PostNotifyLoginsChanged();
+}
+
 syncer::SyncData PasswordSyncableService::CreateSyncData(
     const autofill::PasswordForm& password_form) {
   sync_pb::EntitySpecifics password_data;
   sync_pb::PasswordSpecificsData* password_specifics =
       password_data.mutable_password()->mutable_client_only_encrypted_data();
   password_specifics->set_scheme(password_form.scheme);
   password_specifics->set_signon_realm(password_form.signon_realm);
   password_specifics->set_origin(password_form.origin.spec());
   password_specifics->set_action(password_form.action.spec());
   password_specifics->set_username_element(
@@ skipping 41 matching lines @@
 // static
 std::string PasswordSyncableService::MakeTag(
     const sync_pb::PasswordSpecificsData& password) {
   return MakeTag(password.origin(),
                  password.username_element(),
                  password.username_value(),
                  password.password_element(),
                  password.signon_realm());
 }
 
+// static
+void PasswordSyncableService::ExtractPasswordFromSpecifics(
+     const sync_pb::PasswordSpecificsData& password,
+     autofill::PasswordForm* new_password) {
+  new_password->scheme =
+      static_cast<autofill::PasswordForm::Scheme>(password.scheme());
+  new_password->signon_realm = password.signon_realm();
+  new_password->origin = GURL(password.origin());
+  new_password->action = GURL(password.action());
+  new_password->username_element =
+      UTF8ToUTF16(password.username_element());
+  new_password->password_element =
+      UTF8ToUTF16(password.password_element());
+  new_password->username_value =
+      UTF8ToUTF16(password.username_value());
+  new_password->password_value =
+      UTF8ToUTF16(password.password_value());
+  new_password->ssl_valid = password.ssl_valid();
+  new_password->preferred = password.preferred();
+  new_password->date_created =
+      base::Time::FromInternalValue(password.date_created());
+  new_password->blacklisted_by_user =
+      password.blacklisted();
+}
+

[Ilya Sherman, 2013/11/05 08:39:21]

nit: Spurious newline.

[lipalani1, 2013/11/19 00:46:54]

Done.