Improve autofill form matching.

components/autofill/core/browser/autofill_manager.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 "components/autofill/core/browser/autofill_manager.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
@@ skipping 42 matching lines @@
 #include "components/autofill/core/browser/validation.h"
 #include "components/autofill/core/common/autofill_clock.h"
 #include "components/autofill/core/common/autofill_constants.h"
 #include "components/autofill/core/common/autofill_data_validation.h"
 #include "components/autofill/core/common/autofill_pref_names.h"
 #include "components/autofill/core/common/autofill_util.h"
 #include "components/autofill/core/common/form_data.h"
 #include "components/autofill/core/common/form_data_predictions.h"
 #include "components/autofill/core/common/form_field_data.h"
 #include "components/autofill/core/common/password_form_fill_data.h"
+#include "components/autofill/core/common/signatures_util.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
 #include "components/rappor/public/rappor_utils.h"
 #include "components/rappor/rappor_service_impl.h"
 #include "components/security_state/core/security_state.h"
 #include "components/strings/grit/components_strings.h"
 #include "google_apis/gaia/identity_provider.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/geometry/rect.h"
 #include "url/gurl.h"
@@ skipping 1599 matching lines @@
   std::unique_ptr<FormStructure> submitted_form(new FormStructure(form));
   if (!ShouldUploadForm(*submitted_form))
     return std::unique_ptr<FormStructure>();
 
   // Ignore forms not present in our cache.  These are typically forms with
   // wonky JavaScript that also makes them not auto-fillable.
   FormStructure* cached_submitted_form;
   if (!FindCachedForm(form, &cached_submitted_form))
     return std::unique_ptr<FormStructure>();
 
