Fix password form matching across frames

NewPasswordFormManager::DoesManage matches FormData by their
unique_renderer_id. This ID is unique within one renderer process.

If two frames are hosted by different processes, and each has a form,
those forms can end up with the same ID but still unrelated. The
DoesManage method would not distinguish them alone, leading to not
creating a form manager for the second form.

Therefore, this CL adds a check against a driver in addition to the
ID.

Bug: 831123
Change-Id: I93502d6cfaf0daa58071046e05e8e93b6c6dacbb
Reviewed-on: https://chromium-review.googlesource.com/1069074
Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560655}

components/password_manager/core/browser/new_password_form_manager.cc

@@ -88,7 +88,11 @@ NewPasswordFormManager::NewPasswordFormManager(
 }
 NewPasswordFormManager::~NewPasswordFormManager() = default;
 
-bool NewPasswordFormManager::DoesManage(const autofill::FormData& form) const {
+bool NewPasswordFormManager::DoesManage(
+    const autofill::FormData& form,
+    const PasswordManagerDriver* driver) const {
+  if (driver != driver_.get())
+    return false;
   if (observed_form_.is_form_tag != form.is_form_tag)
     return false;
   // All unowned input elements are considered as one synthetic form.
@@ -131,8 +135,9 @@ void NewPasswordFormManager::ProcessMatches(
 }
 
 bool NewPasswordFormManager::SetSubmittedFormIfIsManaged(
-    const autofill::FormData& submitted_form) {
-  if (!DoesManage(submitted_form))
+    const autofill::FormData& submitted_form,
+    const PasswordManagerDriver* driver) {
+  if (!DoesManage(submitted_form, driver))
     return false;
   submitted_form_ = submitted_form;
   is_submitted_ = true;

components/password_manager/core/browser/new_password_form_manager.h

@@ -35,13 +35,16 @@ class NewPasswordFormManager : public FormFetcher::Consumer {
   ~NewPasswordFormManager() override;
 
   // Compares |observed_form_| with |form| and returns true if they are the
-  // same.
-  bool DoesManage(const autofill::FormData& form) const;
-
-  // If |submitted_form| is managed by *this (i.e. DoesManage returns
-  // true) then saves |submitted_form| to |submitted_form_| field, sets
-  // |is_submitted| = true and returns true. Otherwise returns false.
-  bool SetSubmittedFormIfIsManaged(const autofill::FormData& submitted_form);
+  // same and if |driver| is the same as |driver_|.
+  bool DoesManage(const autofill::FormData& form,
+                  const PasswordManagerDriver* driver) const;
+
+  // If |submitted_form| is managed by *this (i.e. DoesManage returns true for
+  // |submitted_form| and |driver|) then saves |submitted_form| to
+  // |submitted_form_| field, sets |is_submitted| = true and returns true.
+  // Otherwise returns false.
+  bool SetSubmittedFormIfIsManaged(const autofill::FormData& submitted_form,
+                                   const PasswordManagerDriver* driver);
   bool is_submitted() { return is_submitted_; }
   void set_not_submitted() { is_submitted_ = false; }
 

components/password_manager/core/browser/new_password_form_manager_unittest.cc

@@ -81,14 +81,16 @@ TEST_F(NewPasswordFormManagerTest, DoesManage) {
   fetcher.Fetch();
   NewPasswordFormManager form_manager(&client_, driver_.AsWeakPtr(),
                                       observed_form_, &fetcher);
-  EXPECT_TRUE(form_manager.DoesManage(observed_form_));
+  EXPECT_TRUE(form_manager.DoesManage(observed_form_, &driver_));
+  // Forms on other drivers are not considered managed.
+  EXPECT_FALSE(form_manager.DoesManage(observed_form_, nullptr));
   FormData another_form = observed_form_;
   another_form.is_form_tag = false;
-  EXPECT_FALSE(form_manager.DoesManage(another_form));
+  EXPECT_FALSE(form_manager.DoesManage(another_form, &driver_));
 
   another_form = observed_form_;
   another_form.unique_renderer_id = observed_form_.unique_renderer_id + 1;
-  EXPECT_FALSE(form_manager.DoesManage(another_form));
+  EXPECT_FALSE(form_manager.DoesManage(another_form, &driver_));
 }
 
 TEST_F(NewPasswordFormManagerTest, DoesManageNoFormTag) {
@@ -100,7 +102,9 @@ TEST_F(NewPasswordFormManagerTest, DoesManageNoFormTag) {
   FormData another_form = observed_form_;
   // Simulate that new input was added by JavaScript.
   another_form.fields.push_back(FormFieldData());
-  EXPECT_TRUE(form_manager.DoesManage(another_form));
+  EXPECT_TRUE(form_manager.DoesManage(another_form, &driver_));
+  // Forms on other drivers are not considered managed.
+  EXPECT_FALSE(form_manager.DoesManage(another_form, nullptr));
 }
 
 TEST_F(NewPasswordFormManagerTest, Autofill) {
@@ -127,21 +131,29 @@ TEST_F(NewPasswordFormManagerTest, SetSubmitted) {
   NewPasswordFormManager form_manager(&client_, driver_.AsWeakPtr(),
                                       observed_form_, &fetcher);
   EXPECT_FALSE(form_manager.is_submitted());
-  EXPECT_TRUE(form_manager.SetSubmittedFormIfIsManaged(observed_form_));
+  EXPECT_TRUE(
+      form_manager.SetSubmittedFormIfIsManaged(observed_form_, &driver_));
   EXPECT_TRUE(form_manager.is_submitted());
 
   FormData another_form = observed_form_;
   another_form.name += ASCIIToUTF16("1");
   // |another_form| is managed because the same |unique_renderer_id| as
   // |observed_form_|.
-  EXPECT_TRUE(form_manager.SetSubmittedFormIfIsManaged(another_form));
+  EXPECT_TRUE(form_manager.SetSubmittedFormIfIsManaged(another_form, &driver_));
   EXPECT_TRUE(form_manager.is_submitted());
 
   form_manager.set_not_submitted();
   EXPECT_FALSE(form_manager.is_submitted());
 
   another_form.unique_renderer_id = observed_form_.unique_renderer_id + 1;
-  EXPECT_FALSE(form_manager.SetSubmittedFormIfIsManaged(another_form));
+  EXPECT_FALSE(
+      form_manager.SetSubmittedFormIfIsManaged(another_form, &driver_));
+  EXPECT_FALSE(form_manager.is_submitted());
+
+  // An identical form but in a different frame (represented here by a null
+  // driver) is also not considered managed.
+  EXPECT_FALSE(
+      form_manager.SetSubmittedFormIfIsManaged(observed_form_, nullptr));
   EXPECT_FALSE(form_manager.is_submitted());
 }
 

components/password_manager/core/browser/password_manager.cc

@@ -472,7 +472,7 @@ void PasswordManager::OnPasswordFormSubmitted(
     const PasswordForm& password_form) {
   if (base::FeatureList::IsEnabled(
           password_manager::features::kNewPasswordFormParsing))
-    ProcessSubmittedForm(password_form.form_data);
+    ProcessSubmittedForm(password_form.form_data, driver);
   ProvisionallySavePassword(password_form, driver);
   pending_login_managers_.clear();
 }
@@ -487,7 +487,7 @@ void PasswordManager::OnPasswordFormSubmittedNoChecks(
 
   if (base::FeatureList::IsEnabled(
           password_manager::features::kNewPasswordFormParsing))
-    ProcessSubmittedForm(password_form.form_data);
+    ProcessSubmittedForm(password_form.form_data, driver);
   ProvisionallySavePassword(password_form, driver);
 
   if (CanProvisionalManagerSave())
@@ -669,8 +669,8 @@ void PasswordManager::CreateFormManagers(
   for (const autofill::PasswordForm& form : forms) {
     bool form_manager_exists =
         std::any_of(form_managers_.begin(), form_managers_.end(),
-                    [&form](const auto& form_manager) {
-                      return form_manager->DoesManage(form.form_data);
+                    [&form, driver](const auto& form_manager) {
+                      return form_manager->DoesManage(form.form_data, driver);
                     });
     if (!form_manager_exists)
       new_forms.push_back(&form.form_data);
@@ -685,10 +685,12 @@ void PasswordManager::CreateFormManagers(
   }
 }
 
-void PasswordManager::ProcessSubmittedForm(const FormData& submitted_form) {
+void PasswordManager::ProcessSubmittedForm(
+    const FormData& submitted_form,
+    const PasswordManagerDriver* driver) {
   NewPasswordFormManager* matching_form_manager = nullptr;
   for (const auto& manager : form_managers_) {
-    if (manager->SetSubmittedFormIfIsManaged(submitted_form)) {
+    if (manager->SetSubmittedFormIfIsManaged(submitted_form, driver)) {
       matching_form_manager = manager.get();
       break;
     }

components/password_manager/core/browser/password_manager.h

@@ -230,8 +230,10 @@ class PasswordManager : public LoginModel {
                           const std::vector<autofill::PasswordForm>& forms);
 
   // Passes |submitted_form| to NewPasswordManager that manages it for using it
-  // after detecting submission success for saving.
-  void ProcessSubmittedForm(const autofill::FormData& submitted_form);
+  // after detecting submission success for saving. |driver| is needed to
+  // determine the match.
+  void ProcessSubmittedForm(const autofill::FormData& submitted_form,
+                            const PasswordManagerDriver* driver);
 
   // Returns the best match in |pending_login_managers_| for |form|. May return
   // nullptr if no match exists.

components/password_manager/core/browser/password_manager_unittest.cc

@@ -1151,17 +1151,85 @@ TEST_F(PasswordManagerTest, FormSubmitWithOnlyPasswordField) {
   form_manager_to_save->Save();
 }
 
-TEST_F(PasswordManagerTest, FillPasswordOnManyFrames) {
-  // A password form should be filled in all frames it appears in.
-  PasswordForm form(MakeSimpleForm());  // The observed and saved form.
+// Test that if there are two "similar" forms in different frames, both get
+// filled. This means slightly different things depending on whether the
+// kNewPasswordFormParsing feature is enabled or not, so it is covered by two
+// tests below.
+
+// If kNewPasswordFormParsing is enabled, then "similar" is governed by
+// NewPasswordFormManager::DoesManage, which in turn delegates to the unique
+// renderer ID of the forms being the same. Note, however, that such ID is only
+// unique within one renderer process. If different frames on the page are
+// rendered by different processes, two unrelated forms can end up with the same
+// ID. The test checks that nevertheless each of them gets assigned its own
+// NewPasswordFormManager and filled as expected.
+TEST_F(PasswordManagerTest, FillPasswordOnManyFrames_SameId) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(features::kNewPasswordFormParsing);
+
+  // Two unrelated forms...
+  FormData form_data;
+  form_data.origin = GURL("http://www.google.com/a/LoginAuth");
+  form_data.action = GURL("http://www.google.com/a/Login");
+  form_data.fields.resize(2);
+  form_data.fields[0].name = ASCIIToUTF16("Email");
+  form_data.fields[0].value = ASCIIToUTF16("googleuser");
+  form_data.fields[0].form_control_type = "text";
+  form_data.fields[1].name = ASCIIToUTF16("Passwd");
+  form_data.fields[1].value = ASCIIToUTF16("p4ssword");
+  form_data.fields[1].form_control_type = "password";
+  PasswordForm first_form;
+  first_form.form_data = form_data;
+
+  form_data.origin = GURL("http://www.example.com/");
+  form_data.action = GURL("http://www.example.com/");
+  form_data.fields[0].name = ASCIIToUTF16("User");
+  form_data.fields[0].value = ASCIIToUTF16("exampleuser");
+  form_data.fields[1].name = ASCIIToUTF16("Pwd");
+  form_data.fields[1].value = ASCIIToUTF16("1234");
+  PasswordForm second_form;
+  second_form.form_data = form_data;
+
+  // Make the forms be "similar".
+  first_form.form_data.unique_renderer_id =
+      second_form.form_data.unique_renderer_id = 7654;
+
+  // The following expectation covers the calls from the old
+  // PasswordFormManager.
+  EXPECT_CALL(*store_, GetLogins(PasswordStore::FormDigest(PasswordForm()), _))
+      .Times(2);
+
+  // Observe the form in the first frame.
+  EXPECT_CALL(*store_,
+              GetLogins(PasswordStore::FormDigest(first_form.form_data), _))
+      .WillOnce(WithArg<1>(InvokeConsumer(first_form)));
+  EXPECT_CALL(driver_, FillPasswordForm(_));
+  manager()->OnPasswordFormsParsed(&driver_, {first_form});
+
+  // Observe the form in the second frame.
+  MockPasswordManagerDriver driver_b;
+  EXPECT_CALL(*store_,
+              GetLogins(PasswordStore::FormDigest(second_form.form_data), _))
+      .WillOnce(WithArg<1>(InvokeConsumer(second_form)));
+  EXPECT_CALL(driver_b, FillPasswordForm(_));
+  manager()->OnPasswordFormsParsed(&driver_b, {second_form});
+}
+
+// If kNewPasswordFormParsing is disabled, "similar" is governed by
+// PasswordFormManager::DoesManage and is related to actual similarity of the
+// forms, including having the same signon realm (and hence origin). Should a
+// page have two frames with the same origin and a form, and those two forms be
+// similar, then it is important to ensure that the single governing
+// PasswordFormManager knows about both PasswordManagerDriver instances and
+// instructs them to fill.
+TEST_F(PasswordManagerTest, FillPasswordOnManyFrames_SameForm) {
+  PasswordForm same_form = MakeSimpleForm();
 
   // Observe the form in the first frame.
-  std::vector<PasswordForm> observed;
-  observed.push_back(form);
   EXPECT_CALL(driver_, FillPasswordForm(_));
   EXPECT_CALL(*store_, GetLogins(_, _))
-      .WillOnce(WithArg<1>(InvokeConsumer(form)));
-  manager()->OnPasswordFormsParsed(&driver_, observed);
+      .WillOnce(WithArg<1>(InvokeConsumer(same_form)));
+  manager()->OnPasswordFormsParsed(&driver_, {same_form});
 
   // Now the form will be seen the second time, in a different frame. The driver
   // for that frame should be told to fill it, but the store should not be asked
@@ -1169,7 +1237,7 @@ TEST_F(PasswordManagerTest, FillPasswordOnManyFrames) {
   MockPasswordManagerDriver driver_b;
   EXPECT_CALL(driver_b, FillPasswordForm(_));
   EXPECT_CALL(*store_, GetLogins(_, _)).Times(0);
-  manager()->OnPasswordFormsParsed(&driver_b, observed);
+  manager()->OnPasswordFormsParsed(&driver_b, {same_form});
 }
 
 TEST_F(PasswordManagerTest, SameDocumentNavigation) {
@@ -2324,7 +2392,8 @@ TEST_F(PasswordManagerTest, CreatingFormManagers) {
   manager()->OnPasswordFormsParsed(&driver_, observed);
   // Check that the form manager is created.
   EXPECT_EQ(1u, manager()->form_managers().size());
-  EXPECT_TRUE(manager()->form_managers()[0]->DoesManage(form.form_data));
+  EXPECT_TRUE(manager()->form_managers()[0]->DoesManage(form.form_data,
+                                                        client_.GetDriver()));
 
   // Check that receiving the same form the second time does not lead to
   // creating new form manager.