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 144 matching lines @@
   form->avatar_url = GURL(string_attr_map["avatar_url"]);
   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"]);
 
   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,

[vabr (Chromium), 2015/03/09 17:44:15]

The gnome_keyring_found_list_free call was dropped intentionally in
https://codereview.chromium.org/2953006/diff/25001/chrome/browser/password_ma...
some years ago.

It is not clear why it was removed. But the comment was out of date all the
time.

Thus I'm keeping this CL as it is (no longer mentioning the free), and will
check with mdm@ more about why the free was removed.

[engedy, 2015/03/11 19:25:34]

Thanks for checking. As per this offline discussion, removing the comment seems
like the right thing to do.

 // |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.
 // TODO(gcasto): Adding 'form_data' would be nice, but we would need to
 // serialize in a way that is guaranteed to not have any embedded NULLs. Pickle
 // doesn't make this guarantee, so we just don't serialize this field. Since
 // it's only used to crowd source data collection it doesn't matter that much
 // if it's not available on this platform.
 const GnomeKeyringPasswordSchema kGnomeSchema = {
   GNOME_KEYRING_ITEM_GENERIC_SECRET, {
@@ skipping 50 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. Clears |forms_| otherwise.
   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 140 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->swap(forms_);
   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 18 matching lines @@
   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());
   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_);
-}