Fix CredentialManagerBrowserTest.UpdateViaAPIAndAutofill flakiness.

Fix CredentialManagerBrowserTest.UpdateViaAPIAndAutofill flakiness.

BUG=628377, 629459
Committed: https://crrev.com/cd8c0b41b62492a20846f4068cab2b5309df1834
Cr-Commit-Position: refs/heads/master@{#406845}

[vasilii, 2016-07-21 12:56:23]

The CQ bit was checked by vasilii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 12:56:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vasilii, 2016-07-21 12:59:15]

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

[vasilii, 2016-07-21 12:59:16]

Hi Vaclav,

please review the CL. Let me know if the updated test flow doesn't make sense in
the real world. Maybe the production code should be changed.

[vabr (Chromium), 2016-07-21 13:19:54]

Hi Vasilii.

LGTM with nits.

Having said that, introducing delays in the tests is always dangerous: the
hard-coded delay in production could easily change from 0.3 to, say 1, making
this test flaky again. Accumulating delays can also lead to timeouts, although
that's less likely here.

An alternative here would be to change the way the Credential Manager API
implementaion prevents the old password manager from working. Perhaps instead of
killing the associated PasswordFormManager we could deactivate it (providing a
method PasswordFormManager::Deactivate() or similar). It would stay alive,
solving the problem with its re-creation after the delay, but it won't do
anything and PasswordManager would ignore it if searching for passwords to save
provisionally.

Cheers,
Vaclav

https://codereview.chromium.org/2169883002/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/credential_manager_browsertest.cc (right):

https://codereview.chromium.org/2169883002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/credential_manager_browsertest.cc:20: class
PasswordStoreSyncer : public password_manager::PasswordStoreConsumer {
nit: The "Syncer" in the name may confuse the reader into thinking of Chrome
sync. Perhaps changing the name to something like PasswordStoreResultsObserver,
or at least adding a comment above line 20 would help?

https://codereview.chromium.org/2169883002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/credential_manager_browsertest.cc:48: void
SyncPasswordStore() {
nit: What about WaitForStoreResults to avoid the confusion with Chrome sync?

https://codereview.chromium.org/2169883002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/credential_manager_browsertest.cc:206: //
Postpone a submit event for 1second. Even for the static html page Chrome
typo: 1second -> 1 second

https://codereview.chromium.org/2169883002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/credential_manager_browsertest.cc:209: // on the
button emulates server analysing the credential and then saving  and
nit: 2 spaces before "and" -> 1 space

[commit-bot: I haz the power, 2016-07-21 13:31:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 13:31:53]

Dry run: This issue passed the CQ dry run.

[vasilii, 2016-07-21 13:48:33]

The Deactivate() solution is an option. Though, I'd prefer not to complicate the
production implementation just for the sake of the test. Do you have a real
world use case in mind?

https://codereview.chromium.org/2169883002/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/credential_manager_browsertest.cc (right):

https://codereview.chromium.org/2169883002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/credential_manager_browsertest.cc:20: class
PasswordStoreSyncer : public password_manager::PasswordStoreConsumer {
On 2016/07/21 13:19:54, vabr (Chromium) wrote:
> nit: The "Syncer" in the name may confuse the reader into thinking of Chrome
> sync. Perhaps changing the name to something like
PasswordStoreResultsObserver,
> or at least adding a comment above line 20 would help?

Done.

https://codereview.chromium.org/2169883002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/credential_manager_browsertest.cc:48: void
SyncPasswordStore() {
On 2016/07/21 13:19:54, vabr (Chromium) wrote:
> nit: What about WaitForStoreResults to avoid the confusion with Chrome sync?

Done.

https://codereview.chromium.org/2169883002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/credential_manager_browsertest.cc:206: //
Postpone a submit event for 1second. Even for the static html page Chrome
On 2016/07/21 13:19:54, vabr (Chromium) wrote:
> typo: 1second -> 1 second

Done.

https://codereview.chromium.org/2169883002/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/credential_manager_browsertest.cc:209: // on the
button emulates server analysing the credential and then saving  and
On 2016/07/21 13:19:54, vabr (Chromium) wrote:
> nit: 2 spaces before "and" -> 1 space

Done.

[vasilii, 2016-07-21 13:48:58]

The CQ bit was checked by vasilii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 13:49:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2016-07-21 13:56:36]

On 2016/07/21 13:48:33, vasilii wrote:
> The Deactivate() solution is an option. Though, I'd prefer not to complicate
the
> production implementation just for the sake of the test. Do you have a real
> world use case in mind?

I was thinking about real world use. Relying on the second notifications about
forms to come quickly enough seems fragile. The 0.3 seconds delay of today might
get increased without us even noticing. The user might be running Chrome on some
low-end phone which could have a inter-process communication hiccups at an
unfortunate moment. The page author might be actually calling save() during
loading the server response, which might happen earlier than the second
notifications. We could wait for these situations to be observed, but there is
always the risk that nobody will report it or we'd fail to investigate it.

You are the main owner of credential manager code, so I defer to and respect
your judgement here. As I said, the current change LGTM already.

Cheers,
Vaclav

[vasilii, 2016-07-21 14:24:21]

On 2016/07/21 13:56:36, vabr (Chromium) wrote:
> On 2016/07/21 13:48:33, vasilii wrote:
> > The Deactivate() solution is an option. Though, I'd prefer not to complicate
> the
> > production implementation just for the sake of the test. Do you have a real
> > world use case in mind?
> 
> I was thinking about real world use. Relying on the second notifications about
> forms to come quickly enough seems fragile. The 0.3 seconds delay of today
might
> get increased without us even noticing. The user might be running Chrome on
some
> low-end phone which could have a inter-process communication hiccups at an
> unfortunate moment. The page author might be actually calling save() during
> loading the server response, which might happen earlier than the second
> notifications. We could wait for these situations to be observed, but there is
> always the risk that nobody will report it or we'd fail to investigate it.
> 
> You are the main owner of credential manager code, so I defer to and respect
> your judgement here. As I said, the current change LGTM already.
> 
> Cheers,
> Vaclav

The root cause of the problems here seems to be that unnecessary FormParsed
message is sent. I think it deserves to be fixed due to possible performance
impact. I'll open a bug for this to keep in mind.
For now I think that having a running and hopefully working test is better than
not having it. Therefore, I'm landing the CL.

[commit-bot: I haz the power, 2016-07-21 14:26:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 14:26:29]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-07-21 14:28:44]

> The root cause of the problems here seems to be that unnecessary FormParsed
> message is sent. I think it deserves to be fixed due to possible performance
> impact. I'll open a bug for this to keep in mind.
> For now I think that having a running and hopefully working test is better
than
> not having it. Therefore, I'm landing the CL.

This sounds good to me. Thanks!
Vaclav

[vasilii, 2016-07-21 14:30:29]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2016-07-21 14:30:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 14:33:55]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-21 14:35:51]

Description was changed from

==========
Fix CredentialManagerBrowserTest.UpdateViaAPIAndAutofill flakiness.

BUG=628377,629459
==========

to

==========
Fix CredentialManagerBrowserTest.UpdateViaAPIAndAutofill flakiness.

BUG=628377,629459

Committed: https://crrev.com/cd8c0b41b62492a20846f4068cab2b5309df1834
Cr-Commit-Position: refs/heads/master@{#406845}
==========

[commit-bot: I haz the power, 2016-07-21 14:35:53]

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