PasswordStore: Clean up expectations about rewriting vectors of forms

chrome/browser/password_manager/native_backend_kwallet_x.cc

Index: chrome/browser/password_manager/native_backend_kwallet_x.cc
diff --git a/chrome/browser/password_manager/native_backend_kwallet_x.cc b/chrome/browser/password_manager/native_backend_kwallet_x.cc
index bb3d5f5e249c259b936aad1f745d60fedff0fd18..c7c271a1e85497815089c8cfb8492abd160fc412 100644
--- a/chrome/browser/password_manager/native_backend_kwallet_x.cc
+++ b/chrome/browser/password_manager/native_backend_kwallet_x.cc
@@ -27,6 +27,10 @@ using content::BrowserThread;
 
 namespace {
 
+// In case the fields in the pickle ever change, version them so we can try to
+// read old pickles. (Note: do not eat old pickles past the expiration date.)
+const int kPickleVersion = 6;
+
 // We could localize this string, but then changing your locale would cause
 // you to lose access to all your stored passwords. Maybe best not to do that.
 // Name of the folder to store passwords in.
@@ -100,6 +104,171 @@ void LogDeserializationWarning(int version,
   }
 }
 
+// Deserializes a list of credentials from the wallet to |forms| (replacing
+// the contents of |forms|). |size_32| controls reading the size field within
+// the pickle as 32 bits. We used to use Pickle::WriteSize() to write the number
+// of password forms, but that has a different size on 32- and 64-bit systems.
+// So, now we always write a 64-bit quantity, but we support trying to read it
+// as either size when reading old pickles that fail to deserialize using the
+// native size. Returns true on success.
+bool DeserializeValueSize(const std::string& signon_realm,
+                          const PickleIterator& init_iter,
+                          int version,
+                          bool size_32,
+                          bool warn_only,
+                          ScopedVector<autofill::PasswordForm>* forms) {
+  PickleIterator iter = init_iter;
+
+  size_t count = 0;
+  if (size_32) {
+    uint32_t count_32 = 0;
+    if (!iter.ReadUInt32(&count_32)) {
+      LOG(ERROR) << "Failed to deserialize KWallet entry "
+                 << "(realm: " << signon_realm << ")";
+      return false;
+    }
+    count = count_32;
+  } else {
+    if (!iter.ReadSizeT(&count)) {
+      LOG(ERROR) << "Failed to deserialize KWallet entry "
+                 << "(realm: " << signon_realm << ")";
+      return false;
+    }
+  }
+
+  if (count > 0xFFFF) {
+    // Trying to pin down the cause of http://crbug.com/80728 (or fix it).
+    // This is a very large number of passwords to be saved for a single realm.
+    // It is almost certainly a corrupt pickle and not real data. Ignore it.
+    // This very well might actually be http://crbug.com/107701, so if we're
+    // reading an old pickle, we don't even log this the first time we try to
+    // read it. (That is, when we're reading the native architecture size.)
+    if (!warn_only) {
+      LOG(ERROR) << "Suspiciously large number of entries in KWallet entry "
+                 << "(" << count << "; realm: " << signon_realm << ")";
+    }
+    return false;
+  }
+
+  // We'll swap |converted_forms| with |*forms| on success, to make sure we
+  // don't return partial results on failure.
+  ScopedVector<autofill::PasswordForm> converted_forms;
+  converted_forms.reserve(count);
+  for (size_t i = 0; i < count; ++i) {
+    scoped_ptr<PasswordForm> form(new PasswordForm());
+    form->signon_realm.assign(signon_realm);
+
+    int scheme = 0;
+    int64 date_created = 0;
+    int type = 0;
+    int generation_upload_status = 0;
+    // Note that these will be read back in the order listed due to
+    // short-circuit evaluation. This is important.
+    if (!iter.ReadInt(&scheme) ||
+        !ReadGURL(&iter, warn_only, &form->origin) ||
+        !ReadGURL(&iter, warn_only, &form->action) ||
+        !iter.ReadString16(&form->username_element) ||
+        !iter.ReadString16(&form->username_value) ||
+        !iter.ReadString16(&form->password_element) ||
+        !iter.ReadString16(&form->password_value) ||
+        !iter.ReadString16(&form->submit_element) ||
+        !iter.ReadBool(&form->ssl_valid) ||
+        !iter.ReadBool(&form->preferred) ||
+        !iter.ReadBool(&form->blacklisted_by_user) ||
+        !iter.ReadInt64(&date_created)) {
+      LogDeserializationWarning(version, signon_realm, warn_only);
+      return false;
+    }
+    form->scheme = static_cast<PasswordForm::Scheme>(scheme);
+
+    if (version > 1) {
+      if (!iter.ReadInt(&type) ||
+          !iter.ReadInt(&form->times_used) ||
+          !autofill::DeserializeFormData(&iter, &form->form_data)) {
+        LogDeserializationWarning(version, signon_realm, false);
+        return false;
+      }
+      form->type = static_cast<PasswordForm::Type>(type);
+    }
+
+    if (version > 2) {
+      int64 date_synced = 0;
+      if (!iter.ReadInt64(&date_synced)) {
+        LogDeserializationWarning(version, signon_realm, false);
+        return false;
+      }
+      form->date_synced = base::Time::FromInternalValue(date_synced);
+    }
+
+    if (version > 3) {
+      if (!iter.ReadString16(&form->display_name) ||
+          !ReadGURL(&iter, warn_only, &form->avatar_url) ||
+          !ReadGURL(&iter, warn_only, &form->federation_url) ||
+          !iter.ReadBool(&form->skip_zero_click)) {
+        LogDeserializationWarning(version, signon_realm, false);
+        return false;
+      }
+    }
+
+    if (version > 4) {
+      form->date_created = base::Time::FromInternalValue(date_created);
+    } else {
+      form->date_created = base::Time::FromTimeT(date_created);
+    }
+
+    if (version > 5) {
+      if (!iter.ReadInt(&generation_upload_status)) {
+        LogDeserializationWarning(version, signon_realm, false);
+      }
+      form->generation_upload_status =
+          static_cast<PasswordForm::GenerationUploadStatus>(
+              generation_upload_status);
+    }
+
+    converted_forms.push_back(form.release());
+  }
+
+  forms->swap(converted_forms);
+  return true;
+}
+
+// Serializes a list of PasswordForms to be stored in the wallet.
+void SerializeValue(const std::vector<autofill::PasswordForm*>& forms,
+                    Pickle* pickle) {
+  pickle->WriteInt(kPickleVersion);
+  pickle->WriteSizeT(forms.size());
+  for (autofill::PasswordForm* form : forms) {
+    pickle->WriteInt(form->scheme);
+    pickle->WriteString(form->origin.spec());
+    pickle->WriteString(form->action.spec());
+    pickle->WriteString16(form->username_element);
+    pickle->WriteString16(form->username_value);
+    pickle->WriteString16(form->password_element);
+    pickle->WriteString16(form->password_value);
+    pickle->WriteString16(form->submit_element);
+    pickle->WriteBool(form->ssl_valid);
+    pickle->WriteBool(form->preferred);
+    pickle->WriteBool(form->blacklisted_by_user);
+    pickle->WriteInt64(form->date_created.ToInternalValue());
+    pickle->WriteInt(form->type);
+    pickle->WriteInt(form->times_used);
+    autofill::SerializeFormData(form->form_data, pickle);
+    pickle->WriteInt64(form->date_synced.ToInternalValue());
+    pickle->WriteString16(form->display_name);
+    pickle->WriteString(form->avatar_url.spec());
+    pickle->WriteString(form->federation_url.spec());
+    pickle->WriteBool(form->skip_zero_click);
+  }
+}
+
+// Moves the content of |second| to the end of |first|.
+void AppendSecondToFirst(ScopedVector<autofill::PasswordForm>* first,
+                         ScopedVector<autofill::PasswordForm> second) {
+  first->reserve(first->size() + second.size());
+  first->insert(first->end(), second.begin(), second.end());
+  second.weak_clear();
+}
+
 }  // namespace
 
 NativeBackendKWallet::NativeBackendKWallet(LocalProfileId id)
