Open
Bug 1472262
Opened 2 years ago
Updated 2 years ago
Assertion failure: deltaToStep > Decimal(0) (stepBelow/stepAbove will be wrong), at /builds/worker/workspace/build/src/dom/html/HTMLInputElement.cpp:4915
Categories
(Core :: DOM: Core & HTML, defect, P3)
Core
DOM: Core & HTML
Tracking
()
NEW
People
(Reporter: tsmith, Unassigned, NeedInfo)
References
(Blocks 1 open bug)
Details
(Keywords: assertion, crash)
Attachments
(2 files)
44 bytes,
text/html

Details  
2.06 KB,
patch

Details  Diff  Splinter Review 
Reduced with mc: BuildID=20180628100518 SourceStamp=b429b9fb68f1a954c4a9f8dba8e845cf7f569a56 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/message_loop.cc:325:10 #11 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298:3 #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/message_loop.cc:325:10 #16 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298:3 #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/plugincontainer.cpp:50:30 #19 main src/browser/app/nsBrowserApp.cpp:287:18 #20 __libc_start_main /build/glibcCl5G7W/glibc2.23/csu/../csu/libcstart.c:291 #21 _start (firefox+0x4236e4)
Flags: intestsuite?
Comment 1•2 years ago


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 maxvalue 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: https://dxr.mozilla.org/mozillacentral/source/xpcom/ds/nsMathUtils.h#114115 ...though note that in this case we're using Decimal version which lives here but has the same form: https://dxr.mozilla.org/mozillacentral/rev/681eb7dfa324dd50403c382888929ea8b8b11b00/dom/html/input/InputType.h#1923
Component: Layout → DOM: Core & HTML
Depends on: 836314
Comment 2•2 years ago


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()) https://dxr.mozilla.org/mozillacentral/rev/681eb7dfa324dd50403c382888929ea8b8b11b00/dom/html/input/InputType.h#1923 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 lowermagnitude hugelynegative value, as you might expect: {m_coefficient = 349999999999999999, m_exponent = 2, m_sign = blink::Decimal::Negative} (4) Second operation is floor(), which is a noop (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.
Comment 3•2 years ago


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.
Comment 4•2 years ago


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.)
Updated•2 years ago

Flags: needinfo?(jwatt)
Updated•2 years ago

Priority:  → P3
Comment 5•2 years ago


Marking as fixoptional 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.
Comment 6•2 years ago


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.
Description
•