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

Status: VERIFIED FIXED

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

Description

[Jesse Ruderman]

Attachment: 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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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?

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

Maybe I should take that debug code out... it really obfuscates things.  :(

Comment 7

[Boris Zbarsky [:bzbarsky]]

Arguably we could also skip the marking/unmarking if this is a shallow unbind...  But maybe not.  This seems safer.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 275731 [details] [diff] [review]
Proposed fix

Trying different reviewer... Jonas is swamped

Comment 9

[Johnny Stenback (:jst)]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

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....

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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.  :(

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Yes, teardown is the main reason I'm worried about performance here.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Doh.  I should have just landed this when I could have... now I really do need the approval.

Comment 16

[Damon Sicore (:damons)]

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).

Comment 17

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 18

[Carsten Book [:Tomcat]]

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