Render crash in FormManager::FindCachedFormElement()

To address the vulnerability of stale WebFrame pointers in the FormManager's cache this CL changes the cache from a map (with the WebFrame pointer as "key") to a flat vector of simplified "FormElement*" items.

To avoid leaking memory, we need to still observe |frameDetached|, and use that as a signal to reap any associated WebFormElements or WebFormControlElements.

BUG=48857
TEST=FormMananagerTest.*, and manual test of regular form filling, form filling a form with sub-iframes, and form filling a form with sub-frames.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=60949

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

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

2 files changed, 71 insertions, 92 deletions

diff --git a/chrome/renderer/form_manager.cc b/chrome/renderer/form_manager.cc
index 17fb9aa..059aa8c 100644
--- a/chrome/renderer/form_manager.cc
+++ b/chrome/renderer/form_manager.cc
@@ -216,7 +216,7 @@ string16 InferLabelFromDefinitionList(
   return inferred_label;
 }
 
-void GetOptionStringsFromElement(WebKit::WebFormControlElement element,
+void GetOptionStringsFromElement(WebFormControlElement element,
                                  std::vector<string16>* option_strings) {
   DCHECK(!element.isNull());
   DCHECK(option_strings);
@@ -450,7 +450,7 @@ void FormManager::ExtractForms(const WebFrame* frame) {
       form_elements->control_elements.push_back(element);
     }
 
-    form_elements_map_[frame].push_back(form_elements);
+    form_elements_.push_back(form_elements);
   }
 }
 
