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.

This is a fix for a reverted change.

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

components/autofill/ios/form_util/resources/fill.js

@@ -815,8 +815,7 @@ __gCrWeb.fill.webFormElementToFormData = function(
   }
 
   form['name'] = __gCrWeb.form.getFormIdentifier(formElement);
-  form['origin'] =
-      __gCrWeb.common.removeQueryAndReferenceFromURL(frame.location.href);
+  form['origin'] = __gCrWeb.common.removeQueryAndReferenceFromURL(frame.origin);
   form['action'] = __gCrWeb.fill.getCanonicalActionForForm(formElement);
 
   // The raw name and id attributes, which may be empty.

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,17 @@ 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;
+  std::vector<FormData> forms;
+  NSString* nsFormData = [NSString stringWithUTF8String:formData.c_str()];
+  autofill::ExtractFormsData(nsFormData, false, base::string16(), pageURL,
+                             pageURL.GetOrigin(), &forms);
+  if (forms.size() != 1)
+    return;
+  [self.delegate formHelper:self
+              didSubmitForm:forms[0]
+                inMainFrame:formInMainFrame];
 }
 
 #pragma mark - Private methods
@@ -232,43 +217,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 =