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" 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 ---------------------------------------------------------------
 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);
+  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
 // 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());
 }