Bug 261081 – INPUT element: Fix integer overflow in input.stepDown()

• Component: Forms
• Status: NEW

[Ahmad Saleem]

hi Team,

Just came across another potential merge:

Blink Commit: https://chromium.googlesource.com/chromium/src.git/+/28c02d6167e1c545716f433dc84e3656da28c816

Test Case (just focus on 'FAIL stepDown("0", "1", null, 2147483648) should be 2147483648. Was -2147483648.'): https://jsfiddle.net/2a0s5mvz/show

Chrome Canary 118 passes this but Firefox Nightly 118 fails this.

Just wanted to raise to get input, it is easier to merge though.

Thanks!

[Anne van Kesteren]

The specification just says to negate delta, not multiply it by -1. I also don't understand the switch from integers to floats in the Chromium patch.

Perhaps this is something that needs to change, but then I'd prefer we have some web-platform-tests.

[Karl Dubost]

Ahmad, could you create a test on WPT.

[Radar WebKit Bug Importer]

<rdar://problem/115247295>

[Ahmad Saleem]

So I am ranting and trying to understand it better, so ignore me, if I am totally wrong.

First - read HTMLInputElement interface and see 'stepUp' and 'stepDown':

--> https://html.spec.whatwg.org/multipage/input.html#htmlinputelement

undefined stepUp(optional long n = 1);
undefined stepDown(optional long n = 1);

Which if I read WebIDL specification:

--> https://webidl.spec.whatwg.org/#idl-long

Where it should be (long type integer):

>> The long type is a signed integer type that has values in the range [−2147483648, 2147483647].

and then in our WebKit implementation:

https://searchfox.org/wubkat/rev/581e116dc6ce254811dbe2da9d1c1168762fc30c/Source/WebCore/html/HTMLInputElement.cpp#390

We have:

ExceptionOr<void> HTMLInputElement::stepDown(int n)
{
    return m_inputType->stepUp(-n);
}

Which as per Blink should be changed to:

ExceptionOr<void> HTMLInputElement::stepDown(int n)
{
    return m_inputType->stepUp(-1.0);
}

I am not sure then why we need 'int n' argument.

As Anne mentioned, even I am not clear on why they change `int` to `double`, while the `step` should be integers as per web-spec.

[Karl Dubost]

Ahmad, 
I guess there is a typo in the comment not "-1.0" but "-1.0 * n"

```
void HTMLInputElement::stepDown(int n, ExceptionState& exceptionState) {
-  m_inputType->stepUp(-n, exceptionState);
+  m_inputType->stepUp(-1.0 * n, exceptionState);
 }
```




The algorithm process is in 
https://html.spec.whatwg.org/multipage/input.html#dom-input-stepdown

stepUp is calling
https://searchfox.org/wubkat/source/Source/WebCore/html/InputType.cpp#988

Gecko is calling applyStep for both stepUp and stepDown
https://searchfox.org/mozilla-central/rev/15f758f06d97ee3c4e382b174836395442c38f71/dom/html/HTMLInputElement.h#666-668

Note that the blink Code is 
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/html_input_element.cc;l=293;drc=3cc22c22d7637bc5604e8fef3b0882b51a762901;bpv=1;bpt=1

void HTMLInputElement::stepUp(int n, ExceptionState& exception_state) {
  input_type_->StepUp(n, exception_state);
}

void HTMLInputElement::stepDown(int n, ExceptionState& exception_state) {
  input_type_->StepUp(-1.0 * n, exception_state);
}


but yes it looks like a typo mistake for chromium code, but who knows? The review doesn't make any specific comment on 
https://chromium.googlesource.com/chromium/src.git/+/28c02d6167e1c545716f433dc84e3656da28c816

[Karl Dubost]

Currently, there is an extra step to step-down.
HTMLInputElement::stepUp(int n) -> InputType::stepUp(int n) -> InputType::applyStep
HTMLInputElement::stepDown(int n) -> HTMLInputElement::stepUp(int n) -> InputType::stepUp(int n) -> InputType::applyStep

if I wonder if it's to avoid to create InputType::stepDown(int n)