Bug 5926 – Assertion failure in HTMLGenericFormElementImpl::removedFromTree

• Status: VERIFIED
• Resolution: FIXED
• Component: Forms

[Alexey Proskuryakov]

WebKit/WebCore/khtml/html/html_formimpl.cpp:864: failed assertion `form == m_form'

Steps to reproduce (debug build of ToT):
1. open the bug URL;
2. after it finishes loading, close the window.

[Joost de Valk (AlthA)]

Created attachment 5251 [details]
Minimal testcase

This is a minimal testcase for the bug, reload or close the page, and any debug
build wil crash.

[Joost de Valk (AlthA)]

changed keyword NeedsReduction to HasReduction :)

Created attachment 5271 [details]
Proposed patch

[Maciej Stachowiak]

Comment on attachment 5271 [details]
Proposed patch

It's a little ugly for the parser to know about this HTMLGenericFormElementImpl
detail directly. Is it possible for the GenericFormElement itself to detect
that it changed forms? Perhaps it can do something on
removedFromDocument/insertedIntoDocument or something?

(In reply to comment #4)
> (From update of attachment 5271 [details] [edit])
> Is it possible for the GenericFormElement itself to detect
> that it changed forms? Perhaps it can do something on
> removedFromDocument/insertedIntoDocument or something?

I tried such an approach, but I couldn't differentiate between "inserted in the right place" and "inserted 
not where it was expected to be inserted" (in both cases insertedIntoDocument is called only once), so I 
ended up calling getForm() every time, which kind of defeats the presumed purpose of passing the 
form to the constructor. Perhaps I didn't try hard enough.

[Maciej Stachowiak]

Comment on attachment 5271 [details]
Proposed patch

r=me

Comment on attachment 5271 [details]
Proposed patch

The patch fixes the minimal testcase but not the given URL.

[Joost de Valk (AlthA)]

I would say, commit the patch, cause it solves a problem, and do not close the bug yet, removing the 
review+ seems a bit harsh.

Created attachment 5307 [details]
Reduced testcase that triggers the assert even with the patch

This is different from the previous reduction, in that in this case, the input
element ends up belonging to form B, even though it isn't a descendant of form
B. That's also what Firefox thinks should happen.

However, if the input element were programatically inserted at the same place,
then getForm() would be used and it would end up belonging to its ancestor,
form A. Firefox says so too.

I see four options:
1) Decide that the above (Firefox-compatible) behavior is correct, and remove
the assert.
2) Decide that the above (Firefox-compatible) behavior is correct, add have the
parser set some flag(s) on the inputElement to bypass the assert in this
special case.
3) Decide that the above logic is incorrect, and change getForm() to answer
"which form would the parser associate this input element with if it were here
in the first place" (if the question has meaning, and if it's even possible to
answer, it probably involves traversing the entire DOM tree).
4) Decide that the above logic is incorrect, and an input element's form should
always be its ancestor.

Hmm... I checked what Firefox has to say about the original testcase, and even though it gives the same 
DOM tree as WebKit, where the input element is a direct descendant of form A, its form is form B. So I 
guess the Firefox logic when parsing is like WebKit's w/o the "proposed patch", which means that option 
1) above should say "...remove the assert and don't apply the 'proposed patch'" and that the only other 
option that makes sense is option 4).

Created attachment 5385 [details]
Don't assert

According to Dave Hyatt on IRC, the assert should be yanked.

Comment on attachment 5385 [details]
Don't assert

I mistakenly deleted the } before the else too. That doesn't even compile!

Created attachment 5388 [details]
Corrected patch

Comment on attachment 5388 [details]
Corrected patch

Added missing curly brace

[Geoffrey Garen]

Comment on attachment 5388 [details]
Corrected patch

Dave?

[Darin Adler]

Comment on attachment 5388 [details]
Corrected patch

I don't think it's right to just remove this assert. It's possible that a form
element is being removed from the DOM tree along with a form that it's nested
inside. The form the element is attached to is still in the main document's DOM
tree. This code will discover the form that's being removed along with the
node, but will leave the form element attached to the form still in the main
document. This is not a good state -- the form from the main document has an
element attached to it that's no longer in the document at all.

[Maciej Stachowiak]

Moving to r- until Darin's comment is addressed.

Created attachment 5719 [details]
Ignore ancestor forms that aren't our form

[Darin Adler]

Comment on attachment 5719 [details]
Ignore ancestor forms that aren't our form

Looks fine, r=me.