-  submitted_form->UpdateFromCache(*cached_submitted_form);
+  submitted_form->UpdateFromCache(*cached_submitted_form, false);
   return submitted_form;
 }
 
 bool AutofillManager::FindCachedForm(const FormData& form,
                                      FormStructure** form_structure) const {
   // Find the FormStructure that corresponds to |form|.
   // Scan backward through the cached |form_structures_|, as updated versions of
   // forms are added to the back of the list, whereas original versions of these
   // forms might appear toward the beginning of the list.  The communication
   // protocol with the crowdsourcing server does not permit us to discard the
   // original versions of the forms.
   *form_structure = NULL;
+  const auto& form_signature = autofill::CalculateFormSignature(form);
   for (auto& cur_form : base::Reversed(form_structures_)) {
-    if (*cur_form == form) {
+    if (cur_form->form_signature() == form_signature || *cur_form == form) {
       *form_structure = cur_form.get();
 
       // The same form might be cached with multiple field counts: in some
       // cases, non-autofillable fields are filtered out, whereas in other cases
       // they are not.  To avoid thrashing the cache, keep scanning until we
       // find a cached version with the same number of fields, if there is one.
       if (cur_form->field_count() == form.fields.size())
         break;
     }
   }
@@ skipping 61 matching lines @@
                                        FormStructure** updated_form) {
   bool needs_update =
       (!cached_form ||
        live_form.fields.size() != cached_form->field_count());
   for (size_t i = 0; !needs_update && i < cached_form->field_count(); ++i)
     needs_update = !cached_form->field(i)->SameFieldAs(live_form.fields[i]);
 
   if (!needs_update)
     return true;
 
-  if (form_structures_.size() >= kMaxFormCacheSize)
-    return false;
-
-  // Add the new or updated form to our cache.

[sense (YandexTeam), 2017/03/07 07:10:28]

This logic is replaced by the call to ParseForm().

-  form_structures_.push_back(base::MakeUnique<FormStructure>(live_form));
-  *updated_form = form_structures_.rbegin()->get();
-  (*updated_form)->DetermineHeuristicTypes();
-
-  // If we have cached data, propagate it to the updated form.
-  if (cached_form) {
-    std::map<base::string16, const AutofillField*> cached_fields;

[sense (YandexTeam), 2017/03/07 07:10:28]

This update logic is replaced by the call to FormStructure::UpdateFromCache().
This code didn't perform the call to UpdateAutofillCount(). Without it the
update breaks the count of autofillable fields and the whole form becomes
non-autofillable if there were only server defined field types.
Also we should keep form_signature here as does UpdateFromCache().

-    for (size_t i = 0; i < cached_form->field_count(); ++i) {
-      const AutofillField* field = cached_form->field(i);
-      cached_fields[field->unique_name()] = field;
-    }
-
-    for (size_t i = 0; i < (*updated_form)->field_count(); ++i) {
-      AutofillField* field = (*updated_form)->field(i);
-      auto cached_field = cached_fields.find(field->unique_name());
-      if (cached_field != cached_fields.end()) {
-        field->set_server_type(cached_field->second->server_type());
-        field->is_autofilled = cached_field->second->is_autofilled;
-        field->set_previously_autofilled(
-            cached_field->second->previously_autofilled());
-      }
-    }
-
-    // Note: We _must not_ remove the original version of the cached form from
-    // the list of |form_structures_|.  Otherwise, we break parsing of the
-    // crowdsourcing server's response to our query.
+  // Note: We _must not_ remove the original version of the cached form from
+  // the list of |form_structures_|. Otherwise, we break parsing of the
+  // crowdsourcing server's response to our query.
+  if (!ParseForm(live_form, false, updated_form)) {
+    // New form was not added, so no update was performed.
+    return cached_form != nullptr;

[sebsg, 2017/03/08 18:32:08]

Can this change the return value compared to before? Before, if
(form_structures_.size() >= kMaxFormCacheSize) we returned false but here if
(form_structures_.size() >= kMaxFormCacheSize) but cached_form != nullptr we
will return true?

[sense (YandexTeam), 2017/03/09 04:56:36]

Yes, this change is incorrect. We shouldn't return the form if it's need to be
updated but we failed to do that.

   }
 
+  if (cached_form)
+    (*updated_form)->UpdateFromCache(*cached_form, true);
+
   // Annotate the updated form with its predicted types.
-  std::vector<FormStructure*> forms(1, *updated_form);
-  driver_->SendAutofillTypePredictionsToRenderer(forms);
+  driver_->SendAutofillTypePredictionsToRenderer({*updated_form});
 
   return true;
 }
 
 std::vector<Suggestion> AutofillManager::GetProfileSuggestions(
     const FormStructure& form,
     const FormFieldData& field,
     const AutofillField& autofill_field) const {
   address_form_event_logger_->OnDidPollSuggestions(field);
 
@@ skipping 40 matching lines @@
 
 void AutofillManager::ParseForms(const std::vector<FormData>& forms) {
   if (forms.empty())
     return;
 
   std::vector<FormStructure*> non_queryable_forms;
   std::vector<FormStructure*> queryable_forms;
   for (const FormData& form : forms) {
     const auto parse_form_start_time = base::TimeTicks::Now();
 
-    std::unique_ptr<FormStructure> form_structure =
-        base::MakeUnique<FormStructure>(form);
-    form_structure->ParseFieldTypesFromAutocompleteAttributes();
-    if (!form_structure->ShouldBeParsed())
+    FormStructure* form_structure = nullptr;
+    if (!ParseForm(form, true, &form_structure))

[sebsg, 2017/03/08 18:32:08]

I'm not sure why this needs to be true here? It means that this can continue if
we already have the form which is a logic that I do not see in the current code?
Could you explain why it is better to have it? Thanks!

[sense (YandexTeam), 2017/03/09 04:56:36]

The check for duplicates is from our internal improvements. It's not meant to be
in this CL. I'll remove it, sorry for that.

       continue;
+    DCHECK(form_structure);
 
-    form_structure->DetermineHeuristicTypes();
-
-    // Ownership is transferred to |form_structures_| which maintains it until
-    // the manager is Reset() or destroyed. It is safe to use references below
-    // as long as receivers don't take ownership.
-    form_structures_.push_back(std::move(form_structure));
-
-    if (form_structures_.back()->ShouldBeCrowdsourced())
-      queryable_forms.push_back(form_structures_.back().get());
+    // Set aside forms with method GET or author-specified types, so that they
+    // are not included in the query to the server.
+    if (form_structure->ShouldBeCrowdsourced())
+      queryable_forms.push_back(form_structure);
     else
-      non_queryable_forms.push_back(form_structures_.back().get());
+      non_queryable_forms.push_back(form_structure);
 
     AutofillMetrics::LogParseFormTiming(base::TimeTicks::Now() -
                                         parse_form_start_time);
   }
 
   if (!queryable_forms.empty() && download_manager_) {
     // Query the server if at least one of the forms was parsed.
     download_manager_->StartQueryRequest(queryable_forms);
   }
 
@@ skipping 21 matching lines @@
       autofill_assistant_.ShowAssistForCreditCard(*cards.front());
   }
 #endif
 
   // For the |non_queryable_forms|, we have all the field type info we're ever
   // going to get about them.  For the other forms, we'll wait until we get a
   // response from the server.
   driver_->SendAutofillTypePredictionsToRenderer(non_queryable_forms);
 }
 
+bool AutofillManager::ParseForm(const FormData& form,
+                                const bool check_for_duplicates,
+                                FormStructure** parsed_form_structure) {
+  DCHECK(parsed_form_structure);
+  if (form_structures_.size() >= kMaxFormCacheSize)

[sense (YandexTeam), 2017/03/07 07:10:28]

Previously this check was only in UpdateCachedForm. Now it also applies to the
initial ParseForms() call (OnFormsSeen->ParseForms).

+    return false;
+
+  auto form_structure = base::MakeUnique<FormStructure>(form);
+  form_structure->ParseFieldTypesFromAutocompleteAttributes();
+  if (!form_structure->ShouldBeParsed())
+    return false;
+
+  if (check_for_duplicates) {
+    // Check if we have got this form before since the agent uses a different
+    // form comparison method that can send us duplicates here.
+    for (const auto& existing_form_structure : form_structures_) {
+      // If we already got this form, skip it.
+      if (form_structure->form_signature() ==
+          existing_form_structure->form_signature()) {
+        return false;
+      }
+    }
+  }
+
+  form_structure->DetermineHeuristicTypes();
+
+  // Ownership is transferred to |form_structures_| which maintains it until
+  // the manager is Reset() or destroyed. It is safe to use references below
+  // as long as receivers don't take ownership.
+  form_structures_.push_back(std::move(form_structure));
+  *parsed_form_structure = form_structures_.back().get();
+  return true;
+}
+
 int AutofillManager::BackendIDToInt(const std::string& backend_id) const {
   if (!base::IsValidGUID(backend_id))
     return 0;
 
   const auto found = backend_to_int_map_.find(backend_id);
   if (found == backend_to_int_map_.end()) {
     // Unknown one, make a new entry.
     int int_id = backend_to_int_map_.size() + 1;
     backend_to_int_map_[backend_id] = int_id;
     int_to_backend_map_[int_id] = backend_id;
@@ skipping 250 matching lines @@
     if (i > 0)
       fputs("Next oldest form:\n", file);
   }
   fputs("\n", file);
 
   fclose(file);
 }
 #endif  // ENABLE_FORM_DEBUG_DUMP
 
 }  // namespace autofill