[Autofill] Only query for newly parsed forms

components/autofill/core/browser/autofill_manager.cc

Index: components/autofill/core/browser/autofill_manager.cc
diff --git a/components/autofill/core/browser/autofill_manager.cc b/components/autofill/core/browser/autofill_manager.cc
index e667d61e77b0877e439fd3c2c534e10e8d2e369a..296d5d4fc7b5eeaba241a84961e2b77e5d01dcd2 100644
--- a/components/autofill/core/browser/autofill_manager.cc
+++ b/components/autofill/core/browser/autofill_manager.cc
@@ -1398,6 +1398,7 @@ void AutofillManager::ParseForms(const std::vector<FormData>& forms) {
     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));
 
@@ -1411,24 +1412,23 @@ void AutofillManager::ParseForms(const std::vector<FormData>& forms) {
 
     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