@@ -458,17 +458,14 @@ void FormManager::GetForms(RequirementsMask requirements,
                            std::vector<FormData>* forms) {
   DCHECK(forms);
 
-  for (WebFrameFormElementMap::iterator iter = form_elements_map_.begin();
-       iter != form_elements_map_.end(); ++iter) {
-    for (std::vector<FormElement*>::iterator form_iter = iter->second.begin();
-         form_iter != iter->second.end(); ++form_iter) {
-      FormData form;
-      if (WebFormElementToFormData((*form_iter)->form_element,
-                                   requirements,
-                                   true,
-                                   &form))
-        forms->push_back(form);
-    }
+  for (FormElementList::iterator form_iter = form_elements_.begin();
+       form_iter != form_elements_.end(); ++form_iter) {
+    FormData form;
+    if (WebFormElementToFormData((*form_iter)->form_element,
+                                 requirements,
+                                 true,
+                                 &form))
+      forms->push_back(form);
   }
 }
 
@@ -478,17 +475,15 @@ void FormManager::GetFormsInFrame(const WebFrame* frame,
   DCHECK(frame);
   DCHECK(forms);
 
-  WebFrameFormElementMap::iterator iter = form_elements_map_.find(frame);
-  if (iter == form_elements_map_.end())
-    return;
-
   // TODO(jhawkins): Factor this out and use it here and in GetForms.
-  const std::vector<FormElement*>& form_elements = iter->second;
-  for (std::vector<FormElement*>::const_iterator form_iter =
-           form_elements.begin();
-       form_iter != form_elements.end(); ++form_iter) {
+  for (FormElementList::const_iterator form_iter =
+           form_elements_.begin();
+       form_iter != form_elements_.end(); ++form_iter) {
     FormElement* form_element = *form_iter;
 
+    if (form_element->form_element.document().frame() != frame)
+      continue;
+
     // We need at least |kRequiredAutoFillFields| fields before appending this
     // form to |forms|.
     if (form_element->control_elements.size() < kRequiredAutoFillFields)
@@ -514,14 +509,11 @@ bool FormManager::FindForm(const WebFormElement& element,
   if (!frame)
     return false;
 
-  WebFrameFormElementMap::const_iterator frame_iter =
-      form_elements_map_.find(frame);
-  if (frame_iter == form_elements_map_.end())
-    return false;
+  for (FormElementList::const_iterator iter = form_elements_.begin();
+       iter != form_elements_.end(); ++iter) {
+    if ((*iter)->form_element.document().frame() != frame)
+      continue;
 
-  for (std::vector<FormElement*>::const_iterator iter =
-           frame_iter->second.begin();
-       iter != frame_iter->second.end(); ++iter) {
     if ((*iter)->form_element.name() != element.name())
       continue;
 
@@ -541,14 +533,13 @@ bool FormManager::FindFormWithFormControlElement(
   if (!frame)
     return false;
 
-  if (form_elements_map_.find(frame) == form_elements_map_.end())
-    return false;
-
-  const std::vector<FormElement*> forms = form_elements_map_[frame];
-  for (std::vector<FormElement*>::const_iterator iter = forms.begin();
-       iter != forms.end(); ++iter) {
+  for (FormElementList::const_iterator iter = form_elements_.begin();
+       iter != form_elements_.end(); ++iter) {
     const FormElement* form_element = *iter;
 
+    if (form_element->form_element.document().frame() != frame)
+      continue;
+
     for (std::vector<WebFormControlElement>::const_iterator iter =
              form_element->control_elements.begin();
          iter != form_element->control_elements.end(); ++iter) {
@@ -563,7 +554,7 @@ bool FormManager::FindFormWithFormControlElement(
   return false;
 }
 
-bool FormManager::FillForm(const FormData& form, const WebKit::WebNode& node) {
+bool FormManager::FillForm(const FormData& form, const WebNode& node) {
   FormElement* form_element = NULL;
   if (!FindCachedFormElement(form, &form_element))
     return false;
@@ -595,7 +586,7 @@ bool FormManager::PreviewForm(const FormData& form) {
   return true;
 }
 
-bool FormManager::ClearFormWithNode(const WebKit::WebNode& node) {
+bool FormManager::ClearFormWithNode(const WebNode& node) {
   FormElement* form_element = NULL;
   if (!FindCachedFormElementWithNode(node, &form_element))
     return false;
@@ -618,7 +609,7 @@ bool FormManager::ClearFormWithNode(const WebKit::WebNode& node) {
   return true;
 }
 
-bool FormManager::ClearPreviewedFormWithNode(const WebKit::WebNode& node) {
+bool FormManager::ClearPreviewedFormWithNode(const WebNode& node) {
   FormElement* form_element = NULL;
   if (!FindCachedFormElementWithNode(node, &form_element))
     return false;
@@ -649,22 +640,22 @@ bool FormManager::ClearPreviewedFormWithNode(const WebKit::WebNode& node) {
 }
 
 void FormManager::Reset() {
-  for (WebFrameFormElementMap::iterator iter = form_elements_map_.begin();
-       iter != form_elements_map_.end(); ++iter) {
-    STLDeleteElements(&iter->second);
-  }
-  form_elements_map_.clear();
+  STLDeleteElements(&form_elements_);
 }
 
 void FormManager::ResetFrame(const WebFrame* frame) {
-  WebFrameFormElementMap::iterator iter = form_elements_map_.find(frame);
-  if (iter != form_elements_map_.end()) {
-    STLDeleteElements(&iter->second);
-    form_elements_map_.erase(iter);
+  FormElementList::iterator iter = form_elements_.begin();
+  while (iter != form_elements_.end()) {
+    if ((*iter)->form_element.document().frame() == frame) {
+      delete *iter;
+      iter = form_elements_.erase(iter);
+    } else {
+      ++iter;
+    }
   }
 }
 
-bool FormManager::FormWithNodeIsAutoFilled(const WebKit::WebNode& node) {
+bool FormManager::FormWithNodeIsAutoFilled(const WebNode& node) {
   FormElement* form_element = NULL;
   if (!FindCachedFormElementWithNode(node, &form_element))
     return false;
@@ -748,21 +739,17 @@ string16 FormManager::InferLabelForElement(
   return inferred_label;
 }
 
-bool FormManager::FindCachedFormElementWithNode(const WebKit::WebNode& node,
+bool FormManager::FindCachedFormElementWithNode(const WebNode& node,
                                                 FormElement** form_element) {
-  for (WebFrameFormElementMap::const_iterator frame_iter =
-           form_elements_map_.begin();
-       frame_iter != form_elements_map_.end(); ++frame_iter) {
-    for (std::vector<FormElement*>::const_iterator form_iter =
-             frame_iter->second.begin();
-         form_iter != frame_iter->second.end(); ++form_iter) {
-      for (std::vector<WebKit::WebFormControlElement>::const_iterator iter =
-               (*form_iter)->control_elements.begin();
-           iter != (*form_iter)->control_elements.end(); ++iter) {
-        if (*iter == node) {
-          *form_element = *form_iter;
-          return true;
-        }
+  for (FormElementList::const_iterator form_iter =
+           form_elements_.begin();
+       form_iter != form_elements_.end(); ++form_iter) {
+    for (std::vector<WebFormControlElement>::const_iterator iter =
+             (*form_iter)->control_elements.begin();
+         iter != (*form_iter)->control_elements.end(); ++iter) {
+      if (*iter == node) {
+        *form_element = *form_iter;
+        return true;
       }
     }
   }
@@ -772,28 +759,22 @@ bool FormManager::FindCachedFormElementWithNode(const WebKit::WebNode& node,
 
 bool FormManager::FindCachedFormElement(const FormData& form,
                                         FormElement** form_element) {
-  for (WebFrameFormElementMap::iterator iter = form_elements_map_.begin();
-       iter != form_elements_map_.end(); ++iter) {
-    const WebFrame* frame = iter->first;
-    // Remove once http://crbug.com/48857.
-    CHECK(frame);
-
-    for (std::vector<FormElement*>::iterator form_iter = iter->second.begin();
-         form_iter != iter->second.end(); ++form_iter) {
-      // TODO(dhollowa): matching on form name here which is not guaranteed to
-      // be unique for the page, nor is it guaranteed to be non-empty.  Need to
-      // find a way to uniquely identify the form cross-process.  For now we'll
-      // check form name and form action for identity.
-      // http://crbug.com/37990 test file sample8.html.
-      // Also note that WebString() == WebString(string16()) does not seem to
-      // evaluate to |true| for some reason TBD, so forcing to string16.
-      string16 element_name((*form_iter)->form_element.name());
-      GURL action(
-          frame->document().completeURL((*form_iter)->form_element.action()));
-      if (element_name == form.name && action == form.action) {
-        *form_element = *form_iter;
-        return true;
-      }
+  for (FormElementList::iterator form_iter = form_elements_.begin();
+       form_iter != form_elements_.end(); ++form_iter) {
+    // TODO(dhollowa): matching on form name here which is not guaranteed to
+    // be unique for the page, nor is it guaranteed to be non-empty.  Need to
+    // find a way to uniquely identify the form cross-process.  For now we'll
+    // check form name and form action for identity.
+    // http://crbug.com/37990 test file sample8.html.
+    // Also note that WebString() == WebString(string16()) does not seem to
+    // evaluate to |true| for some reason TBD, so forcing to string16.
+    string16 element_name((*form_iter)->form_element.name());
+    GURL action(
+        (*form_iter)->form_element.document().completeURL(
+            (*form_iter)->form_element.action()));
+    if (element_name == form.name && action == form.action) {
+      *form_element = *form_iter;
+      return true;
     }
   }
 
@@ -801,7 +782,7 @@ bool FormManager::FindCachedFormElement(const FormData& form,
 }
 
 void FormManager::ForEachMatchingFormField(FormElement* form,
-                                           const WebKit::WebNode& node,
+                                           const WebNode& node,
                                            RequirementsMask requirements,
                                            const FormData& data,
                                            Callback* callback) {
@@ -864,7 +845,7 @@ void FormManager::ForEachMatchingFormField(FormElement* form,
   delete callback;
 }
 
-void FormManager::FillFormField(WebKit::WebFormControlElement* field,
+void FormManager::FillFormField(WebFormControlElement* field,
                                 const FormField* data) {
   // Nothing to fill.
   if (data->value().empty())
@@ -889,7 +870,7 @@ void FormManager::FillFormField(WebKit::WebFormControlElement* field,
   }
 }
 
-void FormManager::PreviewFormField(WebKit::WebFormControlElement* field,
+void FormManager::PreviewFormField(WebFormControlElement* field,
                                    const FormField* data) {
   // Nothing to preview.
   if (data->value().empty())
diff --git a/chrome/renderer/form_manager.h b/chrome/renderer/form_manager.h
index ce34f9a..dbb71d3 100644
--- a/chrome/renderer/form_manager.h
+++ b/chrome/renderer/form_manager.h
@@ -122,10 +122,8 @@ class FormManager {
     std::vector<WebKit::WebFormControlElement> control_elements;
   };
 
-  // A map of vectors of FormElements keyed by the WebFrame containing each
-  // form.
-  typedef std::map<const WebKit::WebFrame*, std::vector<FormElement*> >
-      WebFrameFormElementMap;
+  // Type for cache of FormElement objects.
+  typedef std::vector<FormElement*> FormElementList;
 
   // The callback type used by ForEachMatchingFormField().
   typedef Callback2<WebKit::WebFormControlElement*,
@@ -176,8 +174,8 @@ class FormManager {
   void PreviewFormField(WebKit::WebFormControlElement* field,
                         const webkit_glue::FormField* data);
 
-  // The map of form elements.
-  WebFrameFormElementMap form_elements_map_;
+  // The cached FormElement objects.
+  FormElementList form_elements_;
 
   DISALLOW_COPY_AND_ASSIGN(FormManager);
 };