Closed Bug 390975 Opened 17 years ago Closed 17 years ago

[FIX]"ASSERTION: Form controls not ordered correctly" with form-in-table

Categories

(Core :: DOM: Core & HTML, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9beta2

People

(Reporter: jruderman, Assigned: bzbarsky)

References

Details

(Keywords: assertion, regression, testcase)

Attachments

(2 files)

Attached file testcase
Loading the testcase triggers:

###!!! ASSERTION: Form controls not ordered correctly: 'CompareFormControlPosition(aControls[i], aControls[i + 1], aForm) < 0', 
file mozilla/content/html/content/src/nsHTMLFormElement.cpp, line 1222

This assertion was added in bug 353415.
Oh, fun.  The form is gone from the tree, the input is still in the tree, and they both think the input is still in the form.  Arguably, this has been wrong all along...  sicking, what do you think of removing the input from the form when this happens?
Flags: blocking1.9?
Yes, removing the input from the form seems like the right thing to do. Can we easily remove all controls that have been explicitly bound to the form but are not descendants of it?
Component: Layout: Form Controls → DOM: HTML
QA Contact: layout.form-controls → general
Sure, but the simplest impl would require a bunch of ContentIsDescendantOf() calls when the form is unbound from the tree.  I doubt that'll happen that much, though, so seems ok to me...  If we discover it's a problem we can keep better track of these nodes.
We do have plenty of bits available in the flags, so I'd be happy with adding a flag indicating that a form control was explicitly set to belong to a form.
That would be true for all parser-created HTML form controls, basically.  We could set the bit from the sink or something if we know the form is closed or detect misplaced content being shipped out, but it'd actually be a bit of a pain to get right...

I guess setting the bit for controls which are known good would probably not be that bad.  But even that might have to deal with misplaced content.

I suspect it's just not worth it.  In any case, I think I may have a better optimization we can do.  Let me check some things.
Attached patch Proposed fixSplinter Review
Maybe I should take that debug code out... it really obfuscates things.  :(
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #275731 - Flags: superreview?(jonas)
Attachment #275731 - Flags: review?(jonas)
OS: Mac OS X → All
Priority: -- → P2
Hardware: PC → All
Summary: "ASSERTION: Form controls not ordered correctly" with form-in-table → [FIX]"ASSERTION: Form controls not ordered correctly" with form-in-table
Target Milestone: --- → mozilla1.9 M8
Arguably we could also skip the marking/unmarking if this is a shallow unbind...  But maybe not.  This seems safer.
Flags: blocking1.9? → blocking1.9+
Comment on attachment 275731 [details] [diff] [review]
Proposed fix

Trying different reviewer... Jonas is swamped
Attachment #275731 - Flags: superreview?(jst)
Attachment #275731 - Flags: superreview?(jonas)
Attachment #275731 - Flags: review?(jst)
Attachment #275731 - Flags: review?(jonas)
Comment on attachment 275731 [details] [diff] [review]
Proposed fix

+  nsINode* ancestor = this;
+  nsINode* cur;
+  do {
+    cur = ancestor->GetNodeParent();
+    if (!cur) {
+      break;
+    }
+    ancestor = cur;
+  } while (1);

Seems like this shouldn't be something you should need to be writing here, this should be nsINode::GetTopNode() or something. But that's a problem for another day...

r+sr=jst
Attachment #275731 - Flags: superreview?(jst)
Attachment #275731 - Flags: superreview+
Attachment #275731 - Flags: review?(jst)
Attachment #275731 - Flags: review+
Comment on attachment 275731 [details] [diff] [review]
Proposed fix

This patch makes sure that if we remove a form from the DOM but do not remove one of the elements which is not inside the form, it;ll be removed from form.elements.

Risk is moderate, but I think we should do this....
Attachment #275731 - Flags: approval1.9?
Boris, given how much code this turned into, do you think it might be better to simply call ContentIsDescendantOf instead? It's a pretty fast function since it basically just walks a linked list.
If we did that, removal of a form from the tree would be O(N*M) where N is the number of controls and M is the depth under the form.  That might be acceptable, yes.  We can probably construct pathological-ish cases where it behaves badly, but I doubt real sites would hit them much... In practice, I would expect M to be on the order of 10-30 (thanks to nested tables).

Which doesn't really answer the question.   To be really honest, I don't know.  It's been almost 3 months since I last looked at this code.  :(
Unfortunately we can only guess on M and N. Honestly I'm more worried about N as walking a linked list should be pretty darn fast.

Unfortunately this'll always happen on tree teardown right? So it's going to be hit fairly often.
Yes, teardown is the main reason I'm worried about performance here.
Doh.  I should have just landed this when I could have... now I really do need the approval.
Target Milestone: mozilla1.9 M8 → mozilla1.9 M10
Comment on attachment 275731 [details] [diff] [review]
Proposed fix

Setting approval1.9+, leaving as targeted for M10, and you are clear to land once the tree is open for M10 (i.e., after tree opens, after beta).
Attachment #275731 - Flags: approval1.9? → approval1.9+
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
verified fixed for trunk with my debug build Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007120520 Minefield/3.0b2pre and the testcase from this bug. 

No Assertion on Testcase -> Verified
Status: RESOLVED → VERIFIED
Component: DOM: HTML → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: