WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
NEW
261081
INPUT element: Fix integer overflow in input.stepDown()
https://bugs.webkit.org/show_bug.cgi?id=261081
Summary
INPUT element: Fix integer overflow in input.stepDown()
Ahmad Saleem
Reported
2023-09-03 04:57:11 PDT
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!
Attachments
Add attachment
proposed patch, testcase, etc.
Anne van Kesteren
Comment 1
2023-09-04 00:11:47 PDT
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
Comment 2
2023-09-04 18:01:44 PDT
Ahmad, could you create a test on WPT.
Radar WebKit Bug Importer
Comment 3
2023-09-10 04:58:13 PDT
<
rdar://problem/115247295
>
Ahmad Saleem
Comment 4
2024-01-17 19:26:12 PST
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
Comment 5
2024-01-17 20:17:09 PST
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
Comment 6
2024-01-17 20:31:29 PST
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)
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug