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.
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.
changed keyword NeedsReduction to HasReduction :)
Created attachment 5271 [details] Proposed patch
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.
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.
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
Comment on attachment 5388 [details] Corrected patch Dave?
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.
Moving to r- until Darin's comment is addressed.
Created attachment 5719 [details] Ignore ancestor forms that aren't our form
Comment on attachment 5719 [details] Ignore ancestor forms that aren't our form Looks fine, r=me.