596785 - "ASSERTION: Something went seriously wrong" with html5 form @required

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Something went seriously wrong!: 
'mInvalidElementsCount >= 0', 
file content/html/content/src/nsHTMLFormElement.cpp, line 1745

nsHTMLFormElement::UpdateValidity [nsHTMLFormElement.cpp:1751]
nsIConstraintValidation::SetValidityState [nsIConstraintValidation.cpp:143]
nsHTMLInputElement::UpdateValueMissingValidityState [nsHTMLInputElement.cpp:3820]
nsHTMLInputElement::UpdateAllValidityStates [nsHTMLInputElement.cpp:3839]
nsHTMLInputElement::HandleTypeChange [nsHTMLInputElement.cpp:2599]
nsHTMLInputElement::ParseAttribute [nsHTMLInputElement.cpp:2662]
nsGenericElement::SetAttr [content/base/src/nsGenericElement.cpp:4651]
nsGenericHTMLElement::SetAttr [nsGenericHTMLElement.cpp:1185]
nsGenericHTMLElement::SetAttr [nsGenericHTMLElement.h:167]
nsGenericHTMLElement::SetAttribute [nsGenericHTMLElement.cpp:352]
nsHTMLInputElement::SetAttribute [nsHTMLInputElement.h:136]
nsIDOMElement_SetAttribute [dom_quickstubs.cpp:4918]
[javascript]

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

This patch is the safest thing we can do. Another solution would be to have form controls not notify the form when their validity changes because of a type change but there is a lot of call of SetBarredFromConstraintValidation. That would be worse to add an attribute here for just two calls.

The best solution would be to not add and remove form controls when there type change but call something like "UpdateElement" which wouldn't call the form validity stuff. Could be done with bug 600754.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

So why are we getting the assertion?

Comment 3

[Mounir Lamouri (:mounir)]

(In reply to comment #2)
> So why are we getting the assertion?

The form update the count of invalid elements when an element is added or removed. When the type changes this code is also run in BeforeSetAttr and AfterSetAttr. If the element was invalid, RemoveElement decrement the counter and AfterSetAttr does nothing. However, the element, because it's type has changed, update it's validity and inform the form. So, again, the counter is decremented and it happens to be negative.
Note that the opposite situation can happen: an element is no longer barred for constraint validation because of a type change. On AddElement, the counter will be incremented and the element will inform the form. In that case, we do not assert but the counter will be equals to 2 which is wrong.

So, the two simple solutions were to make the form not update its counter on type changes or make the elements not informing the form. I think the former is simple because RemoveElement and AddElement are both called twice. In addition, the best fix, I think, would be to have some kind of UpdateElement in BeforeSetAttr which would move the element from the two lists if needed according to the current type and the new one.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

It is still unclear to me why Add/RemoveElement can't handle this situation
automatically.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 479666 [details] [diff] [review]
Patch v1

But ok, this is reasonable simple way to fix this.

Comment 6

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/aa06119e745f