Fix PasswordManagerInteractiveTest flakiness.

- rewrite WaitForElementValue for the UI test. The existing implementation uses
onchange event. It's problematic for the real keystroke because the event is
triggered on focus change only. oninput is the right choice here.
- use WaitForElementValue in FillElementWithValue to enforce that the right
input handles the keystrokes.

Bug: 1024902
Change-Id: Ib7752e39b62d20c73ed06ff85f4fdea641d0955d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1919154
Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#716115}

chrome/browser/password_manager/password_manager_interactive_test_base.cc

@@ -9,6 +9,15 @@
 #include "ui/events/keycodes/dom_us_layout_data.h"
 #include "ui/events/keycodes/keyboard_code_conversion.h"
 
+namespace {
+enum ReturnCodes {  // Possible results of the JavaScript code.
+  RETURN_CODE_OK,
+  RETURN_CODE_NO_ELEMENT,
+  RETURN_CODE_WRONG_VALUE,
+  RETURN_CODE_INVALID,
+};
+}  // namespace
+
 PasswordManagerInteractiveTestBase::PasswordManagerInteractiveTestBase() =
     default;
 
@@ -17,7 +26,8 @@ PasswordManagerInteractiveTestBase::~PasswordManagerInteractiveTestBase() =
 
 void PasswordManagerInteractiveTestBase::FillElementWithValue(
     const std::string& element_id,
-    const std::string& value) {
+    const std::string& value,
+    const std::string& expected_value) {
   ASSERT_TRUE(content::ExecuteScript(
       RenderFrameHost(),
       base::StringPrintf("document.getElementById('%s').focus();",
@@ -37,6 +47,54 @@ void PasswordManagerInteractiveTestBase::FillElementWithValue(
                               ui::DomCodeToUsLayoutKeyboardCode(dom_code),
                               false, shift, false, false);
   }
+  // Enforce that the keystroke were processed. It's very important because
+  // keystrokes aren't synchronized with JS and they take longer to process. The
+  // test could move the focus later and divert the keystrokes to another input.
+  WaitForElementValue(element_id, expected_value);
+}
+
+void PasswordManagerInteractiveTestBase::FillElementWithValue(
+    const std::string& element_id,
+    const std::string& value) {
+  CheckElementValue(element_id, std::string());
+  FillElementWithValue(element_id, value, value);
+}
+
+void PasswordManagerInteractiveTestBase::WaitForElementValue(
+    const std::string& element_id,
+    const std::string& expected_value) {
+  const std::string value_check_function = base::StringPrintf(
+      "function valueCheck() {"
+      "  var element = document.getElementById('%s');"
+      "  return element && element.value == '%s';"
+      "}",
+      element_id.c_str(), expected_value.c_str());
+  const std::string script =
+      value_check_function +
+      base::StringPrintf(
+          "if (valueCheck()) {"
+          "  /* Spin the event loop with setTimeout. */"
+          "  setTimeout(window.domAutomationController.send(%d), 0);"
+          "} else {"
+          "  var element = document.getElementById('%s');"
+          "  if (!element)"
+          "    window.domAutomationController.send(%d);"
+          "  element.oninput = function() {"
+          "    if (valueCheck()) {"
+          "      /* Spin the event loop with setTimeout. */"
+          "      setTimeout(window.domAutomationController.send(%d), 0);"
+          "      element.oninput = undefined;"
+          "    }"
+          "  };"
+          "}",
+          RETURN_CODE_OK, element_id.c_str(), RETURN_CODE_NO_ELEMENT,
+          RETURN_CODE_OK);
+  int return_value = RETURN_CODE_INVALID;
+  ASSERT_TRUE(content::ExecuteScriptWithoutUserGestureAndExtractInt(
+      RenderFrameHost(), script, &return_value));
+  EXPECT_EQ(RETURN_CODE_OK, return_value)
+      << "element_id = " << element_id
+      << ", expected_value = " << expected_value;
 }
 
 void PasswordManagerInteractiveTestBase::VerifyPasswordIsSavedAndFilled(
@@ -91,4 +149,6 @@ void PasswordManagerInteractiveTestBase::SimulateUserDeletingFieldContent(
   content::SimulateKeyPress(WebContents(), ui::DomKey::BACKSPACE,
                             ui::DomCode::BACKSPACE, ui::VKEY_BACK, false, false,
                             false, false);
+  // A test may rely on empty field value.
+  WaitForElementValue(field_id, std::string());
 }

chrome/browser/password_manager/password_manager_interactive_test_base.h

@@ -16,10 +16,23 @@ class PasswordManagerInteractiveTestBase
   ~PasswordManagerInteractiveTestBase() override;
 
   // Focuses an input element with id |element_id| in the main frame and
-  // emulates typing |value| into it.
+  // emulates typing |value| into it. The method waits until the field value
+  // switches to |expected_value|.
+  void FillElementWithValue(const std::string& element_id,
+                            const std::string& value,
+                            const std::string& expected_value);
+
+  // Same as above but the field is considered empty before the call.
   void FillElementWithValue(const std::string& element_id,
                             const std::string& value);
 
+  // Replaces the version in PasswordManagerBrowserTestBase which isn't suitable
+  // for the interactive UI tests.
+  // Wait until the element has |expected_value| value. The value is supposed to
+  // be changed by the actual keystrokes.
+  void WaitForElementValue(const std::string& element_id,
+                           const std::string& expected_value);
+
   // Navigates to |filename|, fills |username_id| and |password_id| if nonempty
   // and runs |submission_script| to submit. The credential is then saved via
   // the password prompt.
@@ -34,7 +47,9 @@ class PasswordManagerInteractiveTestBase
   void SimulateUserDeletingFieldContent(const std::string& field_id);
 
  private:
-  DISALLOW_COPY_AND_ASSIGN(PasswordManagerInteractiveTestBase);
+  // WaitForElementValue from the base class don't do the right thing with the
+  // real keystrokes.
+  using PasswordManagerBrowserTestBase::WaitForElementValue;
 };
 
 #endif  // CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_MANAGER_INTERACTIVE_TEST_BASE_H_

chrome/browser/password_manager/password_manager_interactive_uitest.cc

@@ -65,13 +65,7 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerInteractiveTest, UsernameChanged) {
 
   // Change username and submit. This should add the characters "orary" to the
   // already autofilled username.
-  FillElementWithValue("username_field", "orary");
-
-  // Move the focus out of the inputs before waiting because WaitForElementValue
-  // uses "onchange" event. The event is triggered only when the control looses
-  // focus.
-  chrome::FocusLocationBar(browser());
-  WaitForElementValue("username_field", "temporary");
+  FillElementWithValue("username_field", "orary", "temporary");
 
   NavigationObserver navigation_observer(WebContents());
   BubbleObserver prompt_observer(WebContents());
@@ -170,6 +164,7 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerInteractiveTest,
 
   NavigateToFile("/password/password_form.html");
 
+  SimulateUserDeletingFieldContent("password_field");
   FillElementWithValue("password_field", "123");
   BubbleObserver prompt_observer(WebContents());
   prompt_observer.WaitForFallbackForSaving();