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. 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 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->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 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();

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

I am somewhat uneasy about this one, given this is an instance variable, and not
so easy to reason about.

Sometimes the contents of |forms_| is still used even when |forms_| !=
GNOME_KEYRING_RESULT_OK. Most notably, when it equals
GNOME_KEYRING_RESULT_NO_MATCH.

While it seems that each GKRMethod instance is only used for one Keyring call,
and so |forms_| would always be empty at this point anyway, this may not
necessarily be the case in the future.

If the GKRMethods were, in fact, reused, then a previous GetLoginsList() call,
with a non-empty |forms| vector passed in, could result in that vector being
returned by a subsequent GetLoginsList() or GetAllLogins() call if there are no
results in the Gnome Keyring.

Similarly, if somebody mistakenly calls WaitResult() without the output
parameter, the data is left in |forms_|, and might be returned to the next call
which calls WaitResult(forms) with the output parameter.

So I think that while this should be OK right now, it is a bit fragile, and we
could make it more solid.

I would prefer to have a clear() in the 'else' branch, and/or change the swap
into a Pass() above.

Other than that, we could either introduce the invariant that |forms_| is always
clear after either Wait() method has been called, or the invariant that |forms_|
will always be overwritten by either of the OnOperation...() methods. This would
allow relaxing the requirement that OnOperationDone() should be followed by
WaitResult(), while OnOperationGetList() followed by WaitResult(forms).

[vabr (Chromium), 2015/03/12 15:30:50]

Thanks for thinking this through, I agree that this was fragile.
Here I chose the clear() in the else-branch + the Pass in WaitResult. I did not
go for documenting any of the invariants, to keep the API simple: the meaning of
the methods is clear, and the possibility to shoot oneself in the foot has been
reduced. Introducing promises which the programmer does not have to use if they
use the API correctly, seems to add clutter.

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

Acknowledged.

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