Fix the flaky PasswordManagerBrowserTest.PasswordValueAccessible test

chrome/browser/password_manager/password_manager_browsertest.cc

Index: chrome/browser/password_manager/password_manager_browsertest.cc
diff --git a/chrome/browser/password_manager/password_manager_browsertest.cc b/chrome/browser/password_manager/password_manager_browsertest.cc
index 4793dedd4f9ba260c4ba7a6d425eca316c934868..0dc3ce8c795992e5f54f6b245a14be3b574c12e2 100644
--- a/chrome/browser/password_manager/password_manager_browsertest.cc
+++ b/chrome/browser/password_manager/password_manager_browsertest.cc
@@ -7,6 +7,7 @@
 #include "base/command_line.h"
 #include "base/metrics/histogram_samples.h"
 #include "base/metrics/statistics_recorder.h"
+#include "base/strings/stringprintf.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/infobars/infobar_service.h"
 #include "chrome/browser/password_manager/password_store_factory.h"
@@ -173,23 +174,57 @@ class PasswordManagerBrowserTest : public InProcessBrowserTest {
     observer.Wait();
   }
 
-  // Executes |script| and uses the EXPECT macros to check the return value
-  // against |expected_return_value|.
-  void CheckScriptReturnValue(std::string& script, bool expected_return_value);
+  // Waits until the "value" atribute of the HTML element with |element_id| is

[bartfab (slow), 2014/06/05 16:45:54]

Nit: s/atribute/attribute/

[vabr (Chromium), 2014/06/05 17:58:13]

Done.

+  // equal to |expected_value|.
+  void WaitForElementValue(const std::string& element_id,
+                           const std::string& expected_value);
 
  private:
   DISALLOW_COPY_AND_ASSIGN(PasswordManagerBrowserTest);
 };
 
-void PasswordManagerBrowserTest::CheckScriptReturnValue(
-    std::string& script,
-    bool expected_return_value) {
-  const std::string wrapped_script =
-      std::string("window.domAutomationController.send(") + script + ");";
-  bool return_value = !expected_return_value;
-  ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
-      RenderViewHost(), wrapped_script, &return_value));
-  EXPECT_EQ(expected_return_value, return_value) << "script = " << script;
+void PasswordManagerBrowserTest::WaitForElementValue(
+    const std::string& element_id,
+    const std::string& expected_value) {
+  enum ReturnCodes {  // Possible results of the JavaScript code.
+    RETURN_CODE_OK,
+    RETURN_CODE_NO_ELEMENT,
+    RETURN_CODE_WRONG_VALUE,
+    RETURN_CODE_INVALID };
+  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()) {"
+          "  window.domAutomationController.send(%d);"
+          "} else {"
+          "  var element = document.getElementById('%s');"
+          "  if (!element)"
+          "    window.domAutomationController.send(%d);"
+          "  element.onchange = function() {"
+          "    if (valueCheck())"
+          "      window.domAutomationController.send(%d);"
+          "    else"
+          "      window.domAutomationController.send(%d);"
+          "  };"
+          "}",
+          RETURN_CODE_OK,
+          element_id.c_str(),
+          RETURN_CODE_NO_ELEMENT,
+          RETURN_CODE_OK,
+          RETURN_CODE_WRONG_VALUE);
+  int return_value = RETURN_CODE_INVALID;
+  ASSERT_TRUE(content::ExecuteScriptAndExtractInt(
+      RenderViewHost(), script, &return_value));
+  EXPECT_EQ(RETURN_CODE_OK, return_value)
+      << "element_id = " << element_id
+      << ", expected_value = " << expected_value;
 }
 
 // Actual tests ---------------------------------------------------------------
@@ -593,14 +628,7 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest, DeleteFrameBeforeSubmit) {
   // The only thing we check here is that there is no use-after-free reported.
 }
 
-// Disabled on Windows due to flakiness: http://crbug.com/346297
-#if defined(OS_WIN)
-#define MAYBE_PasswordValueAccessible DISABLED_PasswordValueAccessible
-#else
-#define MAYBE_PasswordValueAccessible PasswordValueAccessible
-#endif
-IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest,
-                       MAYBE_PasswordValueAccessible) {
+IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest, PasswordValueAccessible) {
   NavigateToFile("/password/form_and_link.html");
 
   // Click on a link to open a new tab, then switch back to the first one.
@@ -628,16 +656,12 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest,
 
   // Check that while the username is immediately available, the password value
   // needs a user interaction to show up.
-  std::string check_username =
-      "document.getElementById('username_field').value == 'temp'";
-  std::string check_password =
-      "document.getElementById('password_field').value == 'random'";
-  CheckScriptReturnValue(check_username, true);
-  CheckScriptReturnValue(check_password, false);
-  content::SimulateMouseClick(
-      WebContents(), 0, blink::WebMouseEvent::ButtonLeft);
-  CheckScriptReturnValue(check_username, true);
-  CheckScriptReturnValue(check_password, true);
+  WaitForElementValue("username_field", "temp");
+  WaitForElementValue("password_field", "");
+  content::SimulateMouseClickAt(
+      WebContents(), 0, blink::WebMouseEvent::ButtonLeft, gfx::Point(1,1));
+  WaitForElementValue("password_field", "random");
+  WaitForElementValue("username_field", "temp");

[bartfab (slow), 2014/06/05 16:45:54]

Nit: That's a bit hacky. Since you expect the user name field to go from "temp"
to "temp," you have no way to know whether any pending changes have actually
happened already, other than by waiting for some other field to change and
concluding that when that happens, all pending changes are done.

If the user name field actually does *change* from "temp" to "temp," you could
improve your code as follows:
- Run JS that sets up an onchange listener.
- Trigger the mouse click.
- Run JS that waits for the onchange listener to fire and sends back the result.

This way, you will know for sure that the transition, even if just from "temp"
to "temp," has happened.

[vabr (Chromium), 2014/06/05 17:58:12]

Following an off-line clarification, I tried to make explicit what I wait for
and why.

 }
 
 // The following test is limited to Aura, because