Fix crash in autofill metrics logging.

Cached forms might not match submitted forms, but the logging code had been assuming that they always do.

BUG=70374
TEST=unit_tests --gtest_filter=AutoFillMetricsTest.SaneMetricsWithCacheMismatch

Review URL: http://codereview.chromium.org/6259017

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@72453 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/autofill/autofill_manager.cc

@@ -498,14 +498,20 @@ void AutoFillManager::DeterminePossibleFieldTypesForUpload(
 void AutoFillManager::LogMetricsAboutSubmittedForm(
     const FormData& form,
     const FormStructure* submitted_form) {
-  FormStructure* cached_submitted_form = NULL;
-  AutoFillField* ignored;
-  if (!FindCachedFormAndField(form, form.fields.front(),
-                              &cached_submitted_form, &ignored)) {
-    cached_submitted_form = NULL;
+  FormStructure* cached_submitted_form;
+  if (!FindCachedForm(form, &cached_submitted_form)) {
+    NOTREACHED();
+    return;
   }
 
-  for (size_t i = 0; i < submitted_form->field_count(); i++) {
+  // Map from field signatures to cached fields.
+  std::map<std::string, const AutoFillField*> cached_fields;
+  for (size_t i = 0; i < cached_submitted_form->field_count(); ++i) {
+    const AutoFillField* field = cached_submitted_form->field(i);
+    cached_fields[field->FieldSignature()] = field;
+  }
+
+  for (size_t i = 0; i < submitted_form->field_count(); ++i) {
     const AutoFillField* field = submitted_form->field(i);
     FieldTypeSet field_types;
     personal_data_->GetPossibleFieldTypes(field->value(), &field_types);
@@ -527,9 +533,15 @@ void AutoFillManager::LogMetricsAboutSubmittedForm(
       } else {
         metric_logger_->Log(AutoFillMetrics::FIELD_AUTOFILL_FAILED);
 
-        AutoFillFieldType heuristic_type = cached_submitted_form?
-            cached_submitted_form->field(i)->heuristic_type() :
-            UNKNOWN_TYPE;
+        AutoFillFieldType heuristic_type = UNKNOWN_TYPE;
+        AutoFillFieldType server_type = NO_SERVER_DATA;
+        std::map<std::string, const AutoFillField*>::const_iterator
+            cached_field = cached_fields.find(field->FieldSignature());
+        if (cached_field != cached_fields.end()) {
+          heuristic_type = cached_field->second->heuristic_type();
+          server_type = cached_field->second->server_type();
+        }
+
         if (heuristic_type == UNKNOWN_TYPE)
           metric_logger_->Log(AutoFillMetrics::FIELD_HEURISTIC_TYPE_UNKNOWN);
         else if (field_types.count(heuristic_type))
@@ -537,9 +549,6 @@ void AutoFillManager::LogMetricsAboutSubmittedForm(
         else
           metric_logger_->Log(AutoFillMetrics::FIELD_HEURISTIC_TYPE_MISMATCH);
 
-        AutoFillFieldType server_type = cached_submitted_form?
-            cached_submitted_form->field(i)->server_type() :
-            NO_SERVER_DATA;
         if (server_type == NO_SERVER_DATA)
           metric_logger_->Log(AutoFillMetrics::FIELD_SERVER_TYPE_UNKNOWN);
         else if (field_types.count(server_type))
@@ -650,14 +659,12 @@ bool AutoFillManager::GetHost(const std::vector<AutoFillProfile*>& profiles,
   return true;
 }
 
-bool AutoFillManager::FindCachedFormAndField(const FormData& form,
-                                             const FormField& field,
-                                             FormStructure** form_structure,
-                                             AutoFillField** autofill_field) {
+bool AutoFillManager::FindCachedForm(const FormData& form,
+                                     FormStructure** form_structure) {
   // Find the FormStructure that corresponds to |form|.
   *form_structure = NULL;
   for (std::vector<FormStructure*>::const_iterator iter =
-           form_structures_.begin();
+       form_structures_.begin();
        iter != form_structures_.end(); ++iter) {
     if (**iter == form) {
       *form_structure = *iter;
@@ -668,6 +675,17 @@ bool AutoFillManager::FindCachedFormAndField(const FormData& form,
   if (!(*form_structure))
     return false;
 
+  return true;
+}
+
+bool AutoFillManager::FindCachedFormAndField(const FormData& form,
+                                             const FormField& field,
+                                             FormStructure** form_structure,
+                                             AutoFillField** autofill_field) {
+  // Find the FormStructure that corresponds to |form|.
+  if (!FindCachedForm(form, form_structure))
+    return false;
+
   // No data to return if there are no auto-fillable fields.
   if (!(*form_structure)->autofill_count())
     return false;

chrome/browser/autofill/autofill_manager.h

@@ -125,6 +125,11 @@ class AutoFillManager : public WebNavigationObserver,
                const std::vector<CreditCard*>& credit_cards,
                RenderViewHost** host) WARN_UNUSED_RESULT;
 
+  // Fills |form_structure| cached element corresponding to |form|.
+  // Returns false if the cached element was not found.
+  bool FindCachedForm(const webkit_glue::FormData& form,
+                      FormStructure** form_structure) WARN_UNUSED_RESULT;
+
   // Fills |form_structure| and |autofill_field| with the cached elements
   // corresponding to |form| and |field|. Returns false if the cached elements
   // were not found.
@@ -228,6 +233,7 @@ class AutoFillManager : public WebNavigationObserver,
   FRIEND_TEST_ALL_PREFIXES(AutoFillMetricsTest, QualityMetrics);
   FRIEND_TEST_ALL_PREFIXES(AutoFillMetricsTest,
                            NoQualityMetricsForNonAutoFillableForms);
+  FRIEND_TEST_ALL_PREFIXES(AutoFillMetricsTest, SaneMetricsWithCacheMismatch);
   FRIEND_TEST_ALL_PREFIXES(AutoFillMetricsTest, QualityMetricsForFailure);
 
   DISALLOW_COPY_AND_ASSIGN(AutoFillManager);

chrome/browser/autofill/autofill_metrics_unittest.cc

@@ -203,6 +203,11 @@ TEST_F(AutoFillMetricsTest, QualityMetrics) {
       "Select", "select", "USA", "select-one", &field);
   form.fields.push_back(field);
 
+  // Simulate having seen this form on page load.
+  // |form_structure| will be owned by |autofill_manager_|.
+  TestFormStructure* form_structure = new TestFormStructure(form);
+  autofill_manager_->AddSeenForm(form_structure);
+
   // Establish our expectations.
   ::testing::InSequence dummy;
   EXPECT_CALL(*autofill_manager_->metric_logger(),
@@ -319,8 +324,7 @@ TEST_F(AutoFillMetricsTest, QualityMetricsForFailure) {
   form_structure->SetFieldTypes(heuristic_types, server_types);
   autofill_manager_->AddSeenForm(form_structure);
 
-  // Establish our expectations. Only print gmock errors, as the warnings are
-  // too verbose.
+  // Establish our expectations.
   ::testing::InSequence dummy;
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(failure_cases); ++i) {
     EXPECT_CALL(*autofill_manager_->metric_logger(),
@@ -337,6 +341,96 @@ TEST_F(AutoFillMetricsTest, QualityMetricsForFailure) {
   EXPECT_NO_FATAL_FAILURE(autofill_manager_->OnFormSubmitted(form));
 }
 
+// Test that we behave sanely when the cached form differs from the submitted
+// one.
+TEST_F(AutoFillMetricsTest, SaneMetricsWithCacheMismatch) {
+  // Set up our form data.
+  FormData form;
+  form.name = ASCIIToUTF16("TestForm");
+  form.method = ASCIIToUTF16("POST");
+  form.origin = GURL("http://example.com/form.html");
+  form.action = GURL("http://example.com/submit.html");
+  form.user_submitted = true;
+
+  std::vector<AutoFillFieldType> heuristic_types, server_types;
+
+  FormField field;
+  autofill_test::CreateTestFormField(
+      "Both match", "match", "Elvis Presley", "text", &field);
+  field.set_autofilled(true);
+  form.fields.push_back(field);
+  heuristic_types.push_back(NAME_FULL);
+  server_types.push_back(NAME_FULL);
+  autofill_test::CreateTestFormField(
+      "Both mismatch", "mismatch", "buddy@gmail.com", "text", &field);
+  form.fields.push_back(field);
+  heuristic_types.push_back(PHONE_HOME_NUMBER);
+  server_types.push_back(PHONE_HOME_NUMBER);
+  autofill_test::CreateTestFormField(
+      "Only heuristics match", "mixed", "Memphis", "text", &field);
+  form.fields.push_back(field);
+  heuristic_types.push_back(ADDRESS_HOME_CITY);
+  server_types.push_back(PHONE_HOME_NUMBER);
+  autofill_test::CreateTestFormField(
+      "Unknown", "unknown", "garbage", "text", &field);
+  form.fields.push_back(field);
+  heuristic_types.push_back(UNKNOWN_TYPE);
+  server_types.push_back(UNKNOWN_TYPE);
+
+  // Simulate having seen this form with the desired heuristic and server types.
+  // |form_structure| will be owned by |autofill_manager_|.
+  TestFormStructure* form_structure = new TestFormStructure(form);
+  form_structure->SetFieldTypes(heuristic_types, server_types);
+  autofill_manager_->AddSeenForm(form_structure);
+
+  // Add a field and re-arrange the remaining form fields before submitting.
+  std::vector<FormField> cached_fields = form.fields;
+  form.fields.clear();
+  autofill_test::CreateTestFormField(
+      "New field", "new field", "Tennessee", "text", &field);
+  form.fields.push_back(field);
+  form.fields.push_back(cached_fields[2]);
+  form.fields.push_back(cached_fields[1]);
+  form.fields.push_back(cached_fields[3]);
+  form.fields.push_back(cached_fields[0]);
+
+  // Establish our expectations.
+  ::testing::InSequence dummy;
+  EXPECT_CALL(*autofill_manager_->metric_logger(),
+              Log(AutoFillMetrics::FIELD_SUBMITTED));
+  EXPECT_CALL(*autofill_manager_->metric_logger(),
+              Log(AutoFillMetrics::FIELD_AUTOFILL_FAILED));
+  EXPECT_CALL(*autofill_manager_->metric_logger(),
+              Log(AutoFillMetrics::FIELD_HEURISTIC_TYPE_UNKNOWN));
+  EXPECT_CALL(*autofill_manager_->metric_logger(),
+              Log(AutoFillMetrics::FIELD_SERVER_TYPE_UNKNOWN));
+  EXPECT_CALL(*autofill_manager_->metric_logger(),
+              Log(AutoFillMetrics::FIELD_SUBMITTED));
+  EXPECT_CALL(*autofill_manager_->metric_logger(),
+              Log(AutoFillMetrics::FIELD_AUTOFILL_FAILED));
+  EXPECT_CALL(*autofill_manager_->metric_logger(),
+              Log(AutoFillMetrics::FIELD_HEURISTIC_TYPE_MATCH));
+  EXPECT_CALL(*autofill_manager_->metric_logger(),
+              Log(AutoFillMetrics::FIELD_SERVER_TYPE_MISMATCH));
+  EXPECT_CALL(*autofill_manager_->metric_logger(),
+              Log(AutoFillMetrics::FIELD_SUBMITTED));
+  EXPECT_CALL(*autofill_manager_->metric_logger(),
+              Log(AutoFillMetrics::FIELD_AUTOFILL_FAILED));
+  EXPECT_CALL(*autofill_manager_->metric_logger(),
+              Log(AutoFillMetrics::FIELD_HEURISTIC_TYPE_MISMATCH));
+  EXPECT_CALL(*autofill_manager_->metric_logger(),
+              Log(AutoFillMetrics::FIELD_SERVER_TYPE_MISMATCH));
+  EXPECT_CALL(*autofill_manager_->metric_logger(),
+              Log(AutoFillMetrics::FIELD_SUBMITTED));
+  EXPECT_CALL(*autofill_manager_->metric_logger(),
+              Log(AutoFillMetrics::FIELD_SUBMITTED));
+  EXPECT_CALL(*autofill_manager_->metric_logger(),
+              Log(AutoFillMetrics::FIELD_AUTOFILLED));
+
+  // Simulate form submission.
+  EXPECT_NO_FATAL_FAILURE(autofill_manager_->OnFormSubmitted(form));
+}
+
 // Test that we don't log quality metrics for non-autofillable forms.
 TEST_F(AutoFillMetricsTest, NoQualityMetricsForNonAutoFillableForms) {
   // Forms must include at least three fields to be auto-fillable.