Open Bug 1472262 Opened 2 years ago Updated 1 year ago

Assertion failure: deltaToStep > Decimal(0) (stepBelow/stepAbove will be wrong), at /builds/worker/workspace/build/src/dom/html/HTMLInputElement.cpp:4915


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




Tracking Status
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- affected


(Reporter: tsmith, Unassigned, NeedInfo)


(Blocks 1 open bug)


(Keywords: assertion, crash)


(2 files)

Attached file testcase.html
Reduced with m-c:

Assertion failure: deltaToStep > Decimal(0) (stepBelow/stepAbove will be wrong), at src/dom/html/HTMLInputElement.cpp:4930

#0 mozilla::dom::HTMLInputElement::SanitizeValue(nsTSubstring<char16_t>&) src/dom/html/HTMLInputElement.cpp:4930:13
#1 mozilla::dom::HTMLInputElement::SetValueInternal(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const*, unsigned int) src/dom/html/HTMLInputElement.cpp:2817:9
#2 mozilla::dom::HTMLInputElement::DoneCreatingElement() src/dom/html/HTMLInputElement.cpp:6319:5
#3 nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**, bool*, bool*) src/parser/html/nsHtml5TreeOperation.cpp:988:7
#4 nsHtml5TreeOpExecutor::RunFlushLoop() src/parser/html/nsHtml5TreeOpExecutor.cpp:489:17
#5 nsHtml5ExecutorFlusher::Run() src/parser/html/nsHtml5StreamParser.cpp:121:18
#6 mozilla::SchedulerGroup::Runnable::Run() src/xpcom/threads/SchedulerGroup.cpp:337:32
#7 nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1051:14
#8 NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:519:10
#9 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
#10 MessageLoop::RunInternal() src/ipc/chromium/src/base/
#11 MessageLoop::Run() src/ipc/chromium/src/base/
#12 nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:158:27
#13 XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:896:22
#14 mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:269:9
#15 MessageLoop::RunInternal() src/ipc/chromium/src/base/
#16 MessageLoop::Run() src/ipc/chromium/src/base/
#17 XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:722:34
#18 content_process_main(mozilla::Bootstrap*, int, char**) src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
#19 main src/browser/app/nsBrowserApp.cpp:287:18
#20 __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
#21 _start (firefox+0x4236e4)
Flags: in-testsuite?
Based on the backtrace, this looks like a DOM:Html bug (in input value sanitization), rather than a layout bug.  --> Reclassifying.

Anyway -- looks like this assertion was added by jwatt (CC'd) in bug 836314, and the code in question currently looks like this:

          Decimal deltaToStep = NS_floorModulo(value - stepBase, step);
          if (deltaToStep != Decimal(0)) {
            // "suffering from a step mismatch"
            // Round the element's value to the nearest number for which the
            // element would not suffer from a step mismatch, and which is
            // greater than or equal to the minimum, and, if the maximum is not
            // less than the minimum, which is less than or equal to the
            // maximum, if there is a number that matches these constraints:
            MOZ_ASSERT(deltaToStep > Decimal(0), "stepBelow/stepAbove will be wrong");

As it happens, deltaToStep is negative here, which we don't expect, because per NS_floorModulo documentation[1], the result here is supposed to have the sign of y (which in this case is "step" which is 2.)

Winding back a bit to that line with NS_floorModulo, we've got these inputs:
  value = 100
  stepBase = 7e19
  step = 2

("value" was originally 7e19, up a bit higher in this function, but it was clamped to the default max-value for input type="range" which is 100.  And stepBase is the unclamped "value" from the HTML -- even though it's out of min/max range, it's used for establishing which integers are valid step positions, in combination with the attribute |step=2|.)

So really, we're doing NS_floorModulo(100 - 7e19, 2), i.e. NS_floorModulo([large negative number], 2).

I haven't worked out quite how we're violating this expectation yet, but I'm poking around in rr.

[1] NS_floorModulo documentation is here:
...though note that in this case we're using Decimal version which lives here but has the same form:
Component: Layout → DOM: Core & HTML
Depends on: 836314
So, breaking down our NS_floorModulo(value - stepBase, step) call:

(1) "value - stepBase" produces this hugely negative value:
  {m_coefficient = 699999999999999999,
   m_exponent = 2,
   m_sign = blink::Decimal::Negative}

So that's our first arg "x", and our second arg "y" is 2:
  {m_coefficient = 2, 
   m_exponent = 0, 
   m_sign = blink::Decimal::Positive}

(2) We enter NS_floorModulo with those ^ args, which is implemented as:
  return (x - y * (x / y).floor())
The following steps will be working through that arithmetic & the incremental results.

(3) First operation there is x / y (i.e. x/2), which returns a lower-magnitude hugely-negative value, as you might expect:
  {m_coefficient = 349999999999999999,
   m_exponent = 2,
   m_sign = blink::Decimal::Negative}

(4) Second operation is floor(), which is a no-op (it hits "return *this") since we have a positive exponent which means we're not a fraction.  So we're still working with the same value quoted above, after floor() has happened.

(5) Next operation is y * [what we've got so far], which is produces this value:
   {m_coefficient = 699999999999999998,
    m_exponent = 2,
    m_sign = blink::Decimal::Negative}}
 Note: this is subtly different from the "x"-value that we started with in (1). Its last digit is 8 rather than 9.

(6) Now we do the subtraction: x - [what we've got so far], which is effectively:
 -(699999999999999999^2) - (-(699999999999999998^2))
...which produces:
  {m_coefficient = 1,
   m_exponent = 2,
   m_sign = blink::Decimal::Negative}
...which, if I'm reading it right, is -(1^2) i.e. -1.

So we end up with -1, which is less than 0, which violates the surrounding code's assumptions because we were supposed to produce a value with the same sign as our "y" value which was positive 2.
So I think there's a subtle rounding error in one of the Decimal operations here.

In particular, we're rounding towards 0 in the initial division operation -- strictly speaking, (699999999999999999^2) is odd, and dividing it by 2 should produce a fractional value.  But we're rounding up (towards 0) when doing that division, which is why there's nothing for floor() to do.  (floor would like to round down, but it doesn't get a chance to because we've already rounded.)

So we end up with something larger (a negative number closer to 0) than we should when we do the multiplication in step (5), which is why we aren't "subtracting" something of enough magnitude to push us into positive territory in step (6).

So bottom line, this is a defect in the Decimal division operator, rounding in a way that our NS_floorModulo impl doesn't seem to expect.
Here's a minimal C++ testcase to test this directly in the Decimal class.

When I apply this patch, rebuild, and run $OBJ/dist/bin/TestDecimal , I get the following output:
> Assertion failure: NS_floorModulo(nHuge, p2) >= Decimal(0), at ../../../mozilla/mfbt/tests/TestDecimal.cpp:32
> Segmentation fault (core dumped)

...which indicates that this standalone testcase is indeed triggering the same problem.

(As you can see in the patch, nHuge is the huge negative number quoted above (representing something like 699999999999999999^2), and p2 is a Decimal representation of 2.)
Flags: needinfo?(jwatt)
Priority: -- → P3
Marking as fix-optional for 63 as we are in beta but if a fix comes up on 64, I'll probably take an uplift for 63 if it doesn't come too late in the beta cycle.
Updating tracking flags as we get closer to the 64 release.
You need to log in before you can comment on or make changes to this bug.