[Autofill] Only query for newly parsed forms

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 <limits>
 #include <map>
@@ skipping 1380 matching lines @@
         MakeFrontendID(suggestions[i].backend_id, std::string());
   }
   return suggestions;
 }
 
 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) {
     scoped_ptr<FormStructure> form_structure(new FormStructure(form));
 
     if (!form_structure->ShouldBeParsed()) {
       if (form_structure->has_password_field()) {
         AutofillMetrics::LogPasswordFormQueryVolume(
             AutofillMetrics::NEW_PASSWORD_QUERY);
       }
       continue;
     }
 
     form_structure->DetermineHeuristicTypes();
 
-    if (form_structure->ShouldBeCrowdsourced()) {
+    FormStructure* structure = form_structure.release();

[Evan Stade, 2015/09/29 22:06:45]

This ownership model is super brittle and needs to be improved. You should not
use release ever. You can do form_structures_.push_back(structure.Pass())
followed by form_structures_.back() to get a reference.

Does SendAutofillTypePredictionsToRenderer take ownership? If so it now has
potential to double-free non queryable forms. If not, then the old code was
leaking.

[Mathieu, 2015/09/30 13:09:54]

SendAutofillTypePredictionsToRenderer does not take ownership (neither does
StartQueryRequest). I don't think the old code was leaking: the pointer was
transferred to |non_queryable_forms| but was put in |form_structures_| (a
ScopedVector) a few lines later (but you're right this was brittle). 

The new code transfers ownership clearly early on and then uses references from
|form_structures_| like you suggested.

+    form_structures_.push_back(structure);
+    if (structure->ShouldBeCrowdsourced()) {
       AutofillMetrics::LogPasswordFormQueryVolume(
           AutofillMetrics::CURRENT_QUERY);
-      form_structures_.push_back(form_structure.release());
+      queryable_forms.push_back(structure);
     } else {
-      non_queryable_forms.push_back(form_structure.release());
+      non_queryable_forms.push_back(structure);
     }
   }
 
-  if (!form_structures_.empty() && download_manager_) {
+  if (!queryable_forms.empty() && download_manager_) {
     // Query the server if at least one of the forms was parsed.
-    download_manager_->StartQueryRequest(form_structures_.get());
+    download_manager_->StartQueryRequest(queryable_forms);
   }
 
-  for (FormStructure* structure : non_queryable_forms)
-    form_structures_.push_back(structure);
-
-  if (!form_structures_.empty()) {
+  if (!queryable_forms.empty() || !non_queryable_forms.empty()) {
     AutofillMetrics::LogUserHappinessMetric(AutofillMetrics::FORMS_LOADED);
 #if defined(OS_IOS)
     // Log this from same location as AutofillMetrics::FORMS_LOADED to ensure
     // that KeyboardAccessoryButtonsIOS and UserHappiness UMA metrics will be
     // directly comparable.
     KeyboardAccessoryMetricsLogger::OnFormsLoaded();
 #endif
   }
 
   // For the |non_queryable_forms|, we have all the field type info we're ever
@@ skipping 118 matching lines @@
     if (i > 0)
       fputs("Next oldest form:\n", file);
   }
   fputs("\n", file);
 
   fclose(file);
 }
 #endif  // ENABLE_FORM_DEBUG_DUMP
 
 }  // namespace autofill