913292 - Assertion failure: value >= minimum && value <= maximum (Unsanitized value), at layout/forms/nsRangeFrame.cpp:450

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Matthew Gregan [:kinetik]]

The assertion in the summary is triggered by the page linked in the URL field.

It's easily reproduced with the following code:

<input type=range>
onload = function() { document.getElementsByTagName("input")[0].max = 9; }

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: simple repro

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Jonathan, can you look into this?

Comment 3

[Jonathan Watt [:jwatt]]

So the issue is that under nsRangeFrame::AttributeChanged we look at the HTMLInputElement's value in order to update the anonymous frames. Unfortunately it isn't until HTMLInputElement::AfterSetAttr is called that we sanitize the element's value (for the change to @max) and HTMLInputElement::GetValueAsDecimal will return the value that should have been returned to nsRangeFrame::AttributeChanged.

In other words we first get here:

  mozilla::dom::HTMLInputElement::GetValueInternal
  mozilla::dom::HTMLInputElement::GetValueAsDecimal
  nsRangeFrame::GetValueAsFractionOfRange
  nsRangeFrame::DoUpdateRangeProgressFrame
  nsRangeFrame::UpdateForValueChange
  nsRangeFrame::AttributeChanged
  mozilla::RestyleManager::AttributeChanged
  PresShell::AttributeChanged
  non-virtual thunk to PresShell::AttributeChanged
  nsNodeUtils::AttributeChanged
* mozilla::dom::Element::SetAttrAndNotify
  mozilla::dom::Element::SetAttr
  nsGenericHTMLElement::SetAttr
  nsGenericHTMLElement::SetAttr
  nsGenericHTMLElement::SetHTMLAttr
  mozilla::dom::HTMLInputElement::SetMax

and then here:

  mozilla::dom::HTMLInputElement::SanitizeValue
  mozilla::dom::HTMLInputElement::SetValueInternal
  mozilla::dom::HTMLInputElement::AfterSetAttr
* mozilla::dom::Element::SetAttrAndNotify
  mozilla::dom::Element::SetAttr
  nsGenericHTMLElement::SetAttr
  nsGenericHTMLElement::SetAttr
  nsGenericHTMLElement::SetHTMLAttr
  mozilla::dom::HTMLInputElement::SetMax

It's really unfortunate that we don't have a better place to do the sanitization than in AfterSetAttr. :/

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: possible patch

It seems like probably we should call AfterSetAttr on elements before calling AttributeChanged on their frames in order to let elements finish doing whatever they might want to do. What do you think, Boris?

This seems to pass Try FWIW:

https://tbpl.mozilla.org/?tree=Try&rev=4f258bc82561

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 802412 [details] [diff] [review]
possible patch

Hmm.  The ordering here is _very_ finicky.  

In particular, if AfterSetAttr can trigger any intrinsic state changes this will lead to incorrect restyling.  Can it?  If so, that just means we don't have good enough tests.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 802412 [details] [diff] [review]
possible patch

I was wrong.  This should be fine.  r=me

Comment 7

[Jonathan Watt [:jwatt]]

Okay, great.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae560f8f4c78

I added a reftest.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ae560f8f4c78