AutoFill: Remove one level of indirection when clearing a previewed form.

AutoFill: Remove one level of indirection when clearing a previewed form.

BUG=48857
TEST=none

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

[James Hawkins, 2010-08-05 22:52:23]

+inferno

[James Hawkins, 2010-08-05 22:58:04]

James, can you take a look as well?

[jamesr, 2010-08-05 23:00:36]

I don't have much context here, but is there an expected change in behavior?

[James Hawkins, 2010-08-05 23:06:22]

On 2010/08/05 23:00:36, jamesr wrote:
> I don't have much context here, but is there an expected change in behavior?

Not feature-wise, but with this change, we aren't converting to a FormData
(which contains not-necessarily specific info about a form) and then using that
FormData to find the Form element.

This version finds the current WebFormElement that contains |node|, so we're
assured the frame still exists.

If you look at the crash stacktrace, you'll notice that we dont' crash on the
call to FindFormWithFormControlElement(), which includes dereferencing |frame|
(so we know that particular frame is valid.

My hypothesis is that there is a stale form (and frame) stored in the cache
which has the same name and action as the form containing |node|.  We are
finding this stale form element first and dereferencing the stale |frame|.

With this change, we will be guaranteed to find the form containg |node|, which
will be valid.

[dhollowa, 2010-08-05 23:23:26]

LGTM with nit.  This seems more robust because we are specifically using the
|node| clicked by the user to identify the form in question.

http://codereview.chromium.org/3029061/diff/1/4
File chrome/renderer/form_manager.h (right):

http://codereview.chromium.org/3029061/diff/1/4#newcode107
chrome/renderer/form_manager.h:107: bool ClearPreviewedFormWithNode(const
WebKit::WebNode& form);
s/form/node

[jamesr, 2010-08-06 00:04:05]

This looks like a definite improvement but I don't think it will fix the crash
we're seeing.  If FormManager's form_elements_map_ has pointers to deleted
memory, then I think we crash with or without this patch (either in
FormManager::FormCachedFormElement() or
FormManager::FindCachedFormElementWithNode()).

Having a repro case would be great, of course.