Fixing race condition in webState:didSubmitDocumentWithFormNamed...

The FormData extraction code was racing against new page load even
though FormData was already passed into a function in string format.
Now the FormData is extracted from a passed in string.

Bug: 418827
Change-Id: I845b539974b57d1f430016f4500af5a4da5fcbf2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2022771Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Commit-Queue: Maria Kazinova <kazinova@google.com>
Cr-Commit-Position: refs/heads/master@{#735936}

components/password_manager/ios/password_form_helper.mm

@@ -56,14 +56,6 @@ constexpr char kCommandPrefix[] = "passwordForm";
 // Handler for injected JavaScript callbacks.
 - (BOOL)handleScriptCommand:(const base::DictionaryValue&)JSONCommand;
 
-// Finds the currently submitted password form named |formName| and calls
-// |completionHandler| with the populated data structure. |found| is YES if the
-// current form was found successfully, NO otherwise.
-- (void)extractSubmittedPasswordForm:(const std::string&)formName
-                   completionHandler:
-                       (void (^)(BOOL found,
-                                 const FormData& form))completionHandler;
-
 // Parses the |jsonString| which contatins the password forms found on a web
 // page to populate the |forms| vector.
 - (void)getPasswordFormsFromJSON:(NSString*)jsonString
@@ -181,24 +173,16 @@ constexpr char kCommandPrefix[] = "passwordForm";
     // origin.
     return;
   }
-  __weak PasswordFormHelper* weakSelf = self;
-  // This code is racing against the new page loading and will not get the
-  // password form data if the page has changed. In most cases this code wins
-  // the race.
-  // TODO(crbug.com/418827): Fix this by passing in more data from the JS side.
-  id completionHandler = ^(BOOL found, const FormData& form) {
-    PasswordFormHelper* strongSelf = weakSelf;
-    id<PasswordFormHelperDelegate> strongDelegate = strongSelf.delegate;
-    if (!strongSelf || !strongSelf->_webState || !strongDelegate) {
-      return;
-    }
-    [strongDelegate formHelper:strongSelf
-                 didSubmitForm:form
-                   inMainFrame:formInMainFrame];
-  };
-  // TODO(crbug.com/418827): Use |formData| instead of extracting form again.
-  [self extractSubmittedPasswordForm:formName
-                   completionHandler:completionHandler];
+  if (!self.delegate || formData.empty())
+    return;
+  FormData form;
+  NSString* nsFormData = [NSString stringWithUTF8String:formData.c_str()];
+  std::unique_ptr<base::Value> formValue = autofill::ParseJson(nsFormData);
+  autofill::ExtractFormData(*formValue, false, base::string16(), pageURL,
+                            pageURL.GetOrigin(), &form);
+  [self.delegate formHelper:self
+              didSubmitForm:form
+                inMainFrame:formInMainFrame];
 }
 
 #pragma mark - Private methods
@@ -232,43 +216,6 @@ constexpr char kCommandPrefix[] = "passwordForm";
   return NO;
 }
 
