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,
-// |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

[engedy, 2015/02/25 15:17:48]

Phrasing suggestion: Converts native credentials in |found| to PasswordForms...

Nit: We should either mention that this destroys/consumes |found|, or make that
a scoped_ptr.

[vabr (Chromium), 2015/03/09 10:56:18]

Done.
Are you sure this destroys |found|?
To destroy it, gnome_keyring_found_list_free would need to be called, but it
currently is not.
Since we do not observe a leak either, I assume gnome_keyring_find_items keeps
owning |found| when it passes the pointer to it to OnOperationGetList (which, in
turn, calls ConvertFromList).
Alas, the documentation
https://developer.gnome.org/gnome-keyring/stable/gnome-keyring-Search-Functio...
does not say whether that's the case.

[engedy, 2015/03/09 13:33:17]

I am not sure, but the original comment mentioned that the function destroys the
lists. All I wanted is pointing out that we should preserve that comment.

+// 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;
        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.
+  // 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

[engedy, 2015/02/25 15:17:47]

Phrasing suggestion: s/GKRMethod instance pointed to by |data|/GKRMethod
instance that |data| points to/.

[vabr (Chromium), 2015/03/09 10:56:18]

Done.

+  // |data|. Saves the result to |result_|. If the result is OK, overwrites

[engedy, 2015/02/25 15:17:47]

Nit: |result|.

[vabr (Chromium), 2015/03/09 10:56:18]

Done.

+  // |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 150 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/24 18:06:21]

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, and am
highlighting this in any case.

I also removed tests designed just to check the appending, as a result of
removing the appending.

-    // 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();
 }
 
 }  // 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();

[engedy, 2015/02/25 15:17:48]

I think this is not needed, WaitResult will always return what
OnOperationGetList leaves in |forms_|, and that will be cleared on error.

[vabr (Chromium), 2015/03/09 10:56:18]

Done.

 
   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 23 matching lines @@
   }
   return true;
 }
 
 bool NativeBackendGnome::GetLoginsBetween(
     base::Time get_begin,
     base::Time get_end,
     TimestampToCompare date_to_compare,
     ScopedVector<autofill::PasswordForm>* forms) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
   // We could walk the list and add items as we find them, but it is much

[engedy, 2015/02/25 15:17:48]

Need to clear |forms| here.

[vabr (Chromium), 2015/03/09 10:56:18]

Done.

   // easier to build the list and then filter the results.
   ScopedVector<autofill::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;
   for (auto& saved_form : all_forms) {
@@ skipping 32 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_);
 }