Closed
Bug 893332
Opened 11 years ago
Closed 8 years ago
<input type=range> — "Assertion failure: !GetValidityState(VALIDITY_STATE_RANGE_UNDERFLOW) (HTML5 spec does not allow this)"
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: jruderman, Assigned: jwatt)
References
Details
(Keywords: assertion, testcase)
Attachments
(6 files)
271 bytes,
text/html
|
Details | |
12.16 KB,
text/plain
|
Details | |
1.06 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
3.44 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
2.52 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
1002 bytes,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
Assertion failure: !GetValidityState(VALIDITY_STATE_RANGE_UNDERFLOW) (HTML5 spec does not allow this), at content/html/content/src/HTMLInputElement.cpp:1103
Reporter | ||
Comment 1•11 years ago
|
||
Comment 2•9 years ago
|
||
I am getting this when running a WebGL demo in a DEBUG build. http://alteredqualia.com/xg/examples/animation_physics_ammo.html
Comment 3•9 years ago
|
||
also found by bughunter on http://www.matalan.co.uk/womens/shop-by-category/coats-and-jackets?page=2&size=36&cols=4&sort=&id=/womens/shop-by-category/coats-and-jackets&priceRange%5Bmin%5D=7&priceRange%5Bmax%5D=55
Comment 4•9 years ago
|
||
jwatt, do you have comments on this? Even though it's not allowed by the spec, it does seem to be happening in the real world, and it looks like our internal state could be inconsistent because of this.
Flags: needinfo?(jwatt)
Assignee | ||
Comment 5•8 years ago
|
||
Jesse's attached testcase now hits a different MOZ_ASSERT prior to hitting the "HTML5 spec does not allow this" assert, so given that the more recent issues hit the "HTML5 spec does not allow this" first there seem to be two different issues.
Flags: needinfo?(jwatt)
Assignee | ||
Comment 6•8 years ago
|
||
In Jesse's testcase, when the 'min' attribute is set we need to increase the value of the input to the minimum value of the input. We do that under: mozilla::dom::HTMLInputElement::SanitizeValue mozilla::dom::HTMLInputElement::SetValueInternal mozilla::dom::HTMLInputElement::AfterSetAttr It looks like the SetValueInternal call that does this is happening after the UpdateRangeUnderflowValidityState though, and so the underflow validity state is set based on the input's value before it changes.
Assignee | ||
Comment 7•8 years ago
|
||
With the above fixed, another problem is that in HTMLInputElement::SanitizeValue here: DebugOnly<bool> ok = value.toString(buf, ArrayLength(buf)); |value| has the values: m_coefficient uint64_t 1000000000000003 1000000000000003 m_exponent int16_t -11 -11 but the toString() call sets |buf| to "10000" instead of "10000.00000000003". The result of this is that the inputs minimum value is "10000.00000000003" but its value is updated to "10000", and hence the input is still underflowing.
Assignee | ||
Comment 8•8 years ago
|
||
The reason toString() sets |buf| to "10000" is because in this code: https://mxr.mozilla.org/mozilla-central/source/mfbt/decimal/Decimal.cpp?rev=421057bb740c&mark=980-984#978 DBL_DIG==15, so a couple of the least significant digits get chopped off by that loop.
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to Jeff Gilbert [:jgilbert] from comment #2) > I am getting this when running a WebGL demo in a DEBUG build. > > http://alteredqualia.com/xg/examples/animation_physics_ammo.html That page reduces to: data:text/html,<input id="gravitySlider" min="-10" max="10" step="0.1" type="range" value="-5"/> In this case the error is happening during parse time. The parser sets the attributes in reverse, so we have the value set to -5, then when the step is set we end up calling GetValidityState while mParserCreating is true: https://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLInputElement.cpp?rev=1079f6d91f65&mark=1210-1211#1201 Since we haven't finished parsing yet, HTMLInputElement::DoneCreatingElement hasn't yet sanitized the value and it is invalid to call GetValidityState (i.e. the asserts are buggy).
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to Carsten Book [:Tomcat] from comment #3) > also found by bughunter on > http://www.matalan.co.uk/womens/shop-by-category/coats-and- > jackets?page=2&size=36&cols=4&sort=&id=/womens/shop-by-category/coats-and- > jackets&priceRange%5Bmin%5D=7&priceRange%5Bmax%5D=55 And this one reduced to: data:text/html,<input max="50" min="10" step="2" type="range" value="7"/> and is the same as the issue described in the previous comment (it happens during parse time, the value is set to 7 followed by min being set to 10, at which point the assertion in the |aName == nsGkAtoms::min| block fails).
Assignee | ||
Comment 11•8 years ago
|
||
(These parse time assertion failures do not result from an actual bug, since as noted above HTMLInputElement::DoneCreatingElement sanitizes.)
Assignee | ||
Comment 12•8 years ago
|
||
Assignee: nobody → jwatt
Attachment #8712743 -
Flags: review?(amarchesini)
Assignee | ||
Comment 13•8 years ago
|
||
Attachment #8712747 -
Flags: review?(amarchesini)
Comment 14•8 years ago
|
||
Comment on attachment 8712743 [details] [diff] [review] part 1 - Rename misnamed SetValidityState/GetValidityState arguments Review of attachment 8712743 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/html/nsIConstraintValidation.h @@ +68,1 @@ > bool mValue); mValue -> aValue @@ +83,4 @@ > nsresult CheckValidity(bool* aValidity); > void SetCustomValidity(const nsAString& aError); > > + bool GetValidityState(ValidityStateType aState) const { { in a new line.
Attachment #8712743 -
Flags: review?(amarchesini) → review+
Updated•8 years ago
|
Attachment #8712747 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 15•8 years ago
|
||
Attachment #8712897 -
Flags: review?(amarchesini)
Assignee | ||
Comment 16•8 years ago
|
||
Attachment #8712898 -
Flags: review?(amarchesini)
Comment 17•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/44e4c2cbfeb8 https://hg.mozilla.org/integration/mozilla-inbound/rev/67373c5e0d35 https://hg.mozilla.org/integration/mozilla-inbound/rev/43179592faee https://hg.mozilla.org/integration/mozilla-inbound/rev/beaea66985d4
Assignee | ||
Comment 18•8 years ago
|
||
Gah. I've been playing with git and meant to push from my git tree to a local inbound tree. Instead I managed to push directly to inbound. Sorry about that, baku. I'll push follow-up commits if you have any review comments that need to be addressed.
Assignee | ||
Comment 19•8 years ago
|
||
These patches fix the in-the-wild cases that have been found, but not Jesse's original testcase since that is a rounding issue in the Decimal code. I'm a lot less concerned about Jesse's case though since it's a lot rarer that content authors will be writing numbers with so many significant digits for a range control. Rather than keep this bug open I'll clone this bug to deal with Jesse's original testcase separately.
Updated•8 years ago
|
Attachment #8712897 -
Flags: review?(amarchesini) → review+
Updated•8 years ago
|
Attachment #8712898 -
Flags: review?(amarchesini) → review+
Comment 20•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/44e4c2cbfeb8 https://hg.mozilla.org/mozilla-central/rev/67373c5e0d35 https://hg.mozilla.org/mozilla-central/rev/43179592faee https://hg.mozilla.org/mozilla-central/rev/beaea66985d4
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment 21•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/44e4c2cbfeb8 https://hg.mozilla.org/mozilla-central/rev/67373c5e0d35 https://hg.mozilla.org/mozilla-central/rev/43179592faee https://hg.mozilla.org/mozilla-central/rev/beaea66985d4
You need to log in
before you can comment on or make changes to this bug.
Description
•