-- (void)extractSubmittedPasswordForm:(const std::string&)formName
-                   completionHandler:
-                       (void (^)(BOOL found,
-                                 const FormData& form))completionHandler {
-  DCHECK(completionHandler);
-
-  if (!_webState) {
-    return;
-  }
-
-  GURL pageURL;
-  if (!GetPageURLAndCheckTrustLevel(_webState, &pageURL)) {
-    completionHandler(NO, FormData());
-    return;
-  }
-
-  id extractSubmittedFormCompletionHandler = ^(NSString* jsonString) {
-    std::unique_ptr<base::Value> formValue = autofill::ParseJson(jsonString);
-    if (!formValue) {
-      completionHandler(NO, FormData());
-      return;
-    }
-
-    FormData form;
-    if (!autofill::ExtractFormData(*formValue, false, base::string16(), pageURL,
-                                   pageURL.GetOrigin(), &form)) {
-      completionHandler(NO, FormData());
-      return;
-    }
-
-    completionHandler(YES, form);
-  };
-
-  [self.jsPasswordManager extractForm:base::SysUTF8ToNSString(formName)
-                    completionHandler:extractSubmittedFormCompletionHandler];
-}
-
 - (void)getPasswordFormsFromJSON:(NSString*)jsonString
                          pageURL:(const GURL&)pageURL
                            forms:(std::vector<FormData>*)forms {

components/password_manager/ios/password_form_helper_unittest.mm

@@ -42,12 +42,6 @@ using test_helpers::SetFillData;
 
 @interface PasswordFormHelper (Testing)
 
-// Provides access to the method below for testing with mocks.
-- (void)extractSubmittedPasswordForm:(const std::string&)formName
-                   completionHandler:
-                       (void (^)(BOOL found,
-                                 const PasswordForm& form))completionHandler;
-
 // Provides access to replace |jsPasswordManager| with Mock one for test.
 - (void)setJsPasswordManager:(JsPasswordManager*)jsPasswordManager;
 
@@ -163,97 +157,6 @@ class PasswordFormHelperTest : public web::WebTestWithWebState {
   DISALLOW_COPY_AND_ASSIGN(PasswordFormHelperTest);
 };
 
-struct GetSubmittedPasswordFormTestData {
-  // HTML String of the form.
-  NSString* html_string;
-  // Javascript to submit the form.
-  NSString* java_script;
-  // 0 based index of the form on the page to submit.
-  const int index_of_the_form_to_submit;
-  // Expected number of fields in found form.
-  const size_t expected_number_of_fields;
-  // Expected form name.
-  const char* expected_form_name;
-};
-
-// Check that HTML forms are captured and converted correctly into
-// PasswordForms on submission.
-TEST_F(PasswordFormHelperTest, GetSubmittedPasswordForm) {
-  // clang-format off
-  const GetSubmittedPasswordFormTestData test_data[] = {
-    // Two forms with no explicit names.
-    {
-      @"<form action='javascript:;'>"
-      "<input type='text' name='user1' value='user1'>"
-      "<input type='password' name='pass1' value='pw1'>"
-      "</form>"
-      "<form action='javascript:;'>"
-      "<input type='text' name='user2' value='user2'>"
-      "<input type='password' name='pass2' value='pw2'>"
-      "<input type='submit' id='s2'>"
-      "</form>",
-      @"document.getElementById('s2').click()",
-      1, 2, "gChrome~form~1"
-    },
-    // Two forms with explicit names.
-    {
-      @"<form name='test2a' action='javascript:;'>"
-      "<input type='text' name='user1' value='user1'>"
-      "<input type='password' name='pass1' value='pw1'>"
-      "<input type='submit' id='s1'>"
-      "</form>"
-      "<form name='test2b' action='javascript:;' value='user2'>"
-      "<input type='text' name='user2'>"
-      "<input type='password' name='pass2' value='pw2'>"
-      "</form>",
-      @"document.getElementById('s1').click()",
-      0, 2, "test2a"
-    },
-    // Not-password form.
-    {
-      @"<form action='javascript:;' name='form1'>"
-      "<input type='text' name='user1' value='user1'>"
-      "<input type='text' name='not_pass1' value='text1'>"
-      "<input type='submit' id='s1'>"
-      "</form>",
-      @"document.getElementById('s1').click()",
-      0, 2, "form1"
-    },
-    // Form with quotes in the form and field names.
-    {
-      @"<form name=\"foo'\" action='javascript:;'>"
-      "<input type='text' name=\"user1'\" value='user1'>"
-      "<input type='password' id='s1' name=\"pass1'\" value='pw2'>"
-      "</form>",
-      @"document.getElementById('s1').click()",
-      0, 2, "foo'"
-    },
-  };
-  // clang-format on
-
-  for (const GetSubmittedPasswordFormTestData& data : test_data) {
-    SCOPED_TRACE(testing::Message() << "for html_string=" << data.html_string
-                                    << " and java_script=" << data.java_script
-                                    << " and index_of_the_form_to_submit="
-                                    << data.index_of_the_form_to_submit);
-    LoadHtml(data.html_string);
-    ExecuteJavaScript(data.java_script);
-    __block BOOL block_was_called = NO;
-    id completion_handler = ^(BOOL found, const FormData& form) {
-      block_was_called = YES;
-      EXPECT_EQ(data.expected_number_of_fields, form.fields.size());
-      EXPECT_EQ(data.expected_form_name, base::UTF16ToUTF8(form.name));
-    };
-    [helper_
-        extractSubmittedPasswordForm:GetFormId(data.index_of_the_form_to_submit)
-                   completionHandler:completion_handler];
-    EXPECT_TRUE(
-        WaitUntilConditionOrTimeout(kWaitForJSCompletionTimeout, ^bool() {
-          return block_was_called;
-        }));
-  }
-}
-
 struct FindPasswordFormTestData {
   // HTML String of the form.
   NSString* html_string;

ios/chrome/browser/passwords/password_controller_unittest.mm

@@ -174,11 +174,6 @@ ACTION(InvokeEmptyConsumerWithForms) {
 // Provides access to JavaScript Manager for testing with mocks.
 @property(readonly) JsPasswordManager* jsPasswordManager;
 
-- (void)extractSubmittedPasswordForm:(const std::string&)formName
-                   completionHandler:
-                       (void (^)(BOOL found,
-                                 const PasswordForm& form))completionHandler;
-
 - (void)findPasswordFormsWithCompletionHandler:
     (void (^)(const std::vector<PasswordForm>&))completionHandler;
 
@@ -392,99 +387,6 @@ TEST_F(PasswordControllerTest, FLAKY_FindPasswordFormsInView) {
   }
 }
 
-struct GetSubmittedPasswordFormTestData {
-  // HTML String of the form.
-  NSString* html_string;
-  // Javascript to submit the form.
-  NSString* java_script;
-  // 0 based index of the form on the page to submit.
-  const int index_of_the_form_to_submit;
-  // Expected number of fields in found form.
-  const size_t expected_number_of_fields;
-  // Expected form name.
-  const char* expected_form_name;
-};
-
-// TODO(crbug.com/403705) This test is flaky.
-// Check that HTML forms are captured and converted correctly into
-// PasswordForms on submission.
-TEST_F(PasswordControllerTest, FLAKY_GetSubmittedPasswordForm) {
-  // clang-format off
-  GetSubmittedPasswordFormTestData test_data[] = {
-    // Two forms with no explicit names.
-    {
-      @"<form action='javascript:;'>"
-          "<input type='text' name='user1' value='user1'>"
-          "<input type='password' name='pass1' value='pw1'>"
-          "</form>"
-          "<form action='javascript:;'>"
-          "<input type='text' name='user2' value='user2'>"
-          "<input type='password' name='pass2' value='pw2'>"
-          "<input type='submit' id='s2'>"
-          "</form>",
-      @"document.getElementById('s2').click()",
-      1, 2, "gChrome~form~1"
-    },
-    // Two forms with explicit names.
-    {
-      @"<form name='test2a' action='javascript:;'>"
-          "<input type='text' name='user1' value='user1'>"
-          "<input type='password' name='pass1' value='pw1'>"
-          "<input type='submit' id='s1'>"
-          "</form>"
-          "<form name='test2b' action='javascript:;' value='user2'>"
-          "<input type='text' name='user2'>"
-          "<input type='password' name='pass2' value='pw2'>"
-          "</form>",
-      @"document.getElementById('s1').click()",
-      0, 2, "test2a"
-    },
-    // No password forms.
-    {
-      @"<form action='javascript:;'>"
-          "<input type='text' name='user1' value='user1'>"
-          "<input type='text' name='pass1' value='text1'>"
-          "<input type='submit' id='s1'>"
-          "</form>",
-      @"document.getElementById('s1').click()",
-      0, 2, "gChrome~form~0"
-    },
-    // Form with quotes in the form and field names.
-    {
-      @"<form name=\"foo'\" action='javascript:;'>"
-          "<input type='text' name=\"user1'\" value='user1'>"
-          "<input type='password' id='s1' name=\"pass1'\" value='pw2'>"
-          "</form>",
-      @"document.getElementById('s1').click()",
-      0, 2, "foo'"
-    },
-  };
-  // clang-format on
-
-  for (const GetSubmittedPasswordFormTestData& data : test_data) {
-    SCOPED_TRACE(testing::Message() << "for html_string="
-                                    << base::SysNSStringToUTF8(data.html_string)
-                                    << " and java_script=" << data.java_script
-                                    << " and index_of_the_form_to_submit="
-                                    << data.index_of_the_form_to_submit);
-    LoadHtml(data.html_string);
-    ExecuteJavaScript(data.java_script);
-    __block BOOL block_was_called = NO;
-    id completion_handler = ^(BOOL found, const FormData& form) {
-      block_was_called = YES;
-      EXPECT_EQ(data.expected_number_of_fields, form.fields.size());
-      EXPECT_EQ(data.expected_form_name, base::UTF16ToUTF8(form.name));
-    };
-    [passwordController_.formHelper
-        extractSubmittedPasswordForm:FormName(data.index_of_the_form_to_submit)
-                   completionHandler:completion_handler];
-    EXPECT_TRUE(
-        WaitUntilConditionOrTimeout(kWaitForJSCompletionTimeout, ^bool() {
-          return block_was_called;
-        }));
-  }
-}
-
 // Test HTML page.  It contains several password forms.  Tests autofill
 // them and verify that the right ones are autofilled.
 static NSString* kHtmlWithMultiplePasswordForms =