Fix CredentialManagerBrowserTest.UpdateViaAPIAndAutofill flakiness.

chrome/browser/password_manager/credential_manager_browsertest.cc

Index: chrome/browser/password_manager/credential_manager_browsertest.cc
diff --git a/chrome/browser/password_manager/credential_manager_browsertest.cc b/chrome/browser/password_manager/credential_manager_browsertest.cc
index 6be28872b6d6b5b7052fe5d5ecd37731984aa07e..72212d34a935978a31aae81121d567f94e3c5be2 100644
--- a/chrome/browser/password_manager/credential_manager_browsertest.cc
+++ b/chrome/browser/password_manager/credential_manager_browsertest.cc
@@ -10,12 +10,32 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/passwords/passwords_model_delegate.h"
 #include "components/password_manager/core/browser/password_bubble_experiment.h"
+#include "components/password_manager/core/browser/password_store_consumer.h"
 #include "components/password_manager/core/browser/test_password_store.h"
 #include "content/public/test/browser_test.h"
 #include "content/public/test/browser_test_utils.h"
 
 namespace {
 
+class PasswordStoreSyncer : public password_manager::PasswordStoreConsumer {

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

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?

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

Done.

+ public:
+  PasswordStoreSyncer() = default;
+
+  void OnGetPasswordStoreResults(
+      ScopedVector<autofill::PasswordForm> results) override {
+    run_loop_.Quit();
+  }
+
+  void Wait() {
+    run_loop_.Run();
+  }
+
+ private:
+  base::RunLoop run_loop_;
+
+  DISALLOW_COPY_AND_ASSIGN(PasswordStoreSyncer);
+};
+
 class CredentialManagerBrowserTest : public PasswordManagerBrowserTestBase {
  public:
   CredentialManagerBrowserTest() = default;
@@ -25,8 +45,16 @@ class CredentialManagerBrowserTest : public PasswordManagerBrowserTestBase {
         password_manager::ui::CREDENTIAL_REQUEST_STATE;
   }
 
+  void SyncPasswordStore() {

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

nit: What about WaitForStoreResults to avoid the confusion with Chrome sync?

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

Done.

+    scoped_refptr<password_manager::PasswordStore> password_store =
+        PasswordStoreFactory::GetForProfile(
+            browser()->profile(), ServiceAccessType::IMPLICIT_ACCESS);
+    PasswordStoreSyncer syncer;
+    password_store->GetAutofillableLoginsWithAffiliatedRealms(&syncer);
+    syncer.Wait();
+  }
+
  private:
-  net::EmbeddedTestServer https_test_server_;
 
   DISALLOW_COPY_AND_ASSIGN(CredentialManagerBrowserTest);
 };
@@ -157,9 +185,7 @@ IN_PROC_BROWSER_TEST_F(CredentialManagerBrowserTest,
   EXPECT_FALSE(prompt_observer->IsShowingSavePrompt());
 }
 
-// Disabled due to flakes; see https://crbug.com/629459.
-IN_PROC_BROWSER_TEST_F(CredentialManagerBrowserTest,
-                       DISABLED_UpdateViaAPIAndAutofill) {
+IN_PROC_BROWSER_TEST_F(CredentialManagerBrowserTest, UpdateViaAPIAndAutofill) {
   // Save credentials with 'skip_zero_click' false.
   scoped_refptr<password_manager::TestPasswordStore> password_store =
       static_cast<password_manager::TestPasswordStore*>(
@@ -168,7 +194,7 @@ IN_PROC_BROWSER_TEST_F(CredentialManagerBrowserTest,
               .get());
   autofill::PasswordForm signin_form;
   signin_form.signon_realm = embedded_test_server()->base_url().spec();
-  signin_form.password_value = base::ASCIIToUTF16("12345");
+  signin_form.password_value = base::ASCIIToUTF16("old_pass");
   signin_form.username_value = base::ASCIIToUTF16("user");
   signin_form.origin = embedded_test_server()->base_url();
   signin_form.skip_zero_click = true;
@@ -176,39 +202,46 @@ IN_PROC_BROWSER_TEST_F(CredentialManagerBrowserTest,
   password_store->AddLogin(signin_form);
 
   NavigateToFile("/password/password_form.html");
-  std::string fill_password =
-      "document.getElementById('username_field').value = 'user';"
-      "document.getElementById('password_field').value = '12345';";
-  ASSERT_TRUE(content::ExecuteScript(RenderViewHost(), fill_password));
 
-  // Call the API to update the form.
+  // Postpone a submit event for 1second. Even for the static html page Chrome

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

typo: 1second -> 1 second

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

Done.

+  // continues to parse and recreate the PasswordFormManager instances after the
+  // page load. Calling the API before this would make the test flaky. Clicking
+  // on the button emulates server analysing the credential and then saving  and

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

nit: 2 spaces before "and" -> 1 space

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

Done.

+  // navigating to the landing page.
   ASSERT_TRUE(content::ExecuteScript(
       RenderViewHost(),
-      "var c = new PasswordCredential({ id: 'user', password: '12345' });"
-      "navigator.credentials.store(c);"));
+      "document.getElementById('input_submit_button').addEventListener('click',"
+      "function(event) {"
+        "setTimeout( function() {"
+          "var c = new PasswordCredential({ id: 'user', password: 'API' });"
+          "navigator.credentials.store(c);"
+          "document.getElementById('testform').submit();"
+        "}, 1000 );"
+        "event.preventDefault();"
+      "});"));
+  // Fill the new password and click the button to submit the page later. The
+  // API should suppress the autofill password manager and overwrite the
+  // password.
+  NavigationObserver form_submit_observer(WebContents());
+  ASSERT_TRUE(content::ExecuteScript(
+      RenderViewHost(),
+      "document.getElementById('username_field').value = 'user';"
+      "document.getElementById('password_field').value = 'autofill';"
+      "document.getElementById('input_submit_button').click();"));
+  form_submit_observer.Wait();
+
   std::unique_ptr<BubbleObserver> prompt_observer(
       new BubbleObserver(WebContents()));
   EXPECT_FALSE(prompt_observer->IsShowingSavePrompt());
   EXPECT_FALSE(prompt_observer->IsShowingUpdatePrompt());
+  SyncPasswordStore();
   signin_form.skip_zero_click = false;
   signin_form.times_used = 1;
+  signin_form.password_value = base::ASCIIToUTF16("API");
   password_manager::TestPasswordStore::PasswordMap stored =
       password_store->stored_passwords();
   ASSERT_EQ(1u, stored.size());
   EXPECT_EQ(signin_form, stored[signin_form.signon_realm][0]);
-
-  // Verify that the autofill password manager was suppressed and didn't touch
-  // the store. It would definitely update the '*_element' fields.
-  NavigationObserver form_submit_observer(WebContents());
-  ASSERT_TRUE(content::ExecuteScript(
-      RenderViewHost(),
-      "document.getElementById('input_submit_button').click();"));
-  form_submit_observer.Wait();
-  EXPECT_FALSE(prompt_observer->IsShowingSavePrompt());
-  EXPECT_FALSE(prompt_observer->IsShowingUpdatePrompt());
-  stored = password_store->stored_passwords();
-  ASSERT_EQ(1u, stored.size());
-  EXPECT_EQ(signin_form, stored[signin_form.signon_realm][0]);
 }
 
 }  // namespace