@@ -272,7 +441,8 @@ password_manager::PasswordStoreChangeList NativeBackendKWallet::AddLogin(
     return password_manager::PasswordStoreChangeList();
 
   ScopedVector<autofill::PasswordForm> forms;
-  GetLoginsList(form.signon_realm, wallet_handle, &forms);
+  if (!GetLoginsList(form.signon_realm, wallet_handle, &forms))
+    return password_manager::PasswordStoreChangeList();
 
   // We search for a login to update, rather than unconditionally appending the
   // login, because in some cases (especially involving sync) we can be asked to
@@ -311,7 +481,8 @@ bool NativeBackendKWallet::UpdateLogin(
     return false;
 
   ScopedVector<autofill::PasswordForm> forms;
-  GetLoginsList(form.signon_realm, wallet_handle, &forms);
+  if (!GetLoginsList(form.signon_realm, wallet_handle, &forms))
+    return false;
 
   bool updated = false;
   for (size_t i = 0; i < forms.size(); ++i) {
@@ -338,7 +509,8 @@ bool NativeBackendKWallet::RemoveLogin(const PasswordForm& form) {
     return false;
 
   ScopedVector<autofill::PasswordForm> all_forms;
-  GetLoginsList(form.signon_realm, wallet_handle, &all_forms);
+  if (!GetLoginsList(form.signon_realm, wallet_handle, &all_forms))
+    return false;
 
   ScopedVector<autofill::PasswordForm> kept_forms;
   kept_forms.reserve(all_forms.size());
@@ -382,7 +554,7 @@ bool NativeBackendKWallet::GetAutofillableLogins(
   int wallet_handle = WalletHandle();
   if (wallet_handle == kInvalidKWalletHandle)
     return false;
-  return GetLoginsList(true, wallet_handle, forms);
+  return GetLoginsList(BlacklistOptions::AUTOFILLABLE, wallet_handle, forms);
 }
 
 bool NativeBackendKWallet::GetBlacklistLogins(
@@ -390,7 +562,7 @@ bool NativeBackendKWallet::GetBlacklistLogins(
   int wallet_handle = WalletHandle();
   if (wallet_handle == kInvalidKWalletHandle)
     return false;
-  return GetLoginsList(false, wallet_handle, forms);
+  return GetLoginsList(BlacklistOptions::BLACKLISTED, wallet_handle, forms);
 }
 
 bool NativeBackendKWallet::GetLoginsList(
@@ -459,24 +631,26 @@ bool NativeBackendKWallet::GetLoginsList(
 
     // Can't we all just agree on whether bytes are signed or not? Please?
     Pickle pickle(reinterpret_cast<const char*>(bytes), length);
-    DeserializeValue(signon_realm, pickle, forms);
+    *forms = DeserializeValue(signon_realm, pickle);
   }
 
   return true;
 }
 
 bool NativeBackendKWallet::GetLoginsList(
-    bool autofillable,
+    BlacklistOptions options,
     int wallet_handle,
     ScopedVector<autofill::PasswordForm>* forms) {
+  forms->clear();
   ScopedVector<autofill::PasswordForm> all_forms;
   if (!GetAllLogins(wallet_handle, &all_forms))
     return false;
 
   // We have to read all the entries, and then filter them here.
-  forms->reserve(forms->size() + all_forms.size());
+  forms->reserve(all_forms.size());
   for (auto& saved_form : all_forms) {
-    if (saved_form->blacklisted_by_user == !autofillable) {
+    if (saved_form->blacklisted_by_user ==
+        (options == BlacklistOptions::BLACKLISTED)) {
       forms->push_back(saved_form);
       saved_form = nullptr;
     }
@@ -511,8 +685,10 @@ bool NativeBackendKWallet::GetAllLogins(
     }
   }
 
-  for (size_t i = 0; i < realm_list.size(); ++i) {
-    const std::string& signon_realm = realm_list[i];
+  // Swapping |collected_forms| with |*forms| just before "return true" makes

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

nit: Is this still needed with the new semantics?

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

The swapping is not the only way to implement this. An alternative would be to
forms->clear() and push_back() to them on the go.

And that alternative is actually shorter and will probably raise fewer confused
questions by anybody reading this, so I'll switch to that. :)

Done.

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

Acknowledged.

+  // sure partial results are not returned on failure.
+  ScopedVector<autofill::PasswordForm> collected_forms;
+  for (const std::string& signon_realm : realm_list) {
     dbus::MethodCall method_call(kKWalletInterface, "readEntry");
     dbus::MessageWriter builder(&method_call);
     builder.AppendInt32(wallet_handle);  // handle
@@ -539,8 +715,10 @@ bool NativeBackendKWallet::GetAllLogins(
 
     // Can't we all just agree on whether bytes are signed or not? Please?
     Pickle pickle(reinterpret_cast<const char*>(bytes), length);
-    DeserializeValue(signon_realm, pickle, forms);
+    AppendSecondToFirst(&collected_forms,
+                        DeserializeValue(signon_realm, pickle));
   }
+  forms->swap(collected_forms);
   return true;
 }
 
@@ -676,8 +854,8 @@ bool NativeBackendKWallet::RemoveLoginsBetween(
 
     // Can't we all just agree on whether bytes are signed or not? Please?
     Pickle pickle(reinterpret_cast<const char*>(bytes), length);
-    ScopedVector<autofill::PasswordForm> all_forms;
-    DeserializeValue(signon_realm, pickle, &all_forms);
+    ScopedVector<autofill::PasswordForm> all_forms =
+        DeserializeValue(signon_realm, pickle);
 
     ScopedVector<autofill::PasswordForm> kept_forms;
     kept_forms.reserve(all_forms.size());
@@ -705,160 +883,9 @@ bool NativeBackendKWallet::RemoveLoginsBetween(
 }
 
 // static
-void NativeBackendKWallet::SerializeValue(
-    const std::vector<autofill::PasswordForm*>& forms,
-    Pickle* pickle) {
-  pickle->WriteInt(kPickleVersion);
-  pickle->WriteSizeT(forms.size());
-  for (autofill::PasswordForm* form : forms) {
-    pickle->WriteInt(form->scheme);
-    pickle->WriteString(form->origin.spec());
-    pickle->WriteString(form->action.spec());
-    pickle->WriteString16(form->username_element);
-    pickle->WriteString16(form->username_value);
-    pickle->WriteString16(form->password_element);
-    pickle->WriteString16(form->password_value);
-    pickle->WriteString16(form->submit_element);
-    pickle->WriteBool(form->ssl_valid);
-    pickle->WriteBool(form->preferred);
-    pickle->WriteBool(form->blacklisted_by_user);
-    pickle->WriteInt64(form->date_created.ToInternalValue());
-    pickle->WriteInt(form->type);
-    pickle->WriteInt(form->times_used);
-    autofill::SerializeFormData(form->form_data, pickle);
-    pickle->WriteInt64(form->date_synced.ToInternalValue());
-    pickle->WriteString16(form->display_name);
-    pickle->WriteString(form->avatar_url.spec());
-    pickle->WriteString(form->federation_url.spec());
-    pickle->WriteBool(form->skip_zero_click);
-    pickle->WriteInt(form->generation_upload_status);
-  }
-}
-
-// static
-bool NativeBackendKWallet::DeserializeValueSize(
-    const std::string& signon_realm,
-    const PickleIterator& init_iter,
-    int version,
-    bool size_32,
-    bool warn_only,
-    ScopedVector<autofill::PasswordForm>* forms) {
-  PickleIterator iter = init_iter;
-
-  size_t count = 0;
-  if (size_32) {
-    uint32_t count_32 = 0;
-    if (!iter.ReadUInt32(&count_32)) {
-      LOG(ERROR) << "Failed to deserialize KWallet entry "
-                 << "(realm: " << signon_realm << ")";
-      return false;
-    }
-    count = count_32;
-  } else {
-    if (!iter.ReadSizeT(&count)) {
-      LOG(ERROR) << "Failed to deserialize KWallet entry "
-                 << "(realm: " << signon_realm << ")";
-      return false;
-    }
-  }
-
-  if (count > 0xFFFF) {
-    // Trying to pin down the cause of http://crbug.com/80728 (or fix it).
-    // This is a very large number of passwords to be saved for a single realm.
-    // It is almost certainly a corrupt pickle and not real data. Ignore it.
-    // This very well might actually be http://crbug.com/107701, so if we're
-    // reading an old pickle, we don't even log this the first time we try to
-    // read it. (That is, when we're reading the native architecture size.)
-    if (!warn_only) {
-      LOG(ERROR) << "Suspiciously large number of entries in KWallet entry "
-                 << "(" << count << "; realm: " << signon_realm << ")";
-    }
-    return false;
-  }
-
-  forms->reserve(forms->size() + count);
-  for (size_t i = 0; i < count; ++i) {
-    scoped_ptr<PasswordForm> form(new PasswordForm());
-    form->signon_realm.assign(signon_realm);
-
-    int scheme = 0;
-    int64 date_created = 0;
-    int type = 0;
-    int generation_upload_status = 0;
-    // Note that these will be read back in the order listed due to
-    // short-circuit evaluation. This is important.
-    if (!iter.ReadInt(&scheme) ||
-        !ReadGURL(&iter, warn_only, &form->origin) ||
-        !ReadGURL(&iter, warn_only, &form->action) ||
-        !iter.ReadString16(&form->username_element) ||
-        !iter.ReadString16(&form->username_value) ||
-        !iter.ReadString16(&form->password_element) ||
-        !iter.ReadString16(&form->password_value) ||
-        !iter.ReadString16(&form->submit_element) ||
-        !iter.ReadBool(&form->ssl_valid) ||
-        !iter.ReadBool(&form->preferred) ||
-        !iter.ReadBool(&form->blacklisted_by_user) ||
-        !iter.ReadInt64(&date_created)) {
-      LogDeserializationWarning(version, signon_realm, warn_only);
-      return false;
-    }
-    form->scheme = static_cast<PasswordForm::Scheme>(scheme);
-
-    if (version > 1) {
-      if (!iter.ReadInt(&type) ||
-          !iter.ReadInt(&form->times_used) ||
-          !autofill::DeserializeFormData(&iter, &form->form_data)) {
-        LogDeserializationWarning(version, signon_realm, false);
-        return false;
-      }
-      form->type = static_cast<PasswordForm::Type>(type);
-    }
-
-    if (version > 2) {
-      int64 date_synced = 0;
-      if (!iter.ReadInt64(&date_synced)) {
-        LogDeserializationWarning(version, signon_realm, false);
-        return false;
-      }
-      form->date_synced = base::Time::FromInternalValue(date_synced);
-    }
-
-    if (version > 3) {
-      if (!iter.ReadString16(&form->display_name) ||
-          !ReadGURL(&iter, warn_only, &form->avatar_url) ||
-          !ReadGURL(&iter, warn_only, &form->federation_url) ||
-          !iter.ReadBool(&form->skip_zero_click)) {
-        LogDeserializationWarning(version, signon_realm, false);
-        return false;
-      }
-    }
-
-    if (version > 4) {
-      form->date_created = base::Time::FromInternalValue(date_created);
-    } else {
-      form->date_created = base::Time::FromTimeT(date_created);
-    }
-
-    if (version > 5) {
-      if (!iter.ReadInt(&generation_upload_status)) {
-        LogDeserializationWarning(version, signon_realm, false);
-      }
-      form->generation_upload_status =
-          static_cast<PasswordForm::GenerationUploadStatus>(
-              generation_upload_status);
-    }
-
-    forms->push_back(form.release());
-  }
-
-  return true;
-}
-
-// static
-void NativeBackendKWallet::DeserializeValue(
+ScopedVector<autofill::PasswordForm> NativeBackendKWallet::DeserializeValue(
     const std::string& signon_realm,
-    const Pickle& pickle,
-    ScopedVector<autofill::PasswordForm>* forms) {
+    const Pickle& pickle) {
   PickleIterator iter(pickle);
 
   int version = -1;
@@ -866,26 +893,26 @@ void NativeBackendKWallet::DeserializeValue(
       version < 0 || version > kPickleVersion) {
     LOG(ERROR) << "Failed to deserialize KWallet entry "
                << "(realm: " << signon_realm << ")";
-    return;
+    return ScopedVector<autofill::PasswordForm>();
   }
 
+  ScopedVector<autofill::PasswordForm> forms;
   if (version > 0) {
     // In current pickles, we expect 64-bit sizes. Failure is an error.
-    DeserializeValueSize(signon_realm, iter, version, false, false, forms);
-    return;
+    DeserializeValueSize(signon_realm, iter, version, false, false, &forms);
+    return forms.Pass();
   }
 
-  const size_t saved_forms_size = forms->size();
   const bool size_32 = sizeof(size_t) == sizeof(uint32_t);
   if (!DeserializeValueSize(
-          signon_realm, iter, version, size_32, true, forms)) {
+          signon_realm, iter, version, size_32, true, &forms)) {
     // We failed to read the pickle using the native architecture of the system.
     // Try again with the opposite architecture. Note that we do this even on
     // 32-bit machines, in case we're reading a 64-bit pickle. (Probably rare,
     // since mostly we expect upgrades, not downgrades, but both are possible.)
-    forms->resize(saved_forms_size);
-    DeserializeValueSize(signon_realm, iter, version, !size_32, false, forms);
+    DeserializeValueSize(signon_realm, iter, version, !size_32, false, &forms);
   }
+  return forms.Pass();
 }
 
 int NativeBackendKWallet::WalletHandle() {