<input type=range> — "Assertion failure: !GetValidityState(VALIDITY_STATE_RANGE_UNDERFLOW) (HTML5 spec does not allow this)"

RESOLVED FIXED in Firefox 47

Status

()

Core
DOM: Core & HTML
--
critical
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: Jesse Ruderman, Assigned: jwatt)

Tracking

(Blocks: 3 bugs, {assertion, testcase})

Trunk
mozilla47
x86_64
Mac OS X
assertion, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(6 attachments)

(Reporter)

Description

5 years ago
Created attachment 775100 [details]
testcase

Assertion failure: !GetValidityState(VALIDITY_STATE_RANGE_UNDERFLOW) (HTML5 spec does not allow this), at content/html/content/src/HTMLInputElement.cpp:1103
(Reporter)

Comment 1

5 years ago
Created attachment 775102 [details]
stack
I am getting this when running a WebGL demo in a DEBUG build.

http://alteredqualia.com/xg/examples/animation_physics_ammo.html
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)
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)
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.
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.
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.
(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).
(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).
(These parse time assertion failures do not result from an actual bug, since as noted above HTMLInputElement::DoneCreatingElement sanitizes.)
Created attachment 8712743 [details] [diff] [review]
part 1 - Rename misnamed SetValidityState/GetValidityState arguments
Assignee: nobody → jwatt
Attachment #8712743 - Flags: review?(amarchesini)
Created attachment 8712747 [details] [diff] [review]
part 2 - Update validity state before asserting range underflow state.
Attachment #8712747 - Flags: review?(amarchesini)
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+
Attachment #8712747 - Flags: review?(amarchesini) → review+
Created attachment 8712897 [details] [diff] [review]
part 3 - Don't assert range underflow state under the parser
Attachment #8712897 - Flags: review?(amarchesini)
Created attachment 8712898 [details] [diff] [review]
part 4 - Crashtests
Attachment #8712898 - Flags: review?(amarchesini)
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.
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.
(Assignee)

Updated

2 years ago
Blocks: 1243560
Attachment #8712897 - Flags: review?(amarchesini) → review+
Attachment #8712898 - Flags: review?(amarchesini) → review+

Comment 20

2 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
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.