Fix the flaky PasswordManagerBrowserTest.PasswordValueAccessible test

chrome/browser/password_manager/password_manager_browsertest.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <string>
 
 #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"
 #include "chrome/browser/password_manager/test_password_store_service.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/test_switches.h"
 #include "chrome/test/base/ui_test_utils.h"
@@ skipping 146 matching lines @@
       ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
 
     ASSERT_FALSE(CommandLine::ForCurrentProcess()->HasSwitch(
         password_manager::switches::kEnableAutomaticPasswordSaving));
     NavigationObserver observer(WebContents());
     GURL url = embedded_test_server()->GetURL(path);
     ui_test_utils::NavigateToURL(browser(), url);
     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" attribute of the HTML element with |element_id| is
+  // equal to |expected_value|. If the current value is not as expected, this
+  // waits until the "change" event is fired for the element. This also
+  // guarantees that once the real value matches the expected, the JavaScript
+  // event loop is spun to allow all other possible events to take place.
+  void WaitForElementValue(const std::string& element_id,
+                           const std::string& expected_value);
+  // Checks that the current "value" attribute of the HTML element with
+  // |element_id| is equal to |expected_value|.
+  void CheckElementValue(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;
+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 };

[bartfab (slow), 2014/06/06 11:19:34]

Nit: Put the }; on its own line.

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

Yeah, this one also looked strange to me, but that's how clang-format formatted
it.

Anyway, I did as you said, and will file a ticket for a bug in clang-format.

+  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.onchange = function() {"
+          "    if (valueCheck()) {"
+          "      /* Spin the event loop with setTimeout. */"
+          "      setTimeout(window.domAutomationController.send(%d), 0);"
+          "    } 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;
+}
+
+void PasswordManagerBrowserTest::CheckElementValue(
+    const std::string& element_id,
+    const std::string& expected_value) {
+  const std::string value_check_script = base::StringPrintf(
+      "var element = document.getElementById('%s');"
+      "window.domAutomationController.send(element && element.value == '%s');",
+      element_id.c_str(),
+      expected_value.c_str());
+  bool return_value = false;
   ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
-      RenderViewHost(), wrapped_script, &return_value));
-  EXPECT_EQ(expected_return_value, return_value) << "script = " << script;
+      RenderViewHost(), value_check_script, &return_value));
+  EXPECT_TRUE(return_value) << "element_id = " << element_id
+                            << ", expected_value = " << expected_value;
 }
 
 // Actual tests ---------------------------------------------------------------
 IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest,
                        PromptForNormalSubmit) {
   NavigateToFile("/password/password_form.html");
 
   // Fill a form and submit through a <input type="submit"> button. Nothing
   // special.
   NavigationObserver observer(WebContents());
@@ skipping 383 matching lines @@
       "document.getElementById('password_field').value = 'random';"
       "document.getElementById('input_submit_button').click();"
       "window.location.href = 'done.html';";
 
   ASSERT_TRUE(content::ExecuteScript(RenderViewHost(), save_and_remove));
   ASSERT_TRUE(content::ExecuteScript(RenderViewHost(), navigate_frame));
   observer.Wait();
   // 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.
   EXPECT_EQ(1, browser()->tab_strip_model()->count());
   std::string click =
       "document.getElementById('testlink').click();";
   ASSERT_TRUE(content::ExecuteScript(RenderViewHost(), click));
   EXPECT_EQ(2, browser()->tab_strip_model()->count());
   browser()->tab_strip_model()->ActivateTabAt(0, false);
 
   // Fill in the credentials, and make sure they are saved.
   NavigationObserver form_submit_observer(WebContents());
   std::string fill_and_submit =
       "document.getElementById('username_field').value = 'temp';"
       "document.getElementById('password_field').value = 'random';"
       "document.getElementById('input_submit_button').click();";
   ASSERT_TRUE(content::ExecuteScript(RenderViewHost(), fill_and_submit));
   form_submit_observer.Wait();
   EXPECT_TRUE(form_submit_observer.infobar_shown());
 
   // Reload the original page to have the saved credentials autofilled.
   NavigationObserver reload_observer(WebContents());
   NavigateToFile("/password/form_and_link.html");
   reload_observer.Wait();
 
-  // 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);
+  // Wait until the username is filled, to make sure autofill kicked in.
+  WaitForElementValue("username_field", "temp");
+  // Now check that the password is not accessible yet.
+  CheckElementValue("password_field", "");
+  // Let the user interact with the page.
+  content::SimulateMouseClickAt(
+      WebContents(), 0, blink::WebMouseEvent::ButtonLeft, gfx::Point(1,1));
+  // Wait until that interaction causes the password value to be revealed.
+  WaitForElementValue("password_field", "random");
+  // And check that after the side-effects of the interaction took place, the
+  // username value stays the same.
+  CheckElementValue("username_field", "temp");
 }
 
 // The following test is limited to Aura, because
 // RenderWidgetHostViewGuest::ProcessAckedTouchEvent is, and
 // ProcessAckedTouchEvent is what triggers the translation of touch events to
 // gesture events.
 #if defined(USE_AURA)
 IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest,
                        PasswordValueAccessibleOnSubmit) {
   NavigateToFile("/password/form_and_link.html");
@@ skipping 96 matching lines @@
   ASSERT_TRUE(content::ExecuteScript(RenderViewHost(), fill));
 
   NavigationObserver observer(WebContents());
   GURL url = embedded_test_server()->GetURL("/password/password_form.html");
   chrome::NavigateParams params(browser(), url,
                                 content::PAGE_TRANSITION_RELOAD);
   ui_test_utils::NavigateToURL(&params);
   observer.Wait();
   EXPECT_FALSE(observer.infobar_shown());
 }