PasswordStore: Clean up expectations about rewriting vectors of forms

PasswordStore: Clean up expectations about rewriting vectors of forms

Many methods in PasswordStore and its backends have a ScopedVector of password forms as an out-argument, into which they put retrieved credentials.
Ideally, those methods would return that vector instead, but that is often not possible, because they already return a bool flag of success.
Keeping the out-argument has one bad consequence: it is not clear, if the retrieved credentials are appended to the vector, or if they replace the contents of the vector.

It looks like those methods are fed empty vectors anyway, so it is natural to make that explicit. This CL adds comments stating that the vectors are erased before anything new is added to them, and also modifies the code to make sure that they are indeed erased, even in failure cases.

The CL also contains some related clean-ups, like moving static methods to anonymous namespace etc.

BUG=324291
Committed: https://crrev.com/c5c4d77323e8bff2028f9e43f52f0be4c3a93a77
Cr-Commit-Position: refs/heads/master@{#320721}

[vabr (Chromium), 2015-02-11 16:27:48]

Patchset #1 (id:1) has been deleted

[vabr (Chromium), 2015-02-11 16:39:33]

vabr@chromium.org changed reviewers:
+ engedy@chromium.org

[vabr (Chromium), 2015-02-11 16:39:33]

Hi Balázs,

When you have time, please have a look.

Thanks!
Vaclav

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

Balázs,

To spare your time -- no need to look at this CL yet. I'll deal with the failing
tests first, and then ping you again.

Cheers,
Vacalv

https://codereview.chromium.org/906973007/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_gnome_x.cc (left):

https://codereview.chromium.org/906973007/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:483: if
(forms->empty()) {
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.

[engedy, 2015-02-12 16:37:43]

Let me know as soon as I can take a look at this. Seems like a very good
activity while WFB.

[vabr (Chromium), 2015-02-12 18:56:18]

On 2015/02/12 16:37:43, engedy wrote:
> Let me know as soon as I can take a look at this. Seems like a very good
> activity while WFB.

Thanks, Balázs, I will let you know.
Today I realised 70% of Canary crashes are due to my recent refactorings, so I
had a rather busy day. :) Hopefully I'll do something tomorrow.

Cheers,
Vaclav

[vabr (Chromium), 2015-02-24 18:06:21]

Hi Balázs,

Patch set 4 should be OK to have a look at, PTAL. No need to rush, this is not
urgent.

I should note that I saw this presubmit warning:
** Presubmit Warnings **
Some UMA_HISTOGRAM lines have been modified and the associated histogram name
has no match in either metrics/histograms.xml or the modifications of it:
   [components/password_manager/core/browser/login_database.cc:933]
PasswordManager.PslDomainMatchTriggering

This seems to be a false positive, and I reported it in
http://crbug.com/445265#c6.

Cheers,
Vaclav

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_gnome_x.cc (left):

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:488: if
(forms->empty()) {
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.

[engedy, 2015-02-25 15:17:49]

As discussed offline, clearing the ScopedVector output parameter in error cases
seems like quite a hassle. I think it may be better to not care about it in the
implementation, and require call sites not use the output in case of an error.
(Which seems to be customary in other Chromium code.)

There are two risk scenarios:

 1.) Having an implementation forgetting to reset the output parameter while
call sites rely on it. (If we require that implementations reset the output
parameter.)

 2.) Having a call site incorrectly using the result even on failure. (If we do
not require that implementations reset the output parameter.)

As demonstrated by this very CL, type #1 bugs can be hard to notice.

I think it is a lot easier/customary to spot (2.) in a code reviews, and this
solution requires less comments as well, as this seems to be the customary
behavior in Chromium code. We can still clear the results in cases where we
absolutely do not want to leak data, as the method contract would allow (but not
require) that, but this would be up to the implementation.

One other thing we could do is to add WARN_UNUSED_RESULT for all these methods.

PS: If we do make this change, we need to update
PasswordStoreX::FillMatchingLogins().

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:165: // Returns all
the results from |found|. If not NULL, |lookup_form| is used to
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.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:318: // This is marked
as static, but acts on the GKRMethod instance pointed to by
Phrasing suggestion: s/GKRMethod instance pointed to by |data|/GKRMethod
instance that |data| points to/.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:319: // |data|. Saves
the result to |result_|. If the result is OK, overwrites
Nit: |result|.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:717: forms->clear();
I think this is not needed, WaitResult will always return what
OnOperationGetList leaves in |forms_|, and that will be cleared on error.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:761: // We could walk
the list and add items as we find them, but it is much
Need to clear |forms| here.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_gnome_x.h (right):

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.h:124: // |autofillable|
being false or true, credentials from the keyring. In case
Phrasing suggestion (just an idea, I do not feel strongly about this): 

Retrieves all autofillable or all blacklisted credentials (depending on
|autofillable|) from the keyring into |forms|, overwriting the original contents
of |forms|. Returns true on success; returns false and erases |forms| on
failure.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_kwallet_x.cc (right):

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_kwallet_x.cc:108: // |forms|
contents). |size_32| controls reading the size field within the
Nit: the contents of |forms|

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_kwallet_x.cc:267: void
AppendSecondToFirst(ScopedVector<autofill::PasswordForm>* first,
In the long run, we should contribute such a method to ScopedVector itself.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_kwallet_x.cc:545: int
wallet_handle = WalletHandle();
Need to clear |forms| here (or above this line). 3 occurrences.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_kwallet_x.h (right):

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_kwallet_x.h:70: // Deserializes a
list of PasswordForms from the wallet. Protected for tests.
Nit: What does "protected for tests" mean?

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_libsecret.cc (right):

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_libsecret.cc:505:
ScopedVector<autofill::PasswordForm> all_forms;
Need to wipe |forms| here.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_libsecret.h (right):

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_libsecret.h:103: // Overwrites
|forms| with credentials matching |options| from the keyring. If
Phrasing suggestion (just an idea, I do not feel strongly about this):

Retrieves credentials matching |options| from the keyring into |forms|,
overwriting the original contents of |forms|. If |lookup_form| is not NULL, only
retrieves credentials PSL-matching it. Returns true on success; false on
failure, in which case |forms| gets erased.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_libsecret.h:112: bool
GetLoginsBetween(base::Time get_begin,
Need to elaborate on the fate of |forms|.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_libsecret.h:130: static
std::string GetProfileSpecificAppString(LocalProfileId id);
I think we can only move this to an anonymous namespace.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/password_store_mac.cc (right):

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_store_mac.cc:1148:
DCHECK(thread_->message_loop() == base::MessageLoop::current());
Need to clear |forms| above here.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/password_store_x.cc (right):

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_store_x.cc:157: void
PasswordStoreX::GetAutofillableLoginsImpl(
Do we override the base class implementation for this and below only so that the
results get sorted?

Given that the Fill[...]() methods are only called (once?) by the Sync backend,
I think we could add the sorting there without much performance impact, and use
the base class implementations for these
Get{Autofillable|Blacklisted}LoginsImpl?

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_store_x.cc:198: CheckMigration();
Need to clear |forms| here.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_store_x.cc:212: CheckMigration();
Need to clear |forms| here.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/password_store_x.h (right):

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_store_x.h:65: virtual bool
GetLogins(const autofill::PasswordForm& form,
The FailingBackend in password_store_x_unittest.cc should clear |forms| too. (3
methods.)

https://codereview.chromium.org/906973007/diff/80001/components/password_mana...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/906973007/diff/80001/components/password_mana...
components/password_manager/core/browser/login_database.cc:904: return false;
Need to clear |forms| before this line. (Or remove the declaration comment to
this effect, it seems to be done by all call sites.)

https://codereview.chromium.org/906973007/diff/80001/components/password_mana...
File components/password_manager/core/browser/login_database.h (right):

https://codereview.chromium.org/906973007/diff/80001/components/password_mana...
components/password_manager/core/browser/login_database.h:80: // Get all logins
created from |begin| onwards (inclusive) and before |end|.
Nit: 'Gets' or 'Retrieves'. (4 occurrences)

https://codereview.chromium.org/906973007/diff/80001/components/password_mana...
File components/password_manager/core/browser/password_store_sync.h (right):

https://codereview.chromium.org/906973007/diff/80001/components/password_mana...
components/password_manager/core/browser/password_store_sync.h:20: // returns
true, otherwise returns false and erases |forms|.
Nit: s/,/;/

https://codereview.chromium.org/906973007/diff/80001/components/password_mana...
components/password_manager/core/browser/password_store_sync.h:21: virtual bool
FillAutofillableLogins(
We also need to update the implementation in PasswordStoreDefault to make sure
it erases form in error cases. (Also for blacklisted.)

https://codereview.chromium.org/906973007/diff/80001/components/password_mana...
components/password_manager/core/browser/password_store_sync.h:25: // returns
true, otherwise returns false and erases |forms|.
Nit: s/,/;/

[engedy, 2015-02-25 15:18:49]

Also, we will need to update PasswordSyncableService::ReadFromPasswordStore(),
which seems to rely on append behavior for Fill{Autofillable|Blacklisted}Logins.

[vabr (Chromium), 2015-03-09 10:56:19]

Hi Balázs,

I addressed your comments, including

> PS: If we do make this change, we need to update
PasswordStoreX::FillMatchingLogins().

and

> Also, we will need to update PasswordSyncableService::ReadFromPasswordStore(),
> which seems to rely on append behavior for
Fill{Autofillable|Blacklisted}Logins.
(Thanks for spotting that, it would be a disaster.)

The very first comment below I did not address, because I was not clear if that
would be correct, please see my response there.

When dealing with the guarantee of the ScopedVector being cleared or not on
failure, I went with not promising anything in case of failure, the caller not
relying on the vector being cleared on failure, and the calee still clearing the
form even in the case of failure. While the last think might be masking issues
where the caller would rely on the vector being cleared on success, not clearing
it could mask the issues, where the caller falsely expects the appending
behaviour. So I chose mostly randomly to clear the vector always.

PTAL.

Cheers,
Vaclav

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:165: // Returns all
the results from |found|. If not NULL, |lookup_form| is used to
On 2015/02/25 15:17:48, engedy wrote:
> Phrasing suggestion: Converts native credentials in |found| to
PasswordForms...
Done.
> 
> Nit: We should either mention that this destroys/consumes |found|, or make
that
> a scoped_ptr.

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.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:318: // This is marked
as static, but acts on the GKRMethod instance pointed to by
On 2015/02/25 15:17:47, engedy wrote:
> Phrasing suggestion: s/GKRMethod instance pointed to by |data|/GKRMethod
> instance that |data| points to/.

Done.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:319: // |data|. Saves
the result to |result_|. If the result is OK, overwrites
On 2015/02/25 15:17:47, engedy wrote:
> Nit: |result|.

Done.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:717: forms->clear();
On 2015/02/25 15:17:48, engedy wrote:
> I think this is not needed, WaitResult will always return what
> OnOperationGetList leaves in |forms_|, and that will be cleared on error.

Done.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:761: // We could walk
the list and add items as we find them, but it is much
On 2015/02/25 15:17:48, engedy wrote:
> Need to clear |forms| here.

Done.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_gnome_x.h (right):

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.h:124: // |autofillable|
being false or true, credentials from the keyring. In case
On 2015/02/25 15:17:48, engedy wrote:
> Phrasing suggestion (just an idea, I do not feel strongly about this): 
> 
> Retrieves all autofillable or all blacklisted credentials (depending on
> |autofillable|) from the keyring into |forms|, overwriting the original
contents
> of |forms|. Returns true on success; returns false and erases |forms| on
> failure.

Done, with adjustment to not caring about |forms| on failure.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_kwallet_x.cc (right):

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_kwallet_x.cc:108: // |forms|
contents). |size_32| controls reading the size field within the
On 2015/02/25 15:17:48, engedy wrote:
> Nit: the contents of |forms|

Done.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_kwallet_x.cc:267: void
AppendSecondToFirst(ScopedVector<autofill::PasswordForm>* first,
On 2015/02/25 15:17:48, engedy wrote:
> In the long run, we should contribute such a method to ScopedVector itself.

Sadly, cannot be done:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/cDHsauJfP5c/zWyyS...

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_kwallet_x.cc:545: int
wallet_handle = WalletHandle();
On 2015/02/25 15:17:48, engedy wrote:
> Need to clear |forms| here (or above this line). 3 occurrences.

Done.

Since I removed the comment guarantees about behaviour in case of failure, it is
strictly not necessary any more, but still feels like a good thing to do. And
should not cost significant performance, because |forms| should be empty to
start with.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_kwallet_x.h (right):

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_kwallet_x.h:70: // Deserializes a
list of PasswordForms from the wallet. Protected for tests.
On 2015/02/25 15:17:48, engedy wrote:
> Nit: What does "protected for tests" mean?

It meant: "Protected, instead of private, so that tests can call it directly."
But that's about the only reason anything here is protected, so I dropped that
sentence again.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_libsecret.cc (right):

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_libsecret.cc:505:
ScopedVector<autofill::PasswordForm> all_forms;
On 2015/02/25 15:17:48, engedy wrote:
> Need to wipe |forms| here.

Done.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_libsecret.h (right):

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_libsecret.h:103: // Overwrites
|forms| with credentials matching |options| from the keyring. If
On 2015/02/25 15:17:48, engedy wrote:
> Phrasing suggestion (just an idea, I do not feel strongly about this):
> 
> Retrieves credentials matching |options| from the keyring into |forms|,
> overwriting the original contents of |forms|. If |lookup_form| is not NULL,
only
> retrieves credentials PSL-matching it. Returns true on success; false on
> failure, in which case |forms| gets erased.

Done.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_libsecret.h:112: bool
GetLoginsBetween(base::Time get_begin,
On 2015/02/25 15:17:48, engedy wrote:
> Need to elaborate on the fate of |forms|.

Done.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_libsecret.h:130: static
std::string GetProfileSpecificAppString(LocalProfileId id);
On 2015/02/25 15:17:48, engedy wrote:
> I think we can only move this to an anonymous namespace.

Done. Also for the Gnome backend.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/password_store_mac.cc (right):

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_store_mac.cc:1148:
DCHECK(thread_->message_loop() == base::MessageLoop::current());
On 2015/02/25 15:17:48, engedy wrote:
> Need to clear |forms| above here.

Done.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/password_store_x.cc (right):

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_store_x.cc:157: void
PasswordStoreX::GetAutofillableLoginsImpl(
On 2015/02/25 15:17:48, engedy wrote:
> Do we override the base class implementation for this and below only so that
the
> results get sorted?
> 
> Given that the Fill[...]() methods are only called (once?) by the Sync
backend,
> I think we could add the sorting there without much performance impact, and
use
> the base class implementations for these
> Get{Autofillable|Blacklisted}LoginsImpl?

This sounds very reasonable.
I propose to do it in a separate CL, though. If there are issues I'm not seeing
yet, or the performance hits us or something, it would be easier to revert. The
CL is at https://codereview.chromium.org/986983002/.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_store_x.cc:198: CheckMigration();
On 2015/02/25 15:17:48, engedy wrote:
> Need to clear |forms| here.

Done.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_store_x.cc:212: CheckMigration();
On 2015/02/25 15:17:48, engedy wrote:
> Need to clear |forms| here.

Done.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/password_store_x.h (right):

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_store_x.h:65: virtual bool
GetLogins(const autofill::PasswordForm& form,
On 2015/02/25 15:17:48, engedy wrote:
> The FailingBackend in password_store_x_unittest.cc should clear |forms| too.
(3
> methods.)

Done.
After dropping the promise about erasing on failure in the comment above, this
is was no longer needed, but I added the clearing calls to the FailingBackend as
well. I'm not entirely sure whether it is actually an improvement (maybe keeping
possible garbage there would help discover the callsite relying on non-existing
promises), but neither clearing, nor keeping seemed like the ideal choice.

https://codereview.chromium.org/906973007/diff/80001/components/password_mana...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/906973007/diff/80001/components/password_mana...
components/password_manager/core/browser/login_database.cc:904: return false;
On 2015/02/25 15:17:48, engedy wrote:
> Need to clear |forms| before this line. (Or remove the declaration comment to
> this effect, it seems to be done by all call sites.)

Done.
(Added the clear() despite also dropping the promise in the comment, for reasons
explained earlier.)

https://codereview.chromium.org/906973007/diff/80001/components/password_mana...
File components/password_manager/core/browser/login_database.h (right):

https://codereview.chromium.org/906973007/diff/80001/components/password_mana...
components/password_manager/core/browser/login_database.h:80: // Get all logins
created from |begin| onwards (inclusive) and before |end|.
On 2015/02/25 15:17:48, engedy wrote:
> Nit: 'Gets' or 'Retrieves'. (4 occurrences)

Done.

https://codereview.chromium.org/906973007/diff/80001/components/password_mana...
File components/password_manager/core/browser/password_store_sync.h (right):

https://codereview.chromium.org/906973007/diff/80001/components/password_mana...
components/password_manager/core/browser/password_store_sync.h:20: // returns
true, otherwise returns false and erases |forms|.
On 2015/02/25 15:17:49, engedy wrote:
> Nit: s/,/;/

Acknowledged, but rewrote completely.

https://codereview.chromium.org/906973007/diff/80001/components/password_mana...
components/password_manager/core/browser/password_store_sync.h:21: virtual bool
FillAutofillableLogins(
On 2015/02/25 15:17:49, engedy wrote:
> We also need to update the implementation in PasswordStoreDefault to make sure
> it erases form in error cases. (Also for blacklisted.)

Done.

https://codereview.chromium.org/906973007/diff/80001/components/password_mana...
components/password_manager/core/browser/password_store_sync.h:25: // returns
true, otherwise returns false and erases |forms|.
On 2015/02/25 15:17:49, engedy wrote:
> Nit: s/,/;/

Acknowledged, but rewrote completely.

[engedy, 2015-03-09 13:33:18]

LGTM % some nits.

Quite frankly, I am not at all sure if the benefits of clearing the vector on
failure is worth the extra effort. On the on hand, it might help to uncover
errors in some unit tests. On the other hand, it seems difficult to maintain
this invariant over time given that it is not documented, and it also looks
somewhat weird that in case both methods A and B guarantee this, and A calls B,
then A has to check the failure condition regardless, because B only has this as
an undocumented 'guarantee'.

But I do not feel strongly about either way, so I am happy with whatever you
consider to be the best.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.cc:165: // Returns all
the results from |found|. If not NULL, |lookup_form| is used to
On 2015/03/09 10:56:18, vabr (Chromium) wrote:
> On 2015/02/25 15:17:48, engedy wrote:
> > Phrasing suggestion: Converts native credentials in |found| to
> PasswordForms...
> Done.
> > 
> > Nit: We should either mention that this destroys/consumes |found|, or make
> that
> > a scoped_ptr.
> 
> 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.

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.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_gnome_x.h (right):

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_gnome_x.h:124: // |autofillable|
being false or true, credentials from the keyring. In case
On 2015/03/09 10:56:18, vabr (Chromium) wrote:
> On 2015/02/25 15:17:48, engedy wrote:
> > Phrasing suggestion (just an idea, I do not feel strongly about this): 
> > 
> > Retrieves all autofillable or all blacklisted credentials (depending on
> > |autofillable|) from the keyring into |forms|, overwriting the original
> contents
> > of |forms|. Returns true on success; returns false and erases |forms| on
> > failure.
> 
> Done, with adjustment to not caring about |forms| on failure.

Acknowledged.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_kwallet_x.cc (right):

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_kwallet_x.cc:267: void
AppendSecondToFirst(ScopedVector<autofill::PasswordForm>* first,
On 2015/03/09 10:56:18, vabr (Chromium) wrote:
> On 2015/02/25 15:17:48, engedy wrote:
> > In the long run, we should contribute such a method to ScopedVector itself.
> 
> Sadly, cannot be done:
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/cDHsauJfP5c/zWyyS...

Thanks a lot for investigating! I think it is actually great news that
std::vector<some smart pointer> is so much just around the block now!

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_kwallet_x.h (right):

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_kwallet_x.h:70: // Deserializes a
list of PasswordForms from the wallet. Protected for tests.
On 2015/03/09 10:56:18, vabr (Chromium) wrote:
> On 2015/02/25 15:17:48, engedy wrote:
> > Nit: What does "protected for tests" mean?
> 
> It meant: "Protected, instead of private, so that tests can call it directly."
> But that's about the only reason anything here is protected, so I dropped that
> sentence again.

Acknowledged.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/native_backend_libsecret.h (right):

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/native_backend_libsecret.h:130: static
std::string GetProfileSpecificAppString(LocalProfileId id);
On 2015/03/09 10:56:19, vabr (Chromium) wrote:
> On 2015/02/25 15:17:48, engedy wrote:
> > I think we can only move this to an anonymous namespace.
> 
> Done. Also for the Gnome backend.

Acknowledged.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/password_store_x.cc (right):

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_store_x.cc:157: void
PasswordStoreX::GetAutofillableLoginsImpl(
On 2015/03/09 10:56:19, vabr (Chromium) wrote:
> On 2015/02/25 15:17:48, engedy wrote:
> > Do we override the base class implementation for this and below only so that
> the
> > results get sorted?
> > 
> > Given that the Fill[...]() methods are only called (once?) by the Sync
> backend,
> > I think we could add the sorting there without much performance impact, and
> use
> > the base class implementations for these
> > Get{Autofillable|Blacklisted}LoginsImpl?
> 
> This sounds very reasonable.
> I propose to do it in a separate CL, though. If there are issues I'm not
seeing
> yet, or the performance hits us or something, it would be easier to revert.
The
> CL is at https://codereview.chromium.org/986983002/.

Acknowledged.

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/password_store_x.h (right):

https://codereview.chromium.org/906973007/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_store_x.h:65: virtual bool
GetLogins(const autofill::PasswordForm& form,
On 2015/03/09 10:56:19, vabr (Chromium) wrote:
> On 2015/02/25 15:17:48, engedy wrote:
> > The FailingBackend in password_store_x_unittest.cc should clear |forms| too.
> (3
> > methods.)
> 
> Done.
> After dropping the promise about erasing on failure in the comment above, this
> is was no longer needed, but I added the clearing calls to the FailingBackend
as
> well. I'm not entirely sure whether it is actually an improvement (maybe
keeping
> possible garbage there would help discover the callsite relying on
non-existing
> promises), but neither clearing, nor keeping seemed like the ideal choice.

Acknowledged.

https://codereview.chromium.org/906973007/diff/80001/components/password_mana...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/906973007/diff/80001/components/password_mana...
components/password_manager/core/browser/login_database.cc:904: return false;
On 2015/03/09 10:56:19, vabr (Chromium) wrote:
> On 2015/02/25 15:17:48, engedy wrote:
> > Need to clear |forms| before this line. (Or remove the declaration comment
to
> > this effect, it seems to be done by all call sites.)
> 
> Done.
> (Added the clear() despite also dropping the promise in the comment, for
reasons
> explained earlier.)

Acknowledged.

https://codereview.chromium.org/906973007/diff/80001/components/password_mana...
File components/password_manager/core/browser/password_store_sync.h (right):

https://codereview.chromium.org/906973007/diff/80001/components/password_mana...
components/password_manager/core/browser/password_store_sync.h:20: // returns
true, otherwise returns false and erases |forms|.
On 2015/03/09 10:56:19, vabr (Chromium) wrote:
> On 2015/02/25 15:17:49, engedy wrote:
> > Nit: s/,/;/
> 
> Acknowledged, but rewrote completely.

Acknowledged.

https://codereview.chromium.org/906973007/diff/80001/components/password_mana...
components/password_manager/core/browser/password_store_sync.h:25: // returns
true, otherwise returns false and erases |forms|.
On 2015/03/09 10:56:19, vabr (Chromium) wrote:
> On 2015/02/25 15:17:49, engedy wrote:
> > Nit: s/,/;/
> 
> Acknowledged, but rewrote completely.

Acknowledged.

https://codereview.chromium.org/906973007/diff/200001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_libsecret.h (right):

https://codereview.chromium.org/906973007/diff/200001/chrome/browser/password...
chrome/browser/password_manager/native_backend_libsecret.h:112: // Retrieves
password created/synced in the time interval into |forms,
nit: |

https://codereview.chromium.org/906973007/diff/200001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_libsecret_unittest.cc
(right):

https://codereview.chromium.org/906973007/diff/200001/chrome/browser/password...
chrome/browser/password_manager/native_backend_libsecret_unittest.cc:154: for
(uint32_t i = 0; i < global_mock_libsecret_items->size();)
nit: If we are already touching these lines, we could also convert this to use
iterators, which would be a lot nicer.

https://codereview.chromium.org/906973007/diff/200001/chrome/browser/password...
File chrome/browser/password_manager/password_store_mac.cc (right):

https://codereview.chromium.org/906973007/diff/200001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:1148: forms->clear();
nit: Sorry, I was meaning to say after the DCHECK. This looks weird. :)

https://codereview.chromium.org/906973007/diff/200001/chrome/browser/password...
File chrome/browser/password_manager/password_store_x.cc (right):

https://codereview.chromium.org/906973007/diff/200001/chrome/browser/password...
chrome/browser/password_manager/password_store_x.cc:152: if
(allow_default_store()) {
nit: -{}

https://codereview.chromium.org/906973007/diff/200001/components/password_man...
File components/password_manager/core/browser/password_syncable_service.cc
(right):

https://codereview.chromium.org/906973007/diff/200001/components/password_man...
components/password_manager/core/browser/password_syncable_service.cc:317:
password_entries->reserve(password_entries->size() +
Is this necessary?

[engedy, 2015-03-09 13:33:43]

In any case, thanks a lot for the cleanup!

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

Hi Balázs,

I addressed your comments. In particular, you convinced me that calling clear()
too much is not a good idea. Crucial was that it established en undocumented
"contract" -- the code made it apparent that we want to clear the vectors in any
case, but comments were silent.

I split the changes for easier review in this way:
* Patch set 12 is addressing everything but removing the clear().
  (Diff against p.s. 11, not 10, to filter out rebasing.)
* Patch set 14 (diff against 12) is just removing the clear() calls. I did it on
places where I felt clear() is not dictated by the contracts in documentation
comments.

PTAL.

Thanks for your thorough and helpful review!

Vaclav

https://codereview.chromium.org/906973007/diff/200001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/906973007/diff/200001/chrome/browser/password...
chrome/browser/password_manager/native_backend_gnome_x.cc:165: // Converts
native credentials in |found| to PasswordForms. If not NULL,
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.

https://codereview.chromium.org/906973007/diff/200001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_libsecret.h (right):

https://codereview.chromium.org/906973007/diff/200001/chrome/browser/password...
chrome/browser/password_manager/native_backend_libsecret.h:112: // Retrieves
password created/synced in the time interval into |forms,
On 2015/03/09 13:33:17, engedy wrote:
> nit: |

Done.

https://codereview.chromium.org/906973007/diff/200001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_libsecret_unittest.cc
(right):

https://codereview.chromium.org/906973007/diff/200001/chrome/browser/password...
chrome/browser/password_manager/native_backend_libsecret_unittest.cc:154: for
(uint32_t i = 0; i < global_mock_libsecret_items->size();)
On 2015/03/09 13:33:18, engedy wrote:
> nit: If we are already touching these lines, we could also convert this to use
> iterators, which would be a lot nicer.

Even with iterators, this uses erase() on vectors, which is generally
unnecessarily moving stuff around. While these are tests, I still felt bad about
the erase(), so I rewrote it to have a second vector of kept items, and swap the
vectors after the for loop.

https://codereview.chromium.org/906973007/diff/200001/chrome/browser/password...
File chrome/browser/password_manager/password_store_mac.cc (right):

https://codereview.chromium.org/906973007/diff/200001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:1148: forms->clear();
On 2015/03/09 13:33:18, engedy wrote:
> nit: Sorry, I was meaning to say after the DCHECK. This looks weird. :)

Done.
I agree that preconditions-checking DCHECK should be the first line, I should
have looked more carefully.

https://codereview.chromium.org/906973007/diff/200001/chrome/browser/password...
File chrome/browser/password_manager/password_store_x.cc (right):

https://codereview.chromium.org/906973007/diff/200001/chrome/browser/password...
chrome/browser/password_manager/password_store_x.cc:152: if
(allow_default_store()) {
On 2015/03/09 13:33:18, engedy wrote:
> nit: -{}

Done.

https://codereview.chromium.org/906973007/diff/200001/components/password_man...
File components/password_manager/core/browser/password_syncable_service.cc
(right):

https://codereview.chromium.org/906973007/diff/200001/components/password_man...
components/password_manager/core/browser/password_syncable_service.cc:317:
password_entries->reserve(password_entries->size() +
On 2015/03/09 13:33:18, engedy wrote:
> Is this necessary?

Ideally, insert() would do resize when called on a range, but I have not found
any documented guarantee for that.

Performance-wise the necessity is also not clear, given that the typical sizes
of the vectors are not clear to me.

But reserve() is used in the codebase well enough to keep this code reasonably
readable, so I decided to keep it. Is that OK with you?

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

LGTM % comments.

Please be aware that WARN_UNUSED_RESULT seems to be not working in clang after
all.

https://codereview.chromium.org/906973007/diff/200001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/906973007/diff/200001/chrome/browser/password...
chrome/browser/password_manager/native_backend_gnome_x.cc:165: // Converts
native credentials in |found| to PasswordForms. If not NULL,
On 2015/03/09 17:44:15, vabr (Chromium) wrote:
> 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.

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

https://codereview.chromium.org/906973007/diff/240001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/906973007/diff/240001/chrome/browser/password...
chrome/browser/password_manager/native_backend_gnome_x.cc:526:
method->forms_.clear();
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).

https://codereview.chromium.org/906973007/diff/240001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_kwallet_x.cc (right):

https://codereview.chromium.org/906973007/diff/240001/chrome/browser/password...
chrome/browser/password_manager/native_backend_kwallet_x.cc:600: // This is not
an error. There just isn't a matching entry.
I think in this case |forms| should be cleared. (The contract promises that
|forms| will be overwritten with the results on success.)

https://codereview.chromium.org/906973007/diff/240001/chrome/browser/password...
chrome/browser/password_manager/native_backend_kwallet_x.cc:634: return true;
Same here. Perhaps it is just easiest to keep the clear() on top.

https://codereview.chromium.org/906973007/diff/260001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/906973007/diff/260001/chrome/browser/password...
chrome/browser/password_manager/native_backend_gnome_x.cc:489:
forms->swap(forms_);
I would suggest changing this to *forms = forms_.Pass();, see reasoning below.

https://codereview.chromium.org/906973007/diff/280001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/906973007/diff/280001/chrome/browser/password...
chrome/browser/password_manager/native_backend_gnome_x.cc:316: // |forms_| with
the found credentials. Clears |forms_| otherwise.
We should update this comment, see comments below, though.

https://codereview.chromium.org/906973007/diff/280001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_kwallet_x.cc (right):

https://codereview.chromium.org/906973007/diff/280001/chrome/browser/password...
chrome/browser/password_manager/native_backend_kwallet_x.cc:688: // Swapping
|collected_forms| with |*forms| just before "return true" makes
nit: Is this still needed with the new semantics?

https://codereview.chromium.org/906973007/diff/280001/chrome/browser/password...
File chrome/browser/password_manager/password_store_mac.cc (right):

https://codereview.chromium.org/906973007/diff/280001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:1114:
FillBlacklistLogins(&obtained_forms);
Need to check return value and clear forms on failure.

https://codereview.chromium.org/906973007/diff/280001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:1121:
FillAutofillableLogins(&obtained_forms);
Need to check return value and clear forms on failure.

https://codereview.chromium.org/906973007/diff/280001/components/password_man...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/906973007/diff/280001/components/password_man...
components/password_manager/core/browser/login_database.cc:891: // Swap
|collected_forms| into |*forms| first on success, to avoid returning
nit: Do we need this anymore?

https://codereview.chromium.org/906973007/diff/280001/components/password_man...
File components/password_manager/core/browser/login_database.h (right):

https://codereview.chromium.org/906973007/diff/280001/components/password_man...
components/password_manager/core/browser/login_database.h:153: // result,
otherwise returns false and clears |forms|.
We should remove the promise about clearing forms on failure.

https://codereview.chromium.org/906973007/diff/280001/components/password_man...
components/password_manager/core/browser/login_database.h:160: // |*psl_match|.
On success returns true, otherwise clears |forms| and returns
Same here.

https://codereview.chromium.org/906973007/diff/280001/components/password_man...
File components/password_manager/core/browser/password_store_default.cc (right):

https://codereview.chromium.org/906973007/diff/280001/components/password_man...
components/password_manager/core/browser/password_store_default.cc:126:
FillAutofillableLogins(&logins);
Need to check return value and clear |logins| on failure.

https://codereview.chromium.org/906973007/diff/280001/components/password_man...
components/password_manager/core/browser/password_store_default.cc:133:
FillBlacklistLogins(&logins);
Need to check return value and clear |logins| on failure.

[vabr (Chromium), 2015-03-12 15:30:52]

Thanks, Balázs,
For your very careful review, and for verifying WARN_UNUSED_RESULT for clang! :)

I addressed your comments in p.s. 15, rebased in 16, and fixed a stupid mistake
in p.s. 17.

Please have a look.

Cheers,
Vaclav

https://codereview.chromium.org/906973007/diff/240001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/906973007/diff/240001/chrome/browser/password...
chrome/browser/password_manager/native_backend_gnome_x.cc:526:
method->forms_.clear();
On 2015/03/11 19:25:35, engedy wrote:
> 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).

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.

https://codereview.chromium.org/906973007/diff/240001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_kwallet_x.cc (right):

https://codereview.chromium.org/906973007/diff/240001/chrome/browser/password...
chrome/browser/password_manager/native_backend_kwallet_x.cc:600: // This is not
an error. There just isn't a matching entry.
On 2015/03/11 19:25:35, engedy wrote:
> I think in this case |forms| should be cleared. (The contract promises that
> |forms| will be overwritten with the results on success.)

Done.

https://codereview.chromium.org/906973007/diff/240001/chrome/browser/password...
chrome/browser/password_manager/native_backend_kwallet_x.cc:634: return true;
On 2015/03/11 19:25:35, engedy wrote:
> Same here. Perhaps it is just easiest to keep the clear() on top.

Agreed. Done.

https://codereview.chromium.org/906973007/diff/260001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/906973007/diff/260001/chrome/browser/password...
chrome/browser/password_manager/native_backend_gnome_x.cc:489:
forms->swap(forms_);
On 2015/03/11 19:25:35, engedy wrote:
> I would suggest changing this to *forms = forms_.Pass();, see reasoning below.

Done.

https://codereview.chromium.org/906973007/diff/280001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/906973007/diff/280001/chrome/browser/password...
chrome/browser/password_manager/native_backend_gnome_x.cc:316: // |forms_| with
the found credentials. Clears |forms_| otherwise.
On 2015/03/11 19:25:35, engedy wrote:
> We should update this comment, see comments below, though.

Done.

https://codereview.chromium.org/906973007/diff/280001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_kwallet_x.cc (right):

https://codereview.chromium.org/906973007/diff/280001/chrome/browser/password...
chrome/browser/password_manager/native_backend_kwallet_x.cc:688: // Swapping
|collected_forms| with |*forms| just before "return true" makes
On 2015/03/11 19:25:35, engedy wrote:
> nit: Is this still needed with the new semantics?

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.

https://codereview.chromium.org/906973007/diff/280001/chrome/browser/password...
File chrome/browser/password_manager/password_store_mac.cc (right):

https://codereview.chromium.org/906973007/diff/280001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:1114:
FillBlacklistLogins(&obtained_forms);
On 2015/03/11 19:25:35, engedy wrote:
> Need to check return value and clear forms on failure.

Done.

https://codereview.chromium.org/906973007/diff/280001/chrome/browser/password...
chrome/browser/password_manager/password_store_mac.cc:1121:
FillAutofillableLogins(&obtained_forms);
On 2015/03/11 19:25:35, engedy wrote:
> Need to check return value and clear forms on failure.

Done.

https://codereview.chromium.org/906973007/diff/280001/components/password_man...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/906973007/diff/280001/components/password_man...
components/password_manager/core/browser/login_database.cc:891: // Swap
|collected_forms| into |*forms| first on success, to avoid returning
On 2015/03/11 19:25:35, engedy wrote:
> nit: Do we need this anymore?

We don't. Removed for the same reason as in KWallet.

Done.

https://codereview.chromium.org/906973007/diff/280001/components/password_man...
File components/password_manager/core/browser/login_database.h (right):

https://codereview.chromium.org/906973007/diff/280001/components/password_man...
components/password_manager/core/browser/login_database.h:153: // result,
otherwise returns false and clears |forms|.
On 2015/03/11 19:25:35, engedy wrote:
> We should remove the promise about clearing forms on failure.

Done.

https://codereview.chromium.org/906973007/diff/280001/components/password_man...
components/password_manager/core/browser/login_database.h:160: // |*psl_match|.
On success returns true, otherwise clears |forms| and returns
On 2015/03/11 19:25:35, engedy wrote:
> Same here.

Done.

https://codereview.chromium.org/906973007/diff/280001/components/password_man...
File components/password_manager/core/browser/password_store_default.cc (right):

https://codereview.chromium.org/906973007/diff/280001/components/password_man...
components/password_manager/core/browser/password_store_default.cc:126:
FillAutofillableLogins(&logins);
On 2015/03/11 19:25:35, engedy wrote:
> Need to check return value and clear |logins| on failure.

Done.

https://codereview.chromium.org/906973007/diff/280001/components/password_man...
components/password_manager/core/browser/password_store_default.cc:133:
FillBlacklistLogins(&logins);
On 2015/03/11 19:25:35, engedy wrote:
> Need to check return value and clear |logins| on failure.

Done.

[engedy, 2015-03-16 10:35:36]

LGTM, with one nit.

Thanks a lot for taking the time to do this, I think it is a super complicated
piece of clean-up; but one that was definitely worth it.

https://codereview.chromium.org/906973007/diff/240001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/906973007/diff/240001/chrome/browser/password...
chrome/browser/password_manager/native_backend_gnome_x.cc:526:
method->forms_.clear();
On 2015/03/12 15:30:50, vabr (Chromium) wrote:
> On 2015/03/11 19:25:35, engedy wrote:
> > 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).
> 
> 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.

Acknowledged.

https://codereview.chromium.org/906973007/diff/280001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_kwallet_x.cc (right):

https://codereview.chromium.org/906973007/diff/280001/chrome/browser/password...
chrome/browser/password_manager/native_backend_kwallet_x.cc:688: // Swapping
|collected_forms| with |*forms| just before "return true" makes
On 2015/03/12 15:30:51, vabr (Chromium) wrote:
> On 2015/03/11 19:25:35, engedy wrote:
> > nit: Is this still needed with the new semantics?
> 
> 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.

Acknowledged.

https://codereview.chromium.org/906973007/diff/300001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/906973007/diff/300001/chrome/browser/password...
chrome/browser/password_manager/native_backend_gnome_x.cc:316: // |forms_| with
the found credentials.
Thanks, looking good!

nit: Could you please reinstate the comment about clearing?

[engedy, 2015-03-16 10:36:18]

Feel free to just land it once the comment is addressed.

[vabr (Chromium), 2015-03-16 10:58:47]

Thanks for your patient review!

Comment addressed, sending to the CQ.

Cheers,
Vaclav

https://codereview.chromium.org/906973007/diff/300001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/906973007/diff/300001/chrome/browser/password...
chrome/browser/password_manager/native_backend_gnome_x.cc:316: // |forms_| with
the found credentials.
On 2015/03/16 10:35:35, engedy wrote:
> Thanks, looking good!
> 
> nit: Could you please reinstate the comment about clearing?

Done.

[vabr (Chromium), 2015-03-16 13:10:26]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2015-03-16 13:10:27]

The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org
Link to the patchset: https://codereview.chromium.org/906973007/#ps380001
(title: "Just rebased")

[commit-bot: I haz the power, 2015-03-16 13:10:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/906973007/380001

[commit-bot: I haz the power, 2015-03-16 13:15:14]

Committed patchset #19 (id:380001)

[commit-bot: I haz the power, 2015-03-16 13:15:52]

Patchset 19 (id:??) landed as
https://crrev.com/c5c4d77323e8bff2028f9e43f52f0be4c3a93a77
Cr-Commit-Position: refs/heads/master@{#320721}