Bug 123661 – EnforceRange doesn't enforce range of a short

• Component: Bindings
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

In WebIDL, short has a range of  [−32768, 32767], and unsigned short has a range of [0, 65535]. But our code generator just does a 32-bit conversion:

    return "toInt32(exec, $value, $intConversion)" if $type eq "long" or $type eq "short";
    return "toUInt32(exec, $value, $intConversion)" if $type eq "unsigned long" or $type eq "unsigned short";

[Chris Dumez]

Created attachment 215835 [details]
Patch

[Build Bot]

Comment on attachment 215835 [details]
Patch

Attachment 215835 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/19668405

[Build Bot]

Comment on attachment 215835 [details]
Patch

Attachment 215835 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/19558427

[Chris Dumez]

Created attachment 215839 [details]
Patch

[Chris Dumez]

Created attachment 215840 [details]
Patch

[Alexey Proskuryakov]

This conflicts with my patch in bug 123669 a little, which has a partial implementation in JSDOMBinding. But it's fine to land this first.

[Alexey Proskuryakov]

Comment on attachment 215840 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=215840&action=review

Some of the comments below definitely don't need to be addressed in this patch. Some probably need to be addressed one way or another.

> Source/WebCore/ChangeLog:17
> +        No new tests, covered by js/dom/webidl-type-mapping.html.

I'd have said "added test cases to js/dom/webidl-type-mapping.html"

> Source/WebCore/bindings/js/JSDOMBinding.cpp:328
> +struct IntTypeLimits<short> {

It's a strange mixup that we use int8_t, but don't use int16_t. Is possible to make this consistent?

> Source/WebCore/bindings/js/JSDOMBinding.cpp:386
> +        return static_cast<T>(d % LimitsTrait::numberOfValues);

I don't understand why taking a remainder is necessary. Why can't we simply cast?

> Source/WebCore/bindings/js/JSDOMBinding.cpp:416
> +short toInt16(ExecState* exec, JSValue value, IntegerConversionConfiguration configuration)

Same comment about int16_t vs. short.

> LayoutTests/js/dom/webidl-type-mapping-expected.txt:746
> +PASS converter.testShort is 0

It's unfortunate that these results are unreadable, they don't tell what the value being converted was.

> LayoutTests/js/dom/webidl-type-mapping.html:426
> +convert(type, "0");

Would be nice to test negative zero everywhere.

[Chris Dumez]

Comment on attachment 215840 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=215840&action=review

>> Source/WebCore/ChangeLog:17
>> +        No new tests, covered by js/dom/webidl-type-mapping.html.
> 
> I'd have said "added test cases to js/dom/webidl-type-mapping.html"

Ok, I will fix this.

>> Source/WebCore/bindings/js/JSDOMBinding.cpp:328
>> +struct IntTypeLimits<short> {
> 
> It's a strange mixup that we use int8_t, but don't use int16_t. Is possible to make this consistent?

Ok, I will switch to int16_t / uint_16t for consistency.

>> Source/WebCore/bindings/js/JSDOMBinding.cpp:386
>> +        return static_cast<T>(d % LimitsTrait::numberOfValues);
> 
> I don't understand why taking a remainder is necessary. Why can't we simply cast?

Yes, we can simply cast. I will get rid of the modulo.

>> Source/WebCore/bindings/js/JSDOMBinding.cpp:416
>> +short toInt16(ExecState* exec, JSValue value, IntegerConversionConfiguration configuration)
> 
> Same comment about int16_t vs. short.

Will fix.

>> LayoutTests/js/dom/webidl-type-mapping-expected.txt:746
>> +PASS converter.testShort is 0
> 
> It's unfortunate that these results are unreadable, they don't tell what the value being converted was.

Actually it does. The results are on 2 lines. Based on the line before, we know 0 was assigned.

>> LayoutTests/js/dom/webidl-type-mapping.html:426
>> +convert(type, "0");
> 
> Would be nice to test negative zero everywhere.

Hmm. The thing is that I am not 100% sure what the result is supposed to be when assigning -0. Is is supposed to become 0 in all cases? (it seems to be what the current code gives as a result).
I was wondering if assigning -0 on to an unsigned short with [EnforceRange] should throw for example. Currently it does not.

[Chris Dumez]

Created attachment 215843 [details]
Improved patch for landing

I took feedback into consideration, thanks.

The only thing is that I am not 100% sure about the results for -0 but at least it is covered by layout test now if we want to change the behavior.

[WebKit Commit Bot]

Comment on attachment 215843 [details]
Improved patch for landing

Clearing flags on attachment: 215843

Committed r158521: <http://trac.webkit.org/changeset/158521>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Thank you for dressing this so quickly!

> The only thing is that I am not 100% sure about the results for -0 but at least it is covered by layout test now if we want to change the behavior.

I think that it's a mistake in WebIDL - it should have a special case for zeroes, because using sign(x) on either +0 or -0 is undefined in ECMAScript.