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 141 matching lines @@
   base::StringToInt64(string_attr_map["date_synced"], &date_synced);
   form->date_synced = base::Time::FromInternalValue(date_synced);
   form->display_name = UTF8ToUTF16(string_attr_map["display_name"]);
   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"];
 
   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,
-// |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) {
+// Returns all the results from |found|. 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.
+ScopedVector<autofill::PasswordForm> ConvertFormList(
+    GList* found,
+    const PasswordForm* lookup_form) {
+  ScopedVector<autofill::PasswordForm> forms;
   password_manager::PSLDomainMatchMetric psl_domain_match_metric =
       password_manager::PSL_DOMAIN_MATCH_NONE;
   for (GList* element = g_list_first(found); element != NULL;
        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 49 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.
+  // GetAllLogins. Replaces the content of |forms| with found logins.
   GnomeKeyringResult WaitResult(ScopedVector<autofill::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 pointed to by
+  // |data|. Saves the 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_;
   // 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_|.
@@ skipping 149 matching lines @@
 GnomeKeyringResult GKRMethod::WaitResult() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
   event_.Wait();
   return result_;
 }
 
 GnomeKeyringResult GKRMethod::WaitResult(
     ScopedVector<autofill::PasswordForm>* forms) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
   event_.Wait();
-  if (forms->empty()) {

[vabr (Chromium), 2015/02/11 17:19:14]

https://codereview.chromium.org/7712024 introduced the requirement that
WaitResult appends (and some of the now failing tests).

I don't think there are any valid reasons to do this any more, but will
double-check, and am highlighting this in any case.

-    // 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(NULL);
   method->event_.Signal();
 }
 
 }  // namespace
 
 NativeBackendGnome::NativeBackendGnome(LocalProfileId id)
     : profile_id_(id) {
   app_string_ = GetProfileSpecificAppString();
 }
@@ skipping 165 matching lines @@
 
 bool NativeBackendGnome::GetBlacklistLogins(
     ScopedVector<autofill::PasswordForm>* forms) {
   return GetLoginsList(false, forms);
 }
 
 bool NativeBackendGnome::GetLoginsList(
     bool autofillable,
     ScopedVector<autofill::PasswordForm>* forms) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
+  forms->clear();
 
   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)
@@ skipping 76 matching lines @@
   }
   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_);
 }