PasswordStore: Clean up expectations about rewriting vectors of forms

chrome/browser/password_manager/native_backend_gnome_x.cc

 // Copyright (c) 2012 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/native_backend_gnome_x.h"
 
 #include <dlfcn.h>
 #include <gnome-keyring.h>
 
 #include <map>
@@ skipping 145 matching lines @@
   form->federation_url = GURL(string_attr_map["federation_url"]);
   form->skip_zero_click = uint_attr_map["skip_zero_click"];
   form->generation_upload_status =
       static_cast<PasswordForm::GenerationUploadStatus>(
           uint_attr_map["generation_upload_status"]);
   DeserializeFormDataFromBase64String(string_attr_map["form_data"],
       &form->form_data);
   return form.Pass();
 }
 
-// Parse all the results from the given GList into a
-// ScopedVector<autofill::PasswordForm>, and free the GList. PasswordForms are
-// allocated on the heap, and should be deleted by the consumer. If not NULL,
+// Converts native credentials in |found| to PasswordForms. If not NULL,
 // |lookup_form| is used to filter out results -- only credentials with signon
 // realms passing the PSL matching against |lookup_form->signon_realm| will be
 // kept. PSL matched results get their signon_realm, origin, and action
 // rewritten to those of |lookup_form_|, with the original signon_realm saved
 // into the result's original_signon_realm data member.
-void ConvertFormList(GList* found,
-                     const PasswordForm* lookup_form,
-                     ScopedVector<autofill::PasswordForm>* forms) {
+ScopedVector<PasswordForm> ConvertFormList(GList* found,
+                                           const PasswordForm* lookup_form) {
+  ScopedVector<PasswordForm> forms;
   password_manager::PSLDomainMatchMetric psl_domain_match_metric =
       password_manager::PSL_DOMAIN_MATCH_NONE;
   for (GList* element = g_list_first(found); element;
        element = g_list_next(element)) {
     GnomeKeyringFound* data = static_cast<GnomeKeyringFound*>(element->data);
     GnomeKeyringAttributeList* attrs = data->attributes;
 
     scoped_ptr<PasswordForm> form(FormFromAttributes(attrs));
     if (form) {
       if (lookup_form && form->signon_realm != lookup_form->signon_realm) {
         // This is not an exact match, we try PSL matching.
         if (lookup_form->scheme != PasswordForm::SCHEME_HTML ||
             form->scheme != PasswordForm::SCHEME_HTML ||
             !(password_manager::IsPublicSuffixDomainMatch(
                 lookup_form->signon_realm, form->signon_realm))) {
           continue;
         }
         psl_domain_match_metric = password_manager::PSL_DOMAIN_MATCH_FOUND;
         form->original_signon_realm = form->signon_realm;
         form->signon_realm = lookup_form->signon_realm;
         form->origin = lookup_form->origin;
         form->action = lookup_form->action;
       }
       if (data->secret) {
         form->password_value = UTF8ToUTF16(data->secret);
       } else {
         LOG(WARNING) << "Unable to access password from list element!";
       }
-      forms->push_back(form.release());
+      forms.push_back(form.release());
     } else {
       LOG(WARNING) << "Could not initialize PasswordForm from attributes!";
     }
   }
   if (lookup_form) {
     const GURL signon_realm(lookup_form->signon_realm);
     std::string registered_domain =
         password_manager::GetRegistryControlledDomain(signon_realm);
     UMA_HISTOGRAM_ENUMERATION(
         "PasswordManager.PslDomainMatchTriggering",
         password_manager::ShouldPSLDomainMatchingApply(registered_domain)
             ? psl_domain_match_metric
             : password_manager::PSL_DOMAIN_MATCH_NOT_USED,
         password_manager::PSL_DOMAIN_MATCH_COUNT);
   }
+  return forms.Pass();
 }
 
 // Schema is analagous to the fields in PasswordForm.
 const GnomeKeyringPasswordSchema kGnomeSchema = {
   GNOME_KEYRING_ITEM_GENERIC_SECRET, {
     { "origin_url", GNOME_KEYRING_ATTRIBUTE_TYPE_STRING },
     { "action_url", GNOME_KEYRING_ATTRIBUTE_TYPE_STRING },
     { "username_element", GNOME_KEYRING_ATTRIBUTE_TYPE_STRING },
     { "username_value", GNOME_KEYRING_ATTRIBUTE_TYPE_STRING },
     { "password_element", GNOME_KEYRING_ATTRIBUTE_TYPE_STRING },
@@ skipping 46 matching lines @@
   void UpdateLoginSearch(const PasswordForm& form, const char* app_string);
   void RemoveLogin(const PasswordForm& form, const char* app_string);
   void GetLogins(const PasswordForm& form, const char* app_string);
   void GetLoginsList(uint32_t blacklisted_by_user, const char* app_string);
   void GetAllLogins(const char* app_string);
 
   // Use after AddLogin, RemoveLogin.
   GnomeKeyringResult WaitResult();
 
   // Use after AddLoginSearch, UpdateLoginSearch, GetLogins, GetLoginsList,
-  // GetAllLogins.
-  GnomeKeyringResult WaitResult(ScopedVector<autofill::PasswordForm>* forms);
+  // GetAllLogins. Replaces the content of |forms| with found logins.
+  GnomeKeyringResult WaitResult(ScopedVector<PasswordForm>* forms);
 
  private:
   struct GnomeKeyringAttributeListFreeDeleter {
     inline void operator()(void* list) const {
       gnome_keyring_attribute_list_free(
           static_cast<GnomeKeyringAttributeList*>(list));
     }
   };
 
   typedef scoped_ptr<GnomeKeyringAttributeList,
                      GnomeKeyringAttributeListFreeDeleter> ScopedAttributeList;
 
   // Helper methods to abbreviate Gnome Keyring long API names.
   static void AppendString(ScopedAttributeList* list,
                            const char* name,
                            const char* value);
   static void AppendString(ScopedAttributeList* list,
                            const char* name,
                            const std::string& value);
   static void AppendUint32(ScopedAttributeList* list,
                            const char* name,
                            guint32 value);
 
   // All these callbacks are called on UI thread.
   static void OnOperationDone(GnomeKeyringResult result, gpointer data);
 
+  // This is marked as static, but acts on the GKRMethod instance that |data|
+  // points to. Saves |result| to |result_|. If the result is OK, overwrites
+  // |forms_| with the found credentials.

[engedy, 2015/03/16 10:35:35]

Thanks, looking good!

nit: Could you please reinstate the comment about clearing?

[vabr (Chromium), 2015/03/16 10:58:47]

Done.

   static void OnOperationGetList(GnomeKeyringResult result, GList* list,
                                  gpointer data);
 
   base::WaitableEvent event_;
   GnomeKeyringResult result_;
-  ScopedVector<autofill::PasswordForm> forms_;
+  ScopedVector<PasswordForm> forms_;
   // If the credential search is specified by a single form and needs to use PSL
   // matching, then the specifying form is stored in |lookup_form_|. If PSL
   // matching is used to find a result, then the results signon realm, origin
   // and action are stored are replaced by those of |lookup_form_|.
   // Additionally, |lookup_form_->signon_realm| is also used to narrow down the
   // found logins to those which indeed PSL-match the look-up. And finally,
   // |lookup_form_| set to NULL means that PSL matching is not required.
   scoped_ptr<PasswordForm> lookup_form_;
 };
 
@@ skipping 143 matching lines @@
                            /*data=*/this,
                            /*destroy_data=*/nullptr);
 }
 
 GnomeKeyringResult GKRMethod::WaitResult() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
   event_.Wait();
   return result_;
 }
 
-GnomeKeyringResult GKRMethod::WaitResult(
-    ScopedVector<autofill::PasswordForm>* forms) {
+GnomeKeyringResult GKRMethod::WaitResult(ScopedVector<PasswordForm>* forms) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
   event_.Wait();
-  if (forms->empty()) {
-    // Normal case. Avoid extra allocation by swapping.
-    forms->swap(forms_);
-  } else {
-    // Rare case. Append forms_ to *forms.
-    forms->insert(forms->end(), forms_.begin(), forms_.end());
-    forms_.weak_clear();
-  }
+  *forms = forms_.Pass();
   return result_;
 }
 
 // static
 void GKRMethod::AppendString(GKRMethod::ScopedAttributeList* list,
                              const char* name,
                              const char* value) {
   gnome_keyring_attribute_list_append_string(list->get(), name, value);
 }
 
@@ skipping 16 matching lines @@
   GKRMethod* method = static_cast<GKRMethod*>(data);
   method->result_ = result;
   method->event_.Signal();
 }
 
 // static
 void GKRMethod::OnOperationGetList(GnomeKeyringResult result, GList* list,
                                    gpointer data) {
   GKRMethod* method = static_cast<GKRMethod*>(data);
   method->result_ = result;
-  method->forms_.clear();
   // |list| will be freed after this callback returns, so convert it now.
-  ConvertFormList(list, method->lookup_form_.get(), &method->forms_);
+  if (result == GNOME_KEYRING_RESULT_OK)
+    method->forms_ = ConvertFormList(list, method->lookup_form_.get());
+  else
+    method->forms_.clear();
   method->lookup_form_.reset();
   method->event_.Signal();
 }
 
+// Generates a profile-specific app string based on profile_id.
+std::string GetProfileSpecificAppString(LocalProfileId profile_id) {
+  // Originally, the application string was always just "chrome" and used only
+  // so that we had *something* to search for since GNOME Keyring won't search
+  // for nothing. Now we use it to distinguish passwords for different profiles.
+  return base::StringPrintf("%s-%d", kGnomeKeyringAppString, profile_id);
+}
+
 }  // namespace
 
 NativeBackendGnome::NativeBackendGnome(LocalProfileId id)
-    : profile_id_(id) {
-  app_string_ = GetProfileSpecificAppString();
+    : profile_id_(id), app_string_(GetProfileSpecificAppString(id)) {
 }
 
 NativeBackendGnome::~NativeBackendGnome() {
 }
 
 bool NativeBackendGnome::Init() {
   return LoadGnomeKeyring() && gnome_keyring_is_available();
 }
 
 bool NativeBackendGnome::RawAddLogin(const PasswordForm& form) {
@@ skipping 18 matching lines @@
   // on origin_url, username_element, username_value, password_element, submit
   // element, and signon_realm first, remove that, and then add the new entry.
   // We'd add the new one first, and then delete the original, but then the
   // delete might actually delete the newly-added entry!
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
   GKRMethod method;
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                           base::Bind(&GKRMethod::AddLoginSearch,
                                      base::Unretained(&method),
                                      form, app_string_.c_str()));
-  ScopedVector<autofill::PasswordForm> forms;
+  ScopedVector<PasswordForm> forms;
   GnomeKeyringResult result = method.WaitResult(&forms);
   if (result != GNOME_KEYRING_RESULT_OK &&
       result != GNOME_KEYRING_RESULT_NO_MATCH) {
     LOG(ERROR) << "Keyring find failed: "
                << gnome_keyring_result_to_message(result);
     return password_manager::PasswordStoreChangeList();
   }
   password_manager::PasswordStoreChangeList changes;
   if (forms.size() > 0) {
     if (forms.size() > 1) {
@@ skipping 23 matching lines @@
   // then add the new entry. We'd add the new one first, and then delete the
   // original, but then the delete might actually delete the newly-added entry!
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
   DCHECK(changes);
   changes->clear();
   GKRMethod method;
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                           base::Bind(&GKRMethod::UpdateLoginSearch,
                                      base::Unretained(&method),
                                      form, app_string_.c_str()));
-  ScopedVector<autofill::PasswordForm> forms;
+  ScopedVector<PasswordForm> forms;
   GnomeKeyringResult result = method.WaitResult(&forms);
   if (result != GNOME_KEYRING_RESULT_OK) {
     LOG(ERROR) << "Keyring find failed: "
                << gnome_keyring_result_to_message(result);
     return false;
   }
 
   bool removed = false;
   for (size_t i = 0; i < forms.size(); ++i) {
     if (*forms[i] != form) {
@@ skipping 39 matching lines @@
       delete_begin, delete_end, CREATION_TIMESTAMP, changes);
 }
 
 bool NativeBackendGnome::RemoveLoginsSyncedBetween(
     base::Time delete_begin,
     base::Time delete_end,
     password_manager::PasswordStoreChangeList* changes) {
   return RemoveLoginsBetween(delete_begin, delete_end, SYNC_TIMESTAMP, changes);
 }
 
-bool NativeBackendGnome::GetLogins(
-    const PasswordForm& form,
-    ScopedVector<autofill::PasswordForm>* forms) {
+bool NativeBackendGnome::GetLogins(const PasswordForm& form,
+                                   ScopedVector<PasswordForm>* forms) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
   GKRMethod method;
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                           base::Bind(&GKRMethod::GetLogins,
                                      base::Unretained(&method),
                                      form, app_string_.c_str()));
   GnomeKeyringResult result = method.WaitResult(forms);
   if (result == GNOME_KEYRING_RESULT_NO_MATCH)
     return true;
   if (result != GNOME_KEYRING_RESULT_OK) {
     LOG(ERROR) << "Keyring find failed: "
                << gnome_keyring_result_to_message(result);
     return false;
   }
   return true;
 }
 
 bool NativeBackendGnome::GetAutofillableLogins(
-    ScopedVector<autofill::PasswordForm>* forms) {
+    ScopedVector<PasswordForm>* forms) {
   return GetLoginsList(true, forms);
 }
 
-bool NativeBackendGnome::GetBlacklistLogins(
-    ScopedVector<autofill::PasswordForm>* forms) {
+bool NativeBackendGnome::GetBlacklistLogins(ScopedVector<PasswordForm>* forms) {
   return GetLoginsList(false, forms);
 }
 
-bool NativeBackendGnome::GetLoginsList(
-    bool autofillable,
-    ScopedVector<autofill::PasswordForm>* forms) {
+bool NativeBackendGnome::GetLoginsList(bool autofillable,
+                                       ScopedVector<PasswordForm>* forms) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
 
   uint32_t blacklisted_by_user = !autofillable;
 
   GKRMethod method;
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                           base::Bind(&GKRMethod::GetLoginsList,
                                      base::Unretained(&method),
                                      blacklisted_by_user, app_string_.c_str()));
   GnomeKeyringResult result = method.WaitResult(forms);
   if (result == GNOME_KEYRING_RESULT_NO_MATCH)
     return true;
   if (result != GNOME_KEYRING_RESULT_OK) {
     LOG(ERROR) << "Keyring find failed: "
                << gnome_keyring_result_to_message(result);
     return false;
   }
   return true;
 }
 
-bool NativeBackendGnome::GetAllLogins(
-    ScopedVector<autofill::PasswordForm>* forms) {
+bool NativeBackendGnome::GetAllLogins(ScopedVector<PasswordForm>* forms) {
   GKRMethod method;
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                           base::Bind(&GKRMethod::GetAllLogins,
                                      base::Unretained(&method),
                                      app_string_.c_str()));
   GnomeKeyringResult result = method.WaitResult(forms);
   if (result == GNOME_KEYRING_RESULT_NO_MATCH)
     return true;
   if (result != GNOME_KEYRING_RESULT_OK) {
     LOG(ERROR) << "Keyring find failed: "
                << gnome_keyring_result_to_message(result);
     return false;
   }
   return true;
 }
 
-bool NativeBackendGnome::GetLoginsBetween(
-    base::Time get_begin,
-    base::Time get_end,
-    TimestampToCompare date_to_compare,
-    ScopedVector<autofill::PasswordForm>* forms) {
+bool NativeBackendGnome::GetLoginsBetween(base::Time get_begin,
+                                          base::Time get_end,
+                                          TimestampToCompare date_to_compare,
+                                          ScopedVector<PasswordForm>* forms) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
+  forms->clear();
   // We could walk the list and add items as we find them, but it is much
   // easier to build the list and then filter the results.
-  ScopedVector<autofill::PasswordForm> all_forms;
+  ScopedVector<PasswordForm> all_forms;
   if (!GetAllLogins(&all_forms))
     return false;
 
-  base::Time autofill::PasswordForm::*date_member =
-      date_to_compare == CREATION_TIMESTAMP
-          ? &autofill::PasswordForm::date_created
-          : &autofill::PasswordForm::date_synced;
+  base::Time PasswordForm::*date_member = date_to_compare == CREATION_TIMESTAMP
+                                              ? &PasswordForm::date_created
+                                              : &PasswordForm::date_synced;
   for (auto& saved_form : all_forms) {
     if (get_begin <= saved_form->*date_member &&
         (get_end.is_null() || saved_form->*date_member < get_end)) {
       forms->push_back(saved_form);
       saved_form = nullptr;
     }
   }
 
   return true;
 }
 
 bool NativeBackendGnome::RemoveLoginsBetween(
     base::Time get_begin,
     base::Time get_end,
     TimestampToCompare date_to_compare,
     password_manager::PasswordStoreChangeList* changes) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
   DCHECK(changes);
   changes->clear();
   // We could walk the list and delete items as we find them, but it is much
   // easier to build the list and use RemoveLogin() to delete them.
-  ScopedVector<autofill::PasswordForm> forms;
+  ScopedVector<PasswordForm> forms;
   if (!GetLoginsBetween(get_begin, get_end, date_to_compare, &forms))
     return false;
 
   bool ok = true;
   for (size_t i = 0; i < forms.size(); ++i) {
     if (RemoveLogin(*forms[i])) {
       changes->push_back(password_manager::PasswordStoreChange(
           password_manager::PasswordStoreChange::REMOVE, *forms[i]));
     } else {
       ok = false;
     }
   }
   return ok;
 }
-
-std::string NativeBackendGnome::GetProfileSpecificAppString() const {
-  // Originally, the application string was always just "chrome" and used only
-  // so that we had *something* to search for since GNOME Keyring won't search
-  // for nothing. Now we use it to distinguish passwords for different profiles.
-  return base::StringPrintf("%s-%d", kGnomeKeyringAppString, profile_id_